Re: [RFC PATCH v3 04/20] vfio/pci: Use access ops for config-space accesses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] vfio/pci: pci_bus_sem lock inversion in config-space FLR under
  access_srcu
--

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

vfio/pci: Use access ops for config-space accesses

This commit runs PCI config-space accesses through vfio_pci_core_access(),
routes power-state transitions through vfio_pci_access_ops, and defers
PCI_PM_CTRL D0 transitions until after leaving access_srcu.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 9914f3ac69aef..1e54e2e8b2ef8 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c

[ ... ]

> @@ -1957,11 +2017,21 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>  		if (copy_from_user(&val, buf, count))
>  			return -EFAULT;
>  
> -		ret = perm->writefn(vdev, *ppos, count, perm, offset, val);
> +		access.val = val;
> +		ret = vfio_pci_core_access(vdev, vfio_pci_access_config, &access);
> +		if (ret < 0)
> +			return ret;
> +		if (access.power_up) {
> +			int err = vfio_lock_and_set_power_state(vdev, PCI_D0);
> +
> +			if (err)
> +				return err;
> +		}

[Severity: High]
Can a config-space FLR write in vfio_exp_config_write() or
vfio_af_config_write() invert the lock order between access_srcu and
pci_bus_sem when the device is in a low-power state?

vfio_pm_config_write() defers D0 transitions outside access_srcu so that
pcie_aspm_pm_state_change() does not block on pci_bus_sem while AER waits
for access_srcu readers.  However, if userspace puts the device into
D1-D3hot via PCI_PM_CTRL and then writes PCI_EXP_DEVCTL_BCR_FLR or
PCI_AF_CTRL_FLR, vfio_pci_core_access() runs the handler inside
access_srcu, where it takes vdev->memory_lock and calls
pci_try_reset_function():

drivers/pci/pci.c:pci_dev_save_and_disable() {
    ...
	pci_set_power_state(dev, PCI_D0);
    ...
}

pci_set_power_state() calls pci_set_full_power_state(dev, false) ->
pcie_aspm_pm_state_change() -> down_read(&pci_bus_sem) while holding both
access_srcu and vdev->memory_lock:

CPU0                              CPU1 (AER recovery)
----                              -------------------
                                  down_read(&pci_bus_sem)
vfio_pci_config_rw_single()
  vfio_pci_core_access()
    srcu_read_lock(&access_srcu)
    ...
    vfio_exp_config_write()
      down_write(&memory_lock)
      pci_try_reset_function()
        ...
        down_read(&pci_bus_sem)
                                  synchronize_srcu(&access_srcu)

 *** DEADLOCK ***

(if a writer is queued on pci_bus_sem, or against any path nesting
pci_bus_sem -> access_srcu / memory_lock)

In addition, letting pci_dev_save_and_disable() perform the D3hot -> D0
transition directly bypasses vfio_pci_try_set_power_state() and skips
restoring vdev->pm_save on NoSoftRst- devices.

Calling vfio_pci_try_set_power_state(vdev, PCI_D0) before
pci_try_reset_function() in vfio_exp_config_write() and
vfio_af_config_write() (as commit 358432f8bbe9 ("vfio/pci: Avoid waiting
for the bus lock when preparing D0") did for vfio_pci_ioctl_reset()), or
deferring the FLR outside access_srcu, would avoid this.

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