Re: [PATCH 4/5] PCI: Enable async shutdown support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Double `put_device()` on `bridge->dev` in the `pci_register_host_bridge()` error path leads to use-after-free.
- [High] Unlocked read-modify-write of C bitfields in `dev_pm_info` via sysfs can clobber PM core states like `in_dpm_list`.
--

commit 5968194913e4b5e2b73a45e8bbc2ab20340b2cfe
Author: David Jeffery <djeffery@redhat.com>

PCI: Enable async shutdown support

This commit allows PCI device shutdown to be performed asynchronously.
It introduces calls to dev_set_async_shutdown() during host bridge
registration and device addition to reduce overall shutdown time.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b3cefec25f518..4782b4d5306b6 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 writing to the power/async sysfs
file trigger an unlocked read-modify-write that can clobber PM core states?

struct dev_pm_info declares adjacent bitfields async_suspend:1 and
in_dpm_list:1 that share a memory word.

device_pm_add() performs a locked read-modify-write under dpm_list_mtx to set
dev->power.in_dpm_list = true.

Because dpm_sysfs_add(dev) executes before device_pm_add(dev) during
initialization, userspace can immediately trigger a concurrent lockless
read-modify-write by writing to the power/async sysfs file. This calls
async_store() -> device_enable_async_suspend() and sets
dev->power.async_suspend = true without holding any locks.

If this lockless RMW races with device_pm_add(), can it overwrite in_dpm_list
back to false after it was set, breaking PM dependency chains and aborting
device linking?

[Severity: Critical]
This is a pre-existing issue, but does the error path in
pci_register_host_bridge() trigger a double put_device() on bridge->dev?

If device_register(&bus->dev) fails, execution jumps to the unregister label
which explicitly drops the bridge->dev reference:

unregister:
	put_device(&bridge->dev);
	device_del(&bridge->dev);
free:
	...
	if (bus_registered)
		put_device(&bus->dev);

However, the release function for the bus, release_pcibus_dev(), also
unconditionally drops the bridge reference:

static void release_pcibus_dev(struct device *dev)
{
	struct pci_bus *pci_bus = to_pci_bus(dev);

	put_device(pci_bus->bridge);
	...
}

When the execution falls through to the free label and calls
put_device(&bus->dev), will release_pcibus_dev() drop the bridge->dev
reference a second time, potentially causing a use-after-free when the caller
handles the registration error?

> +	dev_set_async_shutdown(bus->bridge);
>  	pci_set_bus_of_node(bus);
>  	pci_set_bus_msi_domain(bus);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616152219.6268-1-djeffery@redhat.com?part=4