Re: [RFC PATCH 13/14] samples/kvm: Back absent pages on demand in the sample provider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] samples/kvm: lockless absent bitmap updates race in get_pfn() and put_pfn()
- [Medium] samples/kvm: mps_gmem_get_pfn() fills 2 MiB when reporting smaller max_order
--

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

samples/kvm: Back absent pages on demand in the sample provider

This commit adds the MPS_NEW_FILE_FILL flag to the KVM sample memory
provider so that get_pfn() backs an absent page along with the surrounding
2 MiB block on demand, and put_pfn_range() marks released pages as absent
again.

> diff --git a/samples/kvm/mem_provider_sample.c b/samples/kvm/mem_provider_sample.c
> index 670e36207e596..41e0ce5b6bc6a 100644
> --- a/samples/kvm/mem_provider_sample.c
> +++ b/samples/kvm/mem_provider_sample.c
> @@ -41,6 +41,7 @@ struct mps_info {
>  
>  	unsigned long *absent;		/* taken away by MPS_SET_PRESENT */
>  	unsigned long *readonly;	/* set by MPS_SET_READONLY */
> +	bool fill;			/* MPS_NEW_FILE_FILL */
>  
>  	struct inode *gmem_inode;
>  };
> @@ -62,8 +63,15 @@ static void mps_gmem_put_pfn_range(void *provider, pgoff_t index,
>  				   size_t nr_pages)
>  {
>  	struct file *resource_file = provider;
> +	struct mps_info *info = resource_file->private_data;
> +	size_t i;
>  
> -	mps_zero_range(resource_file->private_data, index, nr_pages);
> +	mps_zero_range(info, index, nr_pages);
> +	if (!info->fill || index >= info->npages)
> +		return;
> +	/* Lockless, like get_pfn(): guest_memfd holds its invalidate lock. */
> +	for (i = index; i < min_t(size_t, index + nr_pages, info->npages); i++)
> +		set_bit(i, info->absent);
>  }

[Severity: High]
Can updating info->absent without info->lock in mps_gmem_put_pfn_range() and
mps_gmem_get_pfn() race with mps_set_present() and concurrent faults?

In samples/kvm/mem_provider_sample.c, mps_set_present() modifies
info->absent under info->lock using non-atomic bitmap_clear() and
bitmap_set() without holding guest_memfd's filemap_invalidate_lock:

samples/kvm/mem_provider_sample.c:mps_set_present() {
	mutex_lock(&info->lock);
	if (present) {
		bitmap_clear(info->absent, start, end - start);
		if (info->gmem_inode)
			kvm_gmem_restore(info->gmem_inode, start, end - start);
	} else {
		bitmap_set(info->absent, start, end - start);
		if (info->gmem_inode) {
			unsigned long pfn = info->base_pfn + start;

			kvm_gmem_revoke(info->gmem_inode, start, end - start, &pfn);
			mps_zero_range(info, start, end - start);
		}
	}
	mutex_unlock(&info->lock);
}

First, if mps_set_present() runs concurrently with mps_gmem_put_pfn_range()
or mps_gmem_get_pfn() on indices within the same unsigned long word of
info->absent, the non-atomic read-modify-write in bitmap_set() or
bitmap_clear() can overwrite concurrent set_bit() or clear_bit() updates.

Second, during revocation in mps_set_present(..., false), bitmap_set() runs
before kvm_gmem_revoke() takes filemap_invalidate_lock, and mps_zero_range()
runs after kvm_gmem_revoke() releases filemap_invalidate_lock. If
mps_gmem_get_pfn() runs in either window, it observes test_bit(index,
info->absent) as true and clears info->absent for the 2 MiB block, which
can either cause kvm_gmem_revoke() to see the pages as present and skip
revoking exports, or allow a fault to map the page before or during
mps_zero_range().

Third, callers of kvm_gmem_pfn_get() (such as kvm_gmem_get_pfn(),
kvm_gmem_fault_user_mapping(), and kvm_gmem_allocate()) hold
filemap_invalidate_lock_shared(). While one thread is clearing the 512 bits
of info->absent in mps_gmem_get_pfn(), a second thread faulting in the same
2 MiB block can observe test_bit(index, info->absent) already cleared for
its index while higher bits in the 2 MiB block are still set, causing it to
compute a smaller *max_order and return before the first thread finishes
clearing the block or updating exports.

[ ... ]

> @@ -241,8 +251,16 @@ static int mps_gmem_get_pfn(void *provider, pgoff_t index, unsigned long *pfn,
>  	if (index >= info->npages)
>  		return -EINVAL;
>  
> -	if (test_bit(index, info->absent))
> -		return -EFAULT;
> +	if (test_bit(index, info->absent)) {
> +		unsigned long first = ALIGN_DOWN(index, PTRS_PER_PMD);
> +		unsigned long i;
> +
> +		if (!info->fill)
> +			return -EFAULT;
> +		/* Absent pages are zeroed already. */
> +		for (i = first; i < min(first + PTRS_PER_PMD, info->npages); i++)
> +			clear_bit(i, info->absent);
> +	}

[Severity: Medium]
What happens to guest_memfd export notifications when mps_gmem_get_pfn()
clears info->absent for the full 2 MiB range, but returns a *max_order
smaller than PMD_ORDER (or when max_order is NULL in
kvm_gmem_pfn_mkwrite())?

In samples/kvm/mem_provider_sample.c, mps_gmem_get_pfn() computes *max_order
after clearing info->absent:

	if (!max_order)
		return 0;
	for (order = PUD_ORDER; order; order--) {
		unsigned long first = ALIGN_DOWN(index, 1UL << order);
		unsigned long n = 1UL << order;

		if (first + n <= info->npages &&
		    mps_bits_are(info->absent, first, n, false) &&
		    mps_bits_are(info->readonly, first, n, ro))
			break;
	}
	*max_order = order;

And in virt/kvm/guest_memfd.c, kvm_gmem_pfn_get() updates exports only for
the (1UL << order) block reported by get_pfn():

virt/kvm/guest_memfd.c:kvm_gmem_pfn_get() {
	if (!list_empty(&gi->exports))
		hole = gi->provider_ops->lookup_pfn_range(gi->provider, index,
							  &raw, &nr) == -ENOENT;

	r = gmem_provider_get_pfn(gi, index, pfn, &order, attrs);
	if (r)
		return r;

	if (hole) {
		first = round_down(index, 1UL << order);
		kvm_gmem_update_exports(inode, first,
					min(first + (1UL << order), npages));
	}
}

If any page in the 2 MiB range has a different readonly setting in
info->readonly (via MPS_SET_READONLY), or if the remaining pages at the end
of the file (info->npages - first) are fewer than PTRS_PER_PMD and not
covered by a single power-of-two block around index, *max_order will be
smaller than PMD_ORDER.

Because info->absent was already cleared across the entire 2 MiB range,
kvm_gmem_pfn_get() only calls kvm_gmem_update_exports() for the smaller
(1UL << order) sub-block, and subsequent calls to kvm_gmem_pfn_get() on the
other pages in that 2 MiB block will see lookup_pfn_range() return 0
(hole == false) and never notify exports for them either.

Should the loop clearing info->absent only clear the range corresponding to
the order that mps_gmem_get_pfn() can actually report back to guest_memfd?

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