[PATCH v5 0/7] Prepare for constifying SCSI host templates

[PATCH v5 0/7] Prepare for constifying SCSI host templates

[Bart Van Assche]

Hi Martin,

This patch series prepares for constifying SCSI host templates by moving the
members that are not constant out of the SCSI host template. This patch series
is based on Linus' master branch instead of the SCSI for-next branch. Please
consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v4:
- Added three additional patches: fail host creation if creating the proc
  directory fails, a use-after-free fix and an improvement for the kernel
  module unload code.

Changes compared to v3:
- Changed the 'present' counter from 8 to 32 bits.
- Fixed a bug in an error path (reported by John Garry).
- Changed EXPORT_SYMBOL() into EXPORT_SYMBOL_GPL().
- Split patch 1/3 into two patches.

Changes compared to v2:
- Optimized the show_info == NULL case.
- Added a patch that removes the code that clears the module pointer in the host
  template.

Changes compared to v1:
- Fix the CONFIG_SCSI_PROC_FS=n build.

Bart Van Assche (7):
  scsi: esas2r: Initialize two host template members implicitly
  scsi: esas2r: Introduce scsi_template_proc_dir()
  scsi: core: Fail host creation if creating the proc directory fails
  scsi: core: Introduce a new list for SCSI proc directory entries
  scsi: core: Fix a use-after-free related to releasing device handlers
  module: Improve support for asynchronous module exit code
  scsi: core: Improve SCSI device removal

 drivers/scsi/esas2r/esas2r_main.c |  19 +++--
 drivers/scsi/hosts.c              |   3 +-
 drivers/scsi/scsi_priv.h          |   6 +-
 drivers/scsi/scsi_proc.c          | 124 +++++++++++++++++++++++++-----
 drivers/scsi/scsi_sysfs.c         |   9 +--
 include/linux/module.h            |   1 +
 include/scsi/scsi_host.h          |  18 ++---
 kernel/module/main.c              |  10 +++
 8 files changed, 143 insertions(+), 47 deletions(-)

[PATCH v5 1/7] scsi: esas2r: Initialize two host template members implicitly

[Bart Van Assche]

Prepare for removing the 'proc_dir' and 'present' members from the SCSI
host template by implicitly initializing 'present' and 'emulated' in
'driver_template'.

Reviewed-by: John Garry <john.garry@huawei.com>
Cc: Bradley Grove <linuxdrivers@attotech.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/esas2r/esas2r_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
index 7a4eadad23d7..27f6e7ccded8 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -248,8 +248,6 @@ static struct scsi_host_template driver_template = {
 	.sg_tablesize			= SG_CHUNK_SIZE,
 	.cmd_per_lun			=
 		ESAS2R_DEFAULT_CMD_PER_LUN,
-	.present			= 0,
-	.emulated			= 0,
 	.proc_name			= ESAS2R_DRVR_NAME,
 	.change_queue_depth		= scsi_change_queue_depth,
 	.max_sectors			= 0xFFFF,

[PATCH v5 2/7] scsi: esas2r: Introduce scsi_template_proc_dir()

[Bart Van Assche]

Prepare for removing the 'proc_dir' and 'present' members from the SCSI
host template. This patch does not change any functionality.

Reviewed-by: John Garry <john.garry@huawei.com>
Cc: Bradley Grove <linuxdrivers@attotech.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/esas2r/esas2r_main.c | 17 +++++++++++------
 drivers/scsi/scsi_proc.c          | 11 +++++++++++
 include/scsi/scsi_host.h          |  6 ++++++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
index 27f6e7ccded8..d7a2c49ff5ee 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -635,10 +635,13 @@ static void __exit esas2r_exit(void)
 	esas2r_log(ESAS2R_LOG_INFO, "%s called", __func__);
 
 	if (esas2r_proc_major > 0) {
+		struct proc_dir_entry *proc_dir;
+
 		esas2r_log(ESAS2R_LOG_INFO, "unregister proc");
 
-		remove_proc_entry(ATTONODE_NAME,
-				  esas2r_proc_host->hostt->proc_dir);
+		proc_dir = scsi_template_proc_dir(esas2r_proc_host->hostt);
+		if (proc_dir)
+			remove_proc_entry(ATTONODE_NAME, proc_dir);
 		unregister_chrdev(esas2r_proc_major, ESAS2R_DRVR_NAME);
 
 		esas2r_proc_major = 0;
@@ -728,11 +731,13 @@ const char *esas2r_info(struct Scsi_Host *sh)
 			       esas2r_proc_major);
 
 		if (esas2r_proc_major > 0) {
-			struct proc_dir_entry *pde;
+			struct proc_dir_entry *proc_dir;
+			struct proc_dir_entry *pde = NULL;
 
-			pde = proc_create(ATTONODE_NAME, 0,
-					  sh->hostt->proc_dir,
-					  &esas2r_proc_ops);
+			proc_dir = scsi_template_proc_dir(sh->hostt);
+			if (proc_dir)
+				pde = proc_create(ATTONODE_NAME, 0, proc_dir,
+						  &esas2r_proc_ops);
 
 			if (!pde) {
 				esas2r_log_dev(ESAS2R_LOG_WARN,
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 95aee1ad1383..456b43097288 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -83,6 +83,17 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 				4 * PAGE_SIZE);
 }
 
+/**
+ * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
+ * @sht: SCSI host template pointer.
+ */
+struct proc_dir_entry *
+scsi_template_proc_dir(const struct scsi_host_template *sht)
+{
+	return sht->proc_dir;
+}
+EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
+
 static const struct proc_ops proc_scsi_ops = {
 	.proc_open	= proc_scsi_host_open,
 	.proc_release	= single_release,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 9b0a028bf053..030faca947d2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -751,6 +751,12 @@ extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int __must_check scsi_add_host_with_dma(struct Scsi_Host *,
 					       struct device *,
 					       struct device *);
+#if defined(CONFIG_SCSI_PROC_FS)
+struct proc_dir_entry *
+scsi_template_proc_dir(const struct scsi_host_template *sht);
+#else
+#define scsi_template_proc_dir(sht) NULL
+#endif
 extern void scsi_scan_host(struct Scsi_Host *);
 extern void scsi_rescan_device(struct device *);
 extern void scsi_remove_host(struct Scsi_Host *);

[PATCH v5 3/7] scsi: core: Fail host creation if creating the proc directory fails

[Bart Van Assche]

Users expect that the contents of /proc/scsi is in sync with the contents
of /sys/class/scsi_host. Hence fail host creation if creating the proc
directory fails.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/hosts.c     |  3 ++-
 drivers/scsi/scsi_priv.h |  4 ++--
 drivers/scsi/scsi_proc.c | 13 +++++++++----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 9857dba09c95..12346e2297fd 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -519,7 +519,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 			     "failed to create tmf workq\n");
 		goto fail;
 	}
-	scsi_proc_hostdir_add(shost->hostt);
+	if (scsi_proc_hostdir_add(shost->hostt) < 0)
+		goto fail;
 	return shost;
  fail:
 	/*
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f385b3f04d6e..8c2e32121db1 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -100,14 +100,14 @@ extern void scsi_evt_thread(struct work_struct *work);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
-extern void scsi_proc_hostdir_add(struct scsi_host_template *);
+extern int scsi_proc_hostdir_add(struct scsi_host_template *);
 extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
 extern void scsi_proc_host_add(struct Scsi_Host *);
 extern void scsi_proc_host_rm(struct Scsi_Host *);
 extern int scsi_init_procfs(void);
 extern void scsi_exit_procfs(void);
 #else
-# define scsi_proc_hostdir_add(sht)	do { } while (0)
+# define scsi_proc_hostdir_add(sht)	0
 # define scsi_proc_hostdir_rm(sht)	do { } while (0)
 # define scsi_proc_host_add(shost)	do { } while (0)
 # define scsi_proc_host_rm(shost)	do { } while (0)
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 456b43097288..8c84f1a74773 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -108,20 +108,25 @@ static const struct proc_ops proc_scsi_ops = {
  *
  * Sets sht->proc_dir to the new directory.
  */
-
-void scsi_proc_hostdir_add(struct scsi_host_template *sht)
+int scsi_proc_hostdir_add(struct scsi_host_template *sht)
 {
+	int ret = 0;
+
 	if (!sht->show_info)
-		return;
+		return 0;
 
 	mutex_lock(&global_host_template_mutex);
 	if (!sht->present++) {
 		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
-        	if (!sht->proc_dir)
+        	if (!sht->proc_dir) {
 			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
 			       __func__, sht->proc_name);
+			ret = -ENOMEM;
+		}
 	}
 	mutex_unlock(&global_host_template_mutex);
+
+	return ret;
 }
 
 /**

[PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries

[Bart Van Assche]

Instead of using scsi_host_template members to track the SCSI proc
directory entries, track these entries in a list. This patch changes the
time needed for looking up the proc dir pointer from O(1) into O(n). I
think this is acceptable since the number of SCSI host adapter types per
host is usually small (less than ten).

This patch has been tested by attaching two USB storage devices to a
qemu host:

$ grep -aH . /proc/scsi/usb-storage/*
/proc/scsi/usb-storage/7:   Host scsi7: usb-storage
/proc/scsi/usb-storage/7:       Vendor: QEMU
/proc/scsi/usb-storage/7:      Product: QEMU USB HARDDRIVE
/proc/scsi/usb-storage/7:Serial Number: 1-0000:00:02.1:00.0-6
/proc/scsi/usb-storage/7:     Protocol: Transparent SCSI
/proc/scsi/usb-storage/7:    Transport: Bulk
/proc/scsi/usb-storage/7:       Quirks: SANE_SENSE
/proc/scsi/usb-storage/8:   Host scsi8: usb-storage
/proc/scsi/usb-storage/8:       Vendor: QEMU
/proc/scsi/usb-storage/8:      Product: QEMU USB HARDDRIVE
/proc/scsi/usb-storage/8:Serial Number: 1-0000:00:02.1:00.0-7
/proc/scsi/usb-storage/8:     Protocol: Transparent SCSI
/proc/scsi/usb-storage/8:    Transport: Bulk
/proc/scsi/usb-storage/8:       Quirks: SANE_SENSE

This patch prepares for constifying most SCSI host templates.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_priv.h |   4 +-
 drivers/scsi/scsi_proc.c | 112 ++++++++++++++++++++++++++++++++-------
 include/scsi/scsi_host.h |  12 -----
 3 files changed, 94 insertions(+), 34 deletions(-)

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 8c2e32121db1..456ff53fe404 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -100,8 +100,8 @@ extern void scsi_evt_thread(struct work_struct *work);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
-extern int scsi_proc_hostdir_add(struct scsi_host_template *);
-extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
+extern int scsi_proc_hostdir_add(const struct scsi_host_template *);
+extern void scsi_proc_hostdir_rm(const struct scsi_host_template *);
 extern void scsi_proc_host_add(struct Scsi_Host *);
 extern void scsi_proc_host_rm(struct Scsi_Host *);
 extern int scsi_init_procfs(void);
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 8c84f1a74773..76229ca89e2f 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -43,8 +43,23 @@
 
 static struct proc_dir_entry *proc_scsi;
 
-/* Protect sht->present and sht->proc_dir */
+/* Protects scsi_proc_list */
 static DEFINE_MUTEX(global_host_template_mutex);
