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

[Yan Zhao]

On Tue, Aug 11, 2026 at 10:07:41AM -0700, Sean Christopherson wrote:
> 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.
Ok. When KVM_REQ_MMU_FREE_OBSOLETE_ROOTS is pending, it is only for direct roots.
This patch still holds that assumption true, right?

If so, since mirror roots cannot be invalid after the first reload, any
subsequent kvm_mmu_reload() calls triggered later in the loop would only reload
direct roots.

Is this necessary for kvm_tdp_mmu_map_private_pfn(), which only maps private pfn?
Or is the purpose of this patch simply to avoid the potential infinite loop
after patch 4?

> > > 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.
Hmm, ok.