Re: [RFC PATCH v3 11/20] vfio/pci: Use access ops for hardware information queries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] vfio/pci: VFIO_DEVICE_GET_INFO fails with -EIO on VF when PF is
  closed
- [Medium] vfio/pci: lockdep circular locking dependency in
  vfio_pci_access_atomic_cap()
--

commit 8a20ca42b1e7365162ac6cecdd060d990d426a4f
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Use access ops for hardware information queries

This commit routes AtomicOps and ROM region hardware information queries
through VFIO PCI access ops so they participate in device error recovery.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index fc6b2673be88c..da7166669dc75 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1178,6 +1178,33 @@ static int vfio_pci_info_atomic_cap(struct vfio_pci_core_device *vdev,
>  	return vfio_info_add_capability(caps, &cap.header, sizeof(cap));
>  }
>  
> +static long vfio_pci_access_atomic_cap_target(struct vfio_pci_core_device *vdev,
> +					      void *data)
> +{
> +	return vfio_pci_info_atomic_cap(vdev, data);
> +}
> +
> +static long vfio_pci_access_atomic_cap(struct vfio_pci_core_device *vdev,
> +				       void *data)
> +{
> +	struct vfio_pci_core_device *pf_vdev = vdev->sriov_pf_core_dev;
> +
> +	if (!pf_vdev || !pf_vdev->pci_recovery_supported)
> +		return vfio_pci_info_atomic_cap(vdev, data);
> +
> +	/*
> +	 * VF AtomicOps queries read the PF's configuration. The PF association
> +	 * lasts until VF removal, but its SRCU instance exists only while
> +	 * the PF is open.
> +	 */
> +	guard(mutex)(&pf_vdev->access_lock);

[Severity: Medium]
Can locking pf_vdev->access_lock here trigger a lockdep circular locking
warning?

vfio_pci_ioctl_get_info() calls vfio_pci_core_access() on the VF, which
takes srcu_read_lock(&vdev->access_srcu) before entering
vfio_pci_access_atomic_cap() and acquiring pf_vdev->access_lock.  Because
every struct vfio_pci_core_device initializes access_lock and access_srcu
at the same mutex_init() and init_srcu_struct() call sites, the VF and PF
share lockdep classes.

Meanwhile, vfio_pci_core_close_device() holds vdev->access_lock while
vfio_pci_core_block_access() calls synchronize_srcu(&vdev->access_srcu),
establishing the opposite access_lock -> access_srcu class order:

CPU0                              CPU1
----                              ----
lock(&vdev->access_lock)
                                  srcu_read_lock(&vdev->access_srcu)
                                  lock(&pf_vdev->access_lock)
synchronize_srcu(&vdev->access_srcu)

 *** DEADLOCK ***

CPU0: vfio_pci_core_close_device() -> vfio_pci_core_block_access()
CPU1: vfio_pci_ioctl_get_info() -> vfio_pci_core_access() ->
      vfio_pci_access_atomic_cap()

> +	if (!pf_vdev->device_open)
> +		return -EIO;
> +
> +	return vfio_pci_core_access(pf_vdev, vfio_pci_access_atomic_cap_target,
> +				    data);
> +}
> +
>  static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
>  				   struct vfio_device_info __user *arg)
>  {
> @@ -1209,10 +1236,12 @@ static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
>  		return ret;
>  	}
>  
> -	ret = vfio_pci_info_atomic_cap(vdev, &caps);
> +	ret = vfio_pci_core_access(vdev, vfio_pci_access_atomic_cap, &caps);
>  	if (ret && ret != -ENODEV) {
> -		pci_warn(vdev->pdev,
> -			 "Failed to setup AtomicOps info capability\n");
> +		if (ret != -EIO)
> +			pci_warn(vdev->pdev,
> +				 "Failed to setup AtomicOps info capability\n");
> +		kfree(caps.buf);
>  		return ret;
>  	}

[Severity: High]
Does this cause VFIO_DEVICE_GET_INFO on a VF to fail with -EIO whenever its
recovery-capable vfio-pci PF is not currently open in userspace?

A PF bound to vfio-pci does not need to stay open while its VFs are in use;
for example, userspace can open the PF only long enough to configure
vf_token and then close the PF fd before opening VFs, while
vfio_pci_core_sriov_configure() keeps the PF in PCI_D0.  When
pf_vdev->device_open is false, vfio_pci_access_atomic_cap() returns -EIO
and vfio_pci_ioctl_get_info() aborts the entire ioctl, breaking
VFIO_DEVICE_GET_INFO on healthy VFs even if AtomicOps are not supported.

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