+static LIST_HEAD(scsi_proc_list);
+
+/**
+ * struct scsi_proc_entry - (host template, SCSI proc dir) association
+ * @entry: entry in scsi_proc_list.
+ * @sht: SCSI host template associated with the procfs directory.
+ * @proc_dir: procfs directory associated with the SCSI host template.
+ * @present: Number of SCSI hosts instantiated for @sht.
+ */
+struct scsi_proc_entry {
+	struct list_head	entry;
+	const struct scsi_host_template *sht;
+	struct proc_dir_entry	*proc_dir;
+	unsigned int		present;
+};
 
 static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
                            size_t count, loff_t *ppos)
@@ -83,6 +98,32 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 				4 * PAGE_SIZE);
 }
 
+static struct scsi_proc_entry *
+__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
+{
+	struct scsi_proc_entry *e;
+
+	lockdep_assert_held(&global_host_template_mutex);
+
+	list_for_each_entry(e, &scsi_proc_list, entry)
+		if (e->sht == sht)
+			return e;
+
+	return NULL;
+}
+
+static struct scsi_proc_entry *
+scsi_lookup_proc_entry(const struct scsi_host_template *sht)
+{
+	struct scsi_proc_entry *e;
+
+	mutex_lock(&global_host_template_mutex);
+	e = __scsi_lookup_proc_entry(sht);
+	mutex_unlock(&global_host_template_mutex);
+
+	return e;
+}
+
 /**
  * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
  * @sht: SCSI host template pointer.
@@ -90,7 +131,9 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 struct proc_dir_entry *
 scsi_template_proc_dir(const struct scsi_host_template *sht)
 {
-	return sht->proc_dir;
+	struct scsi_proc_entry *e = scsi_lookup_proc_entry(sht);
+
+	return e ? e->proc_dir : NULL;
 }
 EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
 
@@ -108,24 +151,38 @@ static const struct proc_ops proc_scsi_ops = {
  *
  * Sets sht->proc_dir to the new directory.
  */
-int scsi_proc_hostdir_add(struct scsi_host_template *sht)
+int scsi_proc_hostdir_add(const struct scsi_host_template *sht)
 {
-	int ret = 0;
+	struct scsi_proc_entry *e;
+	int ret = -ENOMEM;
 
 	if (!sht->show_info)
 		return 0;
 
 	mutex_lock(&global_host_template_mutex);
-	if (!sht->present++) {
-		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
-        	if (!sht->proc_dir) {
-			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
-			       __func__, sht->proc_name);
-			ret = -ENOMEM;
-		}
+	e = __scsi_lookup_proc_entry(sht);
+	if (!e) {
+		e = kzalloc(sizeof(*e), GFP_KERNEL);
+		if (!e)
+			goto unlock;
 	}
+	if (e->present++)
+		goto success;
+	e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
+	if (!e->proc_dir) {
+		printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
+		       sht->proc_name);
+		goto unlock;
+	}
+	e->sht = sht;
+	list_add_tail(&e->entry, &scsi_proc_list);
+success:
+	e = NULL;
+	ret = 0;
+unlock:
 	mutex_unlock(&global_host_template_mutex);
 
