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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `continue` statement executed on a stale page fault inadvertently terminates the retry loop, causing a fatal `-EIO` failure instead of retrying.
- [High] The KVM MMU invalidation retry protocol order is violated by capturing the sequence after acquiring `mmu_lock` and after the PFN is resolved.
--

--- Patch [4]: [PATCH 4/4] KVM: x86/mmu: Add sanity check to detect stale page faults in "map private PFN" ---
commit 8ecbe739b111e2504c32794278902d5137202a16
Author: Sean Christopherson <[email protected]>

KVM: x86/mmu: Add sanity check to detect stale page faults in "map private PFN"
    
This commit hardens the "map private PFN" flow against stale page faults prior
to mapping the PFN into the guest. It adds a sanity check using a sequence
counter snapshot taken under the mmu_lock.

> 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?

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?

> +		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?

>  		r = kvm_tdp_mmu_map(vcpu, &fault);
>  	} while (r == RET_PF_RETRY);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4