mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock
@ 2026-04-22  6:32 Ionut Nechita (Wind River)
  2026-04-22  6:32 ` [PATCH v14 1/2] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Ionut Nechita (Wind River)
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Ionut Nechita (Wind River) @ 2026-04-22  6:32 UTC (permalink / raw)
  To: linux-pci, bhelgaas
  Cc: helgaas, sebott, schnelle, bblock, linux, lukas, stable,
	linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

From: Ionut Nechita <ionut.nechita@windriver.com>

From: Ionut Nechita <ionut.nechita@windriver.com>

Hi Bjorn,

This is v14 of the fix for the SR-IOV race between driver .remove()
and concurrent hotplug events.

Changes since v13 (Apr 21):
  - Patch 1/2: declared pci_rescan_remove_owner as
    'const struct task_struct *' to make it clear the pointer is
    only used for identity comparisons and never to modify the
    task_struct.  Suggested by Benjamin Block.
  - Patch 1/2: picked up Reviewed-by and Tested-by from
    Benjamin Block (given on v13).
  - Patch 2/2: unchanged from v13 (keeps R-b from Niklas Schnelle
    and R-b/T-b from Benjamin Block).

Changes since v12 (Apr 21):
  - Patch 1/2: replace mutex_get_owner() with explicit owner tracking
    (struct task_struct * + depth counter).  mutex_get_owner() is not
    exported outside the scheduler core and caused link failures in
    some build configurations:

      ld: vmlinux.o: in function `pci_lock_rescan_remove':
      drivers/pci/probe.c: undefined reference to `mutex_get_owner'

    The new implementation records the owner on first acquisition and
    clears it on final release, with a WARN_ON() catching mismatched
    unlock calls.  Semantics are unchanged.
  - Patch 1/2: dropped Reviewed-by/Tested-by since the locking
    primitive changed; please re-review.
  - Patch 2/2: unchanged from v12 (keeps R-b from Niklas Schnelle and
    R-b/T-b from Benjamin Block).

Changes since v11 (Mar 26):
  - Patch 2/2 picked up R-b Niklas Schnelle and R-b/T-b Benjamin Block
  - Rebased on linux-next (next-20260420)

Changes since v10 (Mar 18):
  - Patch 2/2: added kill_device() before device_release_driver() to
    prevent a new driver from binding between unbind and removal,
    closing the TOCTOU race window identified by Benjamin Block
  - Patch 1/2 unchanged from v10

Changes since v9 (Mar 10):
  - NEW patch 2/2: fix AB-BA deadlock in remove_store() by calling
    device_release_driver() before pci_stop_and_remove_bus_device_locked(),
    as suggested by Benjamin Block (addresses Guenter Roeck's report)
  - Patch 1/2 unchanged from v9

Changes since v8 (Mar 9):
  - Added Reviewed-by from Niklas Schnelle (IBM) and Tested-by (s390)
  - Added Fixes tags for the three related commits
  - Removed rescan/remove locking from sriov_numvfs_store() since
    locking is now handled in sriov_add_vfs() and sriov_del_vfs()
  - Rebased on linux-next (20260309)

The AB-BA deadlock:

  CPU0 (remove_store)               CPU1 (unbind_store)
  --------------------              --------------------
  pci_lock_rescan_remove()
                                    device_lock()
                                    driver .remove()
                                      sriov_del_vfs()
                                        pci_lock_rescan_remove()  <-- WAITS
  pci_stop_bus_device()
    device_release_driver()
      device_lock()                                               <-- WAITS

Patch 2/2 fixes this by:
  1. Marking the device as dead via kill_device() so no new driver
     can bind (prevents TOCTOU race between unbind and removal)
  2. Calling device_release_driver() before
     pci_stop_and_remove_bus_device_locked(), so both paths take
     locks in the same order: device_lock first, then
     pci_rescan_remove_lock

Note: the concurrent unbind_store + hotplug-event case (where the
hotplug handler takes pci_rescan_remove_lock before device_lock)
remains a known limitation.  This is a pre-existing issue that
Benjamin Block is addressing separately in:
  https://lore.kernel.org/linux-pci/354b9e4a54ced67f3c89df198041df19434fe4c8.1773235561.git.bblock@linux.ibm.com/

This race has been independently observed by multiple organizations:
  - IBM (s390 platform-generated hot-unplug events racing with
    sriov_del_vfs during PF driver unload)
  - NVIDIA (tested by Dragos Tatulea in earlier versions)
  - Intel (xe driver hitting lockdep warnings and deadlocks when
    calling pci_disable_sriov from .remove)
  - Wind River (original reporter and patch author)

Test environment:
  - Tested on s390 by Benjamin Block and Niklas Schnelle (IBM)
  - Tested on x86_64 with Intel and NVIDIA SR-IOV devices (earlier
    versions)

Based on linux-next (next-20260420).

Link: https://lore.kernel.org/linux-pci/20260214193235.262219-3-ionut.nechita@windriver.com/ [v1]
Link: https://lore.kernel.org/linux-pci/20260219212648.82606-1-ionut.nechita@windriver.com/ [v2]
Link: https://lore.kernel.org/lkml/20260225202434.18737-1-ionut.nechita@windriver.com/ [v3]
Link: https://lore.kernel.org/linux-pci/20260228120138.51197-2-ionut.nechita@windriver.com/ [v4]
Link: https://lore.kernel.org/linux-pci/20260303080903.28693-1-ionut.nechita@windriver.com/ [v5]
Link: https://lore.kernel.org/linux-pci/20260306082108.17322-1-ionut.nechita@windriver.com/ [v6]
Link: https://lore.kernel.org/linux-pci/20260308135352.80346-1-ionut.nechita@windriver.com/ [v7]
Link: https://lore.kernel.org/linux-pci/20260309194920.16459-1-ionut.nechita@windriver.com/ [v8]
Link: https://lore.kernel.org/linux-pci/20260310074303.17480-1-ionut.nechita@windriver.com/ [v9]
Link: https://lore.kernel.org/linux-pci/20260318210316.61975-1-ionut.nechita@windriver.com/ [v10]
Link: https://lore.kernel.org/linux-pci/20260326083534.23602-1-ionut.nechita@windriver.com/ [v11]
Link: https://lore.kernel.org/linux-pci/cover.1776755661.git.ionut.nechita@windriver.com/ [v12]
Link: https://lore.kernel.org/linux-pci/cover.1776756380.git.ionut.nechita@windriver.com/ [v13]

Ionut Nechita (Wind River) (2):
  PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect
    sriov_add_vfs/sriov_del_vfs
  PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock
    in remove_store

 drivers/pci/iov.c       |  9 +++++----
 drivers/pci/pci-sysfs.c | 30 +++++++++++++++++++++++++++++-
 drivers/pci/probe.c     | 18 ++++++++++++++++--
 3 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v14 1/2] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs
  2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
@ 2026-04-22  6:32 ` Ionut Nechita (Wind River)
  2026-04-22  6:32 ` [PATCH v14 2/2] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Ionut Nechita (Wind River)
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Ionut Nechita (Wind River) @ 2026-04-22  6:32 UTC (permalink / raw)
  To: linux-pci, bhelgaas
  Cc: helgaas, sebott, schnelle, bblock, linux, lukas, stable,
	linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

From: Ionut Nechita <ionut.nechita@windriver.com>

After reverting commit 05703271c3cd ("PCI/IOV: Add PCI rescan-remove
locking when enabling/disabling SR-IOV") and moving the lock to
sriov_numvfs_store(), the path through driver .remove() (e.g. rmmod,
or manual unbind) that calls pci_disable_sriov() directly remains
unprotected against concurrent hotplug events. This affects any SR-IOV
capable driver that calls pci_disable_sriov() from its .remove()
callback (i40e, ice, mlx5, bnxt, etc.).

On s390, platform-generated hot-unplug events for VFs can race with
sriov_del_vfs() when a PF driver is being unloaded. The platform event
handler takes pci_rescan_remove_lock, but sriov_del_vfs() does not,
leading to double removal and list corruption.

We cannot use a plain mutex_lock() here because sriov_del_vfs() may also
be called from paths that already hold pci_rescan_remove_lock (e.g.
remove_store -> pci_stop_and_remove_bus_device_locked, or
sriov_numvfs_store with the lock taken by the previous patch). Using
mutex_lock() in those cases would deadlock.

Make pci_lock_rescan_remove() itself reentrant by tracking the current
owner task and a recursion depth counter, as suggested by Lukas Wunner
and Benjamin Block, since these recursive locking scenarios exist
elsewhere in the PCI subsystem:
 - If the lock is already held by the current task (owner == current):
   increments the depth counter and returns without re-acquiring,
   avoiding deadlock.
 - If the lock is held by another task: blocks until the lock is
   released, then records the owner and sets depth to 1.
 - If the lock is not held: acquires the mutex normally.

pci_unlock_rescan_remove() decrements the depth counter and releases
the mutex (clearing the owner) only when the depth reaches zero.
A WARN_ON catches mismatched unlock calls from tasks that do not own
the lock.

This avoids relying on mutex_get_owner(), which is not exported to
modules and caused link failures for builds that inline this code
outside of the core kernel image.

This approach keeps the API unchanged: callers simply pair lock/unlock
calls without needing to track any return value or use separate
reentrant variants.

Add pci_lock_rescan_remove()/pci_unlock_rescan_remove() calls to
sriov_add_vfs() and sriov_del_vfs() to protect VF addition and
removal against concurrent hotplug events.

Remove the rescan/remove locking from sriov_numvfs_store() that was
introduced by commit a5338e365c45 ("PCI/IOV: Fix race between SR-IOV
enable/disable and hotplug"), since the locking is now handled directly
in sriov_add_vfs() and sriov_del_vfs() where it is actually needed,
reducing the lock scope.

Fixes: 18f9e9d150fc ("PCI/IOV: Factor out sriov_add_vfs()")
Fixes: 05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV")
Fixes: a5338e365c45 ("PCI/IOV: Fix race between SR-IOV enable/disable and hotplug")
Cc: stable@vger.kernel.org
Suggested-by: Lukas Wunner <lukas@wunner.de>
Suggested-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Tested-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Ionut Nechita <ionut.nechita@windriver.com>
---
 drivers/pci/iov.c   |  9 +++++----
 drivers/pci/probe.c | 18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 91ac4e37ecb9..7ed902539051 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -495,9 +495,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 
 	if (num_vfs == 0) {
 		/* disable VFs */
-		pci_lock_rescan_remove();
 		ret = pdev->driver->sriov_configure(pdev, 0);
-		pci_unlock_rescan_remove();
 		goto exit;
 	}
 
@@ -509,9 +507,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 		goto exit;
 	}
 
-	pci_lock_rescan_remove();
 	ret = pdev->driver->sriov_configure(pdev, num_vfs);
-	pci_unlock_rescan_remove();
 	if (ret < 0)
 		goto exit;
 
@@ -633,15 +629,18 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
 	if (dev->no_vf_scan)
 		return 0;
 
+	pci_lock_rescan_remove();
 	for (i = 0; i < num_vfs; i++) {
 		rc = pci_iov_add_virtfn(dev, i);
 		if (rc)
 			goto failed;
 	}
+	pci_unlock_rescan_remove();
 	return 0;
 failed:
 	while (i--)
 		pci_iov_remove_virtfn(dev, i);
+	pci_unlock_rescan_remove();
 
 	return rc;
 }
@@ -766,8 +765,10 @@ static void sriov_del_vfs(struct pci_dev *dev)
 	struct pci_sriov *iov = dev->sriov;
 	int i;
 
+	pci_lock_rescan_remove();
 	for (i = 0; i < iov->num_VFs; i++)
 		pci_iov_remove_virtfn(dev, i);
+	pci_unlock_rescan_remove();
 }
 
 static void sriov_disable(struct pci_dev *dev)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b63cd0c310bc..91f1dae6943b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3513,16 +3513,30 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
  * routines should always be executed under this mutex.
  */
 DEFINE_MUTEX(pci_rescan_remove_lock);
+static const struct task_struct *pci_rescan_remove_owner;
+static unsigned int pci_rescan_remove_depth;
 
 void pci_lock_rescan_remove(void)
 {
-	mutex_lock(&pci_rescan_remove_lock);
+	if (pci_rescan_remove_owner == current) {
+		pci_rescan_remove_depth++;
+	} else {
+		mutex_lock(&pci_rescan_remove_lock);
+		pci_rescan_remove_owner = current;
+		pci_rescan_remove_depth = 1;
+	}
 }
 EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
 
 void pci_unlock_rescan_remove(void)
 {
-	mutex_unlock(&pci_rescan_remove_lock);
+	if (WARN_ON(pci_rescan_remove_owner != current))
+		return;
+
+	if (--pci_rescan_remove_depth == 0) {
+		pci_rescan_remove_owner = NULL;
+		mutex_unlock(&pci_rescan_remove_lock);
+	}
 }
 EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v14 2/2] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store
  2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
  2026-04-22  6:32 ` [PATCH v14 1/2] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Ionut Nechita (Wind River)