+	kfree(e);
 	return ret;
 }
 
@@ -133,15 +190,19 @@ int scsi_proc_hostdir_add(struct scsi_host_template *sht)
  * scsi_proc_hostdir_rm - remove directory in /proc for a scsi host
  * @sht: owner of directory
  */
-void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
+void scsi_proc_hostdir_rm(const struct scsi_host_template *sht)
 {
+	struct scsi_proc_entry *e;
+
 	if (!sht->show_info)
 		return;
 
 	mutex_lock(&global_host_template_mutex);
-	if (!--sht->present && sht->proc_dir) {
+	e = __scsi_lookup_proc_entry(sht);
+	if (e && !--e->present) {
 		remove_proc_entry(sht->proc_name, proc_scsi);
-		sht->proc_dir = NULL;
+		list_del(&e->entry);
+		kfree(e);
 	}
 	mutex_unlock(&global_host_template_mutex);
 }
@@ -153,16 +214,21 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
  */
 void scsi_proc_host_add(struct Scsi_Host *shost)
 {
-	struct scsi_host_template *sht = shost->hostt;
+	const struct scsi_host_template *sht = shost->hostt;
+	struct scsi_proc_entry *e;
 	struct proc_dir_entry *p;
 	char name[10];
 
-	if (!sht->proc_dir)
+	if (!sht->show_info)
+		return;
+
+	e = scsi_lookup_proc_entry(sht);
+	if (!e)
 		return;
 
 	sprintf(name,"%d", shost->host_no);
-	p = proc_create_data(name, S_IRUGO | S_IWUSR,
-		sht->proc_dir, &proc_scsi_ops, shost);
+	p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
+			     &proc_scsi_ops, shost);
 	if (!p)
 		printk(KERN_ERR "%s: Failed to register host %d in"
 		       "%s\n", __func__, shost->host_no,
@@ -175,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
  */
 void scsi_proc_host_rm(struct Scsi_Host *shost)
 {
+	const struct scsi_host_template *sht = shost->hostt;
+	struct scsi_proc_entry *e;
 	char name[10];
 
-	if (!shost->hostt->proc_dir)
+	if (!sht->show_info)
+		return;
+
+	e = scsi_lookup_proc_entry(sht);
+	if (!e)
 		return;
 
 	sprintf(name,"%d", shost->host_no);
-	remove_proc_entry(name, shost->hostt->proc_dir);
+	remove_proc_entry(name, e->proc_dir);
 }
 /**
  * proc_print_scsidevice - return data about this host
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 030faca947d2..fb8184d87384 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -357,12 +357,6 @@ struct scsi_host_template {
 	 */
 	const char *proc_name;
 
-	/*
-	 * Used to store the procfs directory if a driver implements the
-	 * show_info method.
-	 */
-	struct proc_dir_entry *proc_dir;
-
 	/*
 	 * This determines if we will use a non-interrupt driven
 	 * or an interrupt driven scheme.  It is set to the maximum number
@@ -423,12 +417,6 @@ struct scsi_host_template {
 	 */
 	short cmd_per_lun;
 
-	/*
-	 * present contains counter indicating how many boards of this
-	 * type were found when we did the scan.
-	 */
-	unsigned char present;
-
 	/* If use block layer to manage tags, this is tag allocation policy */
 	int tag_alloc_policy;
 

[PATCH v5 5/7] scsi: core: Fix a use-after-free related to releasing device handlers

[Bart Van Assche]

The SCSI device name can be freed by kobject_cleanup() before
scsi_device_dev_release_usercontext() is called since the latter function
may be called asynchronously. Hence, the SCSI device name must not be
dereferenced from inside the SCSI device release function. Since
scsi_dh_release_device() dereferences the SCSI device name, call it
earlier. This patch fixes the following use-after-free:

BUG: KASAN: use-after-free in string+0xdc/0x1d0
Read of size 1 at addr ffff8881280d05f0 by task kworker/54:2/1373

CPU: 54 PID: 1373 Comm: kworker/54:2 Tainted: G            E      6.0.0-rc5-dbg #12
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
Workqueue: events scsi_device_dev_release_usercontext
Call Trace:
 <TASK>
 show_stack+0x4e/0x53
 dump_stack_lvl+0x51/0x66
 print_address_description.constprop.0.cold+0xd5/0x412
 print_report.cold+0x90/0x219
 kasan_report+0xb1/0xe0
 __asan_load1+0x4d/0x50
 string+0xdc/0x1d0
 vsnprintf+0x44d/0x7f0
 snprintf+0x88/0xa0
 dev_vprintk_emit+0x19c/0x1dc
 dev_printk_emit+0x8c/0xa6
 __dev_printk+0x73/0x8f
 _dev_printk+0xa8/0xbe
 sdev_prefix_printk+0x12c/0x180
 scsi_dh_release_device+0x74/0xa0
 scsi_device_dev_release_usercontext+0x60/0x8a0
 process_one_work+0x571/0xa40
 worker_thread+0x90/0x650
 kthread+0x185/0x1c0
 ret_from_fork+0x1f/0x30

Freed by task 509:
 kasan_save_stack+0x26/0x50
 kasan_set_track+0x25/0x30
 kasan_set_free_info+0x24/0x40
 ____kasan_slab_free+0x155/0x1c0
 __kasan_slab_free+0x12/0x20
 kfree+0x1fe/0x3e0
 kfree_const+0x21/0x30
 kobject_cleanup+0x8d/0x1c0
 kobject_put+0x6e/0x90
 put_device+0x13/0x20
 __scsi_remove_device+0x140/0x200
 scsi_forget_host+0xa7/0xb0
 scsi_remove_host+0x9b/0x1b0
 srp_remove_work+0x12b/0x2e0 [ib_srp]
 process_one_work+0x571/0xa40
 worker_thread+0x90/0x650
 kthread+0x185/0x1c0
 ret_from_fork+0x1f/0x30

Cc: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Fixes: 23695e41a1ca ("scsi_dh: fix use-after-free when removing scsi device")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5d61f58399dc..a3aaafdeac1d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -456,8 +456,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 
 	mod = sdev->host->hostt->module;
 
-	scsi_dh_release_device(sdev);
-
 	parent = sdev->sdev_gendev.parent;
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
@@ -1479,6 +1477,9 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	kref_put(&sdev->host->tagset_refcnt, scsi_mq_free_tags);
 	cancel_work_sync(&sdev->requeue_work);
 
+	/* Only detach the device handler after I/O processing has finished. */
+	scsi_dh_release_device(sdev);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

[PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

Some kernel modules call device_del() from their module exit code and
schedule asynchronous work from inside the .release callback without waiting
until that callback has finished. As an example, many SCSI LLD drivers call
scsi_remove_host() from their module exit code. scsi_remove_host() may
invoke scsi_device_dev_release_usercontext() asynchronously.
scsi_device_dev_release_usercontext() uses the host template pointer and
that pointer usually exists in static storage in the SCSI LLD. Support
using the module reference count to keep the module around until
asynchronous module exiting has completed by waiting in the delete_module()
system call until the module reference count drops to zero.

The following debug patch has been used to make the new wait_event()
call wait:

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 8be8e08fb67d..fead694ff95a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -14,6 +14,7 @@
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/bsg.h>
+#include <linux/delay.h>

 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
@@ -518,6 +519,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)

 	if (parent)
 		put_device(parent);
+	msleep(100);
 	module_put(mod);
 }

diff --git a/kernel/module/main.c b/kernel/module/main.c
index a271126d7d59..0bf75ec3f5a8 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -756,8 +756,10 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	 * unloading is not forced, wait for the module reference count to drop
 	 * to zero again.
 	 */
-	if (!forced)
+	if (!forced) {
+		WARN_ON_ONCE(atomic_read(&mod->refcnt));
 		wait_event(mod->refcnt_wq, atomic_read(&mod->refcnt) == 0);
+	}
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index aeea9731ef80..f021625f2caa 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3355,7 +3355,7 @@ int schedule_on_each_cpu(work_func_t func)
  */
 int execute_in_process_context(work_func_t fn, struct execute_work *ew)
 {
-	if (!in_interrupt()) {
+	if (false && !in_interrupt()) {
 		fn(&ew->work);
 		return 0;
 	}

Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-modules@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/module.h |  1 +
 kernel/module/main.c   | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 518296ea7f73..3a77d2bd4198 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -533,6 +533,7 @@ struct module {
 	/* Destruction function. */
 	void (*exit)(void);
 
+	wait_queue_head_t refcnt_wq;
 	atomic_t refcnt;
 #endif
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index a4e4d84b6f4e..a271126d7d59 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -550,6 +550,7 @@ static int module_unload_init(struct module *mod)
 
 	/* Hold reference count during initialization. */
 	atomic_inc(&mod->refcnt);
+	init_waitqueue_head(&mod->refcnt_wq);
 
 	return 0;
 }
@@ -750,6 +751,13 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	/* Final destruction now no one is using it. */
 	if (mod->exit != NULL)
 		mod->exit();
+	/*
+	 * If the module reference count was increased by mod->exit() and if
+	 * unloading is not forced, wait for the module reference count to drop
+	 * to zero again.
+	 */
+	if (!forced)
+		wait_event(mod->refcnt_wq, atomic_read(&mod->refcnt) == 0);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
@@ -854,6 +862,8 @@ void module_put(struct module *module)
 		WARN_ON(ret < 0);	/* Failed to put refcount */
 		trace_module_put(module, _RET_IP_);
 		preempt_enable();
+		if (ret == 0)
+			wake_up(&module->refcnt_wq);
 	}
 }
 EXPORT_SYMBOL(module_put);

[PATCH v5 7/7] scsi: core: Improve SCSI device removal

[Bart Van Assche]

Instead of clearing the host template module pointer if the LLD kernel
module is being unloaded, call __module_get() unconditionally. This
patch is a bug fix because it prevents that a SCSI LLD is unloaded after
scsi_device_dev_release() returns and before
scsi_device_dev_release_usercontext() is called.

Suggested-by: Christoph Hellwig <hch@lst.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_sysfs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index a3aaafdeac1d..661c2bdd4588 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -523,9 +523,7 @@ static void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdp = to_scsi_device(dev);
 
-	/* Set module pointer as NULL in case of module unloading */
-	if (!try_module_get(sdp->host->hostt->module))
-		sdp->host->hostt->module = NULL;
+	__module_get(sdp->host->hostt->module);
 
 	execute_in_process_context(scsi_device_dev_release_usercontext,
 				   &sdp->ew);

Re: [PATCH v5 3/7] scsi: core: Fail host creation if creating the proc directory fails

[John Garry]

On 14/09/2022 23:56, Bart Van Assche wrote:
> Users expect that the contents of /proc/scsi is in sync with the contents
> of /sys/class/scsi_host. Hence fail host creation if creating the proc
> directory fails.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: John Garry <john.garry@huawei.com>

Re: [PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries

[John Garry]

On 14/09/2022 23:56, Bart Van Assche wrote:
> Instead of using scsi_host_template members to track the SCSI proc
> directory entries, track these entries in a list. This patch changes the
> time needed for looking up the proc dir pointer from O(1) into O(n). I
> think this is acceptable since the number of SCSI host adapter types per
> host is usually small (less than ten).
> 
> This patch has been tested by attaching two USB storage devices to a
> qemu host:
> 
> $ grep -aH . /proc/scsi/usb-storage/*
> /proc/scsi/usb-storage/7:   Host scsi7: usb-storage
> /proc/scsi/usb-storage/7:       Vendor: QEMU
> /proc/scsi/usb-storage/7:      Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/7:Serial Number: 1-0000:00:02.1:00.0-6
> /proc/scsi/usb-storage/7:     Protocol: Transparent SCSI
> /proc/scsi/usb-storage/7:    Transport: Bulk
> /proc/scsi/usb-storage/7:       Quirks: SANE_SENSE
> /proc/scsi/usb-storage/8:   Host scsi8: usb-storage
> /proc/scsi/usb-storage/8:       Vendor: QEMU
> /proc/scsi/usb-storage/8:      Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/8:Serial Number: 1-0000:00:02.1:00.0-7
> /proc/scsi/usb-storage/8:     Protocol: Transparent SCSI
> /proc/scsi/usb-storage/8:    Transport: Bulk
> /proc/scsi/usb-storage/8:       Quirks: SANE_SENSE
> 
> This patch prepares for constifying most SCSI host templates.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

I have some comments below, but no big ones, so:

Reviewed-by: John Garry <john.garry@huawei.com>

> ---
>   drivers/scsi/scsi_priv.h |   4 +-
>   drivers/scsi/scsi_proc.c | 112 ++++++++++++++++++++++++++++++++-------
>   include/scsi/scsi_host.h |  12 -----
>   3 files changed, 94 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 8c2e32121db1..456ff53fe404 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -100,8 +100,8 @@ extern void scsi_evt_thread(struct work_struct *work);
>   
>   /* scsi_proc.c */
>   #ifdef CONFIG_SCSI_PROC_FS
> -extern int scsi_proc_hostdir_add(struct scsi_host_template *);
> -extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
> +extern int scsi_proc_hostdir_add(const struct scsi_host_template *);
> +extern void scsi_proc_hostdir_rm(const struct scsi_host_template *);
>   extern void scsi_proc_host_add(struct Scsi_Host *);
>   extern void scsi_proc_host_rm(struct Scsi_Host *);
>   extern int scsi_init_procfs(void);
> diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
> index 8c84f1a74773..76229ca89e2f 100644
> --- a/drivers/scsi/scsi_proc.c
> +++ b/drivers/scsi/scsi_proc.c
> @@ -43,8 +43,23 @@
>   
>   static struct proc_dir_entry *proc_scsi;
>   
> -/* Protect sht->present and sht->proc_dir */
> +/* Protects scsi_proc_list */
>   static DEFINE_MUTEX(global_host_template_mutex);
> +static LIST_HEAD(scsi_proc_list);
> +
> +/**
> + * struct scsi_proc_entry - (host template, SCSI proc dir) association
> + * @entry: entry in scsi_proc_list.
> + * @sht: SCSI host template associated with the procfs directory.
> + * @proc_dir: procfs directory associated with the SCSI host template.
> + * @present: Number of SCSI hosts instantiated for @sht.
> + */
> +struct scsi_proc_entry {
> +	struct list_head	entry;
> +	const struct scsi_host_template *sht;
> +	struct proc_dir_entry	*proc_dir;
> +	unsigned int		present;
> +};
>   
>   static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
>                              size_t count, loff_t *ppos)
> @@ -83,6 +98,32 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
>   				4 * PAGE_SIZE);
>   }
>   
> +static struct scsi_proc_entry *
> +__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> +	struct scsi_proc_entry *e;
> +
> +	lockdep_assert_held(&global_host_template_mutex);
> +
> +	list_for_each_entry(e, &scsi_proc_list, entry)
> +		if (e->sht == sht)
> +			return e;
> +
> +	return NULL;
> +}
> +
> +static struct scsi_proc_entry *
> +scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> +	struct scsi_proc_entry *e;
> +
> +	mutex_lock(&global_host_template_mutex);
> +	e = __scsi_lookup_proc_entry(sht);
> +	mutex_unlock(&global_host_template_mutex);
> +
> +	return e;
> +}
> +
>   /**
>    * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
>    * @sht: SCSI host template pointer.
> @@ -90,7 +131,9 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
>   struct proc_dir_entry *
>   scsi_template_proc_dir(const struct scsi_host_template *sht)
>   {
> -	return sht->proc_dir;
> +	struct scsi_proc_entry *e = scsi_lookup_proc_entry(sht);
> +
> +	return e ? e->proc_dir : NULL;
>   }
>   EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
>   
> @@ -108,24 +151,38 @@ static const struct proc_ops proc_scsi_ops = {
>    *
>    * Sets sht->proc_dir to the new directory.
>    */
> -int scsi_proc_hostdir_add(struct scsi_host_template *sht)
> +int scsi_proc_hostdir_add(const struct scsi_host_template *sht)
>   {
> -	int ret = 0;
> +	struct scsi_proc_entry *e;
> +	int ret = -ENOMEM;
>   
>   	if (!sht->show_info)
>   		return 0;
>   
>   	mutex_lock(&global_host_template_mutex);
> -	if (!sht->present++) {
> -		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> -        	if (!sht->proc_dir) {
> -			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
> -			       __func__, sht->proc_name);
> -			ret = -ENOMEM;
> -		}
> +	e = __scsi_lookup_proc_entry(sht);
> +	if (!e) {
> +		e = kzalloc(sizeof(*e), GFP_KERNEL);
> +		if (!e)
> +			goto unlock;

maybe it's better to set ret = -ENOMEM here (and not initialize ret), as 
every other path it is set, AFAICS

>   	}
> +	if (e->present++)
> +		goto success;
> +	e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> +	if (!e->proc_dir) {
> +		printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
> +		       sht->proc_name);
> +		goto unlock;
> +	}
> +	e->sht = sht;
> +	list_add_tail(&e->entry, &scsi_proc_list);
> +success:
> +	e = NULL;
> +	ret = 0;
> +unlock:
>   	mutex_unlock(&global_host_template_mutex);

