Re: [PATCH] PCI: Fix AB-BA deadlock between remove and SR-IOV sysfs

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unbinding the PF driver outside of `pci_rescan_remove_lock` violates locking rules for SR-IOV VF cleanup, causing a lockdep splat and a race condition.

Pre-existing issues:
- [High] The patch fails to resolve the AB-BA deadlock between sriov_numvfs_store() and device removal for the upstream bridge removal scenario and native PCIe hotplug.
- [High] Potential Use-After-Free of `dev` due to a lack of reference counting after breaking sysfs active protection.
--

commit 6bda954b3e229ff563e88b2a1788be84c1d6f539
Author: Guangshuo Li <lgs201920130244@gmail.com>
Subject: PCI: Fix AB-BA deadlock between remove and SR-IOV sysfs

This patch attempts to fix an AB-BA deadlock between SR-IOV sysfs operations
and PCI device removal by unbinding the driver before entering
pci_stop_and_remove_bus_device_locked().

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5ec0b245a69bd..5eae725081297 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -519,8 +519,35 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>  	if (kstrtoul(buf, 0, &val) < 0)
>  		return -EINVAL;
>  
> -	if (val && device_remove_file_self(dev, attr))
> +	if (val && device_remove_file_self(dev, attr)) {
> +		/*
> +		 * Mark the device as dead so that no new driver can bind
> +		 * between the unbind and the removal below. Once the dead
> +		 * flag is set, the device core will refuse any new driver
> +		 * probe.
> +		 */
> +		device_lock(dev);

[Severity: High]
This is a pre-existing issue, but can dev be freed after
device_remove_file_self() completes?

device_remove_file_self() breaks the sysfs active protection that guarantees
dev won't be freed. If a concurrent removal (like PCIe hotplug) completes
device_del() and drops the final reference to dev before the subsequent calls
to device_lock(), kill_device(), and device_release_driver(), will this
result in a use-after-free?

> +		kill_device(dev);
> +		device_unlock(dev);
> +
> +		/*
> +		 * Unbind the driver before removing the device to avoid an
> +		 * AB-BA deadlock between device_lock() and
> +		 * pci_rescan_remove_lock. Without this, remove_store() takes
> +		 * pci_rescan_remove_lock first via
> +		 * pci_stop_and_remove_bus_device_locked(), then takes
> +		 * device_lock() during driver release, while a concurrent
> +		 * sriov_numvfs_store() takes device_lock() first and then
> +		 * pci_rescan_remove_lock via SR-IOV VF removal.
> +		 *
> +		 * By unbinding first, the driver's .remove() callback,
> +		 * including any SR-IOV VF cleanup, completes before
> +		 * pci_rescan_remove_lock is acquired.
> +		 */
> +		device_release_driver(dev);

[Severity: Critical]
Does this bypass the required synchronization for SR-IOV VF cleanup?

If the driver's .remove() callback (e.g., in ice or igb) calls
pci_disable_sriov(), it will eventually call
pci_stop_and_remove_bus_device(). This generic function explicitly requires
the bus rescan/remove lock via lockdep_assert_held(&pci_rescan_remove_lock).

Since device_release_driver() is now called outside of
pci_rescan_remove_lock, will this cause a lockdep splat and allow concurrent
unsynchronized modifications of the PCI bus lists?

>  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));

[Severity: High]
This isn't a bug introduced by this patch, but does the AB-BA deadlock
still exist when removing an upstream bridge?

The explicit unbind here only applies to the targeted device (dev). When
pci_stop_and_remove_bus_device_locked() is called, it acquires
pci_rescan_remove_lock and recursively unbinds child devices.

If it reaches a downstream SR-IOV PF, it will acquire device_lock(&pf->dev)
while holding pci_rescan_remove_lock.

If a concurrent write to sriov_numvfs on that PF acquires
device_lock(&pf->dev) and waits for pci_rescan_remove_lock, wouldn't the
exact same deadlock occur?

> +	}
> +
>  	return count;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705152458.291832-1-lgs201920130244@gmail.com?part=1