[PATCH v3 0/2] Call blk_mq_free_tag_set() earlier

[PATCH v3 0/2] 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 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 (1):
  scsi: core: Call blk_mq_free_tag_set() earlier

Ming Lei (1):
  scsi: core: Make sure that hosts outlive targets and devices

 drivers/scsi/hosts.c      | 19 ++++++++++++++-----
 drivers/scsi/scsi.c       |  9 ++++++---
 drivers/scsi/scsi_lib.c   |  3 +++
 drivers/scsi/scsi_scan.c  |  7 +++++++
 drivers/scsi/scsi_sysfs.c |  9 ---------
 include/scsi/scsi_host.h  |  3 +++
 6 files changed, 33 insertions(+), 17 deletions(-)

[PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[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: 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 ]
---
 drivers/scsi/hosts.c      | 9 +++++++++
 drivers/scsi/scsi.c       | 9 ++++++---
 drivers/scsi/scsi_scan.c  | 7 +++++++
 drivers/scsi/scsi_sysfs.c | 9 ---------
 include/scsi/scsi_host.h  | 3 +++
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index ef6c0e37acce..e0a56a8f1f74 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -168,6 +168,7 @@ void scsi_remove_host(struct Scsi_Host *shost)
 
 	mutex_lock(&shost->scan_mutex);
 	spin_lock_irqsave(shost->host_lock, flags);
+	/* Prevent that new SCSI targets or SCSI devices are added. */
 	if (scsi_host_set_state(shost, SHOST_CANCEL))
 		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
 			spin_unlock_irqrestore(shost->host_lock, flags);
@@ -190,6 +191,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 +402,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.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_scan.c b/drivers/scsi/scsi_scan.c
index 91ac901a6682..00c5754f0081 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);
 }
 
@@ -521,6 +526,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;
+	atomic_inc(&shost->target_count);
+
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 43949798a2e4..72bf5131e9d8 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -450,12 +450,9 @@ 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;
-
 	scsi_dh_release_device(sdev);
 
 	parent = sdev->sdev_gendev.parent;
@@ -518,17 +515,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);
 }
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

Re: [PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[Mike Christie]

On 7/7/22 1:21 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: 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 ]
> ---
>  drivers/scsi/hosts.c      | 9 +++++++++
>  drivers/scsi/scsi.c       | 9 ++++++---
>  drivers/scsi/scsi_scan.c  | 7 +++++++
>  drivers/scsi/scsi_sysfs.c | 9 ---------
>  include/scsi/scsi_host.h  | 3 +++
>  5 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ef6c0e37acce..e0a56a8f1f74 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -168,6 +168,7 @@ void scsi_remove_host(struct Scsi_Host *shost)
>  
>  	mutex_lock(&shost->scan_mutex);
>  	spin_lock_irqsave(shost->host_lock, flags);
> +	/* Prevent that new SCSI targets or SCSI devices are added. */
>  	if (scsi_host_set_state(shost, SHOST_CANCEL))
>  		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
>  			spin_unlock_irqrestore(shost->host_lock, flags);
> @@ -190,6 +191,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);


Do we still have a possible use after free at the target removal level?

If you have a driver supports multiple targets and target removal (any of
he FC ones, HW iscsi, etc), then you can still hit:

1. thread1 does sysfs device delete. It's now waiting in blk_cleanup_queue
which is waiting on a cmd that has the SCSI error handler running on it or
for whatever reason.

2. thread2 decides to delete the target (dev loss tmo or user request). That
hits __scsi_remove_device for the device in #1 and sees it's in SDEV_DEL
state so it returns.

3. scsi_remove_target returns. The transport/driver then frees it's rport/session
for that target.

4. The thread1 in then makes progress in the EH callback and wants to reference
the rport/session struct we deleted in #3.

The drivers want to know that after scsi_remove_target has returned that nothing
is using devices under it similar to the scsi_remove_host case.

Every scsi_device has a scsi_target as a parent (scsi_device -> scsi_target) or
grandparent (scsi_device -> transport class struct like rport/session ->
scsi_target) now right. I was thinking maybe like 20 years ago when scsi_forget_host
was made we didn't?

If so, could we move what are doing in this patch down a level? Put the wait in
scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
you have a scsi_target sdev_count.

scsi_forget_host would then need to loop over the targets and do
scsi_target_remove on them instead of doing it at the scsi_device level.