Note: The code might be just neater to have multiple calls to 
mutex_unlock(). Maybe you have looked at that, I don't know.

>   
> +	kfree(e);
>   	return ret;
>   }
>   
> @@ -133,15 +190,19 @@ int scsi_proc_hostdir_add(struct scsi_host_template *sht)
>    * scsi_proc_hostdir_rm - remove directory in /proc for a scsi host
>    * @sht: owner of directory
>    */
> -void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
> +void scsi_proc_hostdir_rm(const struct scsi_host_template *sht)
>   {
> +	struct scsi_proc_entry *e;
> +
>   	if (!sht->show_info)
>   		return;
>   
>   	mutex_lock(&global_host_template_mutex);
> -	if (!--sht->present && sht->proc_dir) {
> +	e = __scsi_lookup_proc_entry(sht);
> +	if (e && !--e->present) {
>   		remove_proc_entry(sht->proc_name, proc_scsi);
> -		sht->proc_dir = NULL;
> +		list_del(&e->entry);
> +		kfree(e);
>   	}
>   	mutex_unlock(&global_host_template_mutex);
>   }
> @@ -153,16 +214,21 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
>    */
>   void scsi_proc_host_add(struct Scsi_Host *shost)
>   {
> -	struct scsi_host_template *sht = shost->hostt;
> +	const struct scsi_host_template *sht = shost->hostt;
> +	struct scsi_proc_entry *e;
>   	struct proc_dir_entry *p;
>   	char name[10];
>   
> -	if (!sht->proc_dir)
> +	if (!sht->show_info)
> +		return;
> +
> +	e = scsi_lookup_proc_entry(sht);

hmm... this really should not fail, right?. Maybe an error message would 
be appropiate here (for failure).

> +	if (!e)
>   		return;
>   
>   	sprintf(name,"%d", shost->host_no);
> -	p = proc_create_data(name, S_IRUGO | S_IWUSR,
> -		sht->proc_dir, &proc_scsi_ops, shost);
> +	p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
> +			     &proc_scsi_ops, shost);
>   	if (!p)
>   		printk(KERN_ERR "%s: Failed to register host %d in"
>   		       "%s\n", __func__, shost->host_no,
> @@ -175,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
>    */
>   void scsi_proc_host_rm(struct Scsi_Host *shost)
>   {
> +	const struct scsi_host_template *sht = shost->hostt;
> +	struct scsi_proc_entry *e;
>   	char name[10];
>   
> -	if (!shost->hostt->proc_dir)
> +	if (!sht->show_info)
> +		return;
> +
> +	e = scsi_lookup_proc_entry(sht);

Same comment as scsi_proc_host_add

> +	if (!e)
>   		return;
>   
>   	sprintf(name,"%d", shost->host_no);
> -	remove_proc_entry(name, shost->hostt->proc_dir);
> +	remove_proc_entry(name, e->proc_dir);
>   }
>   /**
>    * proc_print_scsidevice - return data about this host
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 030faca947d2..fb8184d87384 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -357,12 +357,6 @@ struct scsi_host_template {
>   	 */
>   	const char *proc_name;
>   
> -	/*
> -	 * Used to store the procfs directory if a driver implements the
> -	 * show_info method.
> -	 */
> -	struct proc_dir_entry *proc_dir;
> -
>   	/*
>   	 * This determines if we will use a non-interrupt driven
>   	 * or an interrupt driven scheme.  It is set to the maximum number
> @@ -423,12 +417,6 @@ struct scsi_host_template {
>   	 */
>   	short cmd_per_lun;
>   
> -	/*
> -	 * present contains counter indicating how many boards of this
> -	 * type were found when we did the scan.
> -	 */
> -	unsigned char present;
> -
>   	/* If use block layer to manage tags, this is tag allocation policy */
>   	int tag_alloc_policy;
>   
> 
> .

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 9/14/22 15:56, Bart Van Assche wrote:
> Some kernel modules call device_del() from their module exit code and
> schedule asynchronous work from inside the .release callback without waiting
> until that callback has finished. As an example, many SCSI LLD drivers call
> scsi_remove_host() from their module exit code. scsi_remove_host() may
> invoke scsi_device_dev_release_usercontext() asynchronously.
> scsi_device_dev_release_usercontext() uses the host template pointer and
> that pointer usually exists in static storage in the SCSI LLD. Support
> using the module reference count to keep the module around until
> asynchronous module exiting has completed by waiting in the delete_module()
> system call until the module reference count drops to zero.

Hi Luis,

I'd like to know your opinion about this patch since you are the 
maintainer of the kernel module system.

Thanks,

Bart.

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Luis Chamberlain]

On Tue, Sep 20, 2022 at 10:13:40AM -0700, Bart Van Assche wrote:
> On 9/14/22 15:56, Bart Van Assche wrote:
> > Some kernel modules call device_del() from their module exit code and
> > schedule asynchronous work from inside the .release callback without waiting
> > until that callback has finished. As an example, many SCSI LLD drivers call
> > scsi_remove_host() from their module exit code. scsi_remove_host() may
> > invoke scsi_device_dev_release_usercontext() asynchronously.
> > scsi_device_dev_release_usercontext() uses the host template pointer and
> > that pointer usually exists in static storage in the SCSI LLD. Support
> > using the module reference count to keep the module around until
> > asynchronous module exiting has completed by waiting in the delete_module()
> > system call until the module reference count drops to zero.
> 
> Hi Luis,
> 
> I'd like to know your opinion about this patch since you are the maintainer
> of the kernel module system.

See this patch which extends the documentation of try_module_get():

https://lkml.kernel.org/r/20211029184500.2821444-7-mcgrof@kernel.org

You can ignore discussion around the thread as sadly it is just
irrelevant stuff not about that patch. But the logic it spells out
is still true.

So, in short, using try_module_get() on exit is actually the wrong
thing to do and it is no surprise it would fail. I haven't gotten
yet around to reviewing Mauro's driver API which let's you unbind
drivers, but it sounds related so I CC'd you on that.

So I'd like to ask instead if an alternative to using try_module_get()
on exit would be better here and for the future.

  Luis

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Ming Lei]

