Re: [PATCH v2 2/3] scsi: Make scsi_forget_host() wait for request queue removal

Bart Van Assche
2022-07-01T23:44:58Z

On 7/1/22 09:25, Mike Christie wrote:
> On 6/30/22 4:37 PM, Bart Van Assche wrote:
>> struct scsi_device *sdev;
>> @@ -1970,8 +1980,21 @@ void scsi_forget_host(struct Scsi_Host *shost)
>> restart:
>> spin_lock_irq(shost->host_lock);
>> list_for_each_entry(sdev, &shost->__devices, siblings) {
>> - if (sdev->sdev_state == SDEV_DEL)
>> + if (sdev->sdev_state == SDEV_DEL &&
>> + blk_queue_dead(sdev->request_queue)) {
>> continue;
>> + }
>> + if (sdev->sdev_state == SDEV_DEL) {
>> + get_device(&sdev->sdev_gendev);
>> + spin_unlock_irq(shost->host_lock);
>> +
>> + while (!blk_queue_dead(sdev->request_queue))
>> + msleep(10);
>> +
>> + spin_lock_irq(shost->host_lock);
>> + put_device(&sdev->sdev_gendev);
>> + goto restart;
>> + }
>> spin_unlock_irq(shost->host_lock);
>> __scsi_remove_device(sdev);
>> goto restart;
>
> Are there 2 ways to hit your issue?
>
> 1. Normal case. srp_remove_one frees srp_device. Then all refs
> to host are dropped and we call srp_exit_cmd_priv which accesses
> the freed srp_device?
>
> You don't need the above patch for this case right.
>
> 2. Are you hitting a race? Something did a scsi_remove_device. It
> set the device state to SDEV_DEL. It's stuck in blk_cleanup_queue
> blk_freeze_queue. Now we do the above code. Without your patch
> we just move on by that device. Commands could still be accessed.
> With your patch we wait for for that other thread to complete the
> device destruction.
>
>
> Could you also solve both issues by adding a scsi_host_template
> scsi_host release function that's called from scsi_host_dev_release. A
> new srp_host_release would free structs like the srp device from there.
> Or would we still have an issue for #2 where we just don't know how
> far blk_freeze_queue has got so commands could still be hitting our
> queue_rq function when we are doing scsi_host_dev_release?
>
> I like how your patch makes it so we know after scsi_remove_host
> has returned that the device is really gone. Could we have a similar
> race as in #2 with someone doing a scsi_remove_device and transport
> doing scsi_remove_target?
>
> We would not hit the same use after free from the tag set exit_cmd_priv
> being called. But, for example, if we wanted to free some transport level
> resources that running commands reference after scsi_target_remove could
> we hit a use after free? If so maybe we want to move this wait/check to
> __scsi_remove_device?

Hi Mike,

Although I'm not sure that I completely understood the above: my goal
with this patch is to make sure that all activity on the host tag set
has stopped by the time scsi_forget_host() returns. I do not only want
to cover the case where scsi_remove_host() is called by the ib_srp
driver but also the case where a SCSI device that was created by the
ib_srp driver is removed before scsi_remove_host() is called. Users can
trigger this scenario by writing into /sys/class/scsi_device/*/*/delete.

Adding a new callback into scsi_host_dev_release() would not help
because the scsi_host_dev_release() call may happen long after
scsi_forget_host() returned.

Does this answer your question?

Bart.