Re: [PATCH v7 08/11] KVM: TDX: Get/put PAMT pages when (un)mapping private memory

[Sean Christopherson <seanjc@google.com>]

On Wed, Jul 22, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-07-22 at 08:12 -0700, Sean Christopherson wrote:
> > Hmm, yeah, I think I agree.  Super duper technically, that's a fix for an
> > existing flaw.  Because very, very, VERY theoretically, the cache of page
> > table pages could be exhausted.  E.g. if some other task managed to free non-
> > leaf page tables while mmu_lock was dropped, thus forcing
> > kvm_tdp_mmu_map_private_pfn() to allocate from its cache over and over.  In
> > practice, that's likely impossible thanks to holding slots_lock, but given
> > that (a) retry should be rare and (b) kvm_mmu_topup_memory_cache()
> > is basically free if no work needs to be done, I don't see any reason to do
> > topup outside of the retry loop.
> > 
> > The other thing we should address is the call to kvm_mmu_reload().  Like cache
> > exhaustion, it *should* be impossible for the root to be
> > invalidated/obsoleted, thanks to holding slots lock.  But as evidenced by the
> > rash of recent shadow MMU bugs, we don't always get things perfect, and lack
> > of defense-in-depth can be *extremely* painful.
> 
> Should we assert that we are holding slots lock then? Otherwise the reason for
> the warnings would be confusing. To me at least.

It's already there:

int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
{
	struct kvm_page_fault fault = {
		.addr = gfn_to_gpa(gfn),
		.error_code = PFERR_GUEST_FINAL_MASK | PFERR_PRIVATE_ACCESS,
		.prefetch = true,
		.is_tdp = true,
		.nx_huge_page_workaround_enabled = is_nx_huge_page_enabled(vcpu->kvm),

		.max_level = PG_LEVEL_4K,
		.req_level = PG_LEVEL_4K,
		.goal_level = PG_LEVEL_4K,
		.is_private = true,

		.gfn = gfn,
		.slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn),
		.pfn = pfn,
		.map_writable = true,
	};
	struct kvm *kvm = vcpu->kvm;
	int r;

	lockdep_assert_held(&kvm->slots_lock);  <=====

> > I don't think I want to just move kvm_mmu_reload() into the loop, because KVM
> > should provide stronger guarantees with respect to the validity of the loop,
> > versus the population of the caches.  I.e. I want to WARN if the root becomes
> > obsolete after the initial reload.  And more importantly, KVM really should
> > check the validity of the root after acquiring mmu_lock.
> > 
> > We can't simply call is_page_fault_stale(), because mmu_invalidate_retry_gfn()
> > is inherently fuzzy, i.e. could get false positives, even though the pfn
> > provided by guest_memfd is guaranteed to be valid.  E.g. if shared gfns
> > surrounding the to-be-mapped gfn are concurrently invalidated.
> > 
> > So, this?
> > 
> > 	r = kvm_mmu_reload(vcpu);
> > 	if (r)
> > 		return r;
> > 
> > 	do {
> > 		if (signal_pending(current))
> > 			return -EINTR;
> > 
> > 		if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
> > 			return -EIO;
> > 
> > 		r = mmu_topup_memory_caches(vcpu, false);
> > 		if (r)
> > 			return r;
> > 
> > 		cond_resched();
> > 	
> > 		guard(read_lock)(&kvm->mmu_lock);
> > 
> > 		if
> > (WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu)))
> > 			return -EIO;
> > 
> > 		r = kvm_tdp_mmu_map(vcpu, &fault);
> > 	} while (r == RET_PF_RETRY);
> > 
> 
> Ok. Is there something we can add to connect the slots lock to the root freeing?
> Like maybe a helper to encode that rule? Or better to not wrap the delicate
> details? A comment instead...

Ya, a comment.

LOL, hilarious.  I just discovered a (not fully functional) patch sitting in one
of my many branches that adds the is_page_fault_stale() check, with this as the
changelog:

    KVM: x86/mmu: Ensure page fault isn't stale/obsolete when mapping private PFN
    
    Add a sanity check in the helper used to map private pages into a TDX guest
    to ensure KVM isn't attempting to map memory into an invalid/obsolete root.
    It _should_ be impossible for the root to be invalid, as the only flow that
    marks TDP MMU roots as invalid is "fast all zap", and doing a "fast zap" is
    mutually exclusive with populating TDX memory thanks to slots_lock (this is
    also why KVM doesn't retry kvm_mmu_reload()).
    
    Note, KVM will already WARN on an invalid root if CONFIG_KVM_PROVE_MMU=y,
    but the check is inexpensive compared to the cost of populating memory into
    a TDX guest, and not having a is_page_fault_stale() check _looks_ wrong.

I'll munge that into a mini-series to add the sanity checks.  No need to hold
the D-PAMT series, I see this as orthogonal hardening.  I'm leaning towards the
"have our cake and eat it too" option as the final resting state:

        do {
                if (signal_pending(current))
                        return -EINTR;

                if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
                        return -EIO;

                r = kvm_mmu_reload(vcpu);
                if (r)
                        return r;

                r = mmu_topup_memory_caches(vcpu, false);
                if (r)
                        return r;

                cond_resched();

                guard(read_lock)(&kvm->mmu_lock);

		/* Comment goes here. */
                WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu));

                /*
                 * Snapshot the invalidation sequence counter after acquiring
                 * mmu_lock, as guest_memfd guarantees the validity of the pfn,
                 * i.e. any concurrent invalidations are guaranteed to be
                 * irrelevant.
                 */
                fault.mmu_seq = vcpu->kvm->mmu_invalidate_seq;
                if (is_page_fault_stale(vcpu, &fault))
                        continue;

                r = kvm_tdp_mmu_map(vcpu, &fault);
        } while (r == RET_PF_RETRY);