On Wed, Sep 14, 2022 at 03:56:20PM -0700, Bart Van Assche wrote:
> Some kernel modules call device_del() from their module exit code and
> schedule asynchronous work from inside the .release callback without waiting
> until that callback has finished. As an example, many SCSI LLD drivers call

It isn't only related with device, any kobject has such issue, or any
reference counter usage has similar potential risk, see previous discussion:

https://lore.kernel.org/lkml/YsZm7lSXYAHT14ui@T590/

IMO, it is one fundamental problem wrt. module vs. reference counting or
kobject uses at least, since the callback depends on module code
segment.

> scsi_remove_host() from their module exit code. scsi_remove_host() may
> invoke scsi_device_dev_release_usercontext() asynchronously.
> scsi_device_dev_release_usercontext() uses the host template pointer and
> that pointer usually exists in static storage in the SCSI LLD. Support
> using the module reference count to keep the module around until
> asynchronous module exiting has completed by waiting in the delete_module()
> system call until the module reference count drops to zero.

The issue can't be addressed by the normal mod->refcnt, since user need
to unload module when the device isn't used.


thanks,
Ming

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 9/27/22 17:02, Luis Chamberlain wrote:
> On Tue, Sep 20, 2022 at 10:13:40AM -0700, Bart Van Assche wrote:
>> On 9/14/22 15:56, Bart Van Assche wrote:
>>> Some kernel modules call device_del() from their module exit code and
>>> schedule asynchronous work from inside the .release callback without waiting
>>> until that callback has finished. As an example, many SCSI LLD drivers call
>>> scsi_remove_host() from their module exit code. scsi_remove_host() may
>>> invoke scsi_device_dev_release_usercontext() asynchronously.
>>> scsi_device_dev_release_usercontext() uses the host template pointer and
>>> that pointer usually exists in static storage in the SCSI LLD. Support
>>> using the module reference count to keep the module around until
>>> asynchronous module exiting has completed by waiting in the delete_module()
>>> system call until the module reference count drops to zero.
>>
>> Hi Luis,
>>
>> I'd like to know your opinion about this patch since you are the maintainer
>> of the kernel module system.
> 
> See this patch which extends the documentation of try_module_get():
> 
> https://lkml.kernel.org/r/20211029184500.2821444-7-mcgrof@kernel.org
> 
> You can ignore discussion around the thread as sadly it is just
> irrelevant stuff not about that patch. But the logic it spells out
> is still true.
> 
> So, in short, using try_module_get() on exit is actually the wrong
> thing to do and it is no surprise it would fail. I haven't gotten
> yet around to reviewing Mauro's driver API which let's you unbind
> drivers, but it sounds related so I CC'd you on that.
> 
> So I'd like to ask instead if an alternative to using try_module_get()
> on exit would be better here and for the future.

