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

[Sean Christopherson]

On Tue, Aug 11, 2026, Yan Zhao wrote:
> On Thu, Aug 06, 2026 at 02:40:48PM -0700, Sean Christopherson wrote:
> > Move kvm_tdp_mmu_map_private_pfn()'s reload of the MMU into its tight loop
> > so that an unexpected root invalidation has a better chance of being
> > handled gracefully, even though it should be impossible for the vCPU's root
> > to be invalidated after the initial reload.  As is, encountering an invalid
> > root is *guaranteed* to put the task into an infinite loop (albeit a
> > breakable loop that honors NEED_RESCHED).
> Note: without the newly added is_page_fault_stale() check in patch 4, an invalid
> root would not put the task into an infinite loop :) 
> 
> BTW: As noted in [1], is_page_fault_stale() only checks !mirror roots, and
> kvm_mmu_reload() reloads mirror roots only when !mirror roots are also invalid,
> since an invalid mirror root was considered impossible. (up to now, no?)
> 
> [1] https://lore.kernel.org/all/anrD8nI8RfYoNvbf@xxxxxxxxxxxxxxxxxxxxxxxxx

> > Add a WARN to try and detect bugs that break KVM's expectations, along with
> > a comment to explain why it should be impossible for the root to be
> > invalidated.
> And there's already a warning in kvm_tdp_mmu_map():
> "KVM_MMU_WARN_ON(!root || root->role.invalid);".
> So the warning also seems redundant.

No, KVM_REQ_MMU_FREE_OBSOLETE_ROOTS can be pending even if the current root is
valid.  And once the is_page_fault_stale() check comes along, the WARN in
kvm_tdp_mmu_map() is effectively unreachable.  The patch ordering is weird, but
there wasn't a great solution because adding is_page_fault_stale() first would
create an obvious infinite loop.

> > Cc: Kai Huang <kai.huang@xxxxxxxxx>
> > Cc: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > Cc: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >  arch/x86/kvm/mmu/mmu.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 621b0a42f2a1..c6cac893cbad 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -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;
> > @@ -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;
> > +
> Since kvm_mmu_reload() is moved inside the loop, should we also move
> kvm_gfn_is_write_tracked() inside the loop to prevent unexpected bugs?

I'm leaning no?  That check was extreme paranoia in the first place.