Re: [PATCH] scsi: fix race condition when removing target

[Jason Yan <yanaijie@huawei.com>]

On 2017/12/1 23:35, James Bottomley wrote:
> On Fri, 2017-12-01 at 16:40 +0800, Jason Yan wrote:
>> On 2017/12/1 7:56, James Bottomley wrote:
>>> b/include/scsi/scsi_device.h
>>> index 571ddb49b926..2e4d48d8cd68 100644
>>> --- a/include/scsi/scsi_device.h
>>> +++ b/include/scsi/scsi_device.h
>>> @@ -380,6 +380,23 @@ extern struct scsi_device
>>> *__scsi_iterate_devices(struct Scsi_Host *,
>>>    #define __shost_for_each_device(sdev, shost) \
>>>    	list_for_each_entry((sdev), &((shost)->__devices),
>>> siblings)
>>>
>>
>> Seems that __shost_for_each_device() is still not safe. scsi device
>> been deleted stays in the list and put_device() can be called
>> anywhere out of the host lock.
>
> Not if it's used with scsi_get_device().  As I said, I only did a
> cursory inspectiont, so if I've missed a loop, please specify.
>
> The point was more a demonstration of how we could fix the problem if
> we don't change get_device().
>
> James
>

Yes, it's OK now. __shost_for_each_device() is not used with
scsi_get_device() yet.

Another problem is that put_device() cannot be called while holding the
host lock, so we need to remove all put_device() out of the lock. Some
places like scsi_device_lookup() and scsi_device_lookup_by_target() need 
rework:

@@ -765,12 +772,22 @@ struct scsi_device *scsi_device_lookup(struct 
Scsi_Host *shost,
         unsigned long flags;

         spin_lock_irqsave(shost->host_lock, flags);
-       sdev = __scsi_device_lookup(shost, channel, id, lun);
-       if (sdev && scsi_device_get(sdev))
-               sdev = NULL;
+       __sdev_for_each_get(sdev, &shost->__devices, siblings) {
+               spin_unlock_irqrestore(shost->host_lock, flags);
+               if (sdev->sdev_state != SDEV_DEL &&
+                   sdev->channel == channel && sdev->id == id &&
+                   sdev->lun ==lun) {
+                       if (!scsi_device_get(sdev)) {
+                               put_device(&sdev->sdev_gendev);
+                               return sdev;
+                       }
+               }
+               put_device(&sdev->sdev_gendev);
+               spin_lock_irqsave(shost->host_lock, flags);
+       }
         spin_unlock_irqrestore(shost->host_lock, flags);

-       return sdev;
+       return NULL;
  }
  EXPORT_SYMBOL(scsi_device_lookup);

>
>