Hi Luis,

The extended documentation of try_module_get() is very helpful. But 
please note that this patch is not related to try_module_get() at all. 
See also patch 7/7 in this series 
(https://lore.kernel.org/linux-scsi/20220914225621.415631-8-bvanassche@acm.org/).

Thanks,

Bart.

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 9/27/22 18:09, Ming Lei wrote:
> On Wed, Sep 14, 2022 at 03:56:20PM -0700, Bart Van Assche wrote:
>> Some kernel modules call device_del() from their module exit code and
>> schedule asynchronous work from inside the .release callback without waiting
>> until that callback has finished. As an example, many SCSI LLD drivers call
> 
> It isn't only related with device, any kobject has such issue, or any
> reference counter usage has similar potential risk, see previous discussion:
> 
> https://lore.kernel.org/lkml/YsZm7lSXYAHT14ui@T590/
> 
> IMO, it is one fundamental problem wrt. module vs. reference counting or
> kobject uses at least, since the callback depends on module code
> segment.
> 
>> scsi_remove_host() from their module exit code. scsi_remove_host() may
>> invoke scsi_device_dev_release_usercontext() asynchronously.
>> scsi_device_dev_release_usercontext() uses the host template pointer and
>> that pointer usually exists in static storage in the SCSI LLD. Support
>> using the module reference count to keep the module around until
>> asynchronous module exiting has completed by waiting in the delete_module()
>> system call until the module reference count drops to zero.
> 
> The issue can't be addressed by the normal mod->refcnt, since user need
> to unload module when the device isn't used.

Hi Ming,

How about removing support for calling scsi_device_put() from atomic context
as is done in the untested patch below?

Thanks,

Bart.

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index c59eac7a32f2..661753a10b47 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -561,6 +561,8 @@ EXPORT_SYMBOL(scsi_report_opcode);
   */
  int scsi_device_get(struct scsi_device *sdev)
  {
+	might_sleep();
+
  	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
  		goto fail;
  	if (!get_device(&sdev->sdev_gendev))
@@ -588,6 +590,7 @@ void scsi_device_put(struct scsi_device *sdev)
  {
  	struct module *mod = sdev->host->hostt->module;

+	might_sleep();
  	put_device(&sdev->sdev_gendev);
  	module_put(mod);
  }
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index a3aaafdeac1d..4cfc9317b4ad 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -441,7 +441,7 @@ static void scsi_device_cls_release(struct device *class_dev)
  	put_device(&sdev->sdev_gendev);
  }

-static void scsi_device_dev_release_usercontext(struct work_struct *work)
+static void scsi_device_dev_release(struct device *dev)
  {
  	struct scsi_device *sdev;
  	struct device *parent;
@@ -450,11 +450,8 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
  	struct scsi_vpd *vpd_pg0 = NULL, *vpd_pg89 = NULL;
  	struct scsi_vpd *vpd_pgb0 = NULL, *vpd_pgb1 = NULL, *vpd_pgb2 = NULL;
  	unsigned long flags;
-	struct module *mod;
-
-	sdev = container_of(work, struct scsi_device, ew.work);

-	mod = sdev->host->hostt->module;
+	sdev = to_scsi_device(dev);

  	parent = sdev->sdev_gendev.parent;

@@ -516,19 +513,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)

  	if (parent)
  		put_device(parent);
-	module_put(mod);
-}
-
-static void scsi_device_dev_release(struct device *dev)
-{
-	struct scsi_device *sdp = to_scsi_device(dev);
-
-	/* Set module pointer as NULL in case of module unloading */
-	if (!try_module_get(sdp->host->hostt->module))
-		sdp->host->hostt->module = NULL;
-
-	execute_in_process_context(scsi_device_dev_release_usercontext,
-				   &sdp->ew);
  }

  static struct class sdev_class = {

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Ming Lei]

