Re: [RFC PATCH 07/14] KVM: guest_memfd: Update exports when pages are allocated

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: guest_memfd: race between lookup and export update in kvm_gmem_pfn_get()
- [High] KVM: guest_memfd: missing export update in kvm_gmem_pfn_mkwrite()
--

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

KVM: guest_memfd: Update exports when pages are allocated

This commit updates attached exports when guest_memfd pages gain backing
memory via host faults, KVM get_pfn(), or fallocate(). In folio mode it
zeroes newly allocated folios, marks them dirty, and notifies exports under
the invalidate lock, while in PFN mode it checks if the page was a hole
before get_pfn() and updates exports for the populated block.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 1c5826c5384ed..42bb5f263e234 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -590,6 +592,64 @@ int kvm_gmem_revoke(struct inode *inode, pgoff_t start, size_t nr_pages,
>  }
>  EXPORT_SYMBOL_GPL(kvm_gmem_revoke);
>  
[ ... ]
> +/*
> + * get_pfn() in PFN mode, which may back a hole: then tell the exports about
> + * the whole block that get_pfn() reports, as the provider may have backed all
> + * of it.  Called with the invalidate lock held.
> + */
> +static int kvm_gmem_pfn_get(struct inode *inode, pgoff_t index,
> +			    kvm_pfn_t *pfn, int *max_order, u32 *attrs)
> +{
> +	pgoff_t npages = i_size_read(inode) >> PAGE_SHIFT;
> +	struct gmem_inode *gi = GMEM_I(inode);
> +	bool hole = false;
> +	unsigned long raw;
> +	pgoff_t first;
> +	int order, r;
> +	size_t nr;
> +
> +	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 (max_order)
> +		*max_order = order;
> +	return 0;
> +}

[Severity: High]
Can concurrent calls to kvm_gmem_pfn_get() race between lookup_pfn_range(),
gmem_provider_get_pfn(), and kvm_gmem_update_exports()?

All shared-access callers of kvm_gmem_pfn_get() -- kvm_gmem_allocate(),
kvm_gmem_pfn_fault(), and kvm_gmem_get_pfn() -> __kvm_gmem_get_pfn() -- hold
inode->i_mapping->invalidate_lock in shared mode via
filemap_invalidate_lock_shared(). Unlike folio mode, where
kvm_gmem_get_folio() returns a locked folio and holds the folio lock across
both kvm_gmem_folio_zero() and kvm_gmem_update_exports(), kvm_gmem_pfn_get()
has no per-range synchronization across the lookup, allocation, and export
notification steps.

If thread A calls kvm_gmem_pfn_get() on an unbacked index,
lookup_pfn_range() returns -ENOENT (setting hole = true) and
gmem_provider_get_pfn() populates the backing frame or 1UL << order block in
the provider. If a concurrent thread B calls kvm_gmem_pfn_get() for the same
index (or another index within that newly populated block) after thread A's
gmem_provider_get_pfn() completes, but before thread A finishes
kvm_gmem_update_exports(), thread B will see lookup_pfn_range() == 0
(hole = false), skip kvm_gmem_update_exports(), and return to fallocate(),
the host fault handler, or the guest vCPU fault handler while the export's
IOMMU mapping is not yet installed.

Conversely, if both thread A and thread B call lookup_pfn_range() before
either thread's gmem_provider_get_pfn() populates the block, both observe
hole = true. After thread A finishes kvm_gmem_update_exports() and returns
the mapped page to userspace or the guest (which may immediately start DMA),
thread B will run a second kvm_gmem_update_exports() on the same block,
causing export consumers such as iommufd to unmap and tear down the live
IOMMU mapping.

[ ... ]
> @@ -819,7 +881,7 @@ static vm_fault_t kvm_gmem_pfn_fault(struct vm_fault *vmf)
>  	u32 attrs;
>  	int r;
>  
> -	r = gmem_provider_get_pfn(GMEM_I(inode), vmf->pgoff, &pfn, NULL, &attrs);
> +	r = kvm_gmem_pfn_get(inode, vmf->pgoff, &pfn, NULL, &attrs);
>  	if (r)
>  		return vmf_error(r);

[Severity: High]
Should kvm_gmem_pfn_mkwrite() also call kvm_gmem_pfn_get() instead of
gmem_provider_get_pfn()?

In virt/kvm/guest_memfd.c, kvm_gmem_pfn_mkwrite() still calls
gmem_provider_get_pfn() directly:

static vm_fault_t kvm_gmem_pfn_mkwrite(struct vm_fault *vmf)
{
	struct inode *inode = file_inode(vmf->vma->vm_file);
	vm_fault_t ret = 0;
	kvm_pfn_t pfn;
	u32 attrs;
	int r;

	filemap_invalidate_lock_shared(inode->i_mapping);
	r = gmem_provider_get_pfn(GMEM_I(inode), vmf->pgoff, &pfn, NULL, &attrs);
	...

When userspace writes to a present read-only PTE in a shared VM_PFNMAP VMA,
handle_pte_fault() -> do_wp_page() -> wp_pfn_shared() drops the PTE lock via
pte_unmap_unlock() before invoking vma->vm_ops->pfn_mkwrite(vmf):

mm/memory.c:wp_pfn_shared() {
	...
	pte_unmap_unlock(vmf->pte, vmf->ptl);
	ret = vmf_can_call_fault(vmf);
	if (ret)
		return ret;

	vmf->flags |= FAULT_FLAG_MKWRITE;
	ret = vma->vm_ops->pfn_mkwrite(vmf);
	if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
		return ret;
	return finish_mkwrite_fault(vmf, NULL);
}

In the window before kvm_gmem_pfn_mkwrite() acquires
filemap_invalidate_lock_shared(), a concurrent hole punch via
kvm_gmem_punch_hole() -> __kvm_gmem_revoke() can acquire the invalidate lock
exclusively, unmap the PTE, notify exports to unmap the hole, and release
the backing frame in the provider.

When kvm_gmem_pfn_mkwrite() then acquires the shared invalidate lock and
calls gmem_provider_get_pfn(), the provider allocates a new frame for the
hole without calling kvm_gmem_update_exports(). Afterwards,
finish_mkwrite_fault() sees that the PTE was cleared and retries the fault
through kvm_gmem_fault_user_mapping() -> kvm_gmem_pfn_fault() ->
kvm_gmem_pfn_get(), where lookup_pfn_range() finds the frame already
allocated by kvm_gmem_pfn_mkwrite() and sets hole = false, leaving the
export unmapped.

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