All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marek Vasut <marek.vasut@gmail.com>
To: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org, Tho Vu <tho.vu.wh@rvc.renesas.com>
Subject: [RFC][PATCH] PCI: Avoid PCI device removing/rescanning through sysfs triggers a deadlock
Date: Tue,  6 Nov 2018 00:25:00 +0100	[thread overview]
Message-ID: <20181105232500.19146-1-marek.vasut+renesas@gmail.com> (raw)

From: Tho Vu <tho.vu.wh@rvc.renesas.com>

This patch fixes deadlock warning in removing/rescanning through sysfs
when CONFIG_PROVE_LOCKING is enabled.

The issue can be reproduced by these steps:
1. Enable CONFIG_PROVE_LOCKING via defconfig or menuconfig
2. Insert Ethernet card into PCIe CH0 and start up.
   After kernel starting up, execute the following command.
   echo 1 > /sys/class/pci_bus/0000\:00/device/0000\:00\:00.0/remove
3. Rescan PCI device by this command
   echo 1 > /sys/class/pci_bus/0000\:00/rescan

The deadlock warnings will occur.
============================================
WARNING: possible recursive locking detected
4.14.70-ltsi-yocto-standard #27 Not tainted
--------------------------------------------
sh/3402 is trying to acquire lock:
 (kn->count#78){++++}, at: kernfs_remove_by_name_ns+0x50/0xa8

but task is already holding lock:
 (kn->count#78){++++}, at: kernfs_remove_self+0xe0/0x130

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(kn->count#78);
  lock(kn->count#78);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

4 locks held by sh/3402:
 #0:  (sb_writers#4){.+.+}, at: vfs_write+0x198/0x1b0
 #1:  (&of->mutex){+.+.}, at: kernfs_fop_write+0x108/0x210
 #2:  (kn->count#78){++++}, at: kernfs_remove_self+0xe0/0x130
 #3:  (pci_rescan_remove_lock){+.+.}, at: pci_lock_rescan_remove+0x1c/0x28

stack backtrace:
CPU: 3 PID: 3402 Comm: sh Not tainted 4.14.70-ltsi-yocto-standard #27
Hardware name: Renesas Salvator-X 2nd version board based on r8a7795
ES3.0+ with 8GiB (4 x 2 GiB) (DT)
Call trace:
dump_backtrace+0x0/0x3d8
show_stack+0x14/0x20
dump_stack+0xbc/0xf4
__lock_acquire+0x930/0x18a8
lock_acquire+0x48/0x68
__kernfs_remove+0x280/0x2f8
kernfs_remove_by_name_ns+0x50/0xa8
remove_files.isra.0+0x38/0x78
sysfs_remove_group+0x4c/0xa0
sysfs_remove_groups+0x38/0x60
device_remove_attrs+0x54/0x78
device_del+0x1ac/0x308
pci_remove_bus_device+0x78/0xf8
pci_remove_bus_device+0x34/0xf8
pci_stop_and_remove_bus_device_locked+0x24/0x38
remove_store+0x6c/0x78
dev_attr_store+0x18/0x28
sysfs_kf_write+0x4c/0x78
kernfs_fop_write+0x138/0x210
__vfs_write+0x18/0x118
vfs_write+0xa4/0x1b0
SyS_write+0x48/0xb0

This warning occurs due to a self-deletion attribute using in the sysfs
PCI device directory. This kind of attribute is really tricky,
it does not allow pci framework drop this attribute until all active
.show() and .store() callbacks have finished unless
sysfs_break_active_protection() is called.
Hence this patch avoids writing into this attribute triggers a deadlock.

Referrence commit 5b55b24cec4c ("scsi: core: Avoid that SCSI device
removal through sysfs triggers a deadlock")
of scsi driver

Signed-off-by: Tho Vu <tho.vu.wh@rvc.renesas.com>
---
 drivers/pci/pci-sysfs.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9ecfe13157c0..d522bd8368d9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -470,12 +470,22 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
 	unsigned long val;
+	struct kernfs_node *kn;
+
+	kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+	WARN_ON_ONCE(!kn);
 
 	if (kstrtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
-	if (val && device_remove_file_self(dev, attr))
+	if (val) {
+		device_remove_file(dev, attr);
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
+	}
+
+	if (kn)
+		sysfs_unbreak_active_protection(kn);
+
 	return count;
 }
 static struct device_attribute dev_remove_attr = __ATTR(remove,
@@ -487,11 +497,15 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 				    const char *buf, size_t count)
 {
 	unsigned long val;
+	struct kernfs_node *kn;
 	struct pci_bus *bus = to_pci_bus(dev);
 
 	if (kstrtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
+	kn = sysfs_break_active_protection(&dev->kobj, &attr->attr);
+	WARN_ON_ONCE(!kn);
+
 	if (val) {
 		pci_lock_rescan_remove();
 		if (!pci_is_root_bus(bus) && list_empty(&bus->devices))
@@ -500,6 +514,10 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 			pci_rescan_bus(bus);
 		pci_unlock_rescan_remove();
 	}
+
+	if (kn)
+		sysfs_unbreak_active_protection(kn);
+
 	return count;
 }
 static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
-- 
2.18.0

             reply	other threads:[~2018-11-06  8:47 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-05 23:25 Marek Vasut [this message]
2018-11-06  8:13 ` [RFC][PATCH] PCI: Avoid PCI device removing/rescanning through sysfs triggers a deadlock Geert Uytterhoeven
2018-11-06  9:45   ` Marek Vasut
2018-11-27 22:55 ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181105232500.19146-1-marek.vasut+renesas@gmail.com \
    --to=marek.vasut@gmail.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=tho.vu.wh@rvc.renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.