On Wed, Sep 28, 2022 at 12:27:07PM -0700, Bart Van Assche wrote:
> On 9/27/22 18:09, Ming Lei wrote:
> > On Wed, Sep 14, 2022 at 03:56:20PM -0700, Bart Van Assche wrote:
> > > Some kernel modules call device_del() from their module exit code and
> > > schedule asynchronous work from inside the .release callback without waiting
> > > until that callback has finished. As an example, many SCSI LLD drivers call
> > 
> > It isn't only related with device, any kobject has such issue, or any
> > reference counter usage has similar potential risk, see previous discussion:
> > 
> > https://lore.kernel.org/lkml/YsZm7lSXYAHT14ui@T590/
> > 
> > IMO, it is one fundamental problem wrt. module vs. reference counting or
> > kobject uses at least, since the callback depends on module code
> > segment.
> > 
> > > scsi_remove_host() from their module exit code. scsi_remove_host() may
> > > invoke scsi_device_dev_release_usercontext() asynchronously.
> > > scsi_device_dev_release_usercontext() uses the host template pointer and
> > > that pointer usually exists in static storage in the SCSI LLD. Support
> > > using the module reference count to keep the module around until
> > > asynchronous module exiting has completed by waiting in the delete_module()
> > > system call until the module reference count drops to zero.
> > 
> > The issue can't be addressed by the normal mod->refcnt, since user need
> > to unload module when the device isn't used.
> 
> Hi Ming,
> 
> How about removing support for calling scsi_device_put() from atomic context
> as is done in the untested patch below?

That can't work.

The problem is that no existed mechanism can guarantee that kobject reference
drops to zero inside module_exit().


Thanks,
Ming

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 9/28/22 18:10, Ming Lei wrote:
> On Wed, Sep 28, 2022 at 12:27:07PM -0700, Bart Van Assche wrote:
>> How about removing support for calling scsi_device_put() from atomic context
>> as is done in the untested patch below?
> 
> That can't work.
> 
> The problem is that no existed mechanism can guarantee that kobject reference
> drops to zero inside module_exit().

Hi Ming,

I agree that the patch in my previous email won't address potential calls of
.release functions while a module is being unloaded or after a module has been
unloaded. However, that's not the purpose of that patch. The purpose of that
patch is to rework all code that modifies members of the scsi host template.

Thanks,

Bart.

