Re: [PATCH RFC] PCI: pciehp: Fix AB-BA deadlocks between reset_lock, pci_rescan_remove_lock and pci_slot_mutex

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Self-deadlock in `pci_reset_bus()` due to unconditionally acquiring `pci_rescan_remove_lock` during device enumeration.
- [High] AB-BA deadlock between `pci_rescan_remove_lock` and VFIO's `dev_set->lock`.
--

--- Patch [1]: [PATCH RFC] PCI: pciehp: Fix AB-BA deadlocks between reset_lock, pci_rescan_remove_lock and pci_slot_mutex ---
commit 0ee14af4875a74717aa4b029ffaaeebbfbfc90a8
Author: Qiang Yu <[email protected]>

PCI: pciehp: Fix AB-BA deadlocks between reset_lock, pci_rescan_remove_lock and pci_slot_mutex

The commit attempts to fix deadlocks involving reset_lock and device_lock
during PCIe hotplug operations by releasing reset_lock across the rescan
and remove sections. It also modifies pci_reset_bridge() and pci_reset_bus()
to serialize with pci_rescan_remove_lock, while introducing an unlocked
variant pci_reset_bus_unlocked() for specific paths.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..9010741d032f7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5759,19 +5770,62 @@ int pci_probe_reset_bus(struct pci_bus *bus)
>  int pci_reset_bus(struct pci_dev *pdev)
>  {
> -	return (!pci_probe_reset_slot(pdev->slot)) ?
> -	    pci_try_reset_slot(pdev->slot) : pci_try_reset_bus(pdev->bus);
> +	int rc;
> +
> +	/*
> +	 * pci_try_reset_slot()/pci_try_reset_bus() below may issue a
> +	 * Secondary Bus Reset, which races with concurrent bus scanning.
> +	 * Serialize against that via pci_rescan_remove_lock, taken before
> +	 * the slot's/bus's device locks to match the lock order used by
> +	 * pciehp.
> +	 */
> +	pci_lock_rescan_remove();

[Severity: High]
Does unconditionally acquiring pci_rescan_remove_lock here introduce a
self-deadlock during device enumeration?

pci_bus_add_devices() is called with pci_rescan_remove_lock held from paths
like pciehp_configure_device() or acpi_pci_root_add(). It synchronously
invokes pci_fixup_device(pci_fixup_final, dev) and driver probes.

If a newly added device triggers a final quirk (like
quirk_reset_lenovo_thinkpad_p50_nvgpu) or a driver probe that calls
pci_reset_bus(), it will attempt to acquire the non-recursive
pci_rescan_remove_lock again:

Thread 1
pciehp_configure_device()
  pci_lock_rescan_remove()
  pci_bus_add_devices()
    pci_bus_add_device()
      pci_fixup_device(pci_fixup_final, dev)
        quirk_reset_lenovo_thinkpad_p50_nvgpu()
          pci_reset_bus()
            pci_lock_rescan_remove() <--- Deadlock

[Severity: High]
Can this introduce an AB-BA deadlock between pci_rescan_remove_lock and VFIO's
dev_set->lock?

If a VFIO hot reset runs concurrently with a PCI hotplug event or driver bind:

Thread 1 (VFIO hot reset ioctl)
vfio_pci_dev_set_hot_reset()
  mutex_lock(&dev_set->lock)
  pci_reset_bus()
    pci_lock_rescan_remove() <--- Blocked on Thread 2

Thread 2 (PCI hotplug)
pciehp_configure_device()
  pci_lock_rescan_remove()
  pci_bus_add_devices()
    driver probe (vfio_pci_core_register_device)
      vfio_assign_device_set()
        mutex_lock(&dev_set->lock) <--- Blocked on Thread 1

> +
> +	rc = __pci_reset_bus(pdev);
> +
> +	pci_unlock_rescan_remove();
> +
> +	return rc;
>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bus);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1