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

Niklas already mentioned it in his recent comments on discussions about pci_rescan_remove_lock here
https://lore.kernel.org/linux-pci/[email protected]/
and here
https://lore.kernel.org/linux-pci/[email protected]/
; we recently found a couple of deadlocks in the s390 architecture PCI implementation with hotplug events on our platform.

So far these have not been observed because on s390 it was not usual to have both PF and attached VFs in the same Linux instance. So far PCI devices have largely been either available as PF without SR-IOV, or as VF without the PF being visible in the same instance. This left us with some blindspots w.r.t. the locking issues here.
This is now changing, and with that we started running into these
deadlocks.

Please Note: this patchset strictly depends on Ionut Nechita's patch that makes
pci_lock_rescan_remove() reentrant:
https://lore.kernel.org/linux-pci/[email protected]/

Sicne the discussion so far sounded positive towards the change I decided to base some of the changes in this patchset on the assumption that his patch gets merged before mine. Otherwise there will be rescursive deadlocks.

Patch 01 helps us insofar it enables us to use lockdep annotations in the architecture code.
Patch 02 goes into detail what deadlocks exactly, and how they are fixed. Patch 03 and 04 make it possible to use lock guards for pci_rescan_remove_lock and make use of them in the s390 architecture PCI implementation.

I've run a /lot/ of tests with affected PCI adapters:
* 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;
* disable/enable power with the hotplug SysFS attribute on PF and VF;
* run zpcictl with --reset/--reset-fw on PF and VF;
* run Configure Off and Configure On on both the PF and VF from a Service Element.

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

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

 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 |  9 +++------
 drivers/pci/pci.h         |  2 --
 drivers/pci/probe.c       |  1 +
 include/linux/pci.h       |  5 +++++
 8 files changed, 45 insertions(+), 29 deletions(-)

base-commit: 5ee8dbf54602dc340d6235b1d6aa17c0f283f48c

2.53.0

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 pci_rescan_remove_lock itself is private to objects residing in drivers/pci/ via the header drivers/pci/pci.h.

With that setup 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.

Since it is useful for pci_rescan_remove_lock to have such annotations, move the variable declaration into include/linux/pci.h.

---
 drivers/pci/pci.h   | 2 --
 drivers/pci/probe.c | 1 +
 include/linux/pci.h | 2 ++
 3 files changed, 3 insertions(+), 2 deletions(-)

index 13d998fbacce..6d611523420f 100644
@@ -110,8 +110,6 @@ struct pcie_tlp_log;
 extern const unsigned char pcie_link_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);

index bccc7a4bdd79..e5b12878e972 100644
@@ -3509,6 +3509,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);
 
 void pci_lock_rescan_remove(void)
 {

index 1c270f1d5123..fd7a962a64ef 100644
@@ -39,6 +39,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>

@@ -1533,6 +1534,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.53.0

This big lock for pci scanning is way to easy to misuse to create deadlocks, many of which still exist today, so I'm not sure making it easier to access is the right direction.

I can fully appreciate that, having had to deal with bug reports that stem from it's use for several months at this point.

The thing is, I really would love to be able to annotate our architecture code where appropriate, and I didn't see any other option. Explicit annotations in the code highlight far better than putting a comment in the function/-header.

Best Regards und Beste Grüße, Benjamin Block
               PGP KeyID: 9610 2BB8 2E17 6F65 2362  6DF2 46E0 4E05 67A3 2E9E

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 or recursive deadlock involving taking the central pci_rescan_remove_lock.

For example:

(A) Thread α receives a CRW 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() # 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) Consider thread β from above; if the thread was to follow the same outlined call-chain for thread α, and not fall into the cyclic deadlock, it would recursive deadlock:
sriov_numvfs_store() # will lock(pci_rescan_remove_lock)

    -> ... (deep chain)
    -> pci_release_dev()
    -> pcibios_release_device()
    -> 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)

