Re: [PATCH 2/4] KVM: x86/mmu: Harden "map private PFN" against unexpected root invalidation

On Thu, Aug 06, 2026, [email protected] wrote:
> @@ -5184,10 +5184,6 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
> >  	if (kvm_gfn_is_write_tracked(kvm, fault.slot, fault.gfn))
> >  		return -EPERM;
> >  
> > -	r = kvm_mmu_reload(vcpu);
> > -	if (r)
> > -		return r;
> > -
> >  	r = mmu_topup_memory_caches(vcpu, false);
> >  	if (r)
> >  		return r;
> 
> [Severity: High]
> Since mmu_topup_memory_caches() is outside the loop, but kvm_mmu_reload()
> was moved inside, is it possible for the reload to consume cache items
> when allocating new roots?
> 
> If the root is invalid and kvm_mmu_load() allocates new roots, this
> consumes items from mmu_page_header_cache and mmu_shadow_page_cache. When
> execution proceeds to kvm_tdp_mmu_map() expecting fully topped up
> caches, could this lead to a cache underflow and a host warning or panic?

Literally the next patch...

> > @@ -5199,10 +5195,21 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
> >  		if (kvm_test_request(KVM_REQ_VM_DEAD, vcpu))
> >  			return -EIO;
> >  
> > +		r = kvm_mmu_reload(vcpu);
> > +		if (r)
> > +			return r;
> > +
> >  		cond_resched();
> >  
> >  		guard(read_lock)(&kvm->mmu_lock);
> >  
> > +		/*
> > +		 * Because slots_lock is held, it should be impossible for roots
> > +		 * to be invalidated after the initial MMU reload.  WARN, but
> > +		 * continue and re-reload the MMU to try and keep the VM alive.
> > +		 */
> > +		WARN_ON_ONCE(kvm_test_request(KVM_REQ_MMU_FREE_OBSOLETE_ROOTS, vcpu));
> > +
> 
> [Severity: Medium]
> The comment mentions "continue and re-reload the MMU", but does this actually
> fall through directly to kvm_tdp_mmu_map() with an obsolete root?

Yes, addressed two patches from now.

> If the mapping succeeds, it might return RET_PF_FIXED. This would exit the
> loop instead of forcing a retry. Should there be a continue statement after
> the WARN_ON_ONCE to enforce the retry behavior described in the comment?

Eh, I'd rather hedge in the changelog.