@ 2026-04-22  6:32 ` Ionut Nechita (Wind River)
  2026-05-21 14:19 ` [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Benjamin Block
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Ionut Nechita (Wind River) @ 2026-04-22  6:32 UTC (permalink / raw)
  To: linux-pci, bhelgaas
  Cc: helgaas, sebott, schnelle, bblock, linux, lukas, stable,
	linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

From: Ionut Nechita <ionut.nechita@windriver.com>

remove_store() calls pci_stop_and_remove_bus_device_locked() which
takes pci_rescan_remove_lock first, then device_lock during driver
release.  Meanwhile, unbind_store() takes device_lock first (via
device_driver_detach), and the driver's .remove() callback may call
pci_disable_sriov() -> sriov_del_vfs() -> pci_lock_rescan_remove().

This creates an AB-BA deadlock:

  CPU0 (remove_store)               CPU1 (unbind_store)
  --------------------              --------------------
  pci_lock_rescan_remove()
                                    device_lock()
                                    driver .remove()
                                      sriov_del_vfs()
                                        pci_lock_rescan_remove()  <-- WAITS
  pci_stop_bus_device()
    device_release_driver()
      device_lock()                                               <-- WAITS

Fix this by first marking the device as dead using kill_device() to
prevent any new driver from binding, then calling device_release_driver()
before pci_stop_and_remove_bus_device_locked().

Marking the device dead closes the race window between unbinding and
removal where a new driver could theoretically bind: once the dead flag
is set, the device core will refuse any new driver probe.

After device_release_driver() returns, the driver is already unbound,
so the subsequent device_release_driver() call inside
pci_stop_and_remove_bus_device_locked() becomes a no-op.

Fixes: a5338e365c45 ("PCI/IOV: Fix race between SR-IOV enable/disable and hotplug")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/linux-pci/0ca9e675-478c-411d-be32-e2d81439288f@roeck-us.net/
Reported-by: Benjamin Block <bblock@linux.ibm.com>
Closes: https://lore.kernel.org/linux-pci/20260317090149.GA3835708@chlorum.ategam.org/
Suggested-by: Benjamin Block <bblock@linux.ibm.com>
Cc: stable@vger.kernel.org
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Tested-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Ionut Nechita <ionut.nechita@windriver.com>
---
 drivers/pci/pci-sysfs.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d37860841260..1426328e9f05 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -521,8 +521,36 @@ 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) and then