(C) And even in case pci_rescan_remove_lock was removed from zpci_cleanup_bus_resources(), the resulting call-chain would recursive deadlock when it tries to release the associated PCI bus:
sriov_numvfs_store() # will lock(pci_rescan_remove_lock)

    -> ... (deep chain)
    -> pci_release_dev()
    -> pcibios_release_device()
    -> zpci_zdev_put()              # will   lock(zpci_add_remove_lock),
                                    # and    lock(zpci_list_lock)
    -> zpci_release_device()        # will unlock(zpci_list_lock)
    -> zpci_bus_device_unregister()
    -> zpci_bus_put()               # will   lock(zbus_list_lock)
    -> zpci_bus_release()           # will unlock(zbus_list_lock),
                                    # will   lock(pci_rescan_remove_lock)

It can also easily be seen that scenario (C) offers the same risk as (A) for a cyclic deadlock in cases where pci_rescan_remove_lock is first locked, and the PCI bus released under its protection.

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 function 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 it is now always already taken when called.

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.

---
 arch/s390/pci/pci.c       | 11 +++++++++--
 arch/s390/pci/pci_bus.c   | 12 +++++++-----
 arch/s390/pci/pci_event.c | 21 +++++++++++++++++++--
 3 files changed, 35 insertions(+), 9 deletions(-)

index 2a430722cbe4..fd16e6ad21c1 100644
@@ -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);

@@ -582,11 +584,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)

@@ -599,7 +603,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)

@@ -629,8 +632,10 @@ void pcibios_release_device(struct pci_dev *pdev)
 {
 	struct zpci_dev *zdev = to_zpci(pdev);
 
+	pci_lock_rescan_remove();
 	zpci_unmap_resources(pdev);
 	zpci_zdev_put(zdev);
+	pci_unlock_rescan_remove();
 }
 
 int pcibios_enable_device(struct pci_dev *pdev, int mask)

@@ -1208,7 +1213,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;
 

index 36a4807285fa..2b598222c621 100644
@@ -132,10 +132,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 +148,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 +215,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 +231,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 +250,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);
 }
 

index 839bd91c056e..edfaeed737ac 100644
@@ -342,7 +342,9 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf)
 no_pdev:
 	if (zdev)
 		mutex_unlock(&zdev->state_lock);
+	pci_lock_rescan_remove();
 	zpci_zdev_put(zdev);
+	pci_unlock_rescan_remove();
 }
 
 void zpci_event_error(void *data)

@@ -387,6 +389,7 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
 	struct zpci_dev *zdev = get_zdev_by_fid(ccdf->fid);
 	bool existing_zdev = !!zdev;
 	enum zpci_state state;
+	int rc;
 
 	zpci_dbg(3, "avl fid:%x, fh:%x, pec:%x\n",
 		 ccdf->fid, ccdf->fh, ccdf->pec);

@@ -400,7 +403,10 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
 			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_CONFIGURED);
 			if (IS_ERR(zdev))
 				break;
-			if (zpci_add_device(zdev)) {
+			pci_lock_rescan_remove();
+			rc = zpci_add_device(zdev);
+			pci_unlock_rescan_remove();
+			if (rc) {
 				kfree(zdev);
 				break;
 			}

@@ -419,7 +425,10 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
 			zdev = zpci_create_device(ccdf->fid, ccdf->fh, ZPCI_FN_STATE_STANDBY);
 			if (IS_ERR(zdev))
 				break;
-			if (zpci_add_device(zdev)) {
+			pci_lock_rescan_remove();
+			rc = zpci_add_device(zdev);
+			pci_unlock_rescan_remove();
+			if (rc) {
 				kfree(zdev);
 				break;
 			}

@@ -450,25 +459,33 @@ static void __zpci_event_availability(struct zpci_ccdf_avail *ccdf)
 			/* The 0x0304 event may immediately reserve the device */
 			if (!clp_get_state(zdev->fid, &state) &&
 			    state == ZPCI_FN_STATE_RESERVED) {
+				pci_lock_rescan_remove();
 				zpci_device_reserved(zdev);
+				pci_unlock_rescan_remove();
 			}
 		}
 		break;
 	case 0x0306: /* 0x308 or 0x302 for multiple devices */
+		pci_lock_rescan_remove();
 		zpci_remove_reserved_devices();
 		zpci_scan_devices();
+		pci_unlock_rescan_remove();
 		break;
 	case 0x0308: /* Standby -> Reserved */
 		if (!zdev)
 			break;
+		pci_lock_rescan_remove();
 		zpci_device_reserved(zdev);
+		pci_unlock_rescan_remove();
 		break;
 	default:
 		break;
 	}
 	if (existing_zdev) {
 		mutex_unlock(&zdev->state_lock);
+		pci_lock_rescan_remove();
 		zpci_zdev_put(zdev);
+		pci_unlock_rescan_remove();
 	}
 }
 

