PCI: s390/pci: Fix deadlocks on s390 when releasing zPCI-bus or -device objects
v4 cover PCI: s390/pci: Fix deadlocks on s390 when releasing zPCI-bus or -device objects
v3 -> v4:
* remove internal tracking ID from patch 03
v2 -> v3:
* added Reviewed-by and Tested-by from Niklas
* base series on current version of Ionut's patch series "PCI/IOV: Fix SR-IOV locking races and AB-BA deadlock"
https://lore.kernel.org/linux-pci/cover.1776839248.git.ionut.nechita%40windriver.com/T/#
to prevent small merge-conflict in patch 01
* adapted description of patch 03 so it reflects the point that the series is now based on Ionut's patch series, and certain deadlocks can't happen anymore (recursive), but others still can (the AB-BA cyclic variants)
v1 -> v2:
* combine patch 02 and 04 - fix and use of guards [Ilpo, Niklas]
* rephrase description of patch 01 to point out that it is already possible today to lock/unlock pci_rescan_remove_lock anywhere
* added Fixes: tags to patch 03 - the fix
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 blind spots 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/cover.1776839248.git.ionut.nechita%40windriver.com/T/#
Since 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 recursive deadlocks.
Patch 01 helps us insofar it enables us to use lockdep annotations in the
architecture code.
Patch 02 makes it possible to use lock guards for pci_rescan_remove_lock. Patch 03 goes into detail what deadlocks exactly exist today, and fixes them.
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;
* 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;
* 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.
There is no more deadlocks and no other lockdep warnings I've witnessed.
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
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: 028ef9c96e96197026887c0f092424679298aae8
prerequisite-patch-id: 04db39c9d883c6d06c9b2400fc445c62177f1c5b
prerequisite-patch-id: 68e07de292969a95b72a26153893281558c3eb0d
Signature · 1 line
2.54.0
PATCH v4 1/3 PCI: Move declaration of pci_rescan_remove_lock into public pci.h
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.
--- 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
@@ -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 +1 -0
@@ -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);static const struct task_struct *pci_rescan_remove_owner;static unsigned int pci_rescan_remove_depth;
include/linux/pci.h +2 -0
@@ -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 );
Signature · 1 line
2.54.0
PATCH v4 2/3 PCI: Provide lock guard for pci_rescan_remove_lock
In reply to Benjamin BlockMake 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().
--- include/linux/pci.h | 3 +++ 1 file changed, 3 insertions(+)
include/linux/pci.h +3 -0
@@ -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);
Signature · 1 line
2.54.0
PATCH v4 3/3 s390/pci: Fix circular/recursive deadlocks in PCI-bus and -device release
In reply to Benjamin BlockWhen 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.
--- 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(-)
arch/s390/pci/pci.c +8 -3
@@ -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,6 +632,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;
arch/s390/pci/pci_bus.c +8 -7
@@ -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); }
arch/s390/pci/pci_event.c +19 -9
@@ -342,6 +342,7 @@ static void __zpci_event_error(struct zpci_ccdf_err *ccdf) no_pdev:if (zdev)mutex_unlock (&zdev->state_lock); +guard (pci_rescan_remove)();zpci_zdev_put (zdev); }
@@ -400,9 +401,11 @@ 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)) { -kfree (zdev); -break ; +scoped_guard (pci_rescan_remove) { +if (zpci_add_device (zdev)) { +kfree (zdev); +break ; + } } }else {if (zdev->state ==ZPCI_FN_STATE_RESERVED )
@@ -419,9 +422,11 @@ 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)) { -kfree (zdev); -break ; +scoped_guard (pci_rescan_remove) { +if (zpci_add_device (zdev)) { +kfree (zdev); +break ; + } } }else {if (zdev->state ==ZPCI_FN_STATE_RESERVED )
@@ -450,24 +455,29 @@ 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 ) { +guard (pci_rescan_remove)();zpci_device_reserved (zdev); } }break ;case 0x0306 :/* 0x308 or 0x302 for multiple devices */ -zpci_remove_reserved_devices (); -zpci_scan_devices (); + scoped_guard(pci_rescan_remove) { +zpci_remove_reserved_devices (); +zpci_scan_devices (); + }break ;case 0x0308 :/* Standby -> Reserved */ if (!zdev)break ; -zpci_device_reserved (zdev); +scoped_guard (pci_rescan_remove) +zpci_device_reserved (zdev);break ;default :break ; }if (existing_zdev) {mutex_unlock (&zdev->state_lock); +guard (pci_rescan_remove)();zpci_zdev_put (zdev); } }
arch/s390/pci/pci_iov.c +1 -2
@@ -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)
arch/s390/pci/pci_sysfs.c +3 -6
@@ -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;
Signature · 1 line
2.54.0