[PATCH v4 0/4] Call blk_mq_free_tag_set() earlier

[PATCH v4 0/4] Call blk_mq_free_tag_set() earlier

[Bart Van Assche]

Hi Martin,

This patch series fixes a recently reported use-after-free in the SRP driver.
Please consider this patch series for kernel v5.20.

Changes compared to v3:
- Added a patch to delay scsi_remove_target() until dependent devices have been
  removed.
- Split a patch into two patches.

Changes compared to v2:
- Dropped the patch that simplifies scsi_forget_host().
- Replaced patch 2/3 with a patch from Ming Lei.

Changes compared to v1:
- Expanded this series from one to three patches.
- Fixed the description of patch 3/3.

Bart Van Assche (2):
  scsi: core: Make sure that targets outlive devices
  scsi: core: Call blk_mq_free_tag_set() earlier

Ming Lei (2):
  scsi: core: Make sure that hosts outlive targets
  scsi: core: Simplify LLD module reference counting

 drivers/scsi/hosts.c       | 18 +++++++++++++-----
 drivers/scsi/scsi.c        |  9 ++++++---
 drivers/scsi/scsi_lib.c    |  3 +++
 drivers/scsi/scsi_scan.c   |  9 +++++++++
 drivers/scsi/scsi_sysfs.c  | 29 +++++++++++++++++------------
 include/scsi/scsi_device.h |  2 ++
 include/scsi/scsi_host.h   |  3 +++
 7 files changed, 53 insertions(+), 20 deletions(-)

[PATCH v4 1/4] scsi: core: Make sure that targets outlive devices

[Bart Van Assche]

This patch prevents that the following sequence triggers a kernel crash:
* Deletion of a SCSI device is requested via sysfs. Device removal takes
  some time because blk_cleanup_queue() is waiting for the SCSI error
  handler.
* The SCSI target associated with that SCSI device is removed.
* scsi_remove_target() returns and its caller frees the resources
  associated with the SCSI target.
* The error handler makes progress and invokes an LLD callback that
  dereferences the SCSI target pointer.

Reported-by: Mike Christie <michael.christie@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Li Zhijian <lizhijian@fujitsu.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_scan.c   |  2 ++
 drivers/scsi/scsi_sysfs.c  | 20 +++++++++++++++++---
 include/scsi/scsi_device.h |  2 ++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 91ac901a6682..4c1efd6a3b0c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -521,6 +521,8 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+	init_waitqueue_head(&starget->sdev_wq);
+
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 43949798a2e4..1bc9c26fe1d4 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -443,7 +443,9 @@ static void scsi_device_cls_release(struct device *class_dev)
 
 static void scsi_device_dev_release_usercontext(struct work_struct *work)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = container_of(work, struct scsi_device,
+						ew.work);
+	struct scsi_target *starget = sdev->sdev_target;
 	struct device *parent;
 	struct list_head *this, *tmp;
 	struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL;
@@ -452,8 +454,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	unsigned long flags;
 	struct module *mod;
 
-	sdev = container_of(work, struct scsi_device, ew.work);
-
 	mod = sdev->host->hostt->module;
 
 	scsi_dh_release_device(sdev);
@@ -516,6 +516,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	kfree(sdev->inquiry);
 	kfree(sdev);
 
+	if (starget && atomic_dec_return(&starget->sdev_count) == 0)
+		wake_up(&starget->sdev_wq);
+
 	if (parent)
 		put_device(parent);
 	module_put(mod);
@@ -1535,6 +1538,14 @@ static void __scsi_remove_target(struct scsi_target *starget)
 		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	/*
+	 * After scsi_remove_target() returns its caller can remove resources
+	 * associated with @starget, e.g. an rport or session. Wait until all
+	 * devices associated with @starget have been removed to prevent that
+	 * a SCSI error handling callback function triggers a use-after-free.
+	 */
+	wait_event(starget->sdev_wq, atomic_read(&starget->sdev_count) == 0);
 }
 
 /**
@@ -1645,6 +1656,9 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	list_add_tail(&sdev->same_target_siblings, &starget->devices);
 	list_add_tail(&sdev->siblings, &shost->__devices);
 	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	atomic_inc(&starget->sdev_count);
+
 	/*
 	 * device can now only be removed via __scsi_remove_device() so hold
 	 * the target.  Target will be held in CREATED state until something
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7cf5f3b7589f..190d2081f4c6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -309,6 +309,8 @@ struct scsi_target {
 	struct list_head	devices;
 	struct device		dev;
 	struct kref		reap_ref; /* last put renders target invisible */
