Re: [RFC PATCH 12/19] vfio/pci: Serialize physical device information queries with recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] VFIO_DEVICE_GET_INFO fails completely during device recovery, contradicting the commit message's intent.

Pre-existing issues:
- [High] Concurrent read-modify-write race on the Physical Function's PCI_EXP_DEVCTL2 register when enabling AtomicOps.
--

commit 7eea0a295d5111634553157702a7e697ec8e62ec
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize physical device information queries with recovery

This commit wraps the AtomicOps capability discovery in VFIO_DEVICE_GET_INFO
with recovery_lock. The lock ensures we do not attempt to read the physical
device configuration space while a PCI recovery is in progress.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1179,7 +1179,13 @@ static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
>  		return ret;
>  	}
>  
> +	ret = vfio_pci_core_access_begin(vdev);
> +	if (ret) {
> +		kfree(caps.buf);
> +		return ret;

[Severity: High]
Does this break the availability of the cached information mentioned in the
commit message?

The commit message states:
"The rest of what VFIO_DEVICE_GET_INFO and VFIO_DEVICE_GET_REGION_INFO report
is cached or software only, and remains available without taking recovery_lock."

However, if vfio_pci_core_access_begin() fails here (for example, returning
-EIO when a recovery is in progress and access is blocked), we abort the
entire ioctl. Doesn't this deprive userspace of the other cached information
that was supposed to remain available?

Could we skip gathering the atomic capability on access failure rather than
aborting the entire ioctl?

> +	}
>  	ret = vfio_pci_info_atomic_cap(vdev, &caps);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent ioctls on
multiple Virtual Functions trigger a lockless read-modify-write race on the
shared Physical Function hardware register?

In vfio_pci_info_atomic_cap(), we call pci_enable_atomic_ops_to_root() on the
Physical Function device (retrieved via pci_physfn()). That function modifies
the PF's config space:

pci_enable_atomic_ops_to_root()
    pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ATOMIC_REQ);

This performs a lockless read-modify-write sequence on the shared PF hardware
register. Because down_read(&vdev->recovery_lock) only acquires a read lock
on the local VF's rw_semaphore, it provides no mutual exclusion across
different VFs.

If userspace concurrently issues VFIO_DEVICE_GET_INFO ioctls on multiple VFs
of the same PF, could this clobber concurrent configuration updates like ARI
or LTR toggles on the PF?

> +	vfio_pci_core_access_end(vdev);
>  	if (ret && ret != -ENODEV) {
>  		pci_warn(vdev->pdev,
>  			 "Failed to setup AtomicOps info capability\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=12