* [PATCH v15 1/5] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs
2026-09-24 16:29 [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks Benjamin Block
@ 2026-09-24 16:29 ` Benjamin Block
2026-09-24 16:40 ` sashiko-bot
2026-09-24 16:29 ` [PATCH v15 2/5] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Benjamin Block
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Benjamin Block @ 2026-09-24 16:29 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Christian Borntraeger, Heiko Carstens, linux-intel-xe,
piotr.piorkowski, Farhan Ali, Halil Pasic, Gerd Bayer,
Lukas Wunner, Guenter Roeck, Manivannan Sadhasivam, Vasily Gorbik,
Alexander Gordeev, Ionut Nechita, Tobias Schumacher,
Niklas Schnelle, Ramesh Errabolu, linux-kernel, Sven Schnelle,
Keith Busch, Andreas Krebbel, Julian Ruess, Matthew Brost,
Ionut Nechita, Omar Elghoul, Michal Wajdeczko, linux-pci,
Ionut Nechita, Matthew Rosato, linux-s390, Dragos Tatulea,
Benjamin Block, stable
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>
Signed-off-by: Ionut Nechita <ionut.nechita@windriver.com>
[bblock@linux.ibm.com: rebase on v7.3, READ_/WRITE_ONCE changes, comments]
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
drivers/pci/iov.c | 9 +++++----
drivers/pci/probe.c | 27 +++++++++++++++++++++++++--
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9d408fb8ac25..885855650dbf 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 27008e2ea5af..cff9f0bf4c4b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3510,16 +3510,39 @@ 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 size_t pci_rescan_remove_depth;
void pci_lock_rescan_remove(void)
{
- mutex_lock(&pci_rescan_remove_lock);
+ if (READ_ONCE(pci_rescan_remove_owner) == current) {
+ /*
+ * read and modify while &pci_rescan_remove_lock is held by
+ * current thread
+ */
+ pci_rescan_remove_depth++;
+ } else {
+ mutex_lock(&pci_rescan_remove_lock);
+ WRITE_ONCE(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(READ_ONCE(pci_rescan_remove_owner) != current))
+ return;
+
+ /*
+ * read and modify while &pci_rescan_remove_lock is held by current
+ * thread
+ */
+ pci_rescan_remove_depth--;
+ if (pci_rescan_remove_depth == 0) {
+ WRITE_ONCE(pci_rescan_remove_owner, NULL);
+ mutex_unlock(&pci_rescan_remove_lock);
+ }
}
EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v15 1/5] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs
2026-09-24 16:29 ` [PATCH v15 1/5] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Benjamin Block
@ 2026-09-24 16:40 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-24 16:40 UTC (permalink / raw)
To: Benjamin Block
Cc: Alexander Gordeev, Christian Borntraeger, linux-s390,
Vasily Gorbik, intel-xe, linux-pci, Heiko Carstens
> 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.
>
> [ ... ]
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Suggested-by: Benjamin Block <bblock@linux.ibm.com>
> Signed-off-by: Ionut Nechita <ionut.nechita@windriver.com>
> [bblock@linux.ibm.com: rebase on v7.3, READ_/WRITE_ONCE changes, comments]
> Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790267348.git.bblock@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v15 2/5] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store
2026-09-24 16:29 [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks Benjamin Block
2026-09-24 16:29 ` [PATCH v15 1/5] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Benjamin Block
@ 2026-09-24 16:29 ` Benjamin Block
2026-09-24 16:38 ` sashiko-bot
2026-09-24 16:29 ` [PATCH v15 3/5] PCI: Move declaration of pci_rescan_remove_lock into public pci.h Benjamin Block
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Benjamin Block @ 2026-09-24 16:29 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Christian Borntraeger, Heiko Carstens, linux-intel-xe,
piotr.piorkowski, Farhan Ali, Halil Pasic, Gerd Bayer,
Lukas Wunner, Guenter Roeck, Manivannan Sadhasivam, Vasily Gorbik,
Alexander Gordeev, Ionut Nechita, Tobias Schumacher,
Niklas Schnelle, Ramesh Errabolu, linux-kernel, Sven Schnelle,
Keith Busch, Andreas Krebbel, Julian Ruess, Matthew Brost,
Ionut Nechita, Omar Elghoul, Michal Wajdeczko, linux-pci,
Ionut Nechita, Matthew Rosato, linux-s390, Dragos Tatulea,
Benjamin Block, stable
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>
Signed-off-by: Benjamin Block <bblock@linux.ibm.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 1f21856aac8a..b2f3e052d903 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -520,8 +520,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.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v15 2/5] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store
2026-09-24 16:29 ` [PATCH v15 2/5] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Benjamin Block
@ 2026-09-24 16:38 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-24 16:38 UTC (permalink / raw)
To: Benjamin Block
Cc: Vasily Gorbik, linux-pci, Heiko Carstens, Alexander Gordeev,
Christian Borntraeger, intel-xe, linux-s390
> 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()
> [ ... ]
> 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>
> Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790267348.git.bblock@linux.ibm.com?part=2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v15 3/5] PCI: Move declaration of pci_rescan_remove_lock into public pci.h
2026-09-24 16:29 [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks Benjamin Block
2026-09-24 16:29 ` [PATCH v15 1/5] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs Benjamin Block
2026-09-24 16:29 ` [PATCH v15 2/5] PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_store Benjamin Block
@ 2026-09-24 16:29 ` Benjamin Block
2026-09-24 16:35 ` sashiko-bot
2026-09-24 16:29 ` [PATCH v15 4/5] PCI: Provide lock guard for pci_rescan_remove_lock Benjamin Block
2026-09-24 16:29 ` [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release Benjamin Block
4 siblings, 1 reply; 13+ messages in thread
From: Benjamin Block @ 2026-09-24 16:29 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Christian Borntraeger, Heiko Carstens, linux-intel-xe,
piotr.piorkowski, Farhan Ali, Halil Pasic, Gerd Bayer,
Lukas Wunner, Guenter Roeck, Manivannan Sadhasivam, Vasily Gorbik,
Alexander Gordeev, Ionut Nechita, Tobias Schumacher,
Niklas Schnelle, Ramesh Errabolu, linux-kernel, Sven Schnelle,
Keith Busch, Andreas Krebbel, Julian Ruess, Matthew Brost,
Ionut Nechita, Omar Elghoul, Michal Wajdeczko, linux-pci,
Ionut Nechita, Matthew Rosato, linux-s390, Dragos Tatulea,
Benjamin Block
So far it is possible to use and call the functions
pci_lock_rescan_remove() and pci_unlock_rescan_remove() from any PCI
code, including modules and architecture code; but the lock variable
`pci_rescan_remove_lock` itself is private to objects residing in
`drivers/pci/` via the header `drivers/pci/pci.h`.
This makes it possible to use the lock - lock it, unlock it - from
anywhere, but it is not possible to use lockdep annotations such as
lockdep_assert_held(), or sparse annotations such as __must_hold() in
modules or architecture code for PCI to make the usage more safe.
Since it is useful for `pci_rescan_remove_lock` to have such
annotations, move the variable declaration into `include/linux/pci.h`.
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
drivers/pci/pci.h | 2 --
drivers/pci/probe.c | 1 +
include/linux/pci.h | 2 ++
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ba3c3fddddc2..811958807b63 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -114,8 +114,6 @@ unsigned char pcie_get_link_speed(unsigned int speed);
extern bool pci_early_dump;
-extern struct mutex pci_rescan_remove_lock;
-
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cff9f0bf4c4b..85b96a8e40e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3510,6 +3510,7 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
* routines should always be executed under this mutex.
*/
DEFINE_MUTEX(pci_rescan_remove_lock);
+EXPORT_SYMBOL_GPL(pci_rescan_remove_lock);
static const struct task_struct *pci_rescan_remove_owner;
static size_t pci_rescan_remove_depth;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d31a8d107b1e..d704a32ff988 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -40,6 +40,7 @@
#include <linux/io.h>
#include <linux/resource_ext.h>
#include <linux/msi_api.h>
+#include <linux/mutex.h>
#include <uapi/linux/pci.h>
#include <linux/pci_ids.h>
@@ -1551,6 +1552,7 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev);
/* Functions for PCI Hotplug drivers to use */
unsigned int pci_rescan_bus(struct pci_bus *bus);
+extern struct mutex pci_rescan_remove_lock;
void pci_lock_rescan_remove(void);
void pci_unlock_rescan_remove(void);
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v15 3/5] PCI: Move declaration of pci_rescan_remove_lock into public pci.h
2026-09-24 16:29 ` [PATCH v15 3/5] PCI: Move declaration of pci_rescan_remove_lock into public pci.h Benjamin Block
@ 2026-09-24 16:35 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-09-24 16:35 UTC (permalink / raw)
To: Benjamin Block
Cc: Vasily Gorbik, Alexander Gordeev, linux-pci, linux-s390,
Heiko Carstens, intel-xe, Christian Borntraeger
> So far it is possible to use and call the functions
> pci_lock_rescan_remove() and pci_unlock_rescan_remove() from any PCI
> code, including modules and architecture code; but the lock variable
> `pci_rescan_remove_lock` itself is private to objects residing in
> `drivers/pci/` via the header `drivers/pci/pci.h`.
>
> This makes it possible to use the lock - lock it, unlock it - from
> anywhere, but it is not possible to use lockdep annotations such as
> lockdep_assert_held(), or sparse annotations such as __must_hold() in
> modules or architecture code for PCI to make the usage more safe.
>
> Since it is useful for `pci_rescan_remove_lock` to have such
> annotations, move the variable declaration into `include/linux/pci.h`.
>
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790267348.git.bblock@linux.ibm.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v15 4/5] PCI: Provide lock guard for pci_rescan_remove_lock
2026-09-24 16:29 [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks Benjamin Block
` (2 preceding siblings ...)
2026-09-24 16:29 ` [PATCH v15 3/5] PCI: Move declaration of pci_rescan_remove_lock into public pci.h Benjamin Block
@ 2026-09-24 16:29 ` Benjamin Block
2026-09-24 16:34 ` sashiko-bot
2026-09-24 16:29 ` [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release Benjamin Block
4 siblings, 1 reply; 13+ messages in thread
From: Benjamin Block @ 2026-09-24 16:29 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Christian Borntraeger, Heiko Carstens, linux-intel-xe,
piotr.piorkowski, Farhan Ali, Halil Pasic, Gerd Bayer,
Lukas Wunner, Guenter Roeck, Manivannan Sadhasivam, Vasily Gorbik,
Alexander Gordeev, Ionut Nechita, Tobias Schumacher,
Niklas Schnelle, Ramesh Errabolu, linux-kernel, Sven Schnelle,
Keith Busch, Andreas Krebbel, Julian Ruess, Matthew Brost,
Ionut Nechita, Omar Elghoul, Michal Wajdeczko, linux-pci,
Ionut Nechita, Matthew Rosato, linux-s390, Dragos Tatulea,
Benjamin Block
Make it possible to use guard() or scoped_guard() to lock, and
automatically unlock `pci_rescan_remove_lock`.
Since the actual mutex `pci_rescan_remove_lock` is always supposed to be
taken and released using the functions pci_lock_rescan_remove() and
pci_unlock_rescan_remove() it is not possible to simply use the already
existing guards for `struct mutex`. Instead define a new guard
`pci_rescan_remove` that will also call the functions in question, but
is usable via guard() or scoped_guard().
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
include/linux/pci.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d704a32ff988..a0e36b7acac0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -42,6 +42,7 @@
#include <linux/msi_api.h>
#include <linux/mutex.h>
#include <uapi/linux/pci.h>
+#include <linux/cleanup.h>
#include <linux/pci_ids.h>
@@ -1555,6 +1556,8 @@ unsigned int pci_rescan_bus(struct pci_bus *bus);
extern struct mutex pci_rescan_remove_lock;
void pci_lock_rescan_remove(void);
void pci_unlock_rescan_remove(void);
+DEFINE_LOCK_GUARD_0(pci_rescan_remove, pci_lock_rescan_remove(),
+ pci_unlock_rescan_remove());
/* Vital Product Data routines */
ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
2026-09-24 16:29 [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks Benjamin Block
` (3 preceding siblings ...)
2026-09-24 16:29 ` [PATCH v15 4/5] PCI: Provide lock guard for pci_rescan_remove_lock Benjamin Block
@ 2026-09-24 16:29 ` Benjamin Block
2026-09-24 16:44 ` sashiko-bot
2026-10-02 12:55 ` [PATCH v16 " Benjamin Block
4 siblings, 2 replies; 13+ messages in thread
From: Benjamin Block @ 2026-09-24 16:29 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Christian Borntraeger, Heiko Carstens, linux-intel-xe,
piotr.piorkowski, Farhan Ali, Halil Pasic, Gerd Bayer,
Lukas Wunner, Guenter Roeck, Manivannan Sadhasivam, Vasily Gorbik,
Alexander Gordeev, Ionut Nechita, Tobias Schumacher,
Niklas Schnelle, Ramesh Errabolu, linux-kernel, Sven Schnelle,
Keith Busch, Andreas Krebbel, Julian Ruess, Matthew Brost,
Ionut Nechita, Omar Elghoul, Michal Wajdeczko, linux-pci,
Ionut Nechita, Matthew Rosato, linux-s390, Dragos Tatulea,
Benjamin Block, stable
When removing PCI device or PCI bus objects there are a couple of
call-chains where it is possible that the kernel runs into a circular
deadlock involving the central `pci_rescan_remove_lock`.
Two examples:
(A) Thread α receives a PCI event notifying the kernel that a PCI
virtual function has been moved into Reserved state, and so the PCI
subsystem will try to remove that PCI function. The call-chain for that
looks like this:
__zpci_event_availability()
-> zpci_zdev_put() # will lock(zpci_add_remove_lock),
# and lock(zpci_list_lock)
-> zpci_release_device() # will unlock(zpci_list_lock)
-> zpci_cleanup_bus_resources() # will lock(pci_rescan_remove_lock)
Thread β is triggered by userspace writing 0 into the SysFS attribute
`sriov_numvfs` of the parent PCI physical function of the same function
we just try to remove. This will also try to release the PCI virtual
function; but this time the call-chain looks like this:
sriov_numvfs_store()
-> ... (device driver dependent)
-> sriov_disable()
-> sriov_del_vfs() # will lock(pci_rescan_remove_lock)
-> ... (deep chain)
-> pci_release_dev()
-> pcibios_release_device()
-> zpci_zdev_put() # will lock(zpci_add_remove_lock)
If thread α and β coincide, this will result in a cyclic deadlock.
(B) Thread γ receives a PCI event notifying the kernel that one or more
PCI functions got hot plugged, and need to be configured. A possible
call-chain that might happen while the PCI subsystem is trying to add
those new function looks like this:
__zpci_event_availability()
-> ... (multiple ways in which a device gets added)
-> zpci_add_device() # will lock(zpci_add_remove_lock)
-> zpci_bus_device_register()
-> zpci_bus_put() # will lock(zbus_list_lock)
-> zpci_bus_release() # will unlock(zbus_list_lock)
# will lock(pci_rescan_remove_lock)
Now the same thread β as above in (A) might coincide, and again could
result in a cyclic deadlock.
`pci_rescan_remove_lock` has to be and is taken at a "high level" in
most call-chains since it is intended to protect/mutual exclude all
rescan and/or removal actions taken in the PCI subsystem. So to prevent
the outlined deadlock scenarios above remove it instead from the "low
level" release functions for both the PCI device and PCI bus objects.
Instead, lock `pci_rescan_remove_lock` in all call-chains leading to
those release functions:
* initialization of the PCI subsystem;
* processing of availability events (CRWs) for PCI functions;
* processing of error events (CRWs) for PCI functions;
* architecture specific release PCI device implementation.
Additionally, remove `pci_rescan_remove_lock` from zpci_bus_scan_bus()
since its only caller zpci_scan_devices() is now always called with
`pci_rescan_remove_lock` already held.
Lastly, document the new locking expectations after these changes. Add
sparse and lockdep annotations to functions that previously locked
`pci_rescan_remove_lock` explicitly, making sure the lock is now
already held when called. Additionally also add the annotations to
zpci_zdev_put() and zpci_bus_put() to make sure that every function that
potentially drops the last reference already holds the lock to prevent
surprises.
Fixes: 05bc1be6db4b2 ("s390/pci: create zPCI bus")
Fixes: ab909509850b2 ("PCI: s390: Fix use-after-free of PCI resources with per-function hotplug")
Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
arch/s390/pci/pci.c | 11 ++++++++---
arch/s390/pci/pci_bus.c | 15 ++++++++-------
arch/s390/pci/pci_event.c | 28 ++++++++++++++++++----------
arch/s390/pci/pci_iov.c | 3 +--
arch/s390/pci/pci_sysfs.c | 25 ++++++++++++-------------
5 files changed, 47 insertions(+), 35 deletions(-)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index d64c544b32b0..4fc3e3dc87ad 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -71,9 +71,11 @@ struct airq_iv *zpci_aif_sbv;
EXPORT_SYMBOL_GPL(zpci_aif_sbv);
void zpci_zdev_put(struct zpci_dev *zdev)
+ __must_hold(&pci_rescan_remove_lock)
{
if (!zdev)
return;
+ lockdep_assert_held(&pci_rescan_remove_lock);
mutex_lock(&zpci_add_remove_lock);
kref_put_lock(&zdev->kref, zpci_release_device, &zpci_list_lock);
mutex_unlock(&zpci_add_remove_lock);
@@ -583,11 +585,13 @@ int zpci_setup_bus_resources(struct zpci_dev *zdev)
}
static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
+ __must_hold(&pci_rescan_remove_lock)
{
struct resource *res;
int i;
- pci_lock_rescan_remove();
+ lockdep_assert_held(&pci_rescan_remove_lock);
+
for (i = 0; i < PCI_STD_NUM_BARS; i++) {
res = zdev->bars[i].res;
if (!res)
@@ -600,7 +604,6 @@ static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
kfree(res);
}
zdev->has_resources = 0;
- pci_unlock_rescan_remove();
}
int pcibios_device_add(struct pci_dev *pdev)
@@ -630,6 +633,7 @@ void pcibios_release_device(struct pci_dev *pdev)
{
struct zpci_dev *zdev = to_zpci(pdev);
+ guard(pci_rescan_remove)();
zpci_unmap_resources(pdev);
zpci_zdev_put(zdev);
}
@@ -1208,7 +1212,8 @@ static int __init pci_base_init(void)
if (rc)
goto out_irq;
- rc = zpci_scan_devices();
+ scoped_guard(pci_rescan_remove)
+ rc = zpci_scan_devices();
if (rc)
goto out_find;
diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index 36a4807285fa..c1b48b572e86 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -82,9 +82,8 @@ int zpci_bus_scan_device(struct zpci_dev *zdev)
if (!pdev)
return -ENODEV;
- pci_lock_rescan_remove();
+ guard(pci_rescan_remove)();
pci_bus_add_device(pdev);
- pci_unlock_rescan_remove();
return 0;
}
@@ -132,10 +131,13 @@ void zpci_bus_remove_device(struct zpci_dev *zdev, bool set_error)
* Return: 0 on success, an error value otherwise
*/
int zpci_bus_scan_bus(struct zpci_bus *zbus)
+ __must_hold(&pci_rescan_remove_lock)
{
struct zpci_dev *zdev;
int devfn, rc, ret = 0;
+ lockdep_assert_held(&pci_rescan_remove_lock);
+
for (devfn = 0; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
zdev = zbus->function[devfn];
if (zdev && zdev->state == ZPCI_FN_STATE_CONFIGURED) {
@@ -145,10 +147,8 @@ int zpci_bus_scan_bus(struct zpci_bus *zbus)
}
}
- pci_lock_rescan_remove();
pci_scan_child_bus(zbus->bus);
pci_bus_add_devices(zbus->bus);
- pci_unlock_rescan_remove();
return ret;
}
@@ -214,11 +214,12 @@ static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *fr, s
* run of the function.
*/
static inline void zpci_bus_release(struct kref *kref)
- __releases(&zbus_list_lock)
+ __releases(&zbus_list_lock) __must_hold(&pci_rescan_remove_lock)
{
struct zpci_bus *zbus = container_of(kref, struct zpci_bus, kref);
lockdep_assert_held(&zbus_list_lock);
+ lockdep_assert_held(&pci_rescan_remove_lock);
list_del(&zbus->bus_next);
mutex_unlock(&zbus_list_lock);
@@ -229,14 +230,12 @@ static inline void zpci_bus_release(struct kref *kref)
*/
if (zbus->bus) {
- pci_lock_rescan_remove();
pci_stop_root_bus(zbus->bus);
zpci_free_domain(zbus->domain_nr);
pci_free_resource_list(&zbus->resources);
pci_remove_root_bus(zbus->bus);
- pci_unlock_rescan_remove();
}
zpci_remove_parent_msi_domain(zbus);
@@ -250,7 +249,9 @@ static inline void __zpci_bus_get(struct zpci_bus *zbus)
}
static inline void zpci_bus_put(struct zpci_bus *zbus)
+ __must_hold(&pci_rescan_remove_lock)
{
+ lockdep_assert_held(&pci_rescan_remove_lock);
kref_put_mutex(&zbus->kref, zpci_bus_release, &zbus_list_lock);
}
diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index f317a1465dad..268f435677ae 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -382,6 +382,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
pci_dev_put(pdev);
no_pdev:
mutex_unlock(&zdev->state_lock);
+ guard(pci_rescan_remove)();
zpci_zdev_put(zdev);
}
@@ -428,6 +429,7 @@ static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
if (ccdf->pec != 0x0306)
return false;
/* 0x308 or 0x302 for multiple devices */
+ guard(pci_rescan_remove)();
zpci_remove_reserved_devices();
zpci_scan_devices();
return true;
@@ -436,26 +438,29 @@ static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
static void zpci_event_avail_new_device(struct zpci_ccdf_avail *ccdf)
{
struct zpci_dev *zdev;
+ bool freed = false;
switch (ccdf->pec) {
case 0x0301: /* Reserved|Standby -> Configured */
zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
if (IS_ERR(zdev))
break;
- if (zpci_add_device(zdev)) {
- kfree(zdev);
- break;
- }
- zpci_scan_configured_device(zdev, ccdf->fh);
+ scoped_guard(pci_rescan_remove)
+ if (zpci_add_device(zdev)) {
+ kfree(zdev);
+ /* break; is incompatible w/ scoped_guard() */
+ freed = true;
+ }
+ if (!freed)
+ zpci_scan_configured_device(zdev, ccdf->fh);
break;
case 0x0302: /* Reserved -> Standby */
zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
if (IS_ERR(zdev))
break;
- if (zpci_add_device(zdev)) {
- kfree(zdev);
- break;
- }
+ scoped_guard(pci_rescan_remove)
+ if (zpci_add_device(zdev))
+ kfree(zdev);
break;
}
}
@@ -497,11 +502,13 @@ static void zpci_event_avail_existing_device(struct zpci_dev *zdev, struct zpci_
/* The 0x0304 event may immediately reserve the device */
if (!clp_get_state(zdev->fid, &state) &&
state == ZPCI_FN_STATE_RESERVED) {
+ guard(pci_rescan_remove)();
zpci_device_reserved(zdev);
}
break;
case 0x0308: /* Standby -> Reserved */
- zpci_device_reserved(zdev);
+ scoped_guard(pci_rescan_remove)
+ zpci_device_reserved(zdev);
break;
}
}
@@ -523,5 +530,6 @@ void zpci_event_availability(void *data)
mutex_lock(&zdev->state_lock);
zpci_event_avail_existing_device(zdev, ccdf);
mutex_unlock(&zdev->state_lock);
+ guard(pci_rescan_remove)();
zpci_zdev_put(zdev);
}
diff --git a/arch/s390/pci/pci_iov.c b/arch/s390/pci/pci_iov.c
index 13050ce5c3e9..1f7e4dd018e7 100644
--- a/arch/s390/pci/pci_iov.c
+++ b/arch/s390/pci/pci_iov.c
@@ -38,10 +38,9 @@ void zpci_iov_map_resources(struct pci_dev *pdev)
void zpci_iov_remove_virtfn(struct pci_dev *pdev, int vfn)
{
- pci_lock_rescan_remove();
+ guard(pci_rescan_remove)();
/* Linux' vfid's start at 0 vfn at 1 */
pci_iov_remove_virtfn(pdev->physfn, vfn - 1);
- pci_unlock_rescan_remove();
}
static int zpci_iov_link_virtfn(struct pci_dev *pdev, struct pci_dev *virtfn, int vfid)
diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
index bbb76113a4d0..1e211bcb284e 100644
--- a/arch/s390/pci/pci_sysfs.c
+++ b/arch/s390/pci/pci_sysfs.c
@@ -76,10 +76,15 @@ static int _do_recover(struct pci_dev *pdev, struct zpci_dev *zdev)
return ret;
}
+/* So we don't have to mix cleanup.h helpers and gotos in recover_store() */
+DEFINE_CLASS(sysfs_break_active_protection, struct kernfs_node *,
+ if (_T) sysfs_unbreak_active_protection(_T),
+ sysfs_break_active_protection(kobj, attr),
+ struct kobject *kobj, const struct attribute *attr);
+
static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct kernfs_node *kn;
struct pci_dev *pdev = to_pci_dev(dev);
struct zpci_dev *zdev = to_zpci(pdev);
int ret = 0;
@@ -94,13 +99,13 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
* This is analogous to sdev_store_delete() in
* drivers/scsi/scsi_sysfs.c
*/
- kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+ CLASS(sysfs_break_active_protection, kn)(&dev->kobj, &attr->attr);
WARN_ON_ONCE(!kn);
/* Device needs to be configured and state must not change */
- mutex_lock(&zdev->state_lock);
+ guard(mutex)(&zdev->state_lock);
if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
- goto out;
+ return count;
/* device_remove_file() serializes concurrent calls ignoring all but
* the first
@@ -109,20 +114,14 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
/* A concurrent call to recover_store() may slip between
* sysfs_break_active_protection() and the sysfs file removal.
- * Once it unblocks from pci_lock_rescan_remove() the original pdev
+ * Once it unblocks from guard(pci_rescan_remove)() the original pdev
* will already be removed.
*/
- pci_lock_rescan_remove();
- if (pci_dev_is_added(pdev)) {
+ guard(pci_rescan_remove)();
+ if (pci_dev_is_added(pdev))
ret = _do_recover(pdev, zdev);
- }
pci_rescan_bus(zdev->zbus->bus);
- pci_unlock_rescan_remove();
-out:
- mutex_unlock(&zdev->state_lock);
- if (kn)
- sysfs_unbreak_active_protection(kn);
return ret ? ret : count;
}
static DEVICE_ATTR_WO(recover);
--
2.55.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
2026-09-24 16:29 ` [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release Benjamin Block
@ 2026-09-24 16:44 ` sashiko-bot
2026-09-25 5:20 ` Benjamin Block
2026-10-02 12:55 ` [PATCH v16 " Benjamin Block
1 sibling, 1 reply; 13+ messages in thread
From: sashiko-bot @ 2026-09-24 16:44 UTC (permalink / raw)
To: Benjamin Block
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, intel-xe, linux-pci
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: mixing goto and cleanup helpers in __zpci_event_error and pci_base_init
--
commit 703530c4b98b53581e1625ffc110fa9701e163ee
Author: Benjamin Block <bblock@linux.ibm.com>
s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
When removing PCI device or PCI bus objects there are a couple of
call-chains where it is possible that the kernel runs into a circular
deadlock involving the central pci_rescan_remove_lock.
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index d64c544b32b0c..4fc3e3dc87ad1 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
[ ... ]
> @@ -1208,7 +1212,8 @@ static int __init pci_base_init(void)
> if (rc)
> goto out_irq;
>
> - rc = zpci_scan_devices();
> + scoped_guard(pci_rescan_remove)
> + rc = zpci_scan_devices();
> if (rc)
> goto out_find;
>
[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
helpers here?
The pci_base_init() function contains multiple goto statements for error
handling, and this patch introduces a scoped_guard() into the same routine.
The cleanup.h guidelines state that usage of goto and cleanup helpers should
never be mixed in the same function.
[ ... ]
> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> index f317a1465dad8..268f435677ae3 100644
> --- a/arch/s390/pci/pci_event.c
> +++ b/arch/s390/pci/pci_event.c
[ ... ]
> @@ -382,6 +382,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
> pci_dev_put(pdev);
> no_pdev:
> mutex_unlock(&zdev->state_lock);
> + guard(pci_rescan_remove)();
> zpci_zdev_put(zdev);
> }
>
[Severity: Medium]
Can this introduction of guard() into __zpci_event_error() also cause issues
with mixed cleanup paradigms?
In __zpci_event_error(), there are goto statements used for error handling,
and this patch introduces a guard() right at the cleanup label. Mixing goto
with cleanup.h helpers in the same function goes against the subsystem
guidelines.
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790267348.git.bblock@linux.ibm.com?part=5
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
2026-09-24 16:44 ` sashiko-bot
@ 2026-09-25 5:20 ` Benjamin Block
0 siblings, 0 replies; 13+ messages in thread
From: Benjamin Block @ 2026-09-25 5:20 UTC (permalink / raw)
To: sashiko-reviews, Heiko Carstens, Vasily Gorbik, Alexander Gordeev
Cc: linux-s390, Christian Borntraeger, intel-xe, linux-pci
On Thu, Sep 24, 2026 at 04:44:06PM +0000, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index d64c544b32b0c..4fc3e3dc87ad1 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -1208,7 +1212,8 @@ static int __init pci_base_init(void)
> > if (rc)
> > goto out_irq;
> >
> > - rc = zpci_scan_devices();
> > + scoped_guard(pci_rescan_remove)
> > + rc = zpci_scan_devices();
> > if (rc)
> > goto out_find;
>
> The pci_base_init() function contains multiple goto statements for error
> handling, and this patch introduces a scoped_guard() into the same routine.
> The cleanup.h guidelines state that usage of goto and cleanup helpers should
> never be mixed in the same function.
--8<--
> > diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
> > index f317a1465dad8..268f435677ae3 100644
> > --- a/arch/s390/pci/pci_event.c
> > +++ b/arch/s390/pci/pci_event.c
> > @@ -382,6 +382,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
> > pci_dev_put(pdev);
> > no_pdev:
> > mutex_unlock(&zdev->state_lock);
> > + guard(pci_rescan_remove)();
> > zpci_zdev_put(zdev);
> > }
>
> In __zpci_event_error(), there are goto statements used for error handling,
> and this patch introduces a guard() right at the cleanup label. Mixing goto
> with cleanup.h helpers in the same function goes against the subsystem
> guidelines.
So as far as I know this rule from cleanup.h is motivated by cases where
someone jumps *into* a cleanup-scope and thus skips the initialization, but
executes the cleanup, which then breaks.
This shouldn't be a problem here, but if our s390 maintainers think this
should be changed regardless, I can use ordinary calls in those two cases,
since it's probably not worth converting the whole functions here.
--
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] 13+ messages in thread
* [PATCH v16 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
2026-09-24 16:29 ` [PATCH v15 5/5] s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release Benjamin Block
2026-09-24 16:44 ` sashiko-bot
@ 2026-10-02 12:55 ` Benjamin Block
1 sibling, 0 replies; 13+ messages in thread
From: Benjamin Block @ 2026-10-02 12:55 UTC (permalink / raw)
To: Benjamin Block, Bjorn Helgaas
Cc: Farhan Ali, linux-kernel, Heiko Carstens, Dragos Tatulea,
Omar Elghoul, Lukas Wunner, Matthew Brost, Halil Pasic,
Ionut Nechita, Sven Schnelle, Alexander Gordeev, Ionut Nechita,
Tobias Schumacher, Julian Ruess, Ionut Nechita, Michal Wajdeczko,
Keith Busch, Matthew Rosato, Christian Borntraeger, Gerd Bayer,
linux-intel-xe, Andreas Krebbel, Ramesh Errabolu, linux-s390,
Vasily Gorbik, Manivannan Sadhasivam, piotr.piorkowski,
Guenter Roeck, Niklas Schnelle, linux-pci, Benjamin Block, stable
When removing PCI device or PCI bus objects there are a couple of
call-chains where it is possible that the kernel runs into a circular
deadlock involving the central `pci_rescan_remove_lock`.
Two examples:
(A) Thread α receives a PCI event notifying the kernel that a PCI
virtual function has been moved into Reserved state, and so the PCI
subsystem will try to remove that PCI function. The call-chain for that
looks like this:
__zpci_event_availability()
-> zpci_zdev_put() # will lock(zpci_add_remove_lock),
# and lock(zpci_list_lock)
-> zpci_release_device() # will unlock(zpci_list_lock)
-> zpci_cleanup_bus_resources() # will lock(pci_rescan_remove_lock)
Thread β is triggered by userspace writing 0 into the SysFS attribute
`sriov_numvfs` of the parent PCI physical function of the same function
we just try to remove. This will also try to release the PCI virtual
function; but this time the call-chain looks like this:
sriov_numvfs_store()
-> ... (device driver dependent)
-> sriov_disable()
-> sriov_del_vfs() # will lock(pci_rescan_remove_lock)
-> ... (deep chain)
-> pci_release_dev()
-> pcibios_release_device()
-> zpci_zdev_put() # will lock(zpci_add_remove_lock)
If thread α and β coincide, this will result in a cyclic deadlock.
(B) Thread γ receives a PCI event notifying the kernel that one or more
PCI functions got hot plugged, and need to be configured. A possible
call-chain that might happen while the PCI subsystem is trying to add
those new function looks like this:
__zpci_event_availability()
-> ... (multiple ways in which a device gets added)
-> zpci_add_device() # will lock(zpci_add_remove_lock)
-> zpci_bus_device_register()
-> zpci_bus_put() # will lock(zbus_list_lock)
-> zpci_bus_release() # will unlock(zbus_list_lock)
# will lock(pci_rescan_remove_lock)
Now the same thread β as above in (A) might coincide, and again could
result in a cyclic deadlock.
`pci_rescan_remove_lock` has to be and is taken at a "high level" in
most call-chains since it is intended to protect/mutual exclude all
rescan and/or removal actions taken in the PCI subsystem. So to prevent
the outlined deadlock scenarios above remove it instead from the "low
level" release functions for both the PCI device and PCI bus objects.
Instead, lock `pci_rescan_remove_lock` in all call-chains leading to
those release functions:
* initialization of the PCI subsystem;
* processing of availability events (CRWs) for PCI functions;
* processing of error events (CRWs) for PCI functions;
* architecture specific release PCI device implementation.
Additionally, remove `pci_rescan_remove_lock` from zpci_bus_scan_bus()
since its only caller zpci_scan_devices() is now always called with
`pci_rescan_remove_lock` already held.
Lastly, document the new locking expectations after these changes. Add
sparse and lockdep annotations to functions that previously locked
`pci_rescan_remove_lock` explicitly, making sure the lock is now
already held when called. Additionally also add the annotations to
zpci_zdev_put() and zpci_bus_put() to make sure that every function that
potentially drops the last reference already holds the lock to prevent
surprises.
Fixes: 05bc1be6db4b2 ("s390/pci: create zPCI bus")
Fixes: ab909509850b2 ("PCI: s390: Fix use-after-free of PCI resources with per-function hotplug")
Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
arch/s390/pci/pci.c | 10 ++++++++--
arch/s390/pci/pci_bus.c | 15 ++++++++-------
arch/s390/pci/pci_event.c | 29 +++++++++++++++++++----------
arch/s390/pci/pci_iov.c | 3 +--
arch/s390/pci/pci_sysfs.c | 25 ++++++++++++-------------
5 files changed, 48 insertions(+), 34 deletions(-)
Hello all,
Since I didn't get any response so far I just rerolled the last patch
that had the medium complaints by Sashiko. Everything else is unchanged.
Change v15 -> v16:
* Patch 5/5: changed two findigs by Sashiko were cleanup helpers were mixed
with gotos in the same function against recommendations
This is no functional change, "just" syntax.
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index d64c544b32b0..92aa88fb9cfc 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -71,9 +71,11 @@ struct airq_iv *zpci_aif_sbv;
EXPORT_SYMBOL_GPL(zpci_aif_sbv);
void zpci_zdev_put(struct zpci_dev *zdev)
+ __must_hold(&pci_rescan_remove_lock)
{
if (!zdev)
return;
+ lockdep_assert_held(&pci_rescan_remove_lock);
mutex_lock(&zpci_add_remove_lock);
kref_put_lock(&zdev->kref, zpci_release_device, &zpci_list_lock);
mutex_unlock(&zpci_add_remove_lock);
@@ -583,11 +585,13 @@ int zpci_setup_bus_resources(struct zpci_dev *zdev)
}
static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
+ __must_hold(&pci_rescan_remove_lock)
{
struct resource *res;
int i;
- pci_lock_rescan_remove();
+ lockdep_assert_held(&pci_rescan_remove_lock);
+
for (i = 0; i < PCI_STD_NUM_BARS; i++) {
res = zdev->bars[i].res;
if (!res)
@@ -600,7 +604,6 @@ static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
kfree(res);
}
zdev->has_resources = 0;
- pci_unlock_rescan_remove();
}
int pcibios_device_add(struct pci_dev *pdev)
@@ -630,6 +633,7 @@ void pcibios_release_device(struct pci_dev *pdev)
{
struct zpci_dev *zdev = to_zpci(pdev);
+ guard(pci_rescan_remove)();
zpci_unmap_resources(pdev);
zpci_zdev_put(zdev);
}
@@ -1208,7 +1212,9 @@ static int __init pci_base_init(void)
if (rc)
goto out_irq;
+ pci_lock_rescan_remove();
rc = zpci_scan_devices();
+ pci_unlock_rescan_remove();
if (rc)
goto out_find;
diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index 36a4807285fa..c1b48b572e86 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -82,9 +82,8 @@ int zpci_bus_scan_device(struct zpci_dev *zdev)
if (!pdev)
return -ENODEV;
- pci_lock_rescan_remove();
+ guard(pci_rescan_remove)();
pci_bus_add_device(pdev);
- pci_unlock_rescan_remove();
return 0;
}
@@ -132,10 +131,13 @@ void zpci_bus_remove_device(struct zpci_dev *zdev, bool set_error)
* Return: 0 on success, an error value otherwise
*/
int zpci_bus_scan_bus(struct zpci_bus *zbus)
+ __must_hold(&pci_rescan_remove_lock)
{
struct zpci_dev *zdev;
int devfn, rc, ret = 0;
+ lockdep_assert_held(&pci_rescan_remove_lock);
+
for (devfn = 0; devfn < ZPCI_FUNCTIONS_PER_BUS; devfn++) {
zdev = zbus->function[devfn];
if (zdev && zdev->state == ZPCI_FN_STATE_CONFIGURED) {
@@ -145,10 +147,8 @@ int zpci_bus_scan_bus(struct zpci_bus *zbus)
}
}
- pci_lock_rescan_remove();
pci_scan_child_bus(zbus->bus);
pci_bus_add_devices(zbus->bus);
- pci_unlock_rescan_remove();
return ret;
}
@@ -214,11 +214,12 @@ static int zpci_bus_create_pci_bus(struct zpci_bus *zbus, struct zpci_dev *fr, s
* run of the function.
*/
static inline void zpci_bus_release(struct kref *kref)
- __releases(&zbus_list_lock)
+ __releases(&zbus_list_lock) __must_hold(&pci_rescan_remove_lock)
{
struct zpci_bus *zbus = container_of(kref, struct zpci_bus, kref);
lockdep_assert_held(&zbus_list_lock);
+ lockdep_assert_held(&pci_rescan_remove_lock);
list_del(&zbus->bus_next);
mutex_unlock(&zbus_list_lock);
@@ -229,14 +230,12 @@ static inline void zpci_bus_release(struct kref *kref)
*/
if (zbus->bus) {
- pci_lock_rescan_remove();
pci_stop_root_bus(zbus->bus);
zpci_free_domain(zbus->domain_nr);
pci_free_resource_list(&zbus->resources);
pci_remove_root_bus(zbus->bus);
- pci_unlock_rescan_remove();
}
zpci_remove_parent_msi_domain(zbus);
@@ -250,7 +249,9 @@ static inline void __zpci_bus_get(struct zpci_bus *zbus)
}
static inline void zpci_bus_put(struct zpci_bus *zbus)
+ __must_hold(&pci_rescan_remove_lock)
{
+ lockdep_assert_held(&pci_rescan_remove_lock);
kref_put_mutex(&zbus->kref, zpci_bus_release, &zbus_list_lock);
}
diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
index d6af4015223e..076d2b3c342b 100644
--- a/arch/s390/pci/pci_event.c
+++ b/arch/s390/pci/pci_event.c
@@ -385,7 +385,9 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
pci_dev_put(pdev);
no_pdev:
mutex_unlock(&zdev->state_lock);
+ pci_lock_rescan_remove();
zpci_zdev_put(zdev);
+ pci_unlock_rescan_remove();
}
void zpci_event_error(void *data)
@@ -431,6 +433,7 @@ static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
if (ccdf->pec != 0x0306)
return false;
/* 0x308 or 0x302 for multiple devices */
+ guard(pci_rescan_remove)();
zpci_remove_reserved_devices();
zpci_scan_devices();
return true;
@@ -439,26 +442,29 @@ static bool zpci_event_avail_any_device(struct zpci_ccdf_avail *ccdf)
static void zpci_event_avail_new_device(struct zpci_ccdf_avail *ccdf)
{
struct zpci_dev *zdev;
+ bool freed = false;
switch (ccdf->pec) {
case 0x0301: /* Reserved|Standby -> Configured */
zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
if (IS_ERR(zdev))
break;
- if (zpci_add_device(zdev)) {
- kfree(zdev);
- break;
- }
- zpci_scan_configured_device(zdev, ccdf->fh);
+ scoped_guard(pci_rescan_remove)
+ if (zpci_add_device(zdev)) {
+ kfree(zdev);
+ /* break; is incompatible w/ scoped_guard() */
+ freed = true;
+ }
+ if (!freed)
+ zpci_scan_configured_device(zdev, ccdf->fh);
break;
case 0x0302: /* Reserved -> Standby */
zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
if (IS_ERR(zdev))
break;
- if (zpci_add_device(zdev)) {
- kfree(zdev);
- break;
- }
+ scoped_guard(pci_rescan_remove)
+ if (zpci_add_device(zdev))
+ kfree(zdev);
break;
}
}
@@ -500,11 +506,13 @@ static void zpci_event_avail_existing_device(struct zpci_dev *zdev, struct zpci_
/* The 0x0304 event may immediately reserve the device */
if (!clp_get_state(zdev->fid, &state) &&
state == ZPCI_FN_STATE_RESERVED) {
+ guard(pci_rescan_remove)();
zpci_device_reserved(zdev);
}
break;
case 0x0308: /* Standby -> Reserved */
- zpci_device_reserved(zdev);
+ scoped_guard(pci_rescan_remove)
+ zpci_device_reserved(zdev);
break;
}
}
@@ -526,5 +534,6 @@ void zpci_event_availability(void *data)
mutex_lock(&zdev->state_lock);
zpci_event_avail_existing_device(zdev, ccdf);
mutex_unlock(&zdev->state_lock);
+ guard(pci_rescan_remove)();
zpci_zdev_put(zdev);
}
diff --git a/arch/s390/pci/pci_iov.c b/arch/s390/pci/pci_iov.c
index 13050ce5c3e9..1f7e4dd018e7 100644
--- a/arch/s390/pci/pci_iov.c
+++ b/arch/s390/pci/pci_iov.c
@@ -38,10 +38,9 @@ void zpci_iov_map_resources(struct pci_dev *pdev)
void zpci_iov_remove_virtfn(struct pci_dev *pdev, int vfn)
{
- pci_lock_rescan_remove();
+ guard(pci_rescan_remove)();
/* Linux' vfid's start at 0 vfn at 1 */
pci_iov_remove_virtfn(pdev->physfn, vfn - 1);
- pci_unlock_rescan_remove();
}
static int zpci_iov_link_virtfn(struct pci_dev *pdev, struct pci_dev *virtfn, int vfid)
diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
index bbb76113a4d0..1e211bcb284e 100644
--- a/arch/s390/pci/pci_sysfs.c
+++ b/arch/s390/pci/pci_sysfs.c
@@ -76,10 +76,15 @@ static int _do_recover(struct pci_dev *pdev, struct zpci_dev *zdev)
return ret;
}
+/* So we don't have to mix cleanup.h helpers and gotos in recover_store() */
+DEFINE_CLASS(sysfs_break_active_protection, struct kernfs_node *,
+ if (_T) sysfs_unbreak_active_protection(_T),
+ sysfs_break_active_protection(kobj, attr),
+ struct kobject *kobj, const struct attribute *attr);
+
static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- struct kernfs_node *kn;
struct pci_dev *pdev = to_pci_dev(dev);
struct zpci_dev *zdev = to_zpci(pdev);
int ret = 0;
@@ -94,13 +99,13 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
* This is analogous to sdev_store_delete() in
* drivers/scsi/scsi_sysfs.c
*/
- kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+ CLASS(sysfs_break_active_protection, kn)(&dev->kobj, &attr->attr);
WARN_ON_ONCE(!kn);
/* Device needs to be configured and state must not change */
- mutex_lock(&zdev->state_lock);
+ guard(mutex)(&zdev->state_lock);
if (zdev->state != ZPCI_FN_STATE_CONFIGURED)
- goto out;
+ return count;
/* device_remove_file() serializes concurrent calls ignoring all but
* the first
@@ -109,20 +114,14 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
/* A concurrent call to recover_store() may slip between
* sysfs_break_active_protection() and the sysfs file removal.
- * Once it unblocks from pci_lock_rescan_remove() the original pdev
+ * Once it unblocks from guard(pci_rescan_remove)() the original pdev
* will already be removed.
*/
- pci_lock_rescan_remove();
- if (pci_dev_is_added(pdev)) {
+ guard(pci_rescan_remove)();
+ if (pci_dev_is_added(pdev))
ret = _do_recover(pdev, zdev);
- }
pci_rescan_bus(zdev->zbus->bus);
- pci_unlock_rescan_remove();
-out:
- mutex_unlock(&zdev->state_lock);
- if (kn)
- sysfs_unbreak_active_protection(kn);
return ret ? ret : count;
}
static DEVICE_ATTR_WO(recover);
--
2.56.0
^ permalink raw reply related [flat|nested] 13+ messages in thread