+		 * device_lock during driver release, while a concurrent
+		 * unbind_store (or sriov_numvfs_store) takes device_lock
+		 * first and then pci_rescan_remove_lock (via
+		 * sriov_del_vfs), creating a circular dependency.
+		 *
+		 * By unbinding first, the driver's .remove() callback
+		 * (including any SR-IOV VF cleanup) completes before
+		 * pci_rescan_remove_lock is acquired, ensuring both
+		 * paths take locks in the same order.
+		 */
+		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.53.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock
  2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
  2026-04-22  6:32 ` [PATCH v14 1/2] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Ionut Nechita (Wind River)
  2026-04-22  6:32 ` [PATCH v14 2/2] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Ionut Nechita (Wind River)
@ 2026-05-21 14:19 ` Benjamin Block
  2026-05-21 20:13 ` Bjorn Helgaas
  2026-07-06 14:29 ` Benjamin Block
  4 siblings, 0 replies; 7+ messages in thread
From: Benjamin Block @ 2026-05-21 14:19 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-pci, Ionut Nechita (Wind River),
	helgaas, schnelle, linux, lukas, stable, linux-kernel, intel-xe,
	matthew.brost, michal.wajdeczko, piotr.piorkowski, dtatulea,
	mani, kbusch, lkml, alifm, julianr, ionut_n2001, sunlightlinux

Hey Bjorn,

On Wed, Apr 22, 2026 at 09:32:40AM +0300, Ionut Nechita (Wind River) wrote:
> From: Ionut Nechita <ionut.nechita@windriver.com>
> 
> This is v14 of the fix for the SR-IOV race between driver .remove()
> and concurrent hotplug events.
> 
--8<--
> 
> This race has been independently observed by multiple organizations:
>   - IBM (s390 platform-generated hot-unplug events racing with
>     sriov_del_vfs during PF driver unload)
>   - NVIDIA (tested by Dragos Tatulea in earlier versions)
>   - Intel (xe driver hitting lockdep warnings and deadlocks when
>     calling pci_disable_sriov from .remove)
>   - Wind River (original reporter and patch author)
> 
> Test environment:
>   - Tested on s390 by Benjamin Block and Niklas Schnelle (IBM)
>   - Tested on x86_64 with Intel and NVIDIA SR-IOV devices (earlier
>     versions)
> 
> Based on linux-next (next-20260420).
> 
--8<--
> 
> Ionut Nechita (Wind River) (2):
>   PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect
>     sriov_add_vfs/sriov_del_vfs
>   PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock
>     in remove_store
> 
>  drivers/pci/iov.c       |  9 +++++----
>  drivers/pci/pci-sysfs.c | 30 +++++++++++++++++++++++++++++-
>  drivers/pci/probe.c     | 18 ++++++++++++++++--
>  3 files changed, 50 insertions(+), 7 deletions(-)

do you think this patchset is ready integration? Or what do you think we'd
still need to do to get there?

This race, and especially those I fix in the follow-on patchset
https://lore.kernel.org/linux-pci/cover.1776868550.git.bblock%40linux.ibm.com/
are pretty painful on s390 with PCI hotplug. It forces us to reboot the
system, since there is no other way to get out of the deadlock.

-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock
  2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
                   ` (2 preceding siblings ...)
  2026-05-21 14:19 ` [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Benjamin Block
@ 2026-05-21 20:13 ` Bjorn Helgaas
  2026-05-22 16:05   ` Benjamin Block
  2026-07-06 14:29 ` Benjamin Block
  4 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2026-05-21 20:13 UTC (permalink / raw)
  To: Ionut Nechita (Wind River)
  Cc: linux-pci, bhelgaas, sebott, schnelle, bblock, linux, lukas,
	stable, linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

