Re: [PATCH RFC v3 71/88] vfio/pci: Implement read and write provider ops for guest_memfd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] vfio/pci: unserialized sub-page BAR access in
  vfio_pci_core_gmem_rw()
- [High] vfio/pci: D3 MMIO access in vfio_pci_core_gmem_rw() when
  runtime PM engaged
--

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

vfio/pci: Implement read and write provider ops for guest_memfd

This commit implements the .gmem_read and .gmem_write provider operations
in vfio-pci to service guest_memfd read and write accesses to mappable
MMIO BARs.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index a9274d95e9066..c976574782663 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2871,6 +2871,42 @@ int vfio_pci_core_gmem_lookup_pfn_range(struct vfio_device *core_vdev,
>  	return -ENOENT;
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_gmem_lookup_pfn_range);
> +
> +static ssize_t vfio_pci_core_gmem_rw(struct vfio_device *core_vdev,
> +				     void *kbuf, size_t count,
> +				     loff_t *ppos, bool iswrite)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> +
> +	if (!count)
> +		return 0;
> +
> +	if (index >= PCI_STD_NUM_BARS || !vdev->bar_mmap_supported[index])
> +		return -EINVAL;

[Severity: High]
Can a sub-page BAR access here race with a reset or power transition?

In vfio_pci_probe_mmaps(), a page-aligned BAR smaller than PAGE_SIZE has
 its remainder reserved via dummy_res and vdev->bar_mmap_supported[bar] set
to true.  Unlike vfio_pci_core_gmem_get_pfn() and
vfio_pci_core_gmem_lookup_pfn_range(), which reject BARs where
pci_resource_len(pdev, bar) >> PAGE_SHIFT is 0, vfio_pci_core_gmem_rw()
allows them and calls vfio_pci_bar_rw_kbuf() with test_mem = false.

Skipping vdev->memory_lock relies on vfio_pci_zap_bars() revoking the BAR
range in guest_memfd so __kvm_gmem_revoke() takes
filemap_invalidate_lock() and drains in-flight kvm_gmem_rw() calls.  For a
sub-page BAR, however, vfio_pci_zap_bars() passes nr_pages = 0:

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_zap_bars() {
	...
			gmem_start = VFIO_PCI_INDEX_TO_OFFSET(i) >> PAGE_SHIFT;
			nr_pages = pci_resource_len(vdev->pdev, i) >> PAGE_SHIFT;
			pfn = pci_resource_start(vdev->pdev, i) >> PAGE_SHIFT;

			vfio_device_gmem_revoke(&vdev->vdev, gmem_start,
						nr_pages, &pfn);
	...
}

When nr_pages is 0, __kvm_gmem_revoke() returns early before acquiring
filemap_invalidate_lock(inode->i_mapping):

CPU0                                  CPU1
kvm_gmem_rw()
  filemap_invalidate_lock_shared()
  vfio_pci_core_gmem_rw()
    vfio_pci_get_bars_available()
                                      vfio_pci_ioctl_reset()
                                        down_write(&vdev->memory_lock)
                                        vfio_pci_zap_bars()
                                          vfio_pci_set_bars_available(false)
                                          __kvm_gmem_revoke(..., nr_pages=0)
                                            return 0;
                                        pci_try_reset_function()
    vfio_pci_bar_rw_kbuf(..., false)
      vfio_pci_core_iowrite()  <-- MMIO during reset

This allows MMIO reads and writes to run concurrently with a PCI reset, a
transition to PCI_D3hot, or clearing PCI_COMMAND_MEMORY, which can cause
Completer Abort / Unsupported Request errors or synchronous external
aborts.

> +
> +	/* Lockless check on availability of BARs. */
> +	if (!vfio_pci_get_bars_available(vdev))
> +		return -EIO;
> +
> +	return vfio_pci_bar_rw_kbuf(vdev, kbuf, count, ppos, iswrite, false);

[Severity: High]
Can vfio_pci_core_gmem_rw() issue MMIO accesses while the device is
runtime-suspended in D3?

Unlike vfio_pci_rw(), vfio_pci_core_gmem_rw() does not call
pm_runtime_resume_and_get() and passes test_mem = false to
vfio_pci_bar_rw_kbuf(), relying on vdev->bars_available to be false while
runtime PM low-power mode is engaged (vdev->pm_runtime_engaged).

When userspace enters low-power mode via
VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY, vfio_pci_runtime_pm_entry() zaps BARs
(setting bars_available = false), sets vdev->pm_runtime_engaged = true,
and drops the runtime PM reference.  If userspace then performs a config
write (or a reset ioctl), vfio_pci_rw() temporarily resumes the device to
D0 for the duration of the call.  Because vdev->pm_wake_eventfd_ctx is
NULL, vfio_pci_core_runtime_resume() leaves vdev->pm_runtime_engaged set
to true, but the config write or reset path re-enables bars_available
without checking !vdev->pm_runtime_engaged:

drivers/vfio/pci/vfio_pci_config.c:vfio_basic_config_write() {
	...
		if (__vfio_pci_memory_enabled(vdev)) {
			vfio_pci_dma_buf_move(vdev, false);
			vfio_pci_set_bars_available(vdev, true);
		}
	...
}

When the config write finishes and calls pm_runtime_put(),
vfio_pci_core_runtime_suspend() suspends the device back to D3 without
clearing bars_available.  Subsequent guest_memfd reads and writes see
vfio_pci_get_bars_available(vdev) == true and perform MMIO against the
device while it is in D3.

Conversely, when waking from VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY_WITH_WAKEUP,
vfio_pci_core_runtime_resume() calls __vfio_pci_runtime_pm_exit() without
restoring bars_available = true, leaving guest_memfd BAR accesses failing
with -EIO after wakeup.

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