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

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

[Guangshuo Li]

sriov_numvfs_store() holds the PF device lock while calling the
driver's sriov_configure() callback. The SR-IOV configure path may add
or remove VFs under pci_rescan_remove_lock.

The PCI remove sysfs path takes the locks in the opposite order.
remove_store() calls pci_stop_and_remove_bus_device_locked(), which takes
pci_rescan_remove_lock and then releases the device driver, requiring
device_lock() on the device being removed.

If a write to sriov_numvfs races with removal of the same PF, or removal
of an upstream bridge that reaches the PF, the two paths can deadlock:

  sriov_numvfs_store()       remove_store()
  --------------------       --------------
  device_lock()
                             pci_rescan_remove_lock
  pci_rescan_remove_lock
                             device_lock()

Avoid the reversed locking order by unbinding the driver before entering
pci_stop_and_remove_bus_device_locked(). Mark the device dead first so a
new driver cannot bind between the unbind and the removal. After the
driver has been unbound, the SR-IOV cleanup in the driver's remove path
has completed before pci_rescan_remove_lock is acquired.

Fixes: a5338e365c45 ("PCI/IOV: Fix race between SR-IOV enable/disable and hotplug")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/pci/pci-sysfs.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5ec0b245a69b..5eae72508129 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);
+		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);
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
+	}
+
 	return count;
 }
 static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,
-- 
2.43.0

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

[sashiko-bot]

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

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

[Niklas Schnelle]

On Sun, 2026-07-05 at 23:24 +0800, Guangshuo Li wrote:
> sriov_numvfs_store() holds the PF device lock while calling the
> driver's sriov_configure() callback. The SR-IOV configure path may add
> or remove VFs under pci_rescan_remove_lock.
> 
> The PCI remove sysfs path takes the locks in the opposite order.
> remove_store() calls pci_stop_and_remove_bus_device_locked(), which takes
> pci_rescan_remove_lock and then releases the device driver, requiring
> device_lock() on the device being removed.
> 
> If a write to sriov_numvfs races with removal of the same PF, or removal
> of an upstream bridge that reaches the PF, the two paths can deadlock:
> 
>   sriov_numvfs_store()       remove_store()
>   --------------------       --------------
>   device_lock()
>                              pci_rescan_remove_lock
>   pci_rescan_remove_lock
>                              device_lock()
> 
> Avoid the reversed locking order by unbinding the driver before entering
> pci_stop_and_remove_bus_device_locked(). Mark the device dead first so a
> new driver cannot bind between the unbind and the removal. After the
> driver has been unbound, the SR-IOV cleanup in the driver's remove path
> has completed before pci_rescan_remove_lock is acquired.
> 
> Fixes: a5338e365c45 ("PCI/IOV: Fix race between SR-IOV enable/disable and hotplug")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/pci/pci-sysfs.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5ec0b245a69b..5eae72508129 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);
> +		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);
>  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> +	}
> +
>  	return count;
>  }
>  static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,

This seems to be a complete duplicate of Ionut Nechita's patch[0] from
April. The comments are close enough, but not identical, that I don't
think this can really have been created independently. Interestingly
though your patch refers to the race with sriov_numvfs_store() while
Ionut's patch refers to an equivalent race with unbind_store().  So
this makes it odd for straight up plagiarism and I wonder if Ionut's
patches have been public for long enough that they ended up in AI
knowledge. Then some AI might have reproduced it so closely in which
case you should at least disclose the AI use. Either way the patch by
Ionut should be taken for being the original source of this fix. And
technically the fix remains valid from my point of view and my R-b on
Ionut's version stands.

Thanks,
Niklas

[0]
https://lore.kernel.org/all/b378529b4afc4f2a6e393498fe9b9b7f056f95c4.1776755661.git.ionut.nechita@windriver.com/

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

[Benjamin Block]

On Mon, Jul 06, 2026 at 10:16:36AM +0200, Niklas Schnelle wrote:
> On Sun, 2026-07-05 at 23:24 +0800, Guangshuo Li wrote:
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5ec0b245a69b..5eae72508129 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);
> > +		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);
> >  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > +	}
> > +
> >  	return count;
> >  }
> >  static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,
> 
> This seems to be a complete duplicate of Ionut Nechita's patch[0] from
> April. The comments are close enough, but not identical, that I don't
> think this can really have been created independently.

Yep, +1.
This is not helpful. We still want the original patchset included (I sent a
ping a few minutes ago), but then in its complete form with the proper
authors.

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt         /        Geschäftsführung: David Faller
Sitz der Ges.: Ehningen     /     Registergericht: AmtsG Stuttgart, HRB 243294

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

[Krzysztof Wilczyński]

Hello,

> > This seems to be a complete duplicate of Ionut Nechita's patch[0] from
> > April. The comments are close enough, but not identical, that I don't
> > think this can really have been created independently.
> 
> Yep, +1.
> This is not helpful. We still want the original patchset included (I sent a
> ping a few minutes ago), but then in its complete form with the proper
> authors.

Older patches (include reviews and conversations about the problem) for reference:

  - https://lore.kernel.org/linux-pci/20240610220304.3162895-2-kbusch@meta.com/
  - https://lore.kernel.org/linux-pci/20240612181625.3604512-1-kbusch@meta.com/
  - https://lore.kernel.org/linux-pci/20251030-revert_sriov_lock-v1-0-70f82ade426f@linux.ibm.com/
  - https://lore.kernel.org/linux-pci/20251119-revert_sriov_lock-v2-0-ea50eb1e8f96@linux.ibm.com/
  - https://lore.kernel.org/linux-pci/20251216-revert_sriov_lock-v3-0-dac4925a7621@linux.ibm.com/
  - https://lore.kernel.org/linux-pci/20260214193235.262219-3-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260219212648.82606-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260225202434.18737-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260228120138.51197-2-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260303080903.28693-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260306082108.17322-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260308135352.80346-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260309194920.16459-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260310074303.17480-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260318210316.61975-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/20260326083534.23602-1-ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/cover.1776755661.git.ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/cover.1776756380.git.ionut.nechita@windriver.com/
  - https://lore.kernel.org/linux-pci/cover.1776839248.git.ionut.nechita@windriver.com/

This patch is a no-go, of course, given all the prior work (as far as 2024)
that had been done to fix this problem.

Thank you,

	Krzysztof