Re: [RFC PATCH 05/14] KVM: guest_memfd: Export PFN mode files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM: guest_memfd: clamp export block order to i_size in kvm_gmem_export_pfn()
- [High] KVM: guest_memfd: missing barrier and range tearing in kvm_gmem_export_pfn()
- [High] KVM: guest_memfd: TOCTOU race between lookup_pfn_range() and get_pfn() in kvm_gmem_export_pfn()
--

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

KVM: guest_memfd: Export PFN mode files

This commit enables export consumers to attach to PFN mode guest_memfd
files, looking up frames via lookup_pfn_range() and get_pfn(), and
updating exports across revocations and hole punches.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 65a951fa49c26..015f6657c3863 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -515,15 +521,32 @@ static int __kvm_gmem_revoke(struct inode *inode, pgoff_t start,
>  	kvm_gmem_invalidate_start(inode, start, end, filter);
>  
>  	if (kvm_gmem_is_pfn_mode(GMEM_I(inode))) {
> +		struct gmem_inode *gi = GMEM_I(inode);
> +
>  		unmap_mapping_range(inode->i_mapping, offset, len, 0);
> +		/*
> +		 * A punch hole gives the frames back only after the reclaim:
> +		 * until then, lookups find holes.  Afterwards, the exports map
> +		 * what the provider backs the range with.
> +		 */
> +		if (put_pfn_range) {
> +			WRITE_ONCE(gi->punch_start, start);
> +			/* Pairs with the smp_rmb() in kvm_gmem_export_pfn(). */
> +			smp_wmb();
> +			WRITE_ONCE(gi->punch_end, end);
> +		}
> +		kvm_gmem_update_exports(inode, start, end);
>  #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_RECLAIM
>  		if (pfn)
>  			kvm_arch_gmem_reclaim(*pfn, nr_pages);
>  		else
> -			kvm_gmem_pfn_reclaim(GMEM_I(inode), start, end);
> +			kvm_gmem_pfn_reclaim(gi, start, end);
>  #endif
> -		if (put_pfn_range)
> -			gmem_provider_put_pfn_range(GMEM_I(inode), start, nr_pages);
> +		if (put_pfn_range) {
> +			gmem_provider_put_pfn_range(gi, start, nr_pages);
> +			WRITE_ONCE(gi->punch_end, 0);
> +			kvm_gmem_update_exports(inode, start, end);
> +		}

[Severity: High]
Can clearing gi->punch_end with a plain WRITE_ONCE() here race with a
concurrent lockless reader in kvm_gmem_export_pfn()?

In __kvm_gmem_revoke(), after the first kvm_gmem_update_exports() finishes,
the consumer lock is released while kvm_gmem_pfn_reclaim() and
gmem_provider_put_pfn_range() execute (which zero pages and update provider
state), and then gi->punch_end is cleared via WRITE_ONCE(gi->punch_end, 0)
without a release barrier or smp_wmb().

On weakly ordered architectures, can the store setting gi->punch_end to 0 be
reordered ahead of the stores in kvm_gmem_pfn_reclaim() and
gmem_provider_put_pfn_range(), allowing a concurrent kvm_gmem_export_pfn()
to observe punch_end == 0 and map the frames before reclaim or provider
state updates are visible?

In addition, kvm_gmem_export_pfn() reads punch_end and then punch_start
without holding inode->i_mapping->invalidate_lock:

virt/kvm/guest_memfd.c:kvm_gmem_export_pfn() {
	punch_end = READ_ONCE(gi->punch_end);
	/* Pairs with the smp_wmb() in __kvm_gmem_revoke(). */
	smp_rmb();
	punch_start = READ_ONCE(gi->punch_start);
	if (index >= punch_start && index < punch_end)
		return -ENOENT;
}

Because kvm_gmem_update_exports(inode, start, end) only notifies exports of
[start, end), an export consumer mapping a disjoint file range (such as
[200, 300)) can skip taking its consumer lock during a hole punch on
[500, 600). If a lookup for index 250 reads punch_end = 600 during that
first hole punch, and a second hole punch on [0, 100) then writes
gi->punch_start = 0 before the lookup reads gi->punch_start, won't
kvm_gmem_export_pfn() observe a torn range [0, 600) and falsely return
-ENOENT for index 250?

[ ... ]

> @@ -1310,28 +1330,83 @@ static bool kvm_gmem_range_is_shared(struct inode *inode, pgoff_t index,
>  	return true;
>  }
>  
> +/*
> + * PFN mode: lookup_pfn_range() first, so that nothing is allocated, then
> + * get_pfn() for the order and attributes.
> + */
> +static int kvm_gmem_export_pfn(struct gmem_inode *gi, pgoff_t index,
> +			       unsigned long *pfn, int *max_order, u32 *attrs)
> +{
> +	pgoff_t punch_start, punch_end;
> +	int order = *max_order, block;
> +	unsigned long raw_pfn;
> +	size_t nr_pages;
> +	kvm_pfn_t frame;
> +	int r;
> +
> +	/*
> +	 * The end first: a new end comes with its start, and an old end of 0
> +	 * makes the window empty.
> +	 */
> +	punch_end = READ_ONCE(gi->punch_end);
> +	/* Pairs with the smp_wmb() in __kvm_gmem_revoke(). */
> +	smp_rmb();
> +	punch_start = READ_ONCE(gi->punch_start);
> +	if (index >= punch_start && index < punch_end)
> +		return -ENOENT;
> +
> +	r = gi->provider_ops->lookup_pfn_range(gi->provider, index, &raw_pfn,
> +					       &nr_pages);
> +	if (r)
> +		return r;
> +
> +	r = gmem_provider_get_pfn(gi, index, &frame, &block, attrs);
> +	if (r)
> +		return r == -EFAULT ? -ENOENT : r;

[Severity: High]
What prevents the page at index from becoming absent between
lookup_pfn_range() and gmem_provider_get_pfn()?

When a consumer calls kvm_gmem_export_lookup_pfn() holding only the consumer
lock, neither the provider lock nor inode->i_mapping->invalidate_lock is
held across these two calls.

If index is present when lookup_pfn_range() runs, and another CPU starts
revoking index (marking it absent in the provider and calling
kvm_gmem_revoke(), which then waits on the consumer lock in
kvm_gmem_update_exports()), kvm_gmem_export_pfn() will proceed to call
gmem_provider_get_pfn() on the now-absent index.

For an on-demand provider where get_pfn() populates absent pages, won't this
allocate and re-populate the range in the middle of the revocation (leaving
the revoked frames mapped after update() runs), or deadlock if get_pfn()
acquires the outer provider lock on its allocation path while
kvm_gmem_revoke() holds the provider lock waiting for the consumer lock?

> +
> +	order = min(order, block);
> +	if (frame ^ index)
> +		order = min_t(int, order, __ffs64(frame ^ index));
> +	/* Nor may the block reach into a punch hole. */
> +	while (order &&
> +	       round_down(index, 1UL << order) < punch_end &&
> +	       round_up(index + 1, 1UL << order) > punch_start)
> +		order--;
> +	*pfn = frame;
> +	*max_order = order;
> +	return 0;
> +}

[Severity: High]
Should kvm_gmem_export_pfn() (or __kvm_gmem_export_lookup()) also clamp
*max_order so the returned block does not extend past inode->i_size?

If a guest_memfd file is created with a size that is smaller than or not
aligned to the provider's contiguous block size (for example, a 4KB file or
a 2MB + 4KB file backed by 2MB order-9 provider blocks),
gmem_provider_get_pfn() can report block = 9 for an in-bounds index.

In __kvm_gmem_export_lookup(), the trailing loop checks whether the block is
shared via kvm_gmem_range_is_shared():

virt/kvm/guest_memfd.c:kvm_gmem_range_is_shared() {
	mt_for_each(&GMEM_I(inode)->attributes, entry, first, index + nr - 1) {
		if (kvm_gmem_get_attributes(inode, entry) &
		    KVM_MEMORY_ATTRIBUTE_PRIVATE)
			return false;
	}
	return true;
}

Because kvm_gmem_init_inode() only populates gi->attributes up to
(size >> PAGE_SHIFT) - 1, indices at or beyond i_size_read(inode) >> PAGE_SHIFT
are NULL in the maple tree and are skipped by mt_for_each(). As a result,
kvm_gmem_range_is_shared() returns true even when the block extends past the
end of the file.

Meanwhile, __kvm_gmem_revoke() clamps revocations to max_pages:

virt/kvm/guest_memfd.c:__kvm_gmem_revoke() {
	pgoff_t max_pages = i_size_read(inode) >> PAGE_SHIFT;
	...
	if (start >= max_pages)
		return 0;

	nr_pages = min_t(size_t, nr_pages, max_pages - start);
}

If a consumer maps the whole block reported by kvm_gmem_export_lookup_pfn()
as allowed by the export API, won't this map provider frames beyond the end
of the guest_memfd file that __kvm_gmem_revoke() will never notify the
consumer to unmap?

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