On Wed, Apr 22, 2026 at 09:32:40AM +0300, Ionut Nechita (Wind River) wrote:
> Hi Bjorn,
> 
> This is v14 of the fix for the SR-IOV race between driver .remove()
> and concurrent hotplug events.

Thanks for your persistence on this!

Can you take a look at the sashiko feedback and see whether there's
any merit to it?

https://sashiko.dev/#/patchset/cover.1776839248.git.ionut.nechita%40windriver.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock
  2026-05-21 20:13 ` Bjorn Helgaas
@ 2026-05-22 16:05   ` Benjamin Block
  0 siblings, 0 replies; 7+ messages in thread
From: Benjamin Block @ 2026-05-22 16:05 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Ionut Nechita (Wind River),
	linux-pci, bhelgaas, sebott, schnelle, linux, lukas, stable,
	linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

On Thu, May 21, 2026 at 03:13:12PM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 22, 2026 at 09:32:40AM +0300, Ionut Nechita (Wind River) wrote:
> > This is v14 of the fix for the SR-IOV race between driver .remove()
> > and concurrent hotplug events.
> 
> Can you take a look at the sashiko feedback and see whether there's
> any merit to it?
> 
> https://sashiko.dev/#/patchset/cover.1776839248.git.ionut.nechita%40windriver.com

