Re: [RFC PATCH v3 15/20] vfio/pci: Use access ops for SR-IOV configuration and shared reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] vfio/pci: vfio_pci_dev_set_hot_reset() calls D0 helper on
  failed devices
- [Medium] vfio/pci: vfio_pci_prepare_hot_reset_blocked() misses host
  recovery and caller checks
- [Medium] vfio/pci: unsynchronized open and recovery state in
  vfio_pci_core_sriov_configure()
--

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

vfio/pci: Use access ops for SR-IOV configuration and shared reset

This commit routes SR-IOV configuration and shared hot-reset preparation
through vfio_pci_access_ops so that recovering open devices are rejected
while unopened or failed devices remain allowed.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 89cebe2522ba0..0ef51810254ea 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -334,10 +339,21 @@ static void vfio_pci_restore_dmabufs_blocked(struct vfio_pci_core_device *vdev)
>  {
>  }
>  
> +static int vfio_pci_sriov_configure_blocked(struct vfio_pci_core_device *vdev,
> +					    int nr_virtfn)
> +{
> +	/* SR-IOV can be configured without an open VFIO device. */
> +	if (!READ_ONCE(vdev->device_open))
> +		return vfio_pci_sriov_configure_live(vdev, nr_virtfn);
> +	return -EIO;
> +}

[Severity: Medium]
Can vfio_pci_core_sriov_configure() race with device open, close, or host
PCI recovery because neither vdev->access_ops nor vdev->device_open is
serialized by device_lock(&vdev->pdev->dev)?

vfio_pci_core_finish_enable() and vfio_pci_core_close_device() update
vdev->device_open and vdev->access_ops under vdev->access_lock without
holding device_lock(&vdev->pdev->dev):

CPU0                                    CPU1
vfio_pci_core_finish_enable()
  WRITE_ONCE(vdev->device_open, true)
                                        vfio_pci_core_sriov_configure()
                                          vfio_pci_access_ops(vdev)
                                            /* sees blocked_ops */
                                          vfio_pci_sriov_configure_blocked()
                                            READ_ONCE(vdev->device_open)
                                            --> returns -EIO
  rcu_assign_pointer(vdev->access_ops,
                     &vfio_pci_live_ops)

In the reverse direction, vfio_pci_core_close_device() sets
vdev->device_open = false before vfio_pci_core_disable() finishes tearing
down the PF, and pcie_do_recovery() drops device_lock(&pdev->dev) between
recovery callbacks while vdev->pci_recovery_host_active is true.  Because
vfio_pci_sriov_configure_blocked() only checks !READ_ONCE(vdev->device_open)
without taking vdev->access_lock or checking vdev->pci_recovery_host_active,
sysfs sriov_numvfs can run vfio_pci_sriov_configure_live() while close
teardown or host recovery is in progress.

[ ... ]

> @@ -2776,6 +2801,23 @@ static int vfio_pci_dev_set_pm_runtime_get(struct vfio_device_set *dev_set)
>  	return ret;
>  }
>  
> +static int vfio_pci_prepare_hot_reset(struct vfio_pci_core_device *vdev)
> +{
> +	lockdep_assert_held_write(&vdev->memory_lock);
> +	vfio_pci_dma_buf_move(vdev, true);
> +	vfio_pci_zap_bars(vdev);
> +	return 0;
> +}
> +
> +static int vfio_pci_prepare_hot_reset_blocked(struct vfio_pci_core_device *vdev)
> +{
> +	/* Unopened and failed devices must not prevent a shared reset. */
> +	if (READ_ONCE(vdev->device_open) &&
> +	    !(READ_ONCE(vdev->pci_recovery_flags) & VFIO_PCI_RECOVERY_FAILED))
> +		return -EBUSY;
> +	return vfio_pci_prepare_hot_reset(vdev);
> +}

[Severity: Medium]
Can vfio_pci_prepare_hot_reset_blocked() allow pci_reset_bus() to run
while host PCI recovery is active or when VFIO_DEVICE_PCI_HOT_RESET is
called directly on a failed device FD?

When host recovery is running on an unopened sibling, or on a sibling that
was closed during recovery (where vfio_pci_core_close_device() clears
device_open and pci_recovery_flags while keeping pci_recovery_host_active
set), vfio_pci_prepare_hot_reset_blocked() sees !vdev->device_open and
allows userspace to trigger pci_reset_bus() concurrently with host
recovery.  In addition, unlike vfio_pci_ioctl_reset(),
vfio_pci_ioctl_pci_hot_reset() does not check whether the calling device FD
itself has blocked access, so invoking VFIO_DEVICE_PCI_HOT_RESET on a
device with VFIO_PCI_RECOVERY_FAILED succeeds and resets the bus.

>  static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  				      struct vfio_pci_group_info *groups,
>  				      struct iommufd_ctx *iommufd_ctx)
> @@ -2849,8 +2891,11 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  			break;
>  		}
>  
> -		vfio_pci_dma_buf_move(vdev, true);
> -		vfio_pci_zap_bars(vdev);
> +		ret = vfio_pci_access_ops(vdev)->prepare_hot_reset(vdev);
> +		if (ret) {
> +			up_write(&vdev->memory_lock);
> +			break;
> +		}
>  	}

[Severity: Medium]
When a failed sibling passes vfio_pci_prepare_hot_reset_blocked(), does the
subsequent loop in vfio_pci_dev_set_hot_reset() still call
vfio_pci_try_set_power_state(vdev, PCI_D0) directly on that failed device?

drivers/vfio/pci/vfio_pci_core.c:vfio_pci_dev_set_hot_reset() {
    ...
	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list) {
		ret = vfio_pci_try_set_power_state(vdev, PCI_D0);
		if (ret)
			goto out_unlock_memory;
	}

If an open sibling in VFIO_PCI_RECOVERY_FAILED is in a non-D0 state and
inaccessible, vfio_pci_try_set_power_state() fails with -EIO and aborts
the shared reset for the healthy siblings.  If the failed sibling was in
PCI_D3hot with vdev->needs_pm_restore set and responds to the transition,
__vfio_pci_set_power_state() loads vdev->pm_save and calls
pci_restore_state(), re-enabling the pre-failure PCI_COMMAND (including
bus mastering and memory decoding) on the failed device before
pci_reset_bus() saves and restores config space.

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