Re: Potential bug in TDP MMU

[Sean Christopherson <seanjc@google.com>]

On Fri, Dec 10, 2021, Ignat Korchagin wrote:
> I've been trying to figure out the difference between "good" runs and
> "bad" runs of gvisor. So, if I've been running the following bpftrace
> onliner:

...

> That is, I never get a stack with
> kvm_tdp_mmu_put_root->..->kvm_set_pfn_dirty with a "good" run.
> Perhaps, this may shed some light onto what is going on.

Hmm, a little?

Based on the WARN backtrace, KVM encounters an entire chain of valid, present TDP
MMU paging structures _after_ exit_mm() in the do_exit() path, as the call to
task_work_run() in do_exit() occurs after exit_mm().

That means that kvm_mmu_zap_all() is guaranteed to have been called before the
fatal kvm_arch_destroy_vm(), as either:

  a) exit_mm() put the last reference to mm_users and thus called __mmput ->
     exit_mmap() -> mmu_notifier_release() -> ... -> kvm_mmu_zap_all().

  b) Something else had a reference to mm_users, and so KVM's ->release hook was
     invoked by kvm_destroy_vm() -> mmu_notifier_unregister().

It's probably fairly safe to assume this is a TDP MMU bug, which rules out races
or bad refcounts in other areas.

That means that KVM (a) is somehow losing track of a root, (b) isn't zapping all
SPTEs in kvm_mmu_zap_all(), or (c) is installing a SPTE after the mm has been released.

(a) is unlikely because kvm_tdp_mmu_get_vcpu_root_hpa() is the only way for a
vCPU to get a reference, and it holds mmu_lock for write, doesn't yield, and
either gets a root from the list or adds a root to the list.

(b) is unlikely because I would expect the fallout to be much larger and not
unique to your setup.

That leaves (c), which isn't all that likely either.  I can think of a variety of
ways KVM might write a defunct SPTE, but I can't concoct a scenario where an
entire tree of a present paging structures is written.

Can you run with the below debug patch and see if you get a hit in the failure
scenario?  Or possibly even a non-failure scenario?  This should either confirm
or rule out (c).


---
 arch/x86/kvm/mmu/mmu.c     | 2 ++
 arch/x86/kvm/mmu/tdp_mmu.c | 5 +++++
 include/linux/kvm_host.h   | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1ccee4d17481..e4e283a38570 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5939,6 +5939,8 @@ void kvm_mmu_zap_all(struct kvm *kvm)
 	LIST_HEAD(invalid_list);
 	int ign;

+	atomic_set(&kvm->mm_released, 1);
+
 	write_lock(&kvm->mmu_lock);
 restart:
 	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index b69e47e68307..432ccf05f446 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -504,6 +504,9 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
 {
 	lockdep_assert_held_read(&kvm->mmu_lock);

+	WARN_ON(atomic_read(&kvm->mm_released) &&
+		new_spte && !is_removed_spte(new_spte));
+
 	/*
 	 * Do not change removed SPTEs. Only the thread that froze the SPTE
 	 * may modify it.
@@ -577,6 +580,8 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter,
 {
 	lockdep_assert_held_write(&kvm->mmu_lock);

+	WARN_ON(atomic_read(&kvm->mm_released) && new_spte);
+
 	/*
 	 * No thread should be using this function to set SPTEs to the
 	 * temporary removed SPTE value.
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e7bfcc3b6b0b..8e76e2f6c3be 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -569,6 +569,8 @@ struct kvm {

 	struct mutex slots_lock;

+	atomic_t mm_released;
+
 	/*
 	 * Protects the arch-specific fields of struct kvm_memory_slots in
 	 * use by the VM. To be used under the slots_lock (above) or in a

base-commit: 1c10f4b4877ffaed602d12ff8cbbd5009e82c970
--