Re: [RFC PATCH 01/14] KVM: guest_memfd: Clamp the order of PFN mode blocks to what KVM can map

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] kvm: guest_memfd: race in __kvm_gmem_invalidate_start() due to late range expansion
- [High] kvm: guest_memfd: infinite loop in kvm_gmem_pfn_map_order() when PUD_ORDER == PMD_ORDER
- [High] KVM: guest_memfd: non-zero order passed to kvm_arch_gmem_make_private()
--

commit ec431028c58523e8ac8bc3e4a0b8d9a1a1868bb2
Author: Fred Griffoul <fgriffo@amazon.co.uk>

KVM: guest_memfd: Clamp the order of PFN mode blocks to what KVM can map

This commit clamps the mapping order returned for PFN-mode guest_memfd
files to block sizes supported by KVM where the gfn, file index, and pfn
share the same alignment and memory attributes. It also expands invalidation
ranges in PFN mode to 1G-aligned blocks so concurrent faults on the same
block retry.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 3c4bedceccfc2..be26bd16d4207 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -355,6 +360,16 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  
>  		flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);
>  
> +		/*
> +		 * In PFN mode, a fault may map a block of up to PUD_ORDER
> +		 * pages, sized before this invalidation (see
> +		 * kvm_gmem_pfn_map_order()): make faults anywhere in the
> +		 * blocks around the range retry.
> +		 */
> +		kvm_mmu_invalidate_range_add(kvm,
> +					     round_down(gfn_range.start, blk),
> +					     round_up(gfn_range.end, blk));

[Severity: Critical]
Does expanding the invalidation range after kvm_mmu_unmap_gfn_range() leave a
window where a concurrent fault can install a stale huge mapping?

In __kvm_gmem_invalidate_start(), gfn_range.may_block is true. When
kvm_mmu_unmap_gfn_range() runs, it registers only the unexpanded
[gfn_range.start, gfn_range.end) in kvm->mmu_invalidate_range_start and
kvm->mmu_invalidate_range_end before calling kvm_unmap_gfn_range():

virt/kvm/kvm_main.c:kvm_mmu_unmap_gfn_range() {
	kvm_mmu_invalidate_range_add(kvm, range->start, range->end);
	return kvm_unmap_gfn_range(kvm, range);
}

Because may_block is true, kvm_unmap_gfn_range() can yield kvm->mmu_lock
(for example via tdp_mmu_iter_cond_resched() in tdp_mmu_zap_leafs() or
cond_resched_rwlock_write() in __walk_slot_rmaps()). Meanwhile,
kvm->mmu_invalidate_seq is not incremented until kvm_mmu_invalidate_end().

If a vCPU fault on another gfn in the same 2M or 1G block already completed
kvm_gmem_get_pfn() with a large *max_order before the invalidation began, it
can acquire kvm->mmu_lock while kvm_unmap_gfn_range() has yielded the lock.
At that point mmu_invalidate_retry_gfn() checks only the unexpanded
range->start..range->end and sees the old mmu_invalidate_seq, so the fault
will not retry and can install a huge SPTE covering the range being revoked
or converted.

Should kvm_mmu_invalidate_range_add() with the blk-aligned range be called
before kvm_mmu_unmap_gfn_range()?

[ ... ]

