Re: [PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing read memory barrier (`smp_rmb()`) before resolving the guest physical address, which breaks the MMU notifier retry protocol.
--

--- Patch [1]: [PATCH 5.10.y] KVM: arm64: Retry fault if vma_lookup() results become invalid ---
commit f032c4129aeef677df2aa27f38db191cca6b6b3d
Author: David Matlack <[email protected]>

KVM: arm64: Retry fault if vma_lookup() results become invalid

This commit reads mmu_invalidate_seq before dropping the mmap_lock to detect
if vma_lookup() results become stale before acquiring kvm->mmu_lock. It also
moves the MMU cache top-up earlier in user_mem_abort() to avoid spurious
fault retries.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 26068456ec0f3..2cda74acada2e 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -818,32 +831,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  		fault_ipa &= ~(vma_pagesize - 1);
>  
>  	gfn = fault_ipa >> PAGE_SHIFT;
> -	mmap_read_unlock(current->mm);
>  
> -	/*
> -	 * Permission faults just need to update the existing leaf entry,
[ ... ]
> -	if (fault_status != FSC_PERM || (logging_active && write_fault)) {
[ ... ]
> -	}
> -
>  	mmu_seq = vcpu->kvm->mmu_notifier_seq;
> -	/*
> -	 * Ensure the read of mmu_notifier_seq happens before we call
> -	 * gfn_to_pfn_prot (which calls get_user_pages), so that we don't risk
> -	 * the page we just got a reference to gets unmapped before we have a
> -	 * chance to grab the mmu_lock, which ensure that if the page gets
> -	 * unmapped afterwards, the call to kvm_unmap_hva will take it away
> -	 * from us again properly. This smp_rmb() interacts with the smp_wmb()
> -	 * in kvm_mmu_notifier_invalidate_<page|range_end>.
> -	 */
> -	smp_rmb();
> +	/*
> +	 * Read mmu_notifier_seq so that KVM can detect if the results of
> +	 * find_vma_intersection() or gfn_to_pfn_prot() become stale prior to
> +	 * acquiring kvm->mmu_lock.
> +	 *
> +	 * Rely on mmap_read_unlock() for an implicit smp_rmb(), which pairs
> +	 * with the smp_wmb() in kvm_mmu_notifier_invalidate_range_end().
> +	 */
> +	mmap_read_unlock(current->mm);
>  
>  	pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);

[Severity: Critical]
Does removing the explicit smp_rmb() here in user_mem_abort() break the MMU
notifier retry protocol?

The comment states that we can rely on mmap_read_unlock() for an implicit
smp_rmb(). However, mmap_read_unlock() via up_read() typically provides only
RELEASE semantics.

RELEASE semantics prevent prior memory operations from being reordered past
the unlock, but they do not prevent subsequent memory reads from being
hoisted up before the unlock.

This means the CPU is permitted to reorder the lockless page table reads
inside gfn_to_pfn_prot() to execute before the read of kvm->mmu_notifier_seq
occurs.

If a concurrent MMU notifier (e.g. from page reclaim) unmaps the page and
increments the sequence counter between the hoisted PFN read and the
mmu_seq read, KVM would capture the new sequence number.

Would this cause the mmu_notifier_retry() check under mmu_lock to falsely
succeed, allowing KVM to map a freed host page into the guest's stage-2
page tables?

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