2.53.0

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 as via guard() or scoped_guard().

---
 include/linux/pci.h | 3 +++
 1 file changed, 3 insertions(+)

index fd7a962a64ef..4c41b5a2c90a 100644
@@ -41,6 +41,7 @@
 #include <linux/msi_api.h>
 #include <linux/mutex.h>
 #include <uapi/linux/pci.h>
+#include <linux/cleanup.h>
 
 #include <linux/pci_ids.h>
 

@@ -1537,6 +1538,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.53.0

There are quite a few places in the s390 architecture code for the PCI subsystem where the kernel needs to lock pci_rescan_remove_lock now; which is done by calling pci_lock_rescan_remove() to lock, and pci_unlock_rescan_remove() to unlock the mutex.

Instead of always manually calling both functions, which induces a certain amount of visual clutter, and in some cases of errors, cleanup, and jumplabels more complexity, use either guard() or scoped_guard() depending on the context.

Convert all users in the s390 architecture code for PCI.

---
 arch/s390/pci/pci.c       |  8 +++----
 arch/s390/pci/pci_bus.c   |  3 +--
 arch/s390/pci/pci_event.c | 45 +++++++++++++++++----------------------
 arch/s390/pci/pci_iov.c   |  3 +--
 arch/s390/pci/pci_sysfs.c |  9 +++-----
 5 files changed, 27 insertions(+), 41 deletions(-)

index fd16e6ad21c1..86ef1e516857 100644
@@ -632,10 +632,9 @@ void pcibios_release_device(struct pci_dev *pdev)
 {
 	struct zpci_dev *zdev = to_zpci(pdev);
 
-	pci_lock_rescan_remove();
+	guard(pci_rescan_remove)();
 	zpci_unmap_resources(pdev);
 	zpci_zdev_put(zdev);
-	pci_unlock_rescan_remove();
 }
 
 int pcibios_enable_device(struct pci_dev *pdev, int mask)

@@ -1213,9 +1212,8 @@ static int __init pci_base_init(void)
 	if (rc)
 		goto out_irq;
 
-	pci_lock_rescan_remove();
-	rc = zpci_scan_devices();
-	pci_unlock_rescan_remove();
+	scoped_guard(pci_rescan_remove)
+		rc = zpci_scan_devices();
 	if (rc)
 		goto out_find;
 

index 2b598222c621..c1b48b572e86 100644
@@ -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;
 }

index 13050ce5c3e9..1f7e4dd018e7 100644
@@ -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)

index c2444a23e26c..f5027aa95928 100644
@@ -98,9 +98,9 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *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

@@ -112,15 +112,12 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
 	 * Once it unblocks from pci_lock_rescan_remove() the original pdev
 	 * will already be removed.
 	 */
-	pci_lock_rescan_remove();
+	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;

2.53.0

The main intention here was to make it easier on me (and others) to retrofit this in stable and/or distribution Kernels. I know this isn't a main concern for the upstream master, but it would make my life a bit easier :)

Although, ofc., it isn't required, so in doubt, I can change it. Given that the Maintainers are actually OK with the use of guards, in any case.

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

I do like the guards but thinking about it, I also do have a slight preference for adding the guard support first and using it directly. I'm not entirely convinced this makes it harder to backport either. From a quick check it looks like all relevant distros have the macro and we do try to keep the code close to upstream so would just end up backporting the lock guard conversion too maybe immediately maybe later.

Thanks,
Niklas