IIRC Ionut already responded to several of the findings a few patch versions
back:
https://lore.kernel.org/linux-pci/20260319202755.16081-1-ionut.nechita%40windriver.com/

Patch 01:

> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> [ ... ]
> > @@ -633,15 +629,18 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
> >  	if (dev->no_vf_scan)
> >  		return 0;
> >  
> > +	pci_lock_rescan_remove();
> Does moving this lock acquisition here create an ABBA deadlock? 
> Looking at sriov_numvfs_store(), the kernel acquires device_lock(&pdev->dev)
> before invoking the driver's sriov_configure() callback, which eventually
> reaches this point and calls pci_lock_rescan_remove(). This establishes a
> dependency of device_lock followed by pci_rescan_remove_lock.
> During a PCI hotplug event or sysfs remove_store, the subsystem first acquires
> pci_rescan_remove_lock, then calls pci_stop_and_remove_bus_device() which
> invokes device_release_driver() and takes device_lock(&pdev->dev). This
> creates the reverse dependency.
> Will an SR-IOV sysfs configuration racing with a hot-unplug event result in a
> deadlock? I understand this regression is fixed later in the patch series by
> commit 'PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock
> in remove_store', but wanted to point out the interaction here.

This is already addressed in Patch 02.

> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b63cd0c310bc0..91f1dae6943b6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> [ ... ]
> > @@ -3513,16 +3513,30 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
> >   * routines should always be executed under this mutex.
> >   */
> >  DEFINE_MUTEX(pci_rescan_remove_lock);
> > +static const struct task_struct *pci_rescan_remove_owner;
> > +static unsigned int pci_rescan_remove_depth;
> >  
> >  void pci_lock_rescan_remove(void)
> >  {
> > -	mutex_lock(&pci_rescan_remove_lock);
> > +	if (pci_rescan_remove_owner == current) {
>
> Can this read cause a data race?
> This reads pci_rescan_remove_owner outside of the mutex critical section.
> Concurrently, another thread executing this same function can acquire the
> mutex and write to this shared variable.
>
> > +		pci_rescan_remove_depth++;
> > +	} else {
> > +		mutex_lock(&pci_rescan_remove_lock);
> > +		pci_rescan_remove_owner = current;
>
> Could the compiler optimize or tear these unprotected accesses? Should these
> reads and writes use READ_ONCE() and WRITE_ONCE() to safely perform the
> lockless accesses and avoid triggering warnings in tools like KCSAN?
>
> > +		pci_rescan_remove_depth = 1;
> > +	}
> >  }