+	atomic_t		sdev_count;
+	wait_queue_head_t	sdev_wq;
 	unsigned int		channel;
 	unsigned int		id; /* target id ... replace
 				     * scsi_device.id eventually */

[PATCH v4 2/4] scsi: core: Make sure that hosts outlive targets

[Bart Van Assche]

From: Ming Lei <ming.lei@redhat.com>

Fix the race conditions between SCSI LLD kernel module unloading and SCSI
device and target removal by making sure that SCSI hosts are destroyed after
all associated target and device objects have been freed.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
[ bvanassche: Reworked Ming's patch and split it ]
---
 drivers/scsi/hosts.c     | 8 ++++++++
 drivers/scsi/scsi_scan.c | 7 +++++++
 include/scsi/scsi_host.h | 3 +++
 3 files changed, 18 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index ef6c0e37acce..8fa98c8d0ee0 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost)
 	transport_unregister_device(&shost->shost_gendev);
 	device_unregister(&shost->shost_dev);
 	device_del(&shost->shost_gendev);
+
+	/*
+	 * After scsi_remove_host() has returned the scsi LLD module can be
+	 * unloaded and/or the host resources can be released. Hence wait until
+	 * the dependent SCSI targets and devices are gone before returning.
+	 */
+	wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
 }
 EXPORT_SYMBOL(scsi_remove_host);
 
@@ -394,6 +401,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
 	mutex_init(&shost->scan_mutex);
+	init_waitqueue_head(&shost->targets_wq);
 
 	index = ida_alloc(&host_index_ida, GFP_KERNEL);
 	if (index < 0) {
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 4c1efd6a3b0c..ac6059702d13 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -406,9 +406,14 @@ static void scsi_target_destroy(struct scsi_target *starget)
 static void scsi_target_dev_release(struct device *dev)
 {
 	struct device *parent = dev->parent;
+	struct Scsi_Host *shost = dev_to_shost(parent);
 	struct scsi_target *starget = to_scsi_target(dev);
 
 	kfree(starget);
+
+	if (atomic_dec_return(&shost->target_count) == 0)
+		wake_up(&shost->targets_wq);
+
 	put_device(parent);
 }
 
@@ -523,6 +528,8 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
 	init_waitqueue_head(&starget->sdev_wq);
 
+	atomic_inc(&shost->target_count);
+
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 667d889b92b5..339f975d356e 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -689,6 +689,9 @@ struct Scsi_Host {
 	/* ldm bits */
 	struct device		shost_gendev, shost_dev;
 
+	atomic_t		target_count;
+	wait_queue_head_t	targets_wq;
+
 	/*
 	 * Points to the transport data (if any) which is allocated
 	 * separately

[PATCH v4 3/4] scsi: core: Simplify LLD module reference counting

[Bart Van Assche]

From: Ming Lei <ming.lei@redhat.com>

Swap two statements in scsi_device_put() now that it is guaranteed that
SCSI hosts outlive SCSI devices. Remove the reference counting code from
scsi_sysfs.c that became superfluous because SCSI hosts now outlive SCSI
devices.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
[ bvanassche: Extracted this patch from a larger patch ]
---
 drivers/scsi/scsi.c       | 9 ++++++---
 drivers/scsi/scsi_sysfs.c | 9 ---------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index c59eac7a32f2..086ec5b5862d 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -586,10 +586,13 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	struct module *mod = sdev->host->hostt->module;
-
+	/*
+	 * Decreasing the module reference count before the device reference
+	 * count is safe since scsi_remove_host() only returns after all
+	 * devices have been removed.
+	 */
+	module_put(sdev->host->hostt->module);
 	put_device(&sdev->sdev_gendev);
-	module_put(mod);
 }
 EXPORT_SYMBOL(scsi_device_put);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 1bc9c26fe1d4..213ebc88f76a 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -452,9 +452,6 @@ 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;
-
-	mod = sdev->host->hostt->module;
 
 	scsi_dh_release_device(sdev);
 
@@ -521,17 +518,11 @@ 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);
 }

[PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[Bart Van Assche]

There are two .exit_cmd_priv implementations. Both implementations use
resources associated with the SCSI host. Make sure that these resources are
still available when .exit_cmd_priv is called by moving the .exit_cmd_priv
calls from scsi_host_dev_release() to scsi_forget_host(). Moving
blk_mq_free_tag_set() from scsi_host_dev_release() to scsi_forget_host() is
safe because scsi_forget_host() waits until all SCSI devices associated
with the host have been removed.

This patch fixes the following use-after-free:

==================================================================
BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
Read of size 8 at addr ffff888100337000 by task multipathd/16727
Call Trace:
 <TASK>
 dump_stack_lvl+0x34/0x44
 print_report.cold+0x5e/0x5db
 kasan_report+0xab/0x120
 srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
 scsi_mq_exit_request+0x4d/0x70
 blk_mq_free_rqs+0x143/0x410
 __blk_mq_free_map_and_rqs+0x6e/0x100
 blk_mq_free_tag_set+0x2b/0x160
 scsi_host_dev_release+0xf3/0x1a0
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 scsi_device_dev_release_usercontext+0x4c1/0x4e0
 execute_in_process_context+0x23/0x90
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 scsi_disk_release+0x3f/0x50
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 disk_release+0x17f/0x1b0
 device_release+0x54/0xe0
 kobject_put+0xa5/0x120
 dm_put_table_device+0xa3/0x160 [dm_mod]
 dm_put_device+0xd0/0x140 [dm_mod]
 free_priority_group+0xd8/0x110 [dm_multipath]
 free_multipath+0x94/0xe0 [dm_multipath]
 dm_table_destroy+0xa2/0x1e0 [dm_mod]
 __dm_destroy+0x196/0x350 [dm_mod]
 dev_remove+0x10c/0x160 [dm_mod]
 ctl_ioctl+0x2c2/0x590 [dm_mod]
 dm_ctl_ioctl+0x5/0x10 [dm_mod]
 __x64_sys_ioctl+0xb4/0xf0
 dm_ctl_ioctl+0x5/0x10 [dm_mod]
 __x64_sys_ioctl+0xb4/0xf0
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x46/0xb0

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Li Zhijian <lizhijian@fujitsu.com>
Reported-by: Li Zhijian <lizhijian@fujitsu.com>
Tested-by: Li Zhijian <lizhijian@fujitsu.com>
Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/hosts.c    | 10 +++++-----
 drivers/scsi/scsi_lib.c |  3 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8fa98c8d0ee0..6c63672971f1 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -197,6 +197,8 @@ void scsi_remove_host(struct Scsi_Host *shost)
 	 * the dependent SCSI targets and devices are gone before returning.
 	 */
 	wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
+
+	scsi_mq_destroy_tags(shost);
 }
 EXPORT_SYMBOL(scsi_remove_host);
 
@@ -302,8 +304,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	return error;
 
 	/*
-	 * Any host allocation in this function will be freed in
-	 * scsi_host_dev_release().
+	 * Any resources associated with the SCSI host in this function except
+	 * the tag set will be freed by scsi_host_dev_release().
 	 */
  out_del_dev:
 	device_del(&shost->shost_dev);
@@ -319,6 +321,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	pm_runtime_disable(&shost->shost_gendev);
 	pm_runtime_set_suspended(&shost->shost_gendev);
 	pm_runtime_put_noidle(&shost->shost_gendev);
+	scsi_mq_destroy_tags(shost);
  fail:
 	return error;
 }
@@ -352,9 +355,6 @@ static void scsi_host_dev_release(struct device *dev)
 		kfree(dev_name(&shost->shost_dev));
 	}
 
