Re: [PATCH RFC 7/8] pci: reference count subordinate

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 22 Jul 2024 08:19:35 -0700
Keith Busch <kbusch@meta.com> wrote:

> From: Keith Busch <kbusch@kernel.org>
> 
> The subordinate is accessed under the pci_bus_sem (or often times no
> lock at at all), but unset under the rescan_remove_lock. Access the
> subordinate buses with reference counts to guard against a concurrent
> removal and use-after-free.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---

Hi Keith,

A few comments inline.

Jonathan



> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e3a49f66982d5..0cd36b7772c8b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3113,9 +3113,14 @@ void pci_bridge_d3_update(struct pci_dev *dev)
>  	 * so we need to go through all children to find out if one of them
>  	 * continues to block D3.
>  	 */
> -	if (d3cold_ok && !bridge->bridge_d3)
> -		pci_walk_bus(bridge->subordinate, pci_dev_check_d3cold,
> -			     &d3cold_ok);
> +	if (d3cold_ok && !bridge->bridge_d3) {
> +		struct pci_bus *bus = pci_dev_get_subordinate(bridge);
> +
> +		if (bus) {
> +			pci_walk_bus(bus, pci_dev_check_d3cold, &d3cold_ok);
> +			pci_bus_put(bus);
I'd be tempted to call pci_bus_put(bus) unconditionally but doesn't matter
a lot.
> +		}
> +	}
>  
>  	if (bridge->bridge_d3 != d3cold_ok) {
>  		bridge->bridge_d3 = d3cold_ok;
> @@ -4824,6 +4829,7 @@ static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
>  int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  {
>  	struct pci_dev *child __free(pci_dev_put) = NULL;

I would moan about constructors and desctructors together, but unrelated
to this patch and would actually break the change I suggest below given
the lifetime of child is longer than the loop where it's gotten.
So I won't moan about it :)

> +	struct pci_bus *bus;
>  	int delay;
>  
>  	if (pci_dev_is_disconnected(dev))
> @@ -4840,7 +4846,17 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  	 * board_added(). In case of ACPI hotplug the firmware is expected
>  	 * to configure the devices before OS is notified.
>  	 */
> -	if (!dev->subordinate || list_empty(&dev->subordinate->devices)) {
> +	bus = pci_dev_get_subordinate(dev);
> +	if (!bus) {
> +		up_read(&pci_bus_sem);
> +		return 0;
> +	}
> +
> +	child = pci_dev_get(list_first_entry_or_null(&bus->devices,
> +						     struct pci_dev,
> +						     bus_list));
> +	if (!child) {
> +		pci_bus_put(bus);
>  		up_read(&pci_bus_sem);
>  		return 0;
>  	}
> @@ -4848,12 +4864,12 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  	/* Take d3cold_delay requirements into account */
>  	delay = pci_bus_max_d3cold_delay(dev->subordinate);
>  	if (!delay) {
> +		pci_bus_put(bus);
>  		up_read(&pci_bus_sem);
>  		return 0;
>  	}
>  
> -	child = pci_dev_get(list_first_entry(&dev->subordinate->devices,
> -					     struct pci_dev, bus_list));
> +	pci_bus_put(bus);
>  	up_read(&pci_bus_sem);

I'd like scoped_guard() {
	struct pci_bus *bus __free(pci_bus_put) = pci_dev_get_sub...
	here so that the manual cleanup can be avoided in the early return paths.

}
}
>  
>  	/*

...

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8b..3c0c83d35ab86 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1212,9 +1212,11 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
>  		link->aspm_capable = link->aspm_support;
>  	}
>  	list_for_each_entry(link, &link_list, sibling) {
> +		struct pci_bus *linkbus;
>  		struct pci_dev *child;
> -		struct pci_bus *linkbus = link->pdev->subordinate;
> -		if (link->root != root)
> +
> +		linkbus = pci_dev_get_subordinate(link->pdev);
Maybe worth a 
DEFINE_FREE() for pci_bus_put to match the one for pci_dev_put?

> +		if (!linkbus || link->root != root)
>  			continue;
>  		list_for_each_entry(child, &linkbus->devices, bus_list) {
>  			if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
> @@ -1222,6 +1224,7 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
>  				continue;
>  			pcie_aspm_check_latency(child);
>  		}
> +		pci_bus_put(linkbus);
>  	}
>  }
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b14b9876c0303..53522685193da 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c

...

> @@ -3380,7 +3383,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)

As far as I can tell the return value of this function is never used.
So could just drop the code below. Maybe clean up this function
to return void or start handling the return value.

>  	/* Scan bridges that need to be reconfigured */
>  	pci_scan_bridge_extend(parent, dev, busnr, available_buses, 1);
>  
> -	if (!dev->subordinate)
> +	if (!READ_ONCE(dev->subordinate))
>  		return -1;
>  
>  	return 0;