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

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.

Signed-off-by Benjamin Block <[email protected]>

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

drivers/pci/pci.h +0 -2

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

drivers/pci/probe.c +0 -0

index bccc7a4bdd79..e5b12878e972 100644

drivers/pci/probe.c +0 -0

 void pci_lock_rescan_remove(void)
 {

include/linux/pci.h +1 -0

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>

drivers/pci/probe.c EXPORT_SYMBOL_GPL(pci_rescan_bus); · line 3512

Quoted 9 lines · Benjamin Block

drivers/pci/probe.c +1 -0

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

This has a (rather trivial) merge conflict with Ionut's patch which at the same time is a prerequisite for this series. Sadly since that isn't in linux-next yet I'm not sure how to best handle this. Maybe it would make sense to just include it in this series? @Ionut would that be ok for you?

Quoted 13 lines

include/linux/pci.h +1 -0

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>

include/linux/pci.h void set_pcie_hotplug_bridge(struct pci_dev *pdev); · line 1540

Quoted 7 lines

include/linux/pci.h +1 -0

@@ -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);
 

I do see Keith's argument that proliferation of the rescan/remove lock is to be minimized. That said, since user's of this header can already lock/unlock I don't think this patch makes matters worse. In fact we want this patch to be able to add better lockdep asserts so it will help against misuse.

With that feel free to add:

Reviewed-by Niklas Schnelle <[email protected]>

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().

Signed-off-by Benjamin Block <[email protected]>

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

include/linux/pci.h +1 -0

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>
 

Quoted 10 lines · 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().

Quoted 5 lines

Signed-off-by Benjamin Block <[email protected]>

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

Looks good to me. Thank you.

Reviewed-by Niklas Schnelle <[email protected]>

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.

Fixes 05bc1be6db4b2 ("s390/pci: create zPCI bus")

Fixes ab909509850b2 ("PCI: s390: Fix use-after-free of PCI resources with per-function hotplug")

Signed-off-by Benjamin Block <[email protected]>

---
 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 +++------
 5 files changed, 39 insertions(+), 27 deletions(-)

Quoted 3 lines · Benjamin Block

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.

Now that this depends on Ionut's patch at[0] and exploits the fact that the rescan/remove lock can be taken recursively we may want to adjust and thereby shorten this description to take this into account dropping the recursive locking as deadlock reason.

Quoted 4 lines

For example:

(A) Thread α receives a CRW event notifying the kernel that a PCI

Nit: Other PCI related code calls these "PCI events" even though technically they use the Channel Report Word (CRW) mechanism and thus are CRW events.

Quoted 11 lines

Fixes 05bc1be6db4b2 ("s390/pci: create zPCI bus")

Fixes ab909509850b2 ("PCI: s390: Fix use-after-free of PCI resources with per-function hotplug")

Signed-off-by Benjamin Block <[email protected]>

---
 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 +++------
 5 files changed, 39 insertions(+), 27 deletions(-)

I had not previously looked at your commit message from the point of view when it is ordered aferr Ionut's patch[0], which it has to for the release case. So I ended up with a few comments now even though I previously read it multiple times and was just very happy with the nice and readable call chain and locking order explanations.

Either way the code itself looks good to me and I also did quite a bit of testing with this and the previous versions. Trying hot(un)plug and SR-IOV with various PCI devices and also just ran this and the previous version on several test systems for a few days doing other testing and development work.

I also put quite a bit of effort into trying to modify this and/or other s390 PCI code to not require the reentrant behavior of the pci_rescan_remove_lock but failed. The really tricky part to me is how e.g. a driver can hold on to a struct pci_dev even after it has been removed while we have to keep both the struct zpci_dev and struct zpci_bus and associated struct pci_bus exactly until the last pci_dev_put(). And then needing to remove the PCI bus via zpci_bus_release() we need to have the pci_rescan_remove lock but now this can happen, in principle, at any pci_dev_put() and that of course sometimes happens when we already have the pci_rescan_remove_lock and sometimes without it.

So feel free to add my:

Reviewed-by Niklas Schnelle <[email protected]>

Tested-by Niklas Schnelle <[email protected]>