-	if (shost->tag_set.tags)
-		scsi_mq_destroy_tags(shost);
-
 	kfree(shost->shost_data);
 
 	ida_free(&host_index_ida, shost->host_no);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2aca0a838ca5..295c48fdb650 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 
 void scsi_mq_destroy_tags(struct Scsi_Host *shost)
 {
+	if (!shost->tag_set.tags)
+		return;
 	blk_mq_free_tag_set(&shost->tag_set);
+	WARN_ON_ONCE(shost->tag_set.tags);
 }
 
 /**

Re: [PATCH v4 1/4] scsi: core: Make sure that targets outlive devices

[Ming Lei]

On Tue, Jul 12, 2022 at 03:19:33PM -0700, Bart Van Assche wrote:
> This patch prevents that the following sequence triggers a kernel crash:
> * Deletion of a SCSI device is requested via sysfs. Device removal takes
>   some time because blk_cleanup_queue() is waiting for the SCSI error
>   handler.
> * The SCSI target associated with that SCSI device is removed.
> * scsi_remove_target() returns and its caller frees the resources
>   associated with the SCSI target.
> * The error handler makes progress and invokes an LLD callback that
>   dereferences the SCSI target pointer.
> 
> Reported-by: Mike Christie <michael.christie@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[Ming Lei]

On Tue, Jul 12, 2022 at 03:19:36PM -0700, Bart Van Assche wrote:
> There are two .exit_cmd_priv implementations. Both implementations use
> resources associated with the SCSI host. Make sure that these resources are
> still available when .exit_cmd_priv is called by moving the .exit_cmd_priv
> calls from scsi_host_dev_release() to scsi_forget_host(). Moving
> blk_mq_free_tag_set() from scsi_host_dev_release() to scsi_forget_host() is
> safe because scsi_forget_host() waits until all SCSI devices associated
> with the host have been removed.
> 
> This patch fixes the following use-after-free:
> 
> ==================================================================
> BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
> Read of size 8 at addr ffff888100337000 by task multipathd/16727
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x34/0x44
>  print_report.cold+0x5e/0x5db
>  kasan_report+0xab/0x120
>  srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>  scsi_mq_exit_request+0x4d/0x70
>  blk_mq_free_rqs+0x143/0x410
>  __blk_mq_free_map_and_rqs+0x6e/0x100
>  blk_mq_free_tag_set+0x2b/0x160
>  scsi_host_dev_release+0xf3/0x1a0
>  device_release+0x54/0xe0
>  kobject_put+0xa5/0x120
>  device_release+0x54/0xe0
>  kobject_put+0xa5/0x120
>  scsi_device_dev_release_usercontext+0x4c1/0x4e0
>  execute_in_process_context+0x23/0x90
>  device_release+0x54/0xe0
>  kobject_put+0xa5/0x120
>  scsi_disk_release+0x3f/0x50
>  device_release+0x54/0xe0
>  kobject_put+0xa5/0x120
>  disk_release+0x17f/0x1b0
>  device_release+0x54/0xe0
>  kobject_put+0xa5/0x120
>  dm_put_table_device+0xa3/0x160 [dm_mod]
>  dm_put_device+0xd0/0x140 [dm_mod]
>  free_priority_group+0xd8/0x110 [dm_multipath]
>  free_multipath+0x94/0xe0 [dm_multipath]
>  dm_table_destroy+0xa2/0x1e0 [dm_mod]
>  __dm_destroy+0x196/0x350 [dm_mod]
>  dev_remove+0x10c/0x160 [dm_mod]
>  ctl_ioctl+0x2c2/0x590 [dm_mod]
>  dm_ctl_ioctl+0x5/0x10 [dm_mod]
>  __x64_sys_ioctl+0xb4/0xf0
>  dm_ctl_ioctl+0x5/0x10 [dm_mod]
>  __x64_sys_ioctl+0xb4/0xf0
>  do_syscall_64+0x3b/0x90
>  entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Tested-by: Li Zhijian <lizhijian@fujitsu.com>
> Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[John Garry]

On 12/07/2022 23:19, Bart Van Assche wrote:
> There are two .exit_cmd_priv implementations. Both implementations use
> resources associated with the SCSI host. Make sure that these resources are
> still available when .exit_cmd_priv is called by moving the .exit_cmd_priv
> calls from scsi_host_dev_release() to scsi_forget_host(). Moving
> blk_mq_free_tag_set() from scsi_host_dev_release() to scsi_forget_host() is
> safe because scsi_forget_host() waits until all SCSI devices associated

It seems to me that blk_mq_free_tag_set() is called from 
scsi_remove_host() now, right?

> with the host have been removed.
> 
> This patch fixes the following use-after-free:
> 
> ==================================================================
> BUG: KASAN: use-after-free in srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
> Read of size 8 at addr ffff888100337000 by task multipathd/16727
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x34/0x44
>   print_report.cold+0x5e/0x5db
>   kasan_report+0xab/0x120
>   srp_exit_cmd_priv+0x27/0xd0 [ib_srp]
>   scsi_mq_exit_request+0x4d/0x70
>   blk_mq_free_rqs+0x143/0x410
>   __blk_mq_free_map_and_rqs+0x6e/0x100
>   blk_mq_free_tag_set+0x2b/0x160
>   scsi_host_dev_release+0xf3/0x1a0
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   scsi_device_dev_release_usercontext+0x4c1/0x4e0
>   execute_in_process_context+0x23/0x90
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   scsi_disk_release+0x3f/0x50
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   disk_release+0x17f/0x1b0
>   device_release+0x54/0xe0
>   kobject_put+0xa5/0x120
>   dm_put_table_device+0xa3/0x160 [dm_mod]
>   dm_put_device+0xd0/0x140 [dm_mod]
>   free_priority_group+0xd8/0x110 [dm_multipath]
>   free_multipath+0x94/0xe0 [dm_multipath]
>   dm_table_destroy+0xa2/0x1e0 [dm_mod]
>   __dm_destroy+0x196/0x350 [dm_mod]
>   dev_remove+0x10c/0x160 [dm_mod]
>   ctl_ioctl+0x2c2/0x590 [dm_mod]
>   dm_ctl_ioctl+0x5/0x10 [dm_mod]
>   __x64_sys_ioctl+0xb4/0xf0
>   dm_ctl_ioctl+0x5/0x10 [dm_mod]
>   __x64_sys_ioctl+0xb4/0xf0
>   do_syscall_64+0x3b/0x90
>   entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Li Zhijian <lizhijian@fujitsu.com>
> Tested-by: Li Zhijian <lizhijian@fujitsu.com>
> Fixes: 65ca846a5314 ("scsi: core: Introduce {init,exit}_cmd_priv()")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/hosts.c    | 10 +++++-----
>   drivers/scsi/scsi_lib.c |  3 +++
>   2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 8fa98c8d0ee0..6c63672971f1 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -197,6 +197,8 @@ void scsi_remove_host(struct Scsi_Host *shost)
>   	 * the dependent SCSI targets and devices are gone before returning.
>   	 */
>   	wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
> +
> +	scsi_mq_destroy_tags(shost);
>   }
>   EXPORT_SYMBOL(scsi_remove_host);
>   
> @@ -302,8 +304,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	return error;
>   
>   	/*
> -	 * Any host allocation in this function will be freed in
> -	 * scsi_host_dev_release().
> +	 * Any resources associated with the SCSI host in this function except
> +	 * the tag set will be freed by scsi_host_dev_release().
>   	 */
>    out_del_dev:
>   	device_del(&shost->shost_dev);
> @@ -319,6 +321,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	pm_runtime_disable(&shost->shost_gendev);
>   	pm_runtime_set_suspended(&shost->shost_gendev);
>   	pm_runtime_put_noidle(&shost->shost_gendev);
> +	scsi_mq_destroy_tags(shost);
>    fail:
>   	return error;
>   }
> @@ -352,9 +355,6 @@ static void scsi_host_dev_release(struct device *dev)
>   		kfree(dev_name(&shost->shost_dev));
>   	}
>   
> -	if (shost->tag_set.tags)
> -		scsi_mq_destroy_tags(shost);
> -
>   	kfree(shost->shost_data);
>   
>   	ida_free(&host_index_ida, shost->host_no);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 2aca0a838ca5..295c48fdb650 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>   
>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>   {
> +	if (!shost->tag_set.tags)
> +		return;
>   	blk_mq_free_tag_set(&shost->tag_set);
> +	WARN_ON_ONCE(shost->tag_set.tags);

blk_mq_free_tag_set() clears the tagset tags pointer, so I don't know 
why you don't trust the semantics of that API - this seems like paranoia.

>   }
>   
>   /**
> .


Thanks,
john

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[Bart Van Assche]

On 7/13/22 01:13, John Garry wrote:
> It seems to me that blk_mq_free_tag_set() is called from 
> scsi_remove_host() now, right?

Right, I will update the patch description. I moved the 
blk_mq_free_tag_set() after I wrote the patch description.

>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 2aca0a838ca5..295c48fdb650 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>>   {
>> +    if (!shost->tag_set.tags)
>> +        return;
>>       blk_mq_free_tag_set(&shost->tag_set);
>> +    WARN_ON_ONCE(shost->tag_set.tags);
> 
> blk_mq_free_tag_set() clears the tagset tags pointer, so I don't know 
> why you don't trust the semantics of that API - this seems like paranoia.

Semantics of the API? Shouldn't this rather be called an undocumented 
aspect of blk_mq_free_tag_set()?

My concern is that the "set->tags = NULL" statement might be removed in 
the future from blk_mq_free_tag_set() and also that it is possible that 
scsi_mq_destroy_tags() is not updated after that change.

Thanks,

Bart.

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[John Garry]

On 13/07/2022 21:04, Bart Van Assche wrote:
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index 2aca0a838ca5..295c48fdb650 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>>>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>>>   {
>>> +    if (!shost->tag_set.tags)
>>> +        return;
>>>       blk_mq_free_tag_set(&shost->tag_set);
>>> +    WARN_ON_ONCE(shost->tag_set.tags);
>>
>> blk_mq_free_tag_set() clears the tagset tags pointer, so I don't know 
>> why you don't trust the semantics of that API - this seems like paranoia.
> 
> Semantics of the API? Shouldn't this rather be called an undocumented 
> aspect of blk_mq_free_tag_set()?
> 
> My concern is that the "set->tags = NULL" statement might be removed in 
> the future from blk_mq_free_tag_set() and also that it is possible that 
> scsi_mq_destroy_tags() is not updated after that change.

Sure, so how it is possible that set->tags == NULL ever when calling 
scsi_mq_setup_tags()? I'm just wondering why you even care.

Thanks,
John

Re: [PATCH v4 2/4] scsi: core: Make sure that hosts outlive targets

[Mike Christie]

On 7/12/22 5:19 PM, Bart Van Assche wrote:
> From: Ming Lei <ming.lei@redhat.com>
> 
> Fix the race conditions between SCSI LLD kernel module unloading and SCSI
> device and target removal by making sure that SCSI hosts are destroyed after
> all associated target and device objects have been freed.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> [ bvanassche: Reworked Ming's patch and split it ]
> ---
>  drivers/scsi/hosts.c     | 8 ++++++++
>  drivers/scsi/scsi_scan.c | 7 +++++++
>  include/scsi/scsi_host.h | 3 +++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ef6c0e37acce..8fa98c8d0ee0 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost)
>  	transport_unregister_device(&shost->shost_gendev);
>  	device_unregister(&shost->shost_dev);
>  	device_del(&shost->shost_gendev);
> +
> +	/*
> +	 * After scsi_remove_host() has returned the scsi LLD module can be
> +	 * unloaded and/or the host resources can be released. Hence wait until
> +	 * the dependent SCSI targets and devices are gone before returning.
> +	 */
> +	wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
>  }

If we only wait here we can still hit the race I described right?

Is the issue where we might be misunderstanding each other that the target
removal is slightly different from the host removal? For host removal we call
scsi_forget_host with the scan_mutex already held. So when scsi_forget_host
loops over the devices we know that there is no thread doing:

sdev_store_delete -> scsi_remove_device -> __scsi_remove_device -> blk_cleanup_queue

Since the sdev_store_delete call to scsi_remove_device call also grabs the scan_mutex,
we can't call scsi_forget_host until sdev_store_delete -> scsi_remove_device has returned.

For target removal,__scsi_remove_target doesn't take the scan_mutex when checking the
device state. So, we have this race:

1. syfs deletion runs  sdev_store_delete -> scsi_remove_device and
that takes the scan_mutex.

It then sets the state to SDEV_DEL.

2. fc/iscsi thread does __scsi_remove_target and it sees the device is in
the SDEV_DEL state. It skips the device and then we return from
__scsi_remove_target without having waited on that device's removal like is done
in other cases.

If the only issue we are concerned with is blk_cleanup_queue completing when we
remove the host or target, then for the target race above I think we can just use
the scan_mutex in __scsi_remove_target (that function then would use __scsi_remove_device).

If the issue is that there would be other threads holding a ref to the scsi_device
and they can call into the driver. and we want to make sure those refs are dropped
when scsi_remove_target returns then we need to do what I described in the other thread.

