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

Hi Bjorn,

Gentle ping on this RFC.

Sashiko's review points out that pci_try_reset_bus(), pci_slot_reset()
and pci_try_reset_slot() have the same problem, since
__pci_bus_trylock(), __pci_bus_lock() and __pci_bus_unlock() also walk
bus->devices independently for locking and unlocking. This matches the
limitation I noted in the commit message.

The review also mentions a possible use-after-free. In those unpatched
paths, bus->devices is walked without pci_bus_sem and without taking
device references, so a device removed and freed concurrently could be
accessed during the walk. With the snapshot approach this is avoided:
the snapshot is taken under pci_bus_sem, a reference is held on every
device in it, and both locking and unlocking use only the snapshot. A
removed device therefore cannot be freed until the reset completes.

Before extending this to the other paths, I would like to confirm the
direction. I see two options:

1. Extend the snapshot approach to the slot and try-reset paths. The
   change stays small and local, but a device added after the snapshot
   may still be reset without its device lock held.

2. Prevent topology changes for the whole reset. This would also cover
   devices added after the snapshot, but it needs a lock held across the
   reset and care to avoid deadlocks with hotplug and AER recovery.

I lean towards option 1 as a fix that is easier to backport, with
option 2 possibly done separately later. Does that sound reasonable, or
should I go with option 2 directly?

Thanks,
Hongtao