Re: [PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[Bart Van Assche]

On 7/9/22 08:57, Mike Christie wrote:
> If so, could we move what are doing in this patch down a level? Put the wait in
> scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
> you have a scsi_target sdev_count.
> 
> scsi_forget_host would then need to loop over the targets and do
> scsi_target_remove on them instead of doing it at the scsi_device level.

Hi Mike,

Thanks for having taken a look.

Could the approach outlined above have user-visible side effects?

A different approach has been selected for v4 of this patch series. Further feedback
is welcome.

Thanks,

Bart.

Re: [PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[Mike Christie]

On 7/12/22 5:29 PM, Bart Van Assche wrote:
> On 7/9/22 08:57, Mike Christie wrote:
>> If so, could we move what are doing in this patch down a level? Put the wait in
>> scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
>> you have a scsi_target sdev_count.
>>
>> scsi_forget_host would then need to loop over the targets and do
>> scsi_target_remove on them instead of doing it at the scsi_device level.
> 
> Hi Mike,
> 
> Thanks for having taken a look.
> 
> Could the approach outlined above have user-visible side effects?
> 

What kind of scenario are you thinking about?

I think we would have similar behavior today for the target behavior:

1. With the current code, if there is no sysfs deletion going on and
scsi_remove_target's call to __scsi_remove_device is the one that blocks
on blk_cleanup_queue then the target removal would wait.


2. With the current code we have:

	1. syfs deletion runs scsi_remove_target -> scsi_remove_device and
that takes the scan_mutex.
	2. scsi_remove_target passed the SDEV_DEL check and is calling
scsi_remove_device which is waiting on the scan_mutex.

So here scsi_remove_target waits for the deletion similar to what I described
above.

My suggestion just handles the case where

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

It then sets the state to SDEV_DEL.

2. scsi_remove_target runs and see 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.




> A different approach has been selected for v4 of this patch series. Further feedback
> is welcome.
> 
> Thanks,
> 
> Bart.

Re: [PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[Mike Christie]

On 7/14/22 10:32 AM, Mike Christie wrote:
> On 7/12/22 5:29 PM, Bart Van Assche wrote:
>> On 7/9/22 08:57, Mike Christie wrote:
>>> If so, could we move what are doing in this patch down a level? Put the wait in
>>> scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
>>> you have a scsi_target sdev_count.
>>>
>>> scsi_forget_host would then need to loop over the targets and do
>>> scsi_target_remove on them instead of doing it at the scsi_device level.
>>
>> Hi Mike,
>>
>> Thanks for having taken a look.
>>
>> Could the approach outlined above have user-visible side effects?
>>
> 
> What kind of scenario are you thinking about?
> 
> I think we would have similar behavior today for the target behavior:
> 
> 1. With the current code, if there is no sysfs deletion going on and
> scsi_remove_target's call to __scsi_remove_device is the one that blocks
> on blk_cleanup_queue then the target removal would wait.
> 
> 
> 2. With the current code we have:
> 
> 	1. syfs deletion runs scsi_remove_target -> scsi_remove_device and
> that takes the scan_mutex.

Meant sdev_store_delete -> scsi_remove_device

> 	2. scsi_remove_target passed the SDEV_DEL check and is calling

Here I did mean scsi_remove_target. It's being called from something like
the fc or iscsi layer's transport threads.

> scsi_remove_device which is waiting on the scan_mutex.
> 
> So here scsi_remove_target waits for the deletion similar to what I described
> above.
> 
> My suggestion just handles the case where
> 
> 1. syfs deletion runs scsi_remove_target -> scsi_remove_device and
> that takes the scan_mutex.

Meant sdev_store_delete -> scsi_remove_device

> 
> It then sets the state to SDEV_DEL.
> 
> 2. scsi_remove_target runs and see the device is in the SDEV_DEL state.


Here I did mean scsi_remove_target. It's being called from something like
the fc or iscsi layer's transport threads.

> It skips the device and then we return from scsi_remove_target without
> having waited on that device's removal.
> 
> 
> 
> 
>> A different approach has been selected for v4 of this patch series. Further feedback
>> is welcome.
>>
>> Thanks,
>>
>> Bart.
> 

Re: [PATCH v3 1/2] scsi: core: Make sure that hosts outlive targets and devices

[Bart Van Assche]

On 7/9/22 08:57, Mike Christie wrote:
> If so, could we move what are doing in this patch down a level? Put the wait in
> scsi_remove_target and wake in scsi_target_dev_release. Instead of a target_count
> you have a scsi_target sdev_count.
> 
> scsi_forget_host would then need to loop over the targets and do
> scsi_target_remove on them instead of doing it at the scsi_device level.

I'm not sure how to implement the above since scsi_remove_target() skips 
targets that have one of the states STARGET_DEL, STARGET_REMOVE or 
STARGET_CREATED_REMOVE?

Thanks,

Bart.

[PATCH v3 2/2] 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 request queues 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: 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 e0a56a8f1f74..643140b22eb9 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -198,6 +198,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);
 
@@ -303,8 +305,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);
@@ -320,6 +322,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;
 }
@@ -353,9 +356,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 6ffc9e4258a8..1aa1a279f8f3 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);
 }
 
 /**