[PATCH RFC 0/2] avoid crashing when reading /proc/scsi/scsi and simultaneously removing devices

[PATCH RFC 0/2] avoid crashing when reading /proc/scsi/scsi and simultaneously removing devices

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

The klist traversal used by the reading of /proc/scsi/scsi is not interlocked
against device removal.  It takes a reference on the containing object, but
this does not prevent the device from being removed from the list.  Thus, we
get errors and eventually panic, as shown in the traces below.  Fix this by
keeping a klist iterator in the seq_file private data.

The problem can be easily reproduced by repeatedly increasing scsi_debug's
max_luns to 30 and then deleting the devices via sysfs, while simulatenously
accessing /proc/scsi/scsi.
    
>From a patch originally developed by David Jeffery <djeffery@redhat.com>

Dec  3 13:22:02 localhost kernel: WARNING: CPU: 2 PID: 28073 at include/linux/kref.h:47 klist_iter_init_node+0x3d/0x50()
Dec  3 13:22:02 localhost kernel: Modules linked in: scsi_debug x86_pkg_temp_thermal kvm_intel kvm irqbypass crc32c_intel joydev iTCO_wdt dcdbas ipmi_devintf acpi_power_meter iTCO_vendor_support ipmi_si imsghandler pcspkr wmi acpi_cpufreq tpm_tis tpm shpchp lpc_ich mfd_core nfsd nfs_acl lockd grace sunrpc tg3 ptp pps_core
Dec  3 13:22:02 localhost kernel: CPU: 2 PID: 28073 Comm: cat Not tainted 4.4.0-rc1+ #2
Dec  3 13:22:02 localhost kernel: Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22 11/19/2013
Dec  3 13:22:02 localhost kernel: ffffffff81a20e77 ffff880613acfd18 ffffffff81321eef 0000000000000000
Dec  3 13:22:02 localhost kernel: ffff880613acfd50 ffffffff8107ca52 ffff88061176b198 0000000000000000
Dec  3 13:22:02 localhost kernel: ffffffff814542b0 ffff880610cfb100 ffff88061176b198 ffff880613acfd60
Dec  3 13:22:02 localhost kernel: Call Trace:
Dec  3 13:22:02 localhost kernel: [<ffffffff81321eef>] dump_stack+0x44/0x55
Dec  3 13:22:02 localhost kernel: [<ffffffff8107ca52>] warn_slowpath_common+0x82/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff814542b0>] ? proc_scsi_show+0x20/0x20
Dec  3 13:22:02 localhost kernel: [<ffffffff8107cb4a>] warn_slowpath_null+0x1a/0x20
Dec  3 13:22:02 localhost kernel: [<ffffffff8167225d>] klist_iter_init_node+0x3d/0x50
Dec  3 13:22:02 localhost kernel: [<ffffffff81421d41>] bus_find_device+0x51/0xb0
Dec  3 13:22:02 localhost kernel: [<ffffffff814545ad>] scsi_seq_next+0x2d/0x40
Dec  3 13:22:02 localhost kernel: [<ffffffff811e2e50>] seq_read+0x290/0x370
Dec  3 13:22:02 localhost kernel: [<ffffffff81225be8>] proc_reg_read+0x48/0x70
Dec  3 13:22:02 localhost kernel: [<ffffffff811c07c8>] __vfs_read+0x28/0xd0
Dec  3 13:22:02 localhost kernel: [<ffffffff812b5323>] ? security_file_permission+0xa3/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0cf3>] ? rw_verify_area+0x53/0xf0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0e16>] vfs_read+0x86/0x130
Dec  3 13:22:02 localhost kernel: [<ffffffff811c1bb6>] SyS_read+0x46/0xa0
Dec  3 13:22:02 localhost kernel: [<ffffffff8167c717>] entry_SYSCALL_64_fastpath+0x12/0x6a
Dec  3 13:22:02 localhost kernel: ---[ end trace 99a60fb1c41fc8c9 ]---
Dec  3 13:22:02 localhost kernel: ------------[ cut here ]------------
Dec  3 13:22:02 localhost kernel: WARNING: CPU: 2 PID: 28073 at lib/klist.c:189 klist_release+0xa8/0xb0()
Dec  3 13:22:02 localhost kernel: Modules linked in: scsi_debug x86_pkg_temp_thermal kvm_intel kvm irqbypass crc32c_intel joydev iTCO_wdt dcdbas ipmi_devintf acpi_power_meter iTCO_vendor_support ipmi_si imsghandler pcspkr wmi acpi_cpufreq tpm_tis tpm shpchp lpc_ich mfd_core nfsd nfs_acl lockd grace sunrpc tg3 ptp pps_core
Dec  3 13:22:02 localhost kernel: CPU: 2 PID: 28073 Comm: cat Tainted: G        W       4.4.0-rc1+ #2
Dec  3 13:22:02 localhost kernel: Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22 11/19/2013
Dec  3 13:22:02 localhost kernel: ffffffff81aaa040 ffff880613acfcc0 ffffffff81321eef 0000000000000000
Dec  3 13:22:02 localhost kernel: ffff880613acfcf8 ffffffff8107ca52 dead0000000000f8 ffff880613acfd80
Dec  3 13:22:02 localhost kernel: ffff88060f7aa368 ffff88060f7aa380 ffff88061176b198 ffff880613acfd08
Dec  3 13:22:02 localhost kernel: Call Trace:
Dec  3 13:22:02 localhost kernel: [<ffffffff81321eef>] dump_stack+0x44/0x55
Dec  3 13:22:02 localhost kernel: [<ffffffff8107ca52>] warn_slowpath_common+0x82/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff8107cb4a>] warn_slowpath_null+0x1a/0x20
Dec  3 13:22:02 localhost kernel: [<ffffffff816720a8>] klist_release+0xa8/0xb0
Dec  3 13:22:02 localhost kernel: [<ffffffff814220a0>] ? bus_uevent_store+0x50/0x50
Dec  3 13:22:02 localhost kernel: [<ffffffff816723f5>] klist_next+0x95/0xf0
Dec  3 13:22:02 localhost kernel: [<ffffffff814542b0>] ? proc_scsi_show+0x20/0x20
Dec  3 13:22:02 localhost kernel: [<ffffffff81421d62>] bus_find_device+0x72/0xb0
Dec  3 13:22:02 localhost kernel: [<ffffffff814545ad>] scsi_seq_next+0x2d/0x40
Dec  3 13:22:02 localhost kernel: [<ffffffff811e2e50>] seq_read+0x290/0x370
Dec  3 13:22:02 localhost kernel: [<ffffffff81225be8>] proc_reg_read+0x48/0x70
Dec  3 13:22:02 localhost kernel: [<ffffffff811c07c8>] __vfs_read+0x28/0xd0
Dec  3 13:22:02 localhost kernel: [<ffffffff812b5323>] ? security_file_permission+0xa3/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0cf3>] ? rw_verify_area+0x53/0xf0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0e16>] vfs_read+0x86/0x130
Dec  3 13:22:02 localhost kernel: [<ffffffff811c1bb6>] SyS_read+0x46/0xa0
Dec  3 13:22:02 localhost kernel: [<ffffffff8167c717>] entry_SYSCALL_64_fastpath+0x12/0x6a
Dec  3 13:22:02 localhost kernel: ---[ end trace 99a60fb1c41fc8ca ]---
Dec  3 13:22:02 localhost kernel: ------------[ cut here ]------------
Dec  3 13:22:02 localhost kernel: WARNING: CPU: 2 PID: 28073 at lib/list_debug.c:53 __list_del_entry+0x61/0xc0()
Dec  3 13:22:02 localhost kernel: list_del corruption, ffff88060f7aa370->next is LIST_POISON1 (dead000000000100)
Dec  3 13:22:02 localhost kernel: Modules linked in: scsi_debug x86_pkg_temp_thermal kvm_intel kvm irqbypass crc32c_intel joydev iTCO_wdt dcdbas ipmi_devintf acpi_power_meter iTCO_vendor_support ipmi_si imsghandler pcspkr wmi acpi_cpufreq tpm_tis tpm shpchp lpc_ich mfd_core nfsd nfs_acl lockd grace sunrpc tg3 ptp pps_core
Dec  3 13:22:02 localhost kernel: CPU: 2 PID: 28073 Comm: cat Tainted: G        W       4.4.0-rc1+ #2
Dec  3 13:22:02 localhost kernel: Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22 11/19/2013
Dec  3 13:22:02 localhost kernel: ffffffff81a516b1 ffff880613acfc48 ffffffff81321eef ffff880613acfc90
Dec  3 13:22:02 localhost kernel: ffff880613acfc80 ffffffff8107ca52 ffff88060f7aa370 ffff880613acfd80
Dec  3 13:22:02 localhost kernel: ffff88060f7aa368 ffff88060f7aa380 ffff88061176b198 ffff880613acfce0
Dec  3 13:22:02 localhost kernel: Call Trace:
Dec  3 13:22:02 localhost kernel: [<ffffffff81321eef>] dump_stack+0x44/0x55
Dec  3 13:22:02 localhost kernel: [<ffffffff8107ca52>] warn_slowpath_common+0x82/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff8107cadc>] warn_slowpath_fmt+0x4c/0x50
Dec  3 13:22:02 localhost kernel: [<ffffffff8133e581>] __list_del_entry+0x61/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff8133e5ed>] list_del+0xd/0x30
Dec  3 13:22:02 localhost kernel: [<ffffffff81672021>] klist_release+0x21/0xb0
Dec  3 13:22:02 localhost kernel: [<ffffffff814220a0>] ? bus_uevent_store+0x50/0x50
Dec  3 13:22:02 localhost kernel: [<ffffffff816723f5>] klist_next+0x95/0xf0
Dec  3 13:22:02 localhost kernel: [<ffffffff814542b0>] ? proc_scsi_show+0x20/0x20
Dec  3 13:22:02 localhost kernel: [<ffffffff81421d62>] bus_find_device+0x72/0xb0
Dec  3 13:22:02 localhost kernel: [<ffffffff814545ad>] scsi_seq_next+0x2d/0x40
Dec  3 13:22:02 localhost kernel: [<ffffffff811e2e50>] seq_read+0x290/0x370
Dec  3 13:22:02 localhost kernel: [<ffffffff81225be8>] proc_reg_read+0x48/0x70
Dec  3 13:22:02 localhost kernel: [<ffffffff811c07c8>] __vfs_read+0x28/0xd0
Dec  3 13:22:02 localhost kernel: [<ffffffff812b5323>] ? security_file_permission+0xa3/0xc0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0cf3>] ? rw_verify_area+0x53/0xf0
Dec  3 13:22:02 localhost kernel: [<ffffffff811c0e16>] vfs_read+0x86/0x130
Dec  3 13:22:02 localhost kernel: [<ffffffff811c1bb6>] SyS_read+0x46/0xa0
Dec  3 13:22:02 localhost kernel: [<ffffffff8167c717>] entry_SYSCALL_64_fastpath+0x12/0x6a
Dec  3 13:22:02 localhost kernel: ---[ end trace 99a60fb1c41fc8cb ]---
Dec  3 13:22:03 localhost kernel: general protection fault: 0000 [#1] SMP
Dec  3 13:22:03 localhost kernel: Modules linked in: scsi_debug x86_pkg_temp_thermal kvm_intel kvm irqbypass crc32c_intel joydev iTCO_wdt dcdbas ipmi_devintf acpi_power_meter iTCO_vendor_support ipmi_si imsghandler pcspkr wmi acpi_cpufreq tpm_tis tpm shpchp lpc_ich mfd_core nfsd nfs_acl lockd grace sunrpc tg3 ptp pps_core
Dec  3 13:22:03 localhost kernel: CPU: 2 PID: 28073 Comm: cat Tainted: G        W       4.4.0-rc1+ #2
Dec  3 13:22:03 localhost kernel: Hardware name: Dell Inc. PowerEdge R320/08VT7V, BIOS 2.0.22 11/19/2013
Dec  3 13:22:03 localhost kernel: task: ffff8806090ef080 ti: ffff880613acc000 task.ti: ffff880613acc000
Dec  3 13:22:03 localhost kernel: RIP: 0010:[<ffffffff816723b2>]  [<ffffffff816723b2>] klist_next+0x52/0xf0
Dec  3 13:22:03 localhost kernel: RSP: 0018:ffff880613acfd48  EFLAGS: 00010287
Dec  3 13:22:03 localhost kernel: RAX: ffff880612ea2ea8 RBX: dead0000000000f8 RCX: 0000000000000000
Dec  3 13:22:03 localhost kernel: RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffffffff81cfcab0
Dec  3 13:22:03 localhost kernel: RBP: ffff880613acfd70 R08: 0000000000000001 R09: 000000000000fffe
Dec  3 13:22:03 localhost kernel: R10: 0000000000002a81 R11: 0000000000000002 R12: ffff880613acfd80
Dec  3 13:22:03 localhost kernel: R13: ffffffff814220a0 R14: ffff88060f7aa368 R15: ffff88061176b101
Dec  3 13:22:03 localhost kernel: FS:  00007f47e9ca4700(0000) GS:ffff880617040000(0000) knlGS:0000000000000000
Dec  3 13:22:03 localhost kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Dec  3 13:22:03 localhost kernel: CR2: 0000000001d22038 CR3: 000000060724c000 CR4: 00000000000406e0
Dec  3 13:22:03 localhost kernel: Stack:
Dec  3 13:22:03 localhost kernel: ffff88061176b198 0000000000000000 ffffffff814542b0 ffff880610cfb100
Dec  3 13:22:03 localhost kernel: ffff88061176b198 ffff880613acfda8 ffffffff81421d62 ffff880612ea2ea8
Dec  3 13:22:03 localhost kernel: 0000000000000000 ffff88061176b198 ffff880613acff20 ffff880035f39b00
Dec  3 13:22:03 localhost kernel: Call Trace:
Dec  3 13:22:03 localhost kernel: [<ffffffff814542b0>] ? proc_scsi_show+0x20/0x20
Dec  3 13:22:03 localhost kernel: [<ffffffff81421d62>] bus_find_device+0x72/0xb0
Dec  3 13:22:03 localhost kernel: [<ffffffff814545ad>] scsi_seq_next+0x2d/0x40
Dec  3 13:22:03 localhost kernel: [<ffffffff811e2e50>] seq_read+0x290/0x370
Dec  3 13:22:03 localhost kernel: [<ffffffff81225be8>] proc_reg_read+0x48/0x70
Dec  3 13:22:03 localhost kernel: [<ffffffff811c07c8>] __vfs_read+0x28/0xd0
Dec  3 13:22:03 localhost kernel: [<ffffffff812b5323>] ? security_file_permission+0xa3/0xc0
Dec  3 13:22:03 localhost kernel: [<ffffffff811c0cf3>] ? rw_verify_area+0x53/0xf0
Dec  3 13:22:03 localhost kernel: [<ffffffff811c0e16>] vfs_read+0x86/0x130
Dec  3 13:22:03 localhost kernel: [<ffffffff811c1bb6>] SyS_read+0x46/0xa0
Dec  3 13:22:03 localhost kernel: [<ffffffff8167c717>] entry_SYSCALL_64_fastpath+0x12/0x6a
Dec  3 13:22:03 localhost kernel: Code: 8b 46 08 49 8d 7e 18 48 8d 58 f8 f0 41 83 6e 18 01 74 56 49 8b 04 24 45 31 ff 45 31 ed 48 39 c3 49 c7 44 24 08 00 00 00 00 74 20 <f6> 03 01 75 5c b8 01 00 00 00 f0 1 43 18 83 c0 01 83 f8 01
Dec  3 13:22:03 localhost kernel: RIP  [<ffffffff816723b2>] klist_next+0x52/0xf0
Dec  3 13:22:03 localhost kernel: RSP <ffff880613acfd48>
Dec  3 13:22:03 localhost kernel: ---[ end trace 99a60fb1c41fc8cc ]---

Ewan D. Milne (2):
  drivers/base: add bus_device_iter_init, bus_device_iter_next,
    bus_device_iter_exit
  scsi_proc: Change /proc/scsi/scsi to use bus device iterator

 drivers/base/bus.c       | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_proc.c | 49 ++++++++++++++++++++++++++++--------------------
 include/linux/device.h   | 14 ++++++++++++++
 3 files changed, 91 insertions(+), 20 deletions(-)

-- 
2.1.0

[PATCH RFC 1/2] drivers/base: add bus_device_iter_init, bus_device_iter_next, bus_device_iter_exit

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

These functions are needed to expose an iterator for SCSI usage.

>From a patch originally developed by David Jeffery <djeffery@redhat.com>

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/base/bus.c     | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  5 +++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 5005924..a472e46 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -318,6 +318,65 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
 EXPORT_SYMBOL_GPL(bus_for_each_dev);
 
 /**
+ * bus_device_iter_init - Initialize an iterator for walking a bus's devices.
+ * @iter: iterator structure to initialize
+ * @bus: bus type
+ *
+ * Initializes an iterator for safely walking a bus's list of devices.  The
+ * iterator can be used by bus_device_iter_next() to safely walk the list, even
+ * if a device is removed from the list while being examined.  Needs to be
+ * matched with a call to bus_device_iter_exit() to clean up the iterator when
+ * finished.
+ *
+ * Return 0 on success, non-zero on failure.  Iterator cannot be used
+ * for a non-zero result
+ */
+int bus_device_iter_init(struct klist_iter *iter,
+			 struct bus_type *bus)
+{
+	if (!bus || !bus->p)
+		return -EINVAL;
+
+	klist_iter_init_node(&bus->p->klist_devices, iter, NULL);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bus_device_iter_init);
+
+/**
+ * bus_device_iter_next - Get a bus's next device from the iterator.
+ * @iter: iterator structure from bus_device_iter_init()
+ *
+ * Finds the next valid device entry on a bus's device list.  Allows the list
+ * to be safely traversed by the caller even when other tasks remove devices
+ * from the list.
+ *
+ * Returns a reference to the next device, or NULL if no more devices.
+ */
+struct device *bus_device_iter_next(struct klist_iter *iter)
+{
+	struct device *dev;
+
+	while ((dev = next_device(iter)))
+		if (get_device(dev))
+			break;
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(bus_device_iter_next);
+
+/**
+ * bus_device_iter_exit - Clean up an iterator from walking a bus's device list.
+ * @iter: iterator structure from bus_device_iter_init() to clean up
+ *
+ * Clean up any remaining state after finishing walking a bus's device list.
+ */
+void bus_device_iter_exit(struct klist_iter *iter)
+{
+	klist_iter_exit(iter);
+}
+EXPORT_SYMBOL_GPL(bus_device_iter_exit);
+
+/**
  * bus_find_device - device iterator for locating a particular device.
  * @bus: bus type
  * @start: Device to begin with
diff --git a/include/linux/device.h b/include/linux/device.h
index b8f411b..a44d912 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -151,6 +151,11 @@ void subsys_dev_iter_exit(struct subsys_dev_iter *iter);
 
 int bus_for_each_dev(struct bus_type *bus, struct device *start, void *data,
 		     int (*fn)(struct device *dev, void *data));
+
+int bus_device_iter_init(struct klist_iter *iter, struct bus_type *bus);
+struct device *bus_device_iter_next(struct klist_iter *iter);
+void bus_device_iter_exit(struct klist_iter *iter);
+
 struct device *bus_find_device(struct bus_type *bus, struct device *start,
 			       void *data,
 			       int (*match)(struct device *dev, void *data));
-- 
2.1.0

[PATCH RFC 2/2] scsi_proc: Change /proc/scsi/scsi to use bus device iterator

[Ewan D. Milne]

From: "Ewan D. Milne" <emilne@redhat.com>

This prevents crashing due to accessing a removed element on the list,
the iterator will now hold the correct reference.  It was not sufficient
to rely on the klist's reference on the containing device object.

>From a patch originally developed by David Jeffery <djeffery@redhat.com>

Signed-off-by: Ewan D. Milne <emilne@redhat.com>
---
 drivers/scsi/scsi_proc.c | 49 ++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 251598e..6c1b79d 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -40,6 +40,11 @@
 /* 4K page size, but our output routines, use some slack for overruns */
 #define PROC_BLOCK_SIZE (3*1024)
 
+struct scsi_proc_state {
+	struct klist_iter iter;
+	int pos;
+};
+
 static struct proc_dir_entry *proc_scsi;
 
 /* Protect sht->present and sht->proc_dir */
@@ -370,47 +375,50 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
 	return err;
 }
 
-static int always_match(struct device *dev, void *data)
-{
-	return 1;
-}
-
-static inline struct device *next_scsi_device(struct device *start)
-{
-	struct device *next = bus_find_device(&scsi_bus_type, start, NULL,
-					      always_match);
-	put_device(start);
-	return next;
-}
-
 static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
 {
+	struct scsi_proc_state *state = sfile->private;
 	struct device *dev = NULL;
 	loff_t n = *pos;
+	int err;
+
+	err = bus_device_iter_init(&state->iter, &scsi_bus_type);
+	if (err < 0)
+		return ERR_PTR(err);
 
-	while ((dev = next_scsi_device(dev))) {
+	while ((dev = bus_device_iter_next(&state->iter))) {
 		if (!n--)
 			break;
-		sfile->private++;
+		put_device(dev);
+		state->pos++;
 	}
 	return dev;
 }
 
 static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
 {
+	struct scsi_proc_state *state = sfile->private;
+
 	(*pos)++;
-	sfile->private++;
-	return next_scsi_device(v);
+	put_device(v);
+	state->pos++;
+
+	return bus_device_iter_next(&state->iter);
 }
 
 static void scsi_seq_stop(struct seq_file *sfile, void *v)
 {
+	struct scsi_proc_state *state = sfile->private;
+
 	put_device(v);
+	bus_device_iter_exit(&state->iter);
 }
 
 static int scsi_seq_show(struct seq_file *sfile, void *dev)
 {
-	if (!sfile->private)
+	struct scsi_proc_state *state = sfile->private;
+
+	if (!state->pos)
 		seq_puts(sfile, "Attached devices:\n");
 
 	return proc_print_scsidevice(dev, sfile);
@@ -436,7 +444,8 @@ static int proc_scsi_open(struct inode *inode, struct file *file)
 	 * We don't really need this for the write case but it doesn't
 	 * harm either.
 	 */
-	return seq_open(file, &scsi_seq_ops);
+	return seq_open_private(file, &scsi_seq_ops,
+				sizeof(struct scsi_proc_state));
 }
 
 static const struct file_operations proc_scsi_operations = {
@@ -445,7 +454,7 @@ static const struct file_operations proc_scsi_operations = {
 	.read		= seq_read,
 	.write		= proc_scsi_write,
 	.llseek		= seq_lseek,
-	.release	= seq_release,
+	.release	= seq_release_private,
 };
 
 /**
-- 
2.1.0

Re: [PATCH RFC 0/2] avoid crashing when reading /proc/scsi/scsi and simultaneously removing devices

[Hannes Reinecke]

On 12/08/2015 03:08 PM, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
>
> The klist traversal used by the reading of /proc/scsi/scsi is not interlocked
> against device removal.  It takes a reference on the containing object, but
> this does not prevent the device from being removed from the list.  Thus, we
> get errors and eventually panic, as shown in the traces below.  Fix this by
> keeping a klist iterator in the seq_file private data.
>
> The problem can be easily reproduced by repeatedly increasing scsi_debug's
> max_luns to 30 and then deleting the devices via sysfs, while simulatenously
> accessing /proc/scsi/scsi.
>
>  From a patch originally developed by David Jeffery <djeffery@redhat.com>
>
That's now, what, the third attempt on fixing this?

All previous attempts have been rejected on the grounds that 
/proc/scsi/scsi is deprecated and we should allow any updates to it.

Maybe this time we get lucky ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 1/2] drivers/base: add bus_device_iter_init, bus_device_iter_next, bus_device_iter_exit

[Hannes Reinecke]

On 12/08/2015 03:08 PM, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
>
> These functions are needed to expose an iterator for SCSI usage.
>
>  From a patch originally developed by David Jeffery <djeffery@redhat.com>
>
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>   drivers/base/bus.c     | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/device.h |  5 +++++
>   2 files changed, 64 insertions(+)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5005924..a472e46 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -318,6 +318,65 @@ int bus_for_each_dev(struct bus_type *bus, struct device *start,
>   EXPORT_SYMBOL_GPL(bus_for_each_dev);
>
>   /**
> + * bus_device_iter_init - Initialize an iterator for walking a bus's devices.
> + * @iter: iterator structure to initialize
> + * @bus: bus type
> + *
> + * Initializes an iterator for safely walking a bus's list of devices.  The
> + * iterator can be used by bus_device_iter_next() to safely walk the list, even
> + * if a device is removed from the list while being examined.  Needs to be
> + * matched with a call to bus_device_iter_exit() to clean up the iterator when
> + * finished.
> + *
> + * Return 0 on success, non-zero on failure.  Iterator cannot be used
> + * for a non-zero result
> + */
> +int bus_device_iter_init(struct klist_iter *iter,
> +			 struct bus_type *bus)
> +{
> +	if (!bus || !bus->p)
> +		return -EINVAL;
> +
> +	klist_iter_init_node(&bus->p->klist_devices, iter, NULL);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bus_device_iter_init);
> +
> +/**
> + * bus_device_iter_next - Get a bus's next device from the iterator.
> + * @iter: iterator structure from bus_device_iter_init()
> + *
> + * Finds the next valid device entry on a bus's device list.  Allows the list
> + * to be safely traversed by the caller even when other tasks remove devices
> + * from the list.
> + *
> + * Returns a reference to the next device, or NULL if no more devices.
> + */
> +struct device *bus_device_iter_next(struct klist_iter *iter)
> +{
> +	struct device *dev;
> +
> +	while ((dev = next_device(iter)))
> +		if (get_device(dev))
> +			break;
> +
> +	return dev;
> +}
> +EXPORT_SYMBOL_GPL(bus_device_iter_next);
> +
> +/**
> + * bus_device_iter_exit - Clean up an iterator from walking a bus's device list.
> + * @iter: iterator structure from bus_device_iter_init() to clean up
> + *
> + * Clean up any remaining state after finishing walking a bus's device list.
> + */
> +void bus_device_iter_exit(struct klist_iter *iter)
> +{
> +	klist_iter_exit(iter);
> +}
> +EXPORT_SYMBOL_GPL(bus_device_iter_exit);
> +
> +/**
>    * bus_find_device - device iterator for locating a particular device.
>    * @bus: bus type
>    * @start: Device to begin with
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b8f411b..a44d912 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -151,6 +151,11 @@ void subsys_dev_iter_exit(struct subsys_dev_iter *iter);
>
>   int bus_for_each_dev(struct bus_type *bus, struct device *start, void *data,
>   		     int (*fn)(struct device *dev, void *data));
> +
> +int bus_device_iter_init(struct klist_iter *iter, struct bus_type *bus);
> +struct device *bus_device_iter_next(struct klist_iter *iter);
> +void bus_device_iter_exit(struct klist_iter *iter);
> +
>   struct device *bus_find_device(struct bus_type *bus, struct device *start,
>   			       void *data,
>   			       int (*match)(struct device *dev, void *data));
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 2/2] scsi_proc: Change /proc/scsi/scsi to use bus device iterator

[Hannes Reinecke]

On 12/08/2015 03:08 PM, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
>
> This prevents crashing due to accessing a removed element on the list,
> the iterator will now hold the correct reference.  It was not sufficient
> to rely on the klist's reference on the containing device object.
>
>  From a patch originally developed by David Jeffery <djeffery@redhat.com>
>
> Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> ---
>   drivers/scsi/scsi_proc.c | 49 ++++++++++++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
> index 251598e..6c1b79d 100644
> --- a/drivers/scsi/scsi_proc.c
> +++ b/drivers/scsi/scsi_proc.c
> @@ -40,6 +40,11 @@
>   /* 4K page size, but our output routines, use some slack for overruns */
>   #define PROC_BLOCK_SIZE (3*1024)
>
> +struct scsi_proc_state {
> +	struct klist_iter iter;
> +	int pos;
> +};
> +
>   static struct proc_dir_entry *proc_scsi;
>
>   /* Protect sht->present and sht->proc_dir */
> @@ -370,47 +375,50 @@ static ssize_t proc_scsi_write(struct file *file, const char __user *buf,
>   	return err;
>   }
>
> -static int always_match(struct device *dev, void *data)
> -{
> -	return 1;
> -}
> -
> -static inline struct device *next_scsi_device(struct device *start)
> -{
> -	struct device *next = bus_find_device(&scsi_bus_type, start, NULL,
> -					      always_match);
> -	put_device(start);
> -	return next;
> -}
> -
>   static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
>   {
> +	struct scsi_proc_state *state = sfile->private;
>   	struct device *dev = NULL;
>   	loff_t n = *pos;
> +	int err;
> +
> +	err = bus_device_iter_init(&state->iter, &scsi_bus_type);
> +	if (err < 0)
> +		return ERR_PTR(err);
>
> -	while ((dev = next_scsi_device(dev))) {
> +	while ((dev = bus_device_iter_next(&state->iter))) {
>   		if (!n--)
>   			break;
> -		sfile->private++;
> +		put_device(dev);
> +		state->pos++;
>   	}
>   	return dev;
>   }
>
>   static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
>   {
> +	struct scsi_proc_state *state = sfile->private;
> +
>   	(*pos)++;
> -	sfile->private++;
> -	return next_scsi_device(v);
> +	put_device(v);
> +	state->pos++;
> +
> +	return bus_device_iter_next(&state->iter);
>   }
>
>   static void scsi_seq_stop(struct seq_file *sfile, void *v)
>   {
> +	struct scsi_proc_state *state = sfile->private;
> +
>   	put_device(v);
> +	bus_device_iter_exit(&state->iter);
>   }
>
>   static int scsi_seq_show(struct seq_file *sfile, void *dev)
>   {
> -	if (!sfile->private)
> +	struct scsi_proc_state *state = sfile->private;
> +
> +	if (!state->pos)
>   		seq_puts(sfile, "Attached devices:\n");
>
>   	return proc_print_scsidevice(dev, sfile);
> @@ -436,7 +444,8 @@ static int proc_scsi_open(struct inode *inode, struct file *file)
>   	 * We don't really need this for the write case but it doesn't
>   	 * harm either.
>   	 */
> -	return seq_open(file, &scsi_seq_ops);
> +	return seq_open_private(file, &scsi_seq_ops,
> +				sizeof(struct scsi_proc_state));
>   }
>
>   static const struct file_operations proc_scsi_operations = {
> @@ -445,7 +454,7 @@ static const struct file_operations proc_scsi_operations = {
>   	.read		= seq_read,
>   	.write		= proc_scsi_write,
>   	.llseek		= seq_lseek,
> -	.release	= seq_release,
> +	.release	= seq_release_private,
>   };
>
>   /**
>
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/2] avoid crashing when reading /proc/scsi/scsi and simultaneously removing devices

[Martin K. Petersen]

>>>>> "Ewan" == Ewan D Milne <emilne@redhat.com> writes:

Ewan> The klist traversal used by the reading of /proc/scsi/scsi is not
Ewan> interlocked against device removal.  It takes a reference on the
Ewan> containing object, but this does not prevent the device from being
Ewan> removed from the list.  Thus, we get errors and eventually panic,
Ewan> as shown in the traces below.  Fix this by keeping a klist
Ewan> iterator in the seq_file private data.

Applied to 4.5/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering