Re: PCI: Prevent device lock leak during bus reset

Re: PCI: Prevent device lock leak during bus reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch fixes pci_bus_reset while leaving identical lock leak and Use-After-Free races unmitigated in pci_try_reset_bus, pci_slot_reset, and pci_try_reset_slot.
--

--- Patch [1]: PCI: Prevent device lock leak during bus reset ---
commit 0d6c89d5cec5eebb33879246fbc7b446f026613a
Author: Zhang Hongtao <zhanghongtao35@huawei.com>

PCI: Prevent device lock leak during bus reset

This patch fixes a device lock leak in pci_bus_reset() caused by
concurrent device removal. It takes a topology snapshot under pci_bus_sem
to guarantee that all locked devices are unlocked during a secondary bus reset.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..25a1e44263c32 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5585,21 +5667,31 @@ static int pci_try_reset_slot(struct pci_slot *slot)
>  
>  static int pci_bus_reset(struct pci_bus *bus, bool probe)
>  {
> +	struct pci_bus_lock_context context;
>  	int ret;
>  
> +	down_read(&pci_bus_sem);
> +
>  	if (!bus->self || !pci_bus_resettable(bus))
> -		return -ENOTTY;
> +		ret = -ENOTTY;
> +	else if (probe)
> +		ret = 0;
> +	else
> +		ret = pci_bus_lock_snapshot_init(bus, &context);
>  
> -	if (probe)
> -		return 0;
> +	up_read(&pci_bus_sem);
> +
> +	if (ret || probe)
> +		return ret;
>  
> -	pci_bus_lock(bus);
> +	pci_bus_lock_snapshot(&context);
>  
>  	might_sleep();
>  
>  	ret = pci_bridge_secondary_bus_reset(bus->self);
>  
> -	pci_bus_unlock(bus);
> +	pci_bus_unlock_snapshot(&context);
> +	pci_bus_lock_snapshot_release(&context);
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but regarding the RFC question in the commit
message about extending this fix:

The identical lock leaks and use-after-free races remain in the slot and
try-reset paths, as they rely on __pci_bus_lock(), __pci_bus_unlock(), and
__pci_bus_trylock().

For example, __pci_bus_trylock() iterates over bus->devices without 
holding pci_bus_sem:

drivers/pci/pci.c:__pci_bus_trylock() {
    ...
    list_for_each_entry(dev, &bus->devices, bus_list) {
        if (slot && (!dev->slot || dev->slot != slot))
            continue;
...
}

If a concurrent device removal executes list_del(&dev->bus_list) during
pci_try_reset_bus(), pci_slot_reset(), or pci_try_reset_slot(), the unlock
loop fails to find the device. This leaves the device permanently locked,
causing the same system hangs in driver bind/unbind paths.

Since these paths are reachable via PCIe AER recovery or hotplug events,
would it be safer to extend this snapshot mechanism to the slot and
try-reset paths to fully close this race condition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821021626.1874602-1-zhanghongtao35@huawei.com?part=1

PCI: Prevent device lock leak during bus reset

[Zhang Hongtao]

pci_bus_lock() and pci_bus_unlock() independently walk the devices below
a bus.  The topology may change between the walks because pci_bus_sem is
not held across the reset.

This causes a device lock leak when AER recovery, device removal, and
driver bind and unbind operations run concurrently.  The relevant order
is:

  bind/unbind                 remove                 AER recovery
  --------------------------------------------------------------------
  bus_find_device_by_name()
                              device_del()
                                                     pci_bus_lock()
                              list_del(&dev->bus_list)
                                                     bus reset
                                                     pci_bus_unlock()
  device_lock()

pci_bus_lock() locks the device before the Secondary Bus Reset.  After
pci_destroy_dev() removes the device from bus->devices, pci_bus_unlock()
no longer finds the device and therefore does not unlock it.  The bind
and unbind paths retain references obtained by bus_find_device_by_name(),
so they can subsequently reach device_lock() and wait indefinitely for
the leaked lock.

The race was reproduced consistently on QEMU Q35 with an e1000e endpoint
and a mainline-based kernel:

  7.2.0-rc4-00366-gf9cf390f34eb

Artificial delays after bus_find_device_by_name(), pci_bus_lock(), and
device_del() widened the race windows.  Concurrent bind, unbind, and
remove operations were started, followed by an injected Data Link
Protocol AER error using CONFIG_PCIEAER_INJECT.  The hung task detector
reported both device_driver_attach() and device_release_driver_internal()
waiting on the device mutex, likely owned by irq/24-aerdrv.

Take a topology snapshot under pci_bus_sem and hold a reference to every
device in it.  Drop pci_bus_sem before acquiring device locks, then use
the snapshot for both locking and unlocking.  This guarantees that every
device lock acquired by pci_bus_reset() is released even if a device is
removed from bus->devices during the reset.

The fix was tested on commit d326f83e819c ("Merge tag 'net-7.2-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net") with the same
forced ordering.  The device was removed from bus->devices between the
reset lock and unlock markers, after which device_driver_attach()
completed and no hung task occurred.

This is intentionally a limited fix.  It makes lock and unlock symmetric,
but does not protect the topology for the entire reset.  In particular, a
device added after the snapshot may be reset without its device lock held.
Similar independent walks also exist in the slot and try-reset paths.  This
RFC seeks feedback on whether the snapshot should be extended to those
paths or reset should use a stronger topology exclusion mechanism.

Fixes: 090a3c5322e9 ("PCI: Add pci_reset_slot() and pci_reset_bus()")
Signed-off-by: Zhang Hongtao <zhanghongtao35@huawei.com>
---
 drivers/pci/pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 77b17b13ee61..25a1e44263c3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5409,6 +5409,88 @@ static int pci_bus_trylock(struct pci_bus *bus)
 	return __pci_bus_trylock(bus, NULL);
 }
 
+struct pci_bus_lock_context {
+	struct pci_dev **devs;
+	size_t nr_devs;
+};
+
+static size_t pci_bus_lock_count(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+	size_t count = 1;
+
+	lockdep_assert_held(&pci_bus_sem);
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		if (dev->subordinate)
+			count += pci_bus_lock_count(dev->subordinate);
+		else
+			count++;
+	}
+
+	return count;
+}
+
+static void pci_bus_lock_fill(struct pci_bus *bus,
+			      struct pci_bus_lock_context *context,
+			      size_t *index)
+{
+	struct pci_dev *dev;
+
+	lockdep_assert_held(&pci_bus_sem);
+
+	context->devs[(*index)++] = pci_dev_get(bus->self);
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		if (dev->subordinate)
+			pci_bus_lock_fill(dev->subordinate, context, index);
+		else
+			context->devs[(*index)++] = pci_dev_get(dev);
+	}
+}
+
+static int pci_bus_lock_snapshot_init(struct pci_bus *bus,
+				      struct pci_bus_lock_context *context)
+{
+	size_t index = 0;
+
+	lockdep_assert_held(&pci_bus_sem);
+
+	context->nr_devs = pci_bus_lock_count(bus);
+	context->devs = kvmalloc_array(context->nr_devs,
+				       sizeof(*context->devs), GFP_KERNEL);
+	if (!context->devs)
+		return -ENOMEM;
+
+	pci_bus_lock_fill(bus, context, &index);
+
+	return 0;
+}
+
+static void pci_bus_lock_snapshot(struct pci_bus_lock_context *context)
+{
+	size_t i;
+
+	for (i = 0; i < context->nr_devs; i++)
+		pci_dev_lock(context->devs[i]);
+}
+
+static void pci_bus_unlock_snapshot(struct pci_bus_lock_context *context)
+{
+	size_t i;
+
+	for (i = context->nr_devs; i > 0; i--)
+		pci_dev_unlock(context->devs[i - 1]);
+}
+
+static void pci_bus_lock_snapshot_release(struct pci_bus_lock_context *context)
+{
+	size_t i;
+
+	for (i = 0; i < context->nr_devs; i++)
+		pci_dev_put(context->devs[i]);
+	kvfree(context->devs);
+}
+
 /* Do any devices on or below this slot prevent a bus reset? */
 static bool pci_slot_resettable(struct pci_slot *slot)
 {
@@ -5585,21 +5667,31 @@ static int pci_try_reset_slot(struct pci_slot *slot)
 
 static int pci_bus_reset(struct pci_bus *bus, bool probe)
 {
+	struct pci_bus_lock_context context;
 	int ret;
 
+	down_read(&pci_bus_sem);
+
 	if (!bus->self || !pci_bus_resettable(bus))
-		return -ENOTTY;
+		ret = -ENOTTY;
+	else if (probe)
+		ret = 0;
+	else
+		ret = pci_bus_lock_snapshot_init(bus, &context);
 
-	if (probe)
-		return 0;
+	up_read(&pci_bus_sem);
+
+	if (ret || probe)
+		return ret;
 
-	pci_bus_lock(bus);
+	pci_bus_lock_snapshot(&context);
 
 	might_sleep();
 
 	ret = pci_bridge_secondary_bus_reset(bus->self);
 
-	pci_bus_unlock(bus);
+	pci_bus_unlock_snapshot(&context);
+	pci_bus_lock_snapshot_release(&context);
 
 	return ret;
 }

base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86
-- 
2.34.1