Re: [PATCH v4 2/4] scsi: core: Make sure that hosts outlive targets

[michael.christie]

On 7/14/22 11:02 AM, Mike Christie wrote:
> On 7/12/22 5:19 PM, Bart Van Assche wrote:
>> From: Ming Lei <ming.lei@redhat.com>
>>
>> Fix the race conditions between SCSI LLD kernel module unloading and SCSI
>> device and target removal by making sure that SCSI hosts are destroyed after
>> all associated target and device objects have been freed.
>>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Ming Lei <ming.lei@redhat.com>
>> Cc: Mike Christie <michael.christie@oracle.com>
>> Cc: Hannes Reinecke <hare@suse.de>
>> Cc: John Garry <john.garry@huawei.com>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> [ bvanassche: Reworked Ming's patch and split it ]
>> ---
>>  drivers/scsi/hosts.c     | 8 ++++++++
>>  drivers/scsi/scsi_scan.c | 7 +++++++
>>  include/scsi/scsi_host.h | 3 +++
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index ef6c0e37acce..8fa98c8d0ee0 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -190,6 +190,13 @@ void scsi_remove_host(struct Scsi_Host *shost)
>>  	transport_unregister_device(&shost->shost_gendev);
>>  	device_unregister(&shost->shost_dev);
>>  	device_del(&shost->shost_gendev);
>> +
>> +	/*
>> +	 * After scsi_remove_host() has returned the scsi LLD module can be
>> +	 * unloaded and/or the host resources can be released. Hence wait until
>> +	 * the dependent SCSI targets and devices are gone before returning.
>> +	 */
>> +	wait_event(shost->targets_wq, atomic_read(&shost->target_count) == 0);
>>  }
> 
> If we only wait here we can still hit the race I described right?
> 

