Re: PCI: Prevent device lock leak during bus reset

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch fixes pci_bus_reset while leaving identical lock leak and Use-After-Free races unmitigated in pci_try_reset_bus, pci_slot_reset, and pci_try_reset_slot.
--

--- Patch [1]: PCI: Prevent device lock leak during bus reset ---
commit 0d6c89d5cec5eebb33879246fbc7b446f026613a
Author: Zhang Hongtao <[email protected]>

PCI: Prevent device lock leak during bus reset

This patch fixes a device lock leak in pci_bus_reset() caused by
concurrent device removal. It takes a topology snapshot under pci_bus_sem
to guarantee that all locked devices are unlocked during a secondary bus reset.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..25a1e44263c32 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5585,21 +5667,31 @@ static int pci_try_reset_slot(struct pci_slot *slot)
>  
>  static int pci_bus_reset(struct pci_bus *bus, bool probe)
>  {
> +	struct pci_bus_lock_context context;
>  	int ret;
>  
> +	down_read(&pci_bus_sem);
> +
>  	if (!bus->self || !pci_bus_resettable(bus))
> -		return -ENOTTY;
> +		ret = -ENOTTY;
> +	else if (probe)
> +		ret = 0;
> +	else
> +		ret = pci_bus_lock_snapshot_init(bus, &context);
>  
> -	if (probe)
> -		return 0;
> +	up_read(&pci_bus_sem);
> +
> +	if (ret || probe)
> +		return ret;
>  
> -	pci_bus_lock(bus);
> +	pci_bus_lock_snapshot(&context);
>  
>  	might_sleep();
>  
>  	ret = pci_bridge_secondary_bus_reset(bus->self);
>  
> -	pci_bus_unlock(bus);
> +	pci_bus_unlock_snapshot(&context);
> +	pci_bus_lock_snapshot_release(&context);
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but regarding the RFC question in the commit
message about extending this fix:

The identical lock leaks and use-after-free races remain in the slot and
try-reset paths, as they rely on __pci_bus_lock(), __pci_bus_unlock(), and
__pci_bus_trylock().

For example, __pci_bus_trylock() iterates over bus->devices without 
holding pci_bus_sem:

drivers/pci/pci.c:__pci_bus_trylock() {
    ...
    list_for_each_entry(dev, &bus->devices, bus_list) {
        if (slot && (!dev->slot || dev->slot != slot))
            continue;
...
}

If a concurrent device removal executes list_del(&dev->bus_list) during
pci_try_reset_bus(), pci_slot_reset(), or pci_try_reset_slot(), the unlock
loop fails to find the device. This leaves the device permanently locked,
causing the same system hangs in driver bind/unbind paths.

Since these paths are reachable via PCIe AER recovery or hotplug events,
would it be safer to extend this snapshot mechanism to the slot and
try-reset paths to fully close this race condition?

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