Those two are wrong. `pci_rescan_remove_depth` is only modified when we
already own the mutex `pci_rescan_remove_owner == current`, so the writes are
safe. The reads are safe because, again, the relevant case is when the current
thread already owns the mutex, and then we already have the lock; when we
don't already have the lock we gonna try to take it, but this is also safe
because nobody gonna change the owner to us other than we ourself, and only
once we have taken the lock.

Patch 02:

> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -521,8 +521,36 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> [ ... ]
> > +		 * By unbinding first, the driver's .remove() callback
> > +		 * (including any SR-IOV VF cleanup) completes before
> > +		 * pci_rescan_remove_lock is acquired, ensuring both
> > +		 * paths take locks in the same order.
> > +		 */
> > +		device_release_driver(dev);
>
> Does calling device_release_driver() here violate the bottom-up teardown
> guarantee if this device is a bridge?
>
> If the device is a PCIe bridge, unbinding the pcieport driver will call
> pci_disable_device(). Because child devices do not increment the parent
> bridge's enable_cnt during probe, the bridge's enable_cnt will drop to zero
> and clear the PCI_COMMAND_MASTER bit.
>
> This disables bus-mastering for the bridge while its child devices are still
> fully active. When they are later unbound during removal, their drivers might
> fail to communicate with the hardware, leading to master aborts, PCIe AER
> errors, and timeouts on DMA flushes.

Ionut previously addressed this in the post I linked above; identifying it as
false-positive.