Re: [PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries

[Bart Van Assche]

On 9/15/22 03:34, John Garry wrote:
> On 14/09/2022 23:56, Bart Van Assche wrote:
>> -int scsi_proc_hostdir_add(struct scsi_host_template *sht)
>> +int scsi_proc_hostdir_add(const struct scsi_host_template *sht)
>>   {
>> -    int ret = 0;
>> +    struct scsi_proc_entry *e;
>> +    int ret = -ENOMEM;
>>       if (!sht->show_info)
>>           return 0;
>>       mutex_lock(&global_host_template_mutex);
>> -    if (!sht->present++) {
>> -        sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>> -            if (!sht->proc_dir) {
>> -            printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
>> -                   __func__, sht->proc_name);
>> -            ret = -ENOMEM;
>> -        }
>> +    e = __scsi_lookup_proc_entry(sht);
>> +    if (!e) {
>> +        e = kzalloc(sizeof(*e), GFP_KERNEL);
>> +        if (!e)
>> +            goto unlock;
> 
> maybe it's better to set ret = -ENOMEM here (and not initialize ret), as every other path it is set, AFAICS

I will make this change and also set ret if proc_mkdir() fails.

>>   void scsi_proc_host_add(struct Scsi_Host *shost)
>>   {
>> -    struct scsi_host_template *sht = shost->hostt;
>> +    const struct scsi_host_template *sht = shost->hostt;
>> +    struct scsi_proc_entry *e;
>>       struct proc_dir_entry *p;
>>       char name[10];
>> -    if (!sht->proc_dir)
>> +    if (!sht->show_info)
>> +        return;
>> +
>> +    e = scsi_lookup_proc_entry(sht);
> 
> hmm... this really should not fail, right?. Maybe an error message would be appropiate here (for failure).

If scsi_proc_hostdir_add() failed scsi_lookup_proc_entry() will return NULL.
I will add an error message.

>> +    if (!e)
>>           return;
>>       sprintf(name,"%d", shost->host_no);
>> -    p = proc_create_data(name, S_IRUGO | S_IWUSR,
>> -        sht->proc_dir, &proc_scsi_ops, shost);
>> +    p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
>> +                 &proc_scsi_ops, shost);
>>       if (!p)
>>           printk(KERN_ERR "%s: Failed to register host %d in"
>>                  "%s\n", __func__, shost->host_no,
>> @@ -175,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
>>    */
>>   void scsi_proc_host_rm(struct Scsi_Host *shost)
>>   {
>> +    const struct scsi_host_template *sht = shost->hostt;
>> +    struct scsi_proc_entry *e;
>>       char name[10];
>> -    if (!shost->hostt->proc_dir)
>> +    if (!sht->show_info)
>> +        return;
>> +
>> +    e = scsi_lookup_proc_entry(sht);
> 
> Same comment as scsi_proc_host_add

If scsi_proc_hostdir_add() failed scsi_lookup_proc_entry() will return NULL.

Thanks,

Bart.

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Luis Chamberlain]

On Wed, Sep 28, 2022 at 11:17:02AM -0700, Bart Van Assche wrote:
> On 9/27/22 17:02, Luis Chamberlain wrote:
> > On Tue, Sep 20, 2022 at 10:13:40AM -0700, Bart Van Assche wrote:
> > > On 9/14/22 15:56, Bart Van Assche wrote:
> > > > Some kernel modules call device_del() from their module exit code and
> > > > schedule asynchronous work from inside the .release callback without waiting
> > > > until that callback has finished. As an example, many SCSI LLD drivers call
> > > > scsi_remove_host() from their module exit code. scsi_remove_host() may
> > > > invoke scsi_device_dev_release_usercontext() asynchronously.
> > > > scsi_device_dev_release_usercontext() uses the host template pointer and
> > > > that pointer usually exists in static storage in the SCSI LLD. Support
> > > > using the module reference count to keep the module around until
> > > > asynchronous module exiting has completed by waiting in the delete_module()
> > > > system call until the module reference count drops to zero.
> > > 
> > > Hi Luis,
> > > 
> > > I'd like to know your opinion about this patch since you are the maintainer
> > > of the kernel module system.
> > 
> > See this patch which extends the documentation of try_module_get():
> > 
> > https://lkml.kernel.org/r/20211029184500.2821444-7-mcgrof@kernel.org
> > 
> > You can ignore discussion around the thread as sadly it is just
> > irrelevant stuff not about that patch. But the logic it spells out
> > is still true.
> > 
> > So, in short, using try_module_get() on exit is actually the wrong
> > thing to do and it is no surprise it would fail. I haven't gotten
> > yet around to reviewing Mauro's driver API which let's you unbind
> > drivers, but it sounds related so I CC'd you on that.
> > 
> > So I'd like to ask instead if an alternative to using try_module_get()
> > on exit would be better here and for the future.
> 
> Hi Luis,
> 
> The extended documentation of try_module_get() is very helpful. But please
> note that this patch is not related to try_module_get() at all. See also
> patch 7/7 in this series (https://lore.kernel.org/linux-scsi/20220914225621.415631-8-bvanassche@acm.org/).

I cannot see how this patch set is no way related to try_module_get()
given the 7/7 patch you posted replaces try_module_get() with __module_get().
My point, and hint, is that the original construct that added try_module_get()
on removal was flawed and I'm not sure trying to expand on that idea would
or even *should* be fruitful given the issues / tribal knowledge I tried
extending documentation for.

It would beg the question if instead re-evaluating the goal could be
done in such a way that the new documentation I suggested on try_module_get()
would be seriously taken into account.

  Luis

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Luis Chamberlain]

On Fri, Sep 30, 2022 at 12:39:27PM -0700, Luis Chamberlain wrote:
> On Wed, Sep 28, 2022 at 11:17:02AM -0700, Bart Van Assche wrote:
> > On 9/27/22 17:02, Luis Chamberlain wrote:
> > > On Tue, Sep 20, 2022 at 10:13:40AM -0700, Bart Van Assche wrote:
> > > > On 9/14/22 15:56, Bart Van Assche wrote:
> > > > > Some kernel modules call device_del() from their module exit code and
> > > > > schedule asynchronous work from inside the .release callback without waiting
> > > > > until that callback has finished. As an example, many SCSI LLD drivers call
> > > > > scsi_remove_host() from their module exit code. scsi_remove_host() may
> > > > > invoke scsi_device_dev_release_usercontext() asynchronously.
> > > > > scsi_device_dev_release_usercontext() uses the host template pointer and
> > > > > that pointer usually exists in static storage in the SCSI LLD. Support
> > > > > using the module reference count to keep the module around until
> > > > > asynchronous module exiting has completed by waiting in the delete_module()
> > > > > system call until the module reference count drops to zero.
> > > > 
> > > > Hi Luis,
> > > > 
> > > > I'd like to know your opinion about this patch since you are the maintainer
> > > > of the kernel module system.
> > > 
> > > See this patch which extends the documentation of try_module_get():
> > > 
> > > https://lkml.kernel.org/r/20211029184500.2821444-7-mcgrof@kernel.org
> > > 
> > > You can ignore discussion around the thread as sadly it is just
> > > irrelevant stuff not about that patch. But the logic it spells out
> > > is still true.
> > > 
> > > So, in short, using try_module_get() on exit is actually the wrong
> > > thing to do and it is no surprise it would fail. I haven't gotten
> > > yet around to reviewing Mauro's driver API which let's you unbind
> > > drivers, but it sounds related so I CC'd you on that.
> > > 
> > > So I'd like to ask instead if an alternative to using try_module_get()
> > > on exit would be better here and for the future.
> > 
> > Hi Luis,
> > 
> > The extended documentation of try_module_get() is very helpful. But please
> > note that this patch is not related to try_module_get() at all. See also
> > patch 7/7 in this series (https://lore.kernel.org/linux-scsi/20220914225621.415631-8-bvanassche@acm.org/).
> 
> I cannot see how this patch set is no way related to try_module_get()
> given the 7/7 patch you posted replaces try_module_get() with __module_get().
> My point, and hint, is that the original construct that added try_module_get()
> on removal was flawed and I'm not sure trying to expand on that idea would
> or even *should* be fruitful given the issues / tribal knowledge I tried
> extending documentation for.
> 
> It would beg the question if instead re-evaluating the goal could be
> done in such a way that the new documentation I suggested on try_module_get()
> would be seriously taken into account.

Yeah I've gone ahead and re-read your original patch again the issue
with that is it waits *once* for the refcnt to go to 0, but that does
not forbit it from going back up, at which point you have a race which
can still create the situation. Every subsystem is different, but for
instance simply running a loop opening a device block file should
suffice to bump the refcnt of a respective block driver module. So
at least the patch itself won't ultimately address this issue I'm
afraid.

  Luis

Re: [PATCH v5 6/7] module: Improve support for asynchronous module exit code

[Bart Van Assche]

On 10/3/22 16:56, Luis Chamberlain wrote:
> Yeah I've gone ahead and re-read your original patch again the issue
> with that is it waits *once* for the refcnt to go to 0, but that does
> not forbit it from going back up, at which point you have a race which
> can still create the situation. Every subsystem is different, but for
> instance simply running a loop opening a device block file should
> suffice to bump the refcnt of a respective block driver module. So
> at least the patch itself won't ultimately address this issue I'm
> afraid.

Hi Luis,

Thanks for the feedback. I will try to find a solution that does not 
require to modify the kernel module code.

Bart.