Re: [PATCH 4/4] KVM: x86/mmu: Add sanity check to detect stale page faults in "map private PFN"

On Thu, Aug 06, 2026, [email protected] wrote:
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 379f570ef04f8..76e3cd717324b 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -5210,6 +5210,16 @@ int kvm_tdp_mmu_map_private_pfn(struct kvm_vcpu *vcpu, gfn_t gfn, kvm_pfn_t pfn)
> >  		 */
> >  		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;
> 
> [Severity: High]
> Does capturing the invalidation sequence after acquiring mmu_lock violate
> the KVM MMU invalidation retry protocol order?

Yes, see the comment and changelog.

> The sequence is explicitly captured inside the mmu_lock critical section
> after the PFN has already been resolved by the caller. This bypasses the
> mandatory generation safety sequence (capture sequence, resolve PFN, acquire
> lock).
> 
> Additionally, could this still cause false positives since
> mmu_invalidate_retry_gfn() checks mmu_invalidate_in_progress?

Yes, it's possible, but it would require zappings GFNs on either side of the GFN
being mapped, such that the target GFN is contained by mmu_invalidate_range_{start,end}.
The odds of that happening in practice are low enough that I'm comfortable eating
false positives even though strictly speaking they _should_ be irrelevant.

> > +		if (is_page_fault_stale(vcpu, &fault))
> > +			continue;
> 
> [Severity: High]
> Does this continue statement inadvertently exit the loop without retrying?
> 
> In C, a continue statement within a do-while loop jumps to the while
> condition evaluation, not the beginning of the loop block. If r is not
> RET_PF_RETRY when this continue is hit, the condition r == RET_PF_RETRY
> will evaluate to false.
> 
> Will this cause the function to exit the loop prematurely and return -EIO?

Doh.  And this is why I'm more than happy to deal with Sashiko's paranoia :-)