Skip to content

PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks

Benjamin Block intel-xe linux-pci linux-s390 lkml sashiko-reviews stable 1 version In review

Get series with b4
b4 am [email protected]

Versions newest first

  1. v15 Latest5 patches 2 repliesIn review

Changes in v15 from the cover letter

* Rebased the patchset on v7.3-rc4
* Patch 1/5: changed type of pci_rescan_remove_depth to size_t
* Patch 1/5: changed accesses of pci_rescan_remove_owner to use READ_ONCE() and/or WRITE_ONCE() since they might happen outside of the lock, and so might be subject to tearing or some such
* Patch 1/5: added comments clarifying that access to pci_rescan_remove_depth is always protected by pci_rescan_remove_lock
* Patch 1/5: since the only Reviewed-by was by myself on the last iteration I removed it
* Patch 5/5: fixed a bug with the use of scoped_guard() in switch statements -> break doesn't work as it seems it should, because scoped_guard() is implemented as for statement
* Patch 5/5: fixed a bug in recover_store() where it wouldn't reverse the call to sysfs_break_active_protection() in case of an early error-return
* Patch 5/5: since I had to make these bug-fixes I removed Niklas' Reviewed-by

Patches in v15 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/5PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfsdrivers/pci/iov.c drivers/pci/probe.c+30 -60 replies
2/5PCI: Fix AB-BA deadlock between device_lock and pci_rescan_remove_lock in remove_storedrivers/pci/pci-sysfs.c+29 -10 replies
3/5PCI: Move declaration of pci_rescan_remove_lock into public pci.hdrivers/pci/pci.h drivers/pci/probe.c include/linux/pci.h+3 -20 replies
4/5PCI: Provide lock guard for pci_rescan_remove_lockinclude/linux/pci.h+3 -00 replies
5/5s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device releasearch/s390/pci/pci.c arch/s390/pci/pci_bus.c arch/s390/pci/pci_event.c +2 more+47 -352 replies

Reviews trailers per patch

3 of 5 patches have a Reviewed-by or Acked-by.

PatchNiklas SchnelleBenjamin Block
1/5
2/5R-b R-b T-b
3/5R-b
4/5R-b
5/5

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v15 thread

Cover letter [PATCH v15 0/5] PCI/IOV: Fix SR-IOV locking races and AB-BA deadlocks

Hello Bjorn,

This is the continuation and combination of two patchsets from earlier
this year that try to fix races and various deadlocks involving the
global mutex pci_rescan_remove_owner:
https://lore.kernel.org/linux-pci/cover.1776839248.git.ionut.nechita%40windriver.com/ [v14]
https://lore.kernel.org/linux-pci/cover.1776868550.git.bblock%40linux.ibm.com/ [v4]

Since we couldn't make any progress since then I decided to rebase and combine them, hoping this would "revitalize" the topic.

v14 -> v15:
* Rebased the patchset on v7.3-rc4
* Patch 1/5: changed type of pci_rescan_remove_depth to size_t
* Patch 1/5: changed accesses of pci_rescan_remove_owner to use READ_ONCE() and/or WRITE_ONCE() since they might happen outside of the lock, and so might be subject to tearing or some such
* Patch 1/5: added comments clarifying that access to pci_rescan_remove_depth is always protected by pci_rescan_remove_lock
* Patch 1/5: since the only Reviewed-by was by myself on the last iteration I removed it
* Patch 5/5: fixed a bug with the use of scoped_guard() in switch statements -> break doesn't work as it seems it should, because scoped_guard() is implemented as for statement
* Patch 5/5: fixed a bug in recover_store() where it wouldn't reverse the call to sysfs_break_active_protection() in case of an early error-return
* Patch 5/5: since I had to make these bug-fixes I removed Niklas' Reviewed-by

Overview:

Patch 1/5: Fixes a regression introduced by
05703271c3cd ("PCI/IOV: Add PCI rescan-remove locking when enabling/disabling SR-IOV") that moves the locking of pci_rescan_remove_lock to a higher
level in the call-chain when enabling/disabling SR-IOV, but by
doing so removed it entirely from the driver's .remove() call-back call-chain.
To add the lock back into that call-chain and also prevent
resulting recursive deadlocks make the lock reentrant via the
existing functions pci_lock_rescan_remove()/pci_unlock_rescan_remove().
Patch 2/5: With the locking back in the .remove() call-back call-chain
it is now possible to trigger an AB-BA deadlock between device
driver unbinds and concurrent PCI device removes. Fix this by unbinding the device driver before starting PCI device
removal (and locking pci_rescan_remove_lock).
Patch 3/5: In order to use annotations like __must_hold() or assertions
like lockdep_assert_held() on pci_rescan_remove_lock in
code outside the PCI core (such as PCI architecture
implementations) move its declaration to the global linux/pci.h.
This doesn't increase the "surface" from where it is possible
to use the lock, since it is already possible today to grab
the lock from said code with the functions pci_lock_rescan_remove()/pci_unlock_rescan_remove().
Patch 4/5: Provide a lock guard for pci_rescan_remove_lock so it is
possible to be used in the linux/cleanup.h macros.
Patch 5/5: Fix several AB-BA deadlocks in the s390 PCI implementation
involving pci_rescan_remove_lock by moving the lock to
higher levels in the respective call-chains.

The race that is fixed in Patch 1/5 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)

We have been running this patch series for several months internally during our development on s390 since otherwise we would routinely "kill" our development machines due to deadlocks. I'm not aware of issues with it that were observed.

I've run a lot of tests with affected PCI adapters (some of which are specific to the IBM Z platform):
* enable/disable SR-IOV on the PF;
* run FLR reset on PF and VF;
* run Bus reset on PF and VF;
* run s390's recover SysFS attribute on PF and VF;
* remove/re-add PCI devices via the remove SysFS attribute;
* unbind/re-bind PCI devices to the vfio-pci device driver;
* disable/enable power with the hotplug SysFS attribute on PF and VF;
* run zpcictl with --reset/--reset-fw on PF and VF (s390);
* remove/re-add vfio modules with bound PCI devices;
* run Configure Off and Configure On on both the PF and VF from a Service Element (s390).

There is no more deadlocks and no other lockdep warnings I've witnessed.

History:

PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock:

PCI: s390/pci: Fix deadlocks on s390 when releasing zPCI-bus or -device objects:

Benjamin Block (3):
PCI: Move declaration of pci_rescan_remove_lock into public pci.h
PCI: Provide lock guard for pci_rescan_remove_lock s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device
release

Ionut Nechita (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

 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 ++++++++++++-------------
 drivers/pci/iov.c         |  9 +++++----
 drivers/pci/pci-sysfs.c   | 30 +++++++++++++++++++++++++++++-
 drivers/pci/pci.h         |  2 --
 drivers/pci/probe.c       | 28 ++++++++++++++++++++++++++--
 include/linux/pci.h       |  5 +++++
 10 files changed, 112 insertions(+), 44 deletions(-)

base-commit: 93f51579e7df248780214094418f205253383cc5

Signature · 1 line
2.55.0