diff mbox series

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

Message ID 20220712221936.1199196-5-bvanassche@acm.org
State New
Headers show
Series Call blk_mq_free_tag_set() earlier | expand

Commit Message

Bart Van Assche July 12, 2022, 10:19 p.m. UTC 
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(-)

Comments

Bart Van Assche July 14, 2022, 6:49 p.m. UTC | #1 
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.
John Garry July 15, 2022, 7:54 a.m. UTC | #2 
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
diff mbox series

Patch

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);
 }
 
 /**