| Message ID | 20260901093217.8539-13-skolothumtho@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/pci: Handle PCI error recovery and report state to userspace | expand |
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");
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 95884e713a4b..4447967413e7 100644 --- 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; + } ret = vfio_pci_info_atomic_cap(vdev, &caps); + vfio_pci_core_access_end(vdev); if (ret && ret != -ENODEV) { pci_warn(vdev->pdev, "Failed to setup AtomicOps info capability\n");
Hold recovery_lock for reading around AtomicOps capability discovery, which reads config space to work out what the device supports. 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. Assisted-by: Claude:claude-opus-5 Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com> --- drivers/vfio/pci/vfio_pci_core.c | 6 ++++++ 1 file changed, 6 insertions(+)