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

[Yan Zhao]

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@xxxxxxxxxx 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@xxxxxxxxxx/

> 
> 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);
> 
> The other option would be to gracefully handle an obsolete root instead of WARNing,
> but as above, I think I prefer to WARN.
> 
> 	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);
> 
> 		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);
> 
> 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()?
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?"