> @@ -1535,6 +1550,44 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
>  	return folio;
>  }
>  
> +/*
> + * The order a PFN mode block can be mapped at: the provider's order, no
> + * larger than the alignment of @gfn and @pfn to @index allows, rounded down to
> + * a page-table size, and never over pages that are not all shared or all
> + * private, since KVM maps a block with a single attribute.  Called with the
> + * invalidate lock held; an invalidation of any page of the block makes the
> + * fault retry (see __kvm_gmem_invalidate_start()).
> + */
> +static int kvm_gmem_pfn_map_order(struct inode *inode, pgoff_t index,
> +				  gfn_t gfn, kvm_pfn_t pfn, int order)
> +{
> +	u64 misalign = (gfn ^ index) | (pfn ^ index);
> +	u64 attrs;
> +
> +	if (WARN_ON_ONCE(order < 0))
> +		return 0;
> +
> +	order = min(order, PUD_ORDER);
> +	if (misalign)
> +		order = min_t(int, order, __ffs64(misalign));
> +
> +	if (order >= PUD_ORDER)
> +		order = PUD_ORDER;
> +	else if (order >= PMD_ORDER)
> +		order = PMD_ORDER;
> +	else
> +		return 0;
> +
> +	attrs = kvm_gmem_get_attributes(inode,
> +					mtree_load(&GMEM_I(inode)->attributes, index));
> +	while (order &&
> +	       !kvm_gmem_range_has_attributes(inode,
> +					      ALIGN_DOWN(index, 1UL << order),
> +					      1UL << order, attrs))
> +		order = order == PUD_ORDER ? PMD_ORDER : 0;

[Severity: High]
Can this while loop hang when PUD_ORDER equals PMD_ORDER?

On arm64 with 2-level page tables (CONFIG_PGTABLE_LEVELS == 2, such as 64K
pages with 42-bit VA or 16K pages with 36-bit VA),
arch/arm64/include/asm/pgtable-types.h includes
include/asm-generic/pgtable-nopmd.h, which defines PMD_SHIFT as PUD_SHIFT.
This makes PUD_ORDER equal to PMD_ORDER.

If a PFN-mode block of order >= PUD_ORDER has mixed shared/private
attributes, kvm_gmem_range_has_attributes() returns false and the loop
evaluates:

	order = order == PUD_ORDER ? PMD_ORDER : 0;

Since order == PUD_ORDER is true and PMD_ORDER == PUD_ORDER, order remains
non-zero and unchanged, spinning forever while holding the shared
filemap_invalidate_lock and kvm->srcu.

Would checking order > PMD_ORDER instead of order == PUD_ORDER avoid this?

> +	return order;
> +}
> +
>  int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  		     gfn_t gfn, kvm_pfn_t *pfn, int *max_order)
>  {
> @@ -1556,6 +1609,10 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  		goto out;
>  	}
>  
> +	if (kvm_gmem_is_pfn_mode(GMEM_I(file_inode(file))))
> +		*max_order = kvm_gmem_pfn_map_order(file_inode(file), index, gfn,
> +						    *pfn, *max_order);
> +
>  	if (kvm_arch_has_gmem_convert() &&
>  	    kvm_gmem_is_private_mem(file_inode(file), index))
>  		r = kvm_arch_gmem_make_private(kvm, gfn, *pfn,

[Severity: High]
What happens on SEV-SNP when kvm_gmem_pfn_map_order() returns a non-zero
order for a private range?

In kvm_gmem_get_pfn(), *max_order is now preserved up to PMD_ORDER or
PUD_ORDER in PFN mode (even when the caller passes max_order == NULL, since
max_order is redirected to &__order), and then passed to
kvm_arch_gmem_make_private() as:

	r = kvm_arch_gmem_make_private(kvm, gfn, *pfn,
				       (kvm_pfn_t)1 << *max_order);

However, the x86 implementation in arch/x86/kvm/svm/sev.c only supports
single-page conversions:

arch/x86/kvm/svm/sev.c:sev_gmem_make_private() {
	if (!sev_snp_guest(kvm))
		return 0;

	if (WARN_ON_ONCE(nr_pages != 1))
		return -EIO;
	...
}

In addition, gfn and *pfn are the faulting subpage's frame numbers rather
than the base of the aligned 2^(*max_order) block. Won't a PFN-mode provider
returning order >= PMD_ORDER for an SEV-SNP guest trigger WARN_ON_ONCE() and
fail with -EIO?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027154649.91571-1-griffoul@gmail.com?part=1