Re: [RFC] KVM: x86/mmu: Prefetch forward run of pages on TDP page faults

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Bypassing the mandatory MMU notifier retry protocol during prefetch leads to a Use-After-Free of host pages.
- [High] Unconditional write privileges in prefetch cause unnecessary host Copy-On-Write and flood the KVM dirty ring.
--

--- Patch [1]: [RFC] KVM: x86/mmu: Prefetch forward run of pages on TDP page faults ---
commit e7ef47a2b5ba2806fbad91e89e8ac0b8f9cf602d
Author: Marco Marangoni <[email protected]>

    KVM: x86/mmu: Prefetch forward run of pages on TDP page faults

This patch implements a fast GUP forward walk to prefetch up to 512
SPTEs (one guest 2MiB region) during a 4KiB page fault in the TDP MMU.
It aims to reduce EPT violations by resolving adjacent pages without
blocking, stopping at the first already-mapped SPTE or host hole.

> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
[ ... ]
> @@ -1209,6 +1209,99 @@ static int tdp_mmu_link_sp(struct kvm *kvm, struct tdp_iter *iter,
>  static int tdp_mmu_split_huge_page(struct kvm *kvm, struct tdp_iter *iter,
>  				   struct kvm_mmu_page *sp, bool shared);
>  
> +/*
> + * Prefetch the forward run of host-present pages after the fault, within the
> + * faulting leaf table (512 pages).  One non-blocking GUP fills the empty SPTEs.
> + * Forward only; capped at the first present SPTE and the first host hole.
> + */
> +static void tdp_mmu_pte_prefetch(struct kvm_vcpu *vcpu,
> +				 struct kvm_page_fault *fault,
> +				 struct tdp_iter *iter)
> +{
> +	struct kvm_mmu_page *sp = sptep_to_sp(rcu_dereference(iter->sptep));
> +	struct page **pages = vcpu->arch.mmu_prefetch_pages;
> +	struct kvm_memory_slot *slot = fault->slot;
> +	unsigned int access = sp->role.access;
> +	bool host_writable = !(slot->flags & KVM_MEM_READONLY);

[Severity: High]
Does this code unconditionally request write privileges? Since host_writable
is based only on the slot flags, it seems to bypass the original fault intent
and whether dirty logging is active. When dirty logging is enabled, this
could result in make_spte() marking up to 511 unmodified pages as dirty,
which might flood the KVM dirty ring. Additionally, because the existing
kvm_prefetch_pages() helper unconditionally uses FOLL_WRITE, could this force
unnecessary host Copy-On-Write for pages the guest only intended to read?

> +	gfn_t start_gfn, slot_end;
> +	int start, count, nr, i;
> +
> +	if (sp_ad_disabled(sp))
> +		return;
> +
> +	/* Mirror (TDX) needs set_external_spte(); gmem pfns aren't in GUP's tables. */
> +	if (is_mirror_sp(sp) || kvm_slot_has_gmem(slot))
> +		return;
> +
> +	/* Racing invalidation may be stale.  No mmu_seq recheck: GUP is under the lock. */
> +	if (unlikely(vcpu->kvm->mmu_invalidate_in_progress))
> +		return;

[Severity: Critical]
Is it safe to bypass the mmu_invalidate_seq retry protocol here?

The TDP MMU operates with mmu_lock held in read mode, which allows
concurrent MMU notifiers like kvm_unmap_gfn_range() to run in parallel.
Could a concurrent host memory unmap free the physical page after
kvm_prefetch_pages() resolves the PFN?

[ ... ]
> +	/* Non-blocking GUP; stops at the first host hole. */
> +	nr = kvm_prefetch_pages(slot, start_gfn, pages, count);
> +	if (nr <= 0)
> +		return;
> +
> +	vcpu->stat.pf_prefetch_pages += nr;
> +
> +	for (i = 0; i < nr; i++) {
> +		u64 *sptep = sp->spt + start + i;
> +		u64 old_spte = SHADOW_NONPRESENT_VALUE;
> +		gfn_t gfn = start_gfn + i;
> +		u64 new_spte;
> +
> +		make_spte(vcpu, sp, slot, access, gfn,
> +			  page_to_pfn(pages[i]), old_spte,
> +			  true /* prefetch */, false, host_writable, &new_spte);
> +
> +		/* cmpxchg from empty is the race check; present/MMIO/frozen fails it. */
> +		if (try_cmpxchg64(sptep, &old_spte, new_spte)) {

[Severity: Critical]
If a concurrent host memory unmap happened during kvm_prefetch_pages(), could
this cmpxchg install a valid, writable SPTE to a freed page since there is no
re-check of the invalidation sequence?

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