Re: [PATCH RFC v3 60/88] vfio/pci: Implement get_pfn provider op for guest_memfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] vfio/pci: sub-page mappable BARs rejected in
  vfio_pci_core_gmem_get_pfn()
- [Medium] vfio/pci: missing vfio_pci_core_get_iomap() check in
  vfio_pci_core_gmem_get_pfn()
--

commit 1661618156a57f40eb0f4fe51d0d5dccb4378f2f
Author: Ackerley Tng <ackerleytng@google.com>

vfio/pci: Implement get_pfn provider op for guest_memfd

This commit implements the .gmem_get_pfn callback for vfio-pci to translate
guest_memfd page offsets into physical PCI BAR frame numbers.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index c7b67fbce4cae..9551a067b5689 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2731,6 +2731,44 @@ void vfio_pci_core_gmem_release(struct vfio_device *core_vdev)
>  	up_write(&vdev->memory_lock);
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_gmem_release);
> +
> +int vfio_pci_core_gmem_get_pfn(struct vfio_device *core_vdev, pgoff_t index,
> +			       unsigned long *pfn, int *max_order,
> +			       u64 *pfn_attrs)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +	struct pci_dev *pdev = vdev->pdev;
> +	unsigned int bar;
> +	u64 pgoff;
> +
> +	/*
> +	 * Lockless check: if memory decoding is disabled or the device is in
> +	 * low-power D3, fail immediately without blocking on memory_lock.
> +	 */
> +	if (!smp_load_acquire(&vdev->bars_available))
> +		return -EFAULT;
> +
> +	bar = index >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
> +	pgoff = index & ((1ULL << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
> +
> +	if (bar >= PCI_STD_NUM_BARS || !vdev->bar_mmap_supported[bar])
> +		return -EINVAL;

[Severity: Medium]
Does vfio_pci_core_gmem_get_pfn() also need to check
vfio_pci_core_get_iomap(vdev, bar) before returning the BAR PFN?

vfio_pci_probe_mmaps() sets vdev->bar_mmap_supported[bar] based on resource
flags and alignment alone, while vfio_pci_core_map_bars() allows
pci_request_selected_regions() and pci_iomap() to fail softly by recording an
ERR_PTR in vdev->barmap[bar]:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_map_bars() {
	...
		if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
			pci_dbg(pdev, "Failed to reserve region %d\n", bar);
			vdev->barmap[bar] = IOMEM_ERR_PTR(-EBUSY);
			continue;
		}
	...
}

Unlike vfio_pci_core_mmap() and vfio_pci_core_feature_dma_buf(), which verify
!IS_ERR(vfio_pci_core_get_iomap()), skipping that check here lets guest_memfd
map physical BAR frames that VFIO failed to reserve (for example, when owned
by another host driver such as efifb).

> +
> +	if (pgoff >= (pci_resource_len(pdev, bar) >> PAGE_SHIFT))
> +		return -EINVAL;

[Severity: Medium]
Does shifting pci_resource_len(pdev, bar) without PAGE_ALIGN() reject
page-aligned sub-page BARs?

When a BAR is smaller than PAGE_SIZE (for example, a 4 KiB BAR on a 64 KiB
PAGE_SIZE kernel) and page-aligned, vfio_pci_probe_mmaps() reserves the rest
of the host page with a dummy resource and sets
vdev->bar_mmap_supported[bar] = true.  Unlike vfio_pci_core_mmap(), which
computes PAGE_ALIGN(pci_resource_len(pdev, index)),
pci_resource_len(pdev, bar) >> PAGE_SHIFT evaluates to 0 for these BARs and
causes even pgoff == 0 to fail with -EINVAL.  Because bar_mmap_supported[bar]
is true, vfio_pci_core_is_ctrl_plane() also returns false and blocks
read()/write() on a VFIO_DEVICE_BIND_FLAG_CTRL_ONLY descriptor, leaving the
BAR inaccessible.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-gmem-alternative-providers-v3-0-8484dde66a7e@google.com?part=60