From: Niklas Schnelle <[email protected]>
[ Upstream commit 576c75e36c689bec6a940e807bae27291ab0c0de ]
With zpci_disable() working, lockdep detected a potential deadlock
(lockdep output at the end).
The deadlock is between recovering a PCI function via the
/sys/bus/pci/devices/<dev>/recover
attribute vs powering it off via
/sys/bus/pci/slots/<slot>/power.
The fix is analogous to the changes in commit 0ee223b2e1f6 ("scsi: core:
Avoid that SCSI device removal through sysfs triggers a deadlock")
that fixed a potential deadlock on removing a SCSI device via sysfs.
[ 204.830107] ======================================================
[ 204.830109] WARNING: possible circular locking dependency detected
[ 204.830111] 5.5.0-rc2-06072-gbc03ecc9a672 #6 Tainted: G W
[ 204.830112] ------------------------------------------------------
[ 204.830113] bash/1034 is trying to acquire lock:
[ 204.830115] 0000000192a1a610 (kn->count#200){++++}, at: kernfs_remove_by_name_ns+0x5c/0xa8
[ 204.830122]
but task is already holding lock:
[ 204.830123] 00000000c16134a8 (pci_rescan_remove_lock){+.+.}, at: pci_stop_and_remove_bus_device_locked+0x26/0x48
[ 204.830128]
which lock already depends on the new lock.
[ 204.830129]
the existing dependency chain (in reverse order) is:
[ 204.830130]
-> #1 (pci_rescan_remove_lock){+.+.}:
[ 204.830134] validate_chain+0x93a/0xd08
[ 204.830136] __lock_acquire+0x4ae/0x9d0
[ 204.830137] lock_acquire+0x114/0x280
[ 204.830140] __mutex_lock+0xa2/0x960
[ 204.830142] mutex_lock_nested+0x32/0x40
[ 204.830145] recover_store+0x4c/0xa8
[ 204.830147] kernfs_fop_write+0xe6/0x218
[ 204.830151] vfs_write+0xb0/0x1b8
[ 204.830152] ksys_write+0x6c/0xf8
[ 204.830154] system_call+0xd8/0x2d8
[ 204.830155]
-> #0 (kn->count#200){++++}:
[ 204.830187] check_noncircular+0x1e6/0x240
[ 204.830189] check_prev_add+0xfc/0xdb0
[ 204.830190] validate_chain+0x93a/0xd08
[ 204.830192] __lock_acquire+0x4ae/0x9d0
[ 204.830193] lock_acquire+0x114/0x280
[ 204.830194] __kernfs_remove.part.0+0x2e4/0x360
[ 204.830196] kernfs_remove_by_name_ns+0x5c/0xa8
[ 204.830198] remove_files.isra.0+0x4c/0x98
[ 204.830199] sysfs_remove_group+0x66/0xc8
[ 204.830201] sysfs_remove_groups+0x46/0x68
[ 204.830204] device_remove_attrs+0x52/0x90
[ 204.830207] device_del+0x182/0x418
[ 204.830208] pci_remove_bus_device+0x8a/0x130
[ 204.830210] pci_stop_and_remove_bus_device_locked+0x3a/0x48
[ 204.830212] disable_slot+0x68/0x100
[ 204.830213] power_write_file+0x7c/0x130
[ 204.830215] kernfs_fop_write+0xe6/0x218
[ 204.830217] vfs_write+0xb0/0x1b8
[ 204.830218] ksys_write+0x6c/0xf8
[ 204.830220] system_call+0xd8/0x2d8
[ 204.830221]
other info that might help us debug this:
[ 204.830223] Possible unsafe locking scenario:
[ 204.830224] CPU0 CPU1
[ 204.830225] ---- ----
[ 204.830226] lock(pci_rescan_remove_lock);
[ 204.830227] lock(kn->count#200);
[ 204.830229] lock(pci_rescan_remove_lock);
[ 204.830231] lock(kn->count#200);
[ 204.830233]
*** DEADLOCK ***
[ 204.830234] 4 locks held by bash/1034:
[ 204.830235] #0: 00000001b6fbc498 (sb_writers#4){.+.+}, at: vfs_write+0x158/0x1b8
[ 204.830239] #1: 000000018c9f5090 (&of->mutex){+.+.}, at: kernfs_fop_write+0xaa/0x218
[ 204.830242] #2: 00000001f7da0810 (kn->count#235){.+.+}, at: kernfs_fop_write+0xb6/0x218
[ 204.830245] #3: 00000000c16134a8 (pci_rescan_remove_lock){+.+.}, at: pci_stop_and_remove_bus_device_locked+0x26/0x48
[ 204.830248]
stack backtrace:
[ 204.830250] CPU: 2 PID: 1034 Comm: bash Tainted: G W 5.5.0-rc2-06072-gbc03ecc9a672 #6
[ 204.830252] Hardware name: IBM 8561 T01 703 (LPAR)
[ 204.830253] Call Trace:
[ 204.830257] [<00000000c05e10c0>] show_stack+0x88/0xf0
[ 204.830260] [<00000000c112dca4>] dump_stack+0xa4/0xe0
[ 204.830261] [<00000000c0694c06>] check_noncircular+0x1e6/0x240
[ 204.830263] [<00000000c0695bec>] check_prev_add+0xfc/0xdb0
[ 204.830264] [<00000000c06971da>] validate_chain+0x93a/0xd08
[ 204.830266] [<00000000c06994c6>] __lock_acquire+0x4ae/0x9d0
[ 204.830267] [<00000000c069867c>] lock_acquire+0x114/0x280
[ 204.830269] [<00000000c09ca15c>] __kernfs_remove.part.0+0x2e4/0x360
[ 204.830270] [<00000000c09cb5c4>] kernfs_remove_by_name_ns+0x5c/0xa8
[ 204.830272] [<00000000c09cee14>] remove_files.isra.0+0x4c/0x98
[ 204.830274] [<00000000c09cf2ae>] sysfs_remove_group+0x66/0xc8
[ 204.830276] [<00000000c09cf356>] sysfs_remove_groups+0x46/0x68
[ 204.830278] [<00000000c0e3dfe2>] device_remove_attrs+0x52/0x90
[ 204.830280] [<00000000c0e40382>] device_del+0x182/0x418
[ 204.830281] [<00000000c0dcfd7a>] pci_remove_bus_device+0x8a/0x130
[ 204.830283] [<00000000c0dcfe92>] pci_stop_and_remove_bus_device_locked+0x3a/0x48
[ 204.830285] [<00000000c0de7190>] disable_slot+0x68/0x100
[ 204.830286] [<00000000c0de6514>] power_write_file+0x7c/0x130
[ 204.830288] [<00000000c09cc846>] kernfs_fop_write+0xe6/0x218
[ 204.830290] [<00000000c08f3480>] vfs_write+0xb0/0x1b8
[ 204.830291] [<00000000c08f378c>] ksys_write+0x6c/0xf8
[ 204.830293] [<00000000c1154374>] system_call+0xd8/0x2d8
[ 204.830294] INFO: lockdep is turned off.
Signed-off-by: Niklas Schnelle <[email protected]>
Reviewed-by: Peter Oberparleiter <[email protected]>
Signed-off-by: Vasily Gorbik <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
arch/s390/pci/pci_sysfs.c | 63 ++++++++++++++++++++++++++-------------
1 file changed, 42 insertions(+), 21 deletions(-)
diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
index a433ba01a3175..215f17437a4f6 100644
--- a/arch/s390/pci/pci_sysfs.c
+++ b/arch/s390/pci/pci_sysfs.c
@@ -13,6 +13,8 @@
#include <linux/stat.h>
#include <linux/pci.h>
+#include "../../../drivers/pci/pci.h"
+
#include <asm/sclp.h>
#define zpci_attr(name, fmt, member) \
@@ -49,31 +51,50 @@ static DEVICE_ATTR_RO(mio_enabled);
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;
-
- if (!device_remove_file_self(dev, attr))
- return count;
-
+ int ret = 0;
+
+ /* Can't use device_remove_self() here as that would lead us to lock
+ * the pci_rescan_remove_lock while holding the device' kernfs lock.
+ * This would create a possible deadlock with disable_slot() which is
+ * not directly protected by the device' kernfs lock but takes it
+ * during the device removal which happens under
+ * pci_rescan_remove_lock.
+ *
+ * This is analogous to sdev_store_delete() in
+ * drivers/scsi/scsi_sysfs.c
+ */
+ kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+ WARN_ON_ONCE(!kn);
+ /* device_remove_file() serializes concurrent calls ignoring all but
+ * the first
+ */
+ device_remove_file(dev, 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
+ * will already be removed.
+ */
pci_lock_rescan_remove();
- pci_stop_and_remove_bus_device(pdev);
- ret = zpci_disable_device(zdev);
- if (ret)
- goto error;
-
- ret = zpci_enable_device(zdev);
- if (ret)
- goto error;
-
- pci_rescan_bus(zdev->bus);
+ if (pci_dev_is_added(pdev)) {
+ pci_stop_and_remove_bus_device(pdev);
+ ret = zpci_disable_device(zdev);
+ if (ret)
+ goto out;
+
+ ret = zpci_enable_device(zdev);
+ if (ret)
+ goto out;
+ pci_rescan_bus(zdev->bus);
+ }
+out:
pci_unlock_rescan_remove();
-
- return count;
-
-error:
- pci_unlock_rescan_remove();
- return ret;
+ if (kn)
+ sysfs_unbreak_active_protection(kn);
+ return ret ? ret : count;
}
static DEVICE_ATTR_WO(recover);
--
2.20.1
On Fri, Feb 14, 2020 at 10:41:26AM -0500, Sasha Levin wrote:
> From: Niklas Schnelle <[email protected]>
>
> [ Upstream commit 576c75e36c689bec6a940e807bae27291ab0c0de ]
>
> With zpci_disable() working, lockdep detected a potential deadlock
> (lockdep output at the end).
>
> The deadlock is between recovering a PCI function via the
>
> /sys/bus/pci/devices/<dev>/recover
>
> attribute vs powering it off via
>
> /sys/bus/pci/slots/<slot>/power.
>
> The fix is analogous to the changes in commit 0ee223b2e1f6 ("scsi: core:
> Avoid that SCSI device removal through sysfs triggers a deadlock")
> that fixed a potential deadlock on removing a SCSI device via sysfs.
[ ... snip ... ]
While technically useful on its own this commit really should go together with
the following upstream commit:
17cdec960cf776b20b1fb08c622221babe591d51
("s390/pci: Recover handle in clp_set_pci_fn()")
While the problem fixed here is independent, writing to the power/recover
attributes will often fail due to an inconsistent function handle without the
second commit.
In particular without it a PCI function in the error state can not be
recovered or powered off.
I would recommend adding the second commit to the backports as well.
Thanks,
Niklas Schnelle
--
Niklas Schnelle
Linux on Z Development
On Mon, Feb 17, 2020 at 10:31:56AM +0100, Niklas Schnelle wrote:
>On Fri, Feb 14, 2020 at 10:41:26AM -0500, Sasha Levin wrote:
>> From: Niklas Schnelle <[email protected]>
>>
>> [ Upstream commit 576c75e36c689bec6a940e807bae27291ab0c0de ]
>>
>> With zpci_disable() working, lockdep detected a potential deadlock
>> (lockdep output at the end).
>>
>> The deadlock is between recovering a PCI function via the
>>
>> /sys/bus/pci/devices/<dev>/recover
>>
>> attribute vs powering it off via
>>
>> /sys/bus/pci/slots/<slot>/power.
>>
>> The fix is analogous to the changes in commit 0ee223b2e1f6 ("scsi: core:
>> Avoid that SCSI device removal through sysfs triggers a deadlock")
>> that fixed a potential deadlock on removing a SCSI device via sysfs.
>[ ... snip ... ]
>
>While technically useful on its own this commit really should go together with
>the following upstream commit:
>
>17cdec960cf776b20b1fb08c622221babe591d51
>("s390/pci: Recover handle in clp_set_pci_fn()")
>
>While the problem fixed here is independent, writing to the power/recover
>attributes will often fail due to an inconsistent function handle without the
>second commit.
>In particular without it a PCI function in the error state can not be
>recovered or powered off.
>
>I would recommend adding the second commit to the backports as well.
I took that commit as well, thank you.
--
Thanks,
Sasha