Re: [RFC PATCH v2 2/2] PCI: Allow hpbussize for specific hotplug bridges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: child bus extension in pci_scan_child_bus_extend() steals sibling buses
- [Medium] PCI: unsigned underflow clamping buses in pci_scan_child_bus_extend()
--

commit be51a5abd9a8e4940755184950c4647a492d014d
Author: Maciej Grochowski <maciej.grochowski@sony.com>

PCI: Allow hpbussize for specific hotplug bridges

This commit extends the pci=hpbussize= boot parameter to accept per-bridge
bus count reservations in the form <count>@<pci_dev>, recomputing the
share of spare bus numbers for unconfigured hotplug bridges on a bus and
applying the scoped minimum when scanning the bridge's child bus.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 9a178e0e5f7f4..b79a931edce55 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c

[ ... ]

> @@ -3163,7 +3228,16 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  			 * Distribute the extra buses between hotplug
>  			 * bridges if any.
>  			 */
> -			buses = available_buses / hotplug_bridges;
> +			if (hotplug_reserve && available_buses) {
> +				if (!dev->subordinate) {
> +					reserve = pci_hotplug_bus_reserve(dev);
> +					buses = available_buses + 1 > used_buses ?
> +						available_buses + 1 - used_buses : 0;
> +					buses = pci_hotplug_bus_share(bus, dev, buses);
> +				}
> +			} else {
> +				buses = available_buses / hotplug_bridges;
> +			}
>  			buses = min(buses, available_buses - used_buses + 1);
>  		}

[Severity: Medium]
Can available_buses - used_buses + 1 underflow in unsigned arithmetic here?

Before calling pci_hotplug_bus_share(), this checks whether
available_buses + 1 > used_buses and passes 0 when used_buses is larger
(for example, when SR-IOV buses plus the one-bus-per-bridge count from the
first pass exceed available_buses + 1, or after earlier bridges consumed
more buses than available_buses).

However, pci_hotplug_bus_share() unconditionally returns at least reserve:

    reserve = pci_hotplug_bus_reserve(bridge) ?: pci_hotplug_bus_size;
    return max(reserve, share);

When min(buses, available_buses - used_buses + 1) then runs without that
guard, available_buses - used_buses + 1 wraps around to a large unsigned
value, leaving the non-zero buses count from pci_hotplug_bus_share()
unclamped instead of clamping it to 0.

>  
>  		cmax = max;
>  		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
>  		/* One bus is already accounted so don't add it again */
>  		if (max - cmax > 1)
>  			used_buses += max - cmax - 1;
> +
> +		if (reserve > max - cmax)
> +			pci_warn(dev, "only %u of %u bus numbers available\n",
> +				 max - cmax, reserve);
> +		else if (reserve)
> +			pci_info(dev, "reserving %u bus numbers\n", max - cmax);
>  	}
>  
>  	/*
> @@ -3180,7 +3260,10 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  	 * bus number if there is room.
>  	 */
>  	if (bus->self && bus->self->is_hotplug_bridge) {
> -		used_buses = max(available_buses, pci_hotplug_bus_size - 1);
> +		unsigned long bus_size = pci_hotplug_bus_reserve(bus->self) ?:
> +					 pci_hotplug_bus_size;
> +
> +		used_buses = max(available_buses, bus_size - 1);

[Severity: High]
When a hotplug bridge's scoped reservation is clamped by the parent bus in
pci_scan_child_bus_extend() so that 1 bus number remains per subsequent
sibling bridge, won't taking max(available_buses, bus_size - 1) on the
child bus override that clamp and consume the sibling bridges' buses?

On the parent bus, pci_scan_child_bus_extend() clamps the bridge's share
via:

    buses = min(buses, available_buses - used_buses + 1);

and passes that clamped count to pci_scan_bridge_extend(). In pass 1,
pci_scan_bridge_extend() creates the child bus with its temporary end set
to the parent's busn_res.end before recursing:

    pci_bus_insert_busn_res(child, next_busnr,
                            bus->busn_res.end);
    ...
    if (available_buses)
        available_buses--;
    ...
    max = pci_scan_child_bus_extend(child, available_buses);

Inside the recursive pci_scan_child_bus_extend(child, available_buses)
call, bus->self is that hotplug bridge. Using max(available_buses,
bus_size - 1) replaces the parent-clamped available_buses with the
unclamped bus_size - 1, and only bounds max = start + used_buses against
child->busn_res.end (which is still the parent bus's busn_res.end).

For example, if 5 hotplug downstream ports share 15 buses and port 3
requests hpbussize=12 after ports 0..2 take 1 bus each, the parent clamps
port 3 to 11 buses so 1 bus remains for port 4. In the child scan for port
3, available_buses is 10 while bus_size - 1 is 11, so max(10, 11) extends
port 3 all the way to the parent's busn_res.end, leaving no bus number for
port 4 and suppressing the "only %u of %u bus numbers available" warning.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793724065.git.Maciej.Grochowski@sony.com?part=2