Sorry Bart. Ignore this mail. I missed patch 1/4. I see we do the wait in
__scsi_remove_target.

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[Bart Van Assche]

On 7/14/22 05:25, John Garry wrote:
> On 13/07/2022 21:04, Bart Van Assche wrote:
>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>> index 2aca0a838ca5..295c48fdb650 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>>>>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>>>>   {
>>>> +    if (!shost->tag_set.tags)
>>>> +        return;
>>>>       blk_mq_free_tag_set(&shost->tag_set);
>>>> +    WARN_ON_ONCE(shost->tag_set.tags);
>>>
>>> blk_mq_free_tag_set() clears the tagset tags pointer, so I don't know 
>>> why you don't trust the semantics of that API - this seems like 
>>> paranoia.
>>
>> Semantics of the API? Shouldn't this rather be called an undocumented 
>> aspect of blk_mq_free_tag_set()?
>>
>> My concern is that the "set->tags = NULL" statement might be removed 
>> in the future from blk_mq_free_tag_set() and also that it is possible 
>> that scsi_mq_destroy_tags() is not updated after that change.
> 
> Sure, so how it is possible that set->tags == NULL ever when calling 
> scsi_mq_setup_tags()? I'm just wondering why you even care.

How about applying the patch below on top of patch 4/4?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 295c48fdb650..2aca0a838ca5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1990,10 +1990,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)

  void scsi_mq_destroy_tags(struct Scsi_Host *shost)
  {
-	if (!shost->tag_set.tags)
-		return;
  	blk_mq_free_tag_set(&shost->tag_set);
-	WARN_ON_ONCE(shost->tag_set.tags);
  }


Thanks,

Bart.

Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

[John Garry]

On 14/07/2022 19:49, Bart Van Assche wrote:
>>> Semantics of the API? Shouldn't this rather be called an undocumented 
>>> aspect of blk_mq_free_tag_set()?
>>>
>>> My concern is that the "set->tags = NULL" statement might be removed 
>>> in the future from blk_mq_free_tag_set() and also that it is possible 
>>> that scsi_mq_destroy_tags() is not updated after that change.
>>
>> Sure, so how it is possible that set->tags == NULL ever when calling 
>> scsi_mq_setup_tags()? I'm just wondering why you even care.
> 
> How about applying the patch below on top of patch 4/4?
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 295c48fdb650..2aca0a838ca5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1990,10 +1990,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
> 
>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>   {
> -    if (!shost->tag_set.tags)
> -        return;
>       blk_mq_free_tag_set(&shost->tag_set);
> -    WARN_ON_ONCE(shost->tag_set.tags);
>   }

That looks better.

Thanks,
John