Re: [PATCH 8/9] PCI: Enable async shutdown support

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

New issues:
- [Medium] Setting the device async shutdown flag after exposing the device via device_add() creates a race condition.

Pre-existing issues:
- [High] Double put_device() on error path leads to premature freeing and subsequent use-after-free of the host bridge device.
- [High] Unhandled device_add() failure leaves PCI device in an indeterminate state and globally visible in the bus list.
- [High] Modifying PM bitfields without locking after a device is exposed causes data corruption if the PM core is active.
--

--- Patch [8]: [PATCH 8/9] PCI: Enable async shutdown support ---
commit f9f4c656f43f8b78ec5f8dac98964decb5920b89
Author: David Jeffery <[email protected]>
Subject: PCI: Enable async shutdown support

Like its async suspend support, allow PCI device shutdown to be performed
asynchronously to reduce shutdown time.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18d..af23814464060 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1046,6 +1046,7 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  
>  	bus->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(bus->bridge);

[Severity: High]
This is a pre-existing issue, but does calling device_enable_async_suspend()
here after the device is exposed to the PM core cause a data corruption race?

Since this performs an unlocked read-modify-write on the PM bitfields,
concurrent PM operations like dpm_prepare() could clobber these flags.

> +	dev_set_async_shutdown(bus->bridge);

[Severity: Medium]
Since this is called right after device_add() exposed the bridge device, could
this create a race condition where a concurrent system shutdown observes the
device before the async_shutdown flag is set?

>  	pci_set_bus_of_node(bus);
>  	pci_set_bus_msi_domain(bus);
>  	if (bridge->msi_domain && !dev_get_msi_domain(&bus->dev) &&

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in the
error path of pci_register_host_bridge(), does the code introduce a
use-after-free?

If device_register() fails, the unregister path calls
put_device(&bridge->dev) and then falls through to the free path which
calls put_device(&bus->dev).

Since release_pcibus_dev() internally drops a reference to
pci_bus->bridge, this would drop a second reference to bridge->dev,
improperly consuming the caller's initial reference.

[ ... ]

> @@ -2748,6 +2749,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	pci_reassigndev_resource_alignment(dev);
>  
>  	pci_init_capabilities(dev);
> +	dev_set_async_shutdown(&dev->dev);
>  
>  	/*
>  	 * Add the device to our list of discovered devices

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
pci_device_add(), does ignoring a failure from device_add() leave the PCI
device in an indeterminate state?

If device_add() fails, it only logs a WARN_ON and returns without cleaning
up the device from bus->devices, which could crash subsystems iterating over
the global bus list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8