Message-ID: <Yh7SwAR/H5dPrqLN@google.com>
Date:   Wed, 2 Mar 2022 02:13:20 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        David Hildenbrand <david@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Matlack <dmatlack@...gle.com>,
        Ben Gardon <bgardon@...gle.com>,
        Mingwei Zhang <mizhang@...gle.com>
Subject: Re: [PATCH v3 20/28] KVM: x86/mmu: Allow yielding when zapping GFNs
 for defunct TDP MMU root

/facepalm

After typing up all of the below, I actually tried the novel idea of compiling
the code... and we can't do xchg() on role.invalid because it occupies a single
bit, it's not a standalone boolean.  I completely agree that the xchg() code is
far, far cleaner, but we'd have to sacrifice using a full byte for "smm" _and_
write some rather ugly code for retrieving a pointer to "invalid".

TL;DR: this

	smp_mb__after_atomic();

	if (root->role.invalid) {
		return;
	}

	root->role.invalid = true;

	smp_mb__before_atomic();

is just a weirdly open coded xchg() that operates on a single bit field.


On Tue, Mar 01, 2022, Paolo Bonzini wrote:
> On 3/1/22 20:43, Sean Christopherson wrote:
> > > and after spending quite some time I wonder if all this should just be
> > > 
> > >          if (refcount_dec_not_one(&root->tdp_mmu_root_count))
> > >                  return;
> > > 
> > > 	if (!xchg(&root->role.invalid, true) {
> > 
> > The refcount being '1' means there's another task currently using root, marking
> > the root invalid will mean checks on the root's validity are non-deterministic
> > for the other task.
> 
> Do you mean it's not possible to use refcount_dec_not_one, otherwise
> kvm_tdp_mmu_get_root is not guaranteed to reject the root?

Scratch my objection, KVM already assumes concurrent readers may or may not see
role.invalid as true.  I deliberately went that route so as to avoid having to
require specific ordering between checking role.invalid and getting a reference.

As my comment further down states, "allocating" a new root is the only flow that
absolutely cannot consume a soon-to-be-invalid root, and it takes mmu_lock for
write so it can't be running concurrently.

So, we don't need to rely on xchg() for barriers, the only consumers of the barriers
are kvm_tdp_mmu_put_root() and they'll obviously always do an atomic xchg().

Invalidating the root while it's refcount is >=1 is also ok, but I thinks that's
flawed for a different reason (see comments on refcount_set(..., 0)).

> > > 	 	tdp_mmu_zap_root(kvm, root, shared);
> > > 
> > > 		/*
> > > 		 * Do not assume the refcount is still 1: because
> > > 		 * tdp_mmu_zap_root can yield, a different task
> > > 		 * might have grabbed a reference to this root.
> > > 		 *
> > > 	        if (refcount_dec_not_one(&root->tdp_mmu_root_count))
> > 
> > This is wrong, _this_ task can't drop a reference taken by the other task.
> 
> This is essentially the "kvm_tdp_mmu_put_root(kvm, root, shared);" (or "goto
> beginning_of_function;") part of your patch.

Gah, I didn't read the code/comments for refcount_dec_not_one().  I assumed it
was "decrement and return true if the result is not '1'", not "decrement unless
the count is already '1', and return true if there was a decrement".  In hindsight,
the former makes no sense at all...

> > >          	        return;
> > > 	}
> > > 
> > > 	/*
> > > 	 * The root is invalid, and its reference count has reached
> > > 	 * zero.  It must have been zapped either in the "if" above or
> > > 	 * by someone else, and we're definitely the last thread to see
> > > 	 * it apart from RCU-protected page table walks.
> > > 	 */
> > > 	refcount_set(&root->tdp_mmu_root_count, 0);
> > 
> > Not sure what you intended here, KVM should never force a refcount to '0'.
> 
> It's turning a refcount_dec_not_one into a refcount_dec_and_test.  It seems
> legit to me, because the only refcount_inc_not_zero is in a write-side
> critical section.  If the refcount goes to zero on the read-side, the root
> is gone for good.

The issue is that by using refcount_dec_not_one() above, there's no guarantee that
this task is the last one to see it as kvm_tdp_mmu_get_root() can succeed and bump
the refcount between refcount_dec_not_one() and here.  Freeing the root would lead
to use-after-free because iterators (rightly) assuming that RCU protection isn't
needed once they have a reference.  RCU protection is needed only if the user of the
iterator wants to dereference page table memory.
 
> > xchg() is a very good idea.  The smp_mb_*() stuff was carried over from the previous
> > version where this sequence set another flag in addition to role.invalid.
> > 
> > Is this less funky (untested)?
> > 
> > 	/*
> > 	 * Invalidate the root to prevent it from being reused by a vCPU while
> > 	 * the root is being zapped, i.e. to allow yielding while zapping the
> > 	 * root (see below).
> > 	 *
> > 	 * Free the root if it's already invalid.  Invalid roots must be zapped
> > 	 * before their last reference is put, i.e. there's no work to be done,
> > 	 * and all roots must be invalidated before they're freed (this code).
> > 	 * Re-zapping invalid roots would put KVM into an infinite loop.
> > 	 *
> > 	 * Note, xchg() provides an implicit barrier to ensure role.invalid is
> > 	 * visible if a concurrent reader acquires a reference after the root's
> > 	 * refcount is reset.
> > 	 */
> > 	if (xchg(root->role.invalid, true))
> > 		spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> > 		list_del_rcu(&root->link);
> > 		spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> > 
> > 		call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);
> > 		return;
> > 	}
> 
> Based on my own version, I guess you mean (without comments due to family
> NMI):
> 
>         if (!refcount_dec_and_test(&root->tdp_mmu_root_count))
>                 return;
> 
> 	if (!xchg(&root->role.invalid, true) {
> 		refcount_set(&root->tdp_mmu_count, 1);
> 	 	tdp_mmu_zap_root(kvm, root, shared);
> 	        if (!refcount_dec_and_test(&root->tdp_mmu_root_count))
>         	        return;
> 	}
> 
>         spin_lock(&kvm->arch.tdp_mmu_pages_lock);
>         list_del_rcu(&root->link);
>         spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
>         call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);

That would work, though I'd prefer to recurse on kvm_tdp_mmu_put_root() instead
of open coding refcount_dec_and_test() so that we get coverage of the xchg()
doing the right thing.

I still slightly prefer having the "free" path be inside the xchg().  To me, even
though the "free" path is the only one that's guaranteed to be reached for every root, 
the fall-through to resetting the refcount and zapping the root is the "normal" path,
and the "free" path is the exception.

Powered by blists - more mailing lists