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

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 23, 2026, Yan Zhao wrote:
> On Wed, Jul 22, 2026 at 08:12:20AM -0700, Sean Christopherson wrote:
> > On Mon, Jul 20, 2026, Rick P Edgecombe wrote:
> > > On Sat, 2026-07-18 at 06:10 +0000, sashiko-bot@kernel.org wrote:
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [High] Infinite kernel loop in `kvm_tdp_mmu_map_private_pfn` due to permanent PAMT cache depletion on transient TDX module contention.
> > > > --
> > > 
> > > Our internal Sashiko found this too. It's a false positive as a real bug.
> > > 
> > > Today kvm_tdp_mmu_map_private_pfn() is only called tdx_gmem_post_populate()
> > > during TD setup. It holds the heavyweight tdx_vm_state_guard which grabs vm-
> > > >lock, kvm->slots_lock, and all vcpu->mutex. So there should be no contention
> > > possible.
> > > 
> > > Any potential confusion is not new either, because a similar thing could happen
> > > with the external page tables.
> > > 
> > > But Yan and I were discussing that it would be a good cleanup to fix this anyway
> > > because the reason it is not a functional issue is not clear from the code. For
> > > improved readability (and quieter sashiko reports) the topup can happen inside
> > > the retry loop. Either by moving the retry loop or moving the topup.
> > 
> > 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.
> > 
> > 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.
> Past you said "No" to checking is_page_fault_stale() [*] :)
> [*] https://lore.kernel.org/all/aPken0s-0MfdSd5o@google.com/

And my reasoning there still stands: there will be false positives, and avoiding
constant false positives requries a weird mmu_seq snapshot.  To be very clear, I
still find the code to be gross, but unfortunately, the onslaught of recent bugs
in scenarios we _thought_ were impossible has made it abundantly clear that, at
least when it's not completely insane, KVM needs to effectively "fail close"
when the impossible happens.  I.e. take action to ensure a bad assumption in KVM
can't be abused to esclate into a DoS or UAF.

> > My only hesitation with manually checking KVM_REQ_MMU_FREE_OBSOLETE_ROOTS is that
> > if more checks/functionality were added to is_page_fault_stale() in the future,
> > then we could end up missing kvm_tdp_mmu_map_private_pfn() and introduce a bug.
> > 
> > Maybe we can have it both ways?  WARN if roots are unexpectedly made obsolete,
> > but fully check is_page_fault_stale() and gracefully handle an obsolete root
> > instead of effectively terminating the guest.
> > 
> > 	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);
> > 
> > 		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);
> > 
> BTW, since kvm_tdp_mmu_map_private_pfn() is currently solely invoked by TDX
> during the TD built phase, and was introduced to avoid redundant
> kvm_gmem_get_pfn() calls in the gmem population path, are there any foreseeable
> future users of kvm_tdp_mmu_map_private_pfn()?

Nope, not that I know of.

> If not, could we simply drop the RETRY loop, given that the locks in the TDX
> path already guarantee that a RETRY error will never occur?"

We could, but I don't think that buys us much, because we still need the
is_page_fault_stale() check, or an equivalent.  At that point, removing the
retry loop is probably a net negative, because it could be the difference between
a race resulting in a WARN but an otherwise usable VM, and an unintentional guest
DoS.