| Message ID | 20221124003505.424617-1-mizhang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] KVM: x86/mmu: replace BUG() with KVM_BUG() in shadow mmu | expand |
> v1 -> v2: > - compile test the code. > - fill KVM_BUG() with kvm_get_running_vcpu()->kvm Nope, the zapping code paths will run often with no running vCPU, for example drop_parent_pte <- kvm_mmu_unlink_parents <- __kvm_mmu_prepare_zap_page <- kvm_zap_obsolete_pages <- kvm_mmu_zap_all_fast <- kvm_mmu_invalidate_zap_pages_in_memslot <- kvm_page_track_flush_slot <- kvm_arch_flush_shadow_memslot <- kvm_invalidate_memslot <- ioctl(KVM_SET_USER_MEMORY_REGION). Paolo > v1: > https://lore.kernel.org/all/20221123231206.274392-1-mizhang@google.com/ > > Cc: Nagareddy Reddy <nspreddy@google.com> > Cc: Jim Mattson <jmattson@google.com> > Cc: David Matlack <dmatlack@google.com> > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 4736d7849c60..f3b307c588ac 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -955,12 +955,12 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) > > if (!rmap_head->val) { > pr_err("%s: %p 0->BUG\n", __func__, spte); > - BUG(); > + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); > } else if (!(rmap_head->val & 1)) { > rmap_printk("%p 1->0\n", spte); > if ((u64 *)rmap_head->val != spte) { > pr_err("%s: %p 1->BUG\n", __func__, spte); > - BUG(); > + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); > } > rmap_head->val = 0; > } else { > @@ -979,7 +979,7 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) > desc = desc->more; > } > pr_err("%s: %p many->many\n", __func__, spte); > - BUG(); > + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); > } > } > > -- > 2.38.1.584.g0f3c55d4c2-goog >
On Wed, Nov 23, 2022 at 5:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > v1 -> v2: > > - compile test the code. > > - fill KVM_BUG() with kvm_get_running_vcpu()->kvm > > Nope, the zapping code paths will run often with no running vCPU, for > example drop_parent_pte <- kvm_mmu_unlink_parents <- > __kvm_mmu_prepare_zap_page <- kvm_zap_obsolete_pages <- > kvm_mmu_zap_all_fast <- kvm_mmu_invalidate_zap_pages_in_memslot <- > kvm_page_track_flush_slot <- kvm_arch_flush_shadow_memslot <- > kvm_invalidate_memslot <- ioctl(KVM_SET_USER_MEMORY_REGION). > > Paolo Make sense. Will plumb through the kvm parameter in the next version.
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 4736d7849c60..f3b307c588ac 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -955,12 +955,12 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) if (!rmap_head->val) { pr_err("%s: %p 0->BUG\n", __func__, spte); - BUG(); + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); } else if (!(rmap_head->val & 1)) { rmap_printk("%p 1->0\n", spte); if ((u64 *)rmap_head->val != spte) { pr_err("%s: %p 1->BUG\n", __func__, spte); - BUG(); + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); } rmap_head->val = 0; } else { @@ -979,7 +979,7 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) desc = desc->more; } pr_err("%s: %p many->many\n", __func__, spte); - BUG(); + KVM_BUG(true, kvm_get_running_vcpu()->kvm, ""); } }
Replace BUG() in pte_list_remove() with KVM_BUG() to avoid crashing the host. MMU bug is difficult to discover due to various racing conditions and corner cases and thus it extremely hard to debug. The situation gets much worse when it triggers the shutdown of a host. Host machine crash eliminates everything including the potential clues for debugging. From cloud computing service perspective, BUG() or BUG_ON() is probably no longer appropriate as the host reliability is top priority. Crashing the physical machine is almost never a good option as it eliminates innocent VMs and cause service outage in a larger scope. Even worse, if attacker can reliably triggers this code by diverting the control flow or corrupting the memory, then this becomes vm-of-death attack. This is a huge attack vector to cloud providers, as the death of one single host machine is not the end of the story. Without manual interferences, a failed cloud job may be dispatched to other hosts and continue host crashes until all of them are dead. Because of the above reasons, shrink the scope of crash to the target VM only. KVM_BUG() and KVM_BUG_ON() requires a valid struct kvm which requires extra plumbing. Avoid it in this version by just using kvm_get_running_vcpu()->kvm instead. Compile tested only. v1 -> v2: - compile test the code. - fill KVM_BUG() with kvm_get_running_vcpu()->kvm v1: https://lore.kernel.org/all/20221123231206.274392-1-mizhang@google.com/ Cc: Nagareddy Reddy <nspreddy@google.com> Cc: Jim Mattson <jmattson@google.com> Cc: David Matlack <dmatlack@google.com> Signed-off-by: Mingwei Zhang <mizhang@google.com> --- arch/x86/kvm/mmu/mmu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)