> > 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
>
> Does this leave child devices vulnerable to the exact same AB-BA deadlock?
>
> When pci_stop_and_remove_bus_device_locked() executes, it acquires the
> pci_rescan_remove_lock and unbinds child devices recursively:
>
> pci_stop_and_remove_bus_device_locked()
>   pci_stop_bus_device()
>     pci_stop_dev(child)
>       device_release_driver(&child->dev)
>
> This attempts to acquire device_lock(&child->dev) while already holding
> pci_rescan_remove_lock.
>
> If a concurrent unbind_store() happens on the child device, it will acquire
> device_lock(&child->dev) first and then wait for pci_rescan_remove_lock,
> reproducing the original circular dependency.ecause then we can't concurrently change the state to `current`.

This is a false positive; we marked the device as "killed" and unbound the
device driver. So, when the concurrent unbind_store() is called `dev->driver`
is NULL and so it never gonna call device_driver_detach() and take the lock.
Or rather, it wouldn't even be called, because unbind_store() is supposedly
only called when some driver binds the device, which would not be the case
here.

At least thats my take on those latest findings.

-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock
  2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
                   ` (3 preceding siblings ...)
  2026-05-21 20:13 ` Bjorn Helgaas
@ 2026-07-06 14:29 ` Benjamin Block
  4 siblings, 0 replies; 7+ messages in thread
From: Benjamin Block @ 2026-07-06 14:29 UTC (permalink / raw)
  To: Bjorn Helgaas, Bjorn Helgaas
  Cc: Ionut Nechita (Wind River),
	linux-pci, bhelgaas, sebott, schnelle, linux, lukas, stable,
	linux-kernel, intel-xe, matthew.brost, michal.wajdeczko,
	piotr.piorkowski, dtatulea, mani, kbusch, lkml, alifm, julianr,
	ionut_n2001, sunlightlinux

Hey Bjorn,

On Wed, Apr 22, 2026 at 09:32:40AM +0300, Ionut Nechita (Wind River) wrote:
> From: Ionut Nechita <ionut.nechita@windriver.com>
> 
> This is v14 of the fix for the SR-IOV race between driver .remove()
> and concurrent hotplug events.
> 
--8<--
> 
> This race has been independently observed by multiple organizations:
>   - IBM (s390 platform-generated hot-unplug events racing with
>     sriov_del_vfs during PF driver unload)
>   - NVIDIA (tested by Dragos Tatulea in earlier versions)
>   - Intel (xe driver hitting lockdep warnings and deadlocks when
>     calling pci_disable_sriov from .remove)
>   - Wind River (original reporter and patch author)
> 
> Test environment:
>   - Tested on s390 by Benjamin Block and Niklas Schnelle (IBM)
>   - Tested on x86_64 with Intel and NVIDIA SR-IOV devices (earlier
>     versions)
> 
> Based on linux-next (next-20260420).
> 
--8<--
> 
> Ionut Nechita (Wind River) (2):
>   PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect
>     sriov_add_vfs/sriov_del_vfs
>   PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock
>     in remove_store
> 
>  drivers/pci/iov.c       |  9 +++++----
>  drivers/pci/pci-sysfs.c | 30 +++++++++++++++++++++++++++++-
>  drivers/pci/probe.c     | 18 ++++++++++++++++--
>  3 files changed, 50 insertions(+), 7 deletions(-)

did you find some time to take a look at this patchset?

I'd love to know how we can continue with this?

As mentioned, I also still have a s390x-specific patchset outstanding:
https://lore.kernel.org/linux-pci/cover.1776868550.git.bblock%40linux.ibm.com/
which depends on this one; and this still regularly deadlocks the kernel on
s390 with PCI hotplug. It really is something I'd like to have fixed as soon
as possible.

-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-07-06 14:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-04-22  6:32 [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Ionut Nechita (Wind River)
2026-04-22  6:32 ` [PATCH v14 1/2] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Ionut Nechita (Wind River)
2026-04-22  6:32 ` [PATCH v14 2/2] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Ionut Nechita (Wind River)
2026-05-21 14:19 ` [PATCH v14 0/2] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock Benjamin Block
2026-05-21 20:13 ` Bjorn Helgaas
2026-05-22 16:05   ` Benjamin Block
2026-07-06 14:29 ` Benjamin Block

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®