[PATCH] PCI: Keep an already-configured bridge's bus number across a rescan

[PATCH] PCI: Keep an already-configured bridge's bus number across a rescan

[Jianjun Wang]

When pcibios_assign_all_busses() is set, pci_scan_bridge_extend() assigns
a bridge's secondary bus number with

	next_busnr = max + 1;

This works correctly during the initial enumeration: the bridges are
visited in order, none of them has a child bus yet, and assigning max + 1
as each one is scanned yields sensible, increasing numbers.

A rescan is different. Bridges that were already present still hold their
child bus (dev->subordinate), and for_each_pci_bridge() iterates
bus->devices, which is built with list_add_tail(). A bridge that was
removed and is later rediscovered during the rescan is therefore appended
to the tail of the list rather than kept in bus-number order.

This is where the positional "max + 1" breaks down. A bridge that still
owns its old child bus can be assigned a number that another bridge is
already using. pci_find_bus() then returns the sibling's bus, the
"if (!child)" allocation is skipped, and the bridge takes over the
sibling's child bus while the sibling is given a newly created one. The
two subtrees end up swapped, and the endpoint behind the reused number is
scanned a second time.

If a bridge already has a child bus, it is being rescanned rather than
enumerated for the first time, so there is no need to renumber it; keep
the number it already holds. Bridges without a subordinate still get
max + 1 as before, the initial scan is unaffected because no bridge has a
child bus yet, and platforms that do not reassign all buses take the
preserve path earlier and never reach this code.

Signed-off-by: Jianjun Wang <jianjun.wang@linux.alibaba.com>
---
Reproduced on a QEMU RISC-V "virt" guest whose host bridge uses
pci-host-generic, so pcibios_assign_all_busses() is effectively true.
The machine has three PCIe root ports, each with an endpoint behind it:

  -[0000:00]-+-00.0
             +-01.0-[01]----00.0   (network controller)
             +-02.0-[02]----00.0   (NVMe controller)
             \-03.0-[03]----00.0   (NVMe controller)

Remove the middle root port and rescan:

  # echo 1 > /sys/bus/pci/devices/0000:00:02.0/remove
  # echo 1 > /sys/bus/pci/rescan

Without this patch the two lower subtrees are swapped, 00:02.0 and
00:03.0 now point at each other's bus, and the endpoint that gets
enumerated a second time fails to probe:

  -[0000:00]-+-00.0
             +-01.0-[01]----00.0
             +-02.0-[03]--
             \-03.0-[02]--+-00.0
                          \-00.0

  nvme nvme0: Duplicate cntlid 0 with nvme1, subsys ..., rejecting
  nvme 0000:02:00.0: probe failed with error -22

With the patch the already-present bridges (01 and 03) keep their bus
numbers, only the re-added bridge gets a fresh one, the topology stays
sane and the endpoint probes normally:

  -[0000:00]-+-00.0
             +-01.0-[01]----00.0
             +-02.0-[04]----00.0
             \-03.0-[03]----00.0

Note the re-added bridge is numbered 04 rather than reusing the freed
02: renumbering only ever hands out max + 1, so once the preserved
bridges have advanced max there is no attempt to reclaim the hole. That
is cosmetic; the numbering is valid and no longer conflicts.

 drivers/pci/probe.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 27008e2ea5af..6b418d20cde3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1513,6 +1513,18 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
 		fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
 		if (fixed_buses)
 			next_busnr = fixed_sec;
+		else if (dev->subordinate)
+			/*
+			 * This bridge already has a child bus, so we are
+			 * rescanning an already-configured bridge (e.g. after a
+			 * sibling bridge was removed and the bus was rescanned).
+			 * Keep its existing bus number instead of renumbering
+			 * it: a re-added sibling is appended to the tail of the
+			 * device list, so handing out max + 1 here would let that
+			 * later sibling steal this bridge's number and swap the
+			 * two subtrees.
+			 */
+			next_busnr = dev->subordinate->busn_res.start;
 		else
 			next_busnr = max + 1;
 
-- 
2.25.1

Re: [PATCH] PCI: Keep an already-configured bridge's bus number across a rescan

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: bus number overlap and integer underflow in used_buses on rescan
- [High] pci: bus number leak and integer overflow truncating bus to 0
--

commit 327bbd815883ab87700e0adf3427c36a03e138ab
Author: Jianjun Wang <jianjun.wang@linux.alibaba.com>

PCI: Keep an already-configured bridge's bus number across a rescan

This commit changes pci_scan_bridge_extend() to reuse an existing child bus
number across rescans instead of assigning max + 1. This prevents bridges
from taking over a sibling's child bus and swapping subtrees when they are
re-discovered out of bus-number order.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5afc..6b418d20cde34 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1513,6 +1513,18 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
>  		fixed_buses = pci_ea_fixed_busnrs(dev, &fixed_sec, &fixed_sub);
>  		if (fixed_buses)
>  			next_busnr = fixed_sec;
> +		else if (dev->subordinate)

[Severity: High]
Does preserving the existing bus number here cause a regression where bus
numbers permanently leak during repeated hot-removes and rescans?

If preserved bridges are never renumbered down to close gaps, and new
bridges are always assigned max + 1, max will only go up and eventually
exceed 255. When next_busnr hits 256, does pci_find_bus() implicitly
truncate it to 0 (due to pci_do_find_bus()'s unsigned char argument), finding
the root bus instead?

If child evaluates to the root bus, it could misconfigure the new bridge
with root bus parameters later in pci_scan_bridge_extend():

	buses = (buses & PCI_SEC_LATENCY_TIMER_MASK) |
		FIELD_PREP(PCI_PRIMARY_BUS_MASK, child->primary) |
		FIELD_PREP(PCI_SECONDARY_BUS_MASK, child->busn_res.start) |
		FIELD_PREP(PCI_SUBORDINATE_BUS_MASK, child->busn_res.end);

> +			/*
> +			 * This bridge already has a child bus, so we are
> +			 * rescanning an already-configured bridge (e.g. after a
> +			 * sibling bridge was removed and the bus was rescanned).
> +			 * Keep its existing bus number instead of renumbering
> +			 * it: a re-added sibling is appended to the tail of the
> +			 * device list, so handing out max + 1 here would let that
> +			 * later sibling steal this bridge's number and swap the
> +			 * two subtrees.
> +			 */
> +			next_busnr = dev->subordinate->busn_res.start;

[Severity: High]
If a hotplug event expands a preceding bridge's subordinate range during
rescan, the global max bus number increases. Does this unconditionally reuse
the old subordinate->busn_res.start as next_busnr, which may now be less
than max?

This regression would cause the local max returned by
pci_scan_bridge_extend() to shrink. In the caller
pci_scan_child_bus_extend(), can max - cmax then severely underflow?

	if (max - cmax > 1)
		used_buses += max - cmax - 1;

This underflow could break hotplug bus distribution and corrupt the PCI
topology.

>  		else
>  			next_busnr = max + 1;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924054115.75574-1-jianjun.wang@linux.alibaba.com?part=1