Re: [PATCH v5 17/27] scsi-multipath: revalidate paths upon device unblock

On 27/07/2026 16:17, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Race condition in `scsi_internal_device_unblock` and `scsi_device_block` leading to NULL pointer dereference.
> - [High] Concurrent execution of `scsi_mpath_dev_alloc` and transport block/unblock leads to Use-After-Free and NULL pointer dereference.
> --
> 
> --- Patch [17]: [PATCH v5 17/27] scsi-multipath: revalidate paths upon device unblock ---
> commit d7b8727956ce248cf872dc90fa72742ade627f54
> Author: John Garry <[email protected]>
> 
> scsi-multipath: revalidate paths upon device unblock
>      
> When a scsi_device is unblocked, this patch clears paths and requeues
> any queued bios to revalidate the paths.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
> [ ... ]
>> @@ -3092,6 +3092,8 @@ static int scsi_internal_device_unblock(struct scsi_device *sdev,
>>   	mutex_lock(&sdev->state_mutex);
>>   	ret = scsi_internal_device_unblock_nowait(sdev, new_state);
>>   	mutex_unlock(&sdev->state_mutex);
>> +	if ((ret == 0) && sdev->scsi_mpath_dev)
>> +		scsi_mpath_revalidate_paths(sdev->scsi_mpath_dev);
>>   
>>   	return ret;
>>   }
> 
> [Severity: High]
> Can this lockless check of sdev->scsi_mpath_dev lead to a use-after-free or
> NULL pointer dereference?

Last time you complained about having locking. Now you complain about no 
locking.

> 
> During device discovery, scsi_alloc_sdev() publishes the scsi_device to
> shost->__devices. After this, scsi_mpath_dev_alloc() assigns
> sdev->scsi_mpath_dev but can fail and jump to out_uninit where it frees
> the structure:
> 
> drivers/scsi/scsi_multipath.c:scsi_mpath_dev_alloc() {
>      ...
>      ret = scsi_multipath_sdev_init(sdev);
>      if (ret)
>          return ret;
> 
>      ret = scsi_mpath_unique_lun_id(sdev);
>      if (ret < 0)
>          goto out_uninit;
>      ...
> }
> 
> If an asynchronous transport event triggers scsi_internal_device_unblock()
> during this window, could it read a freed sdev->scsi_mpath_dev, or one where
> scsi_mpath_head is not yet initialized?

Give a proper example of when this can happen and not just mention 
"asynchronous transport event"
> 
> Could a concurrent device removal also trigger a similar race?
> 
> If __scsi_remove_device() executes after the mutex_unlock() but before
> sdev->scsi_mpath_dev is evaluated

Again, this should not happen as __scsi_remove_device() should not be 
called before sdev->scsi_mpath_dev is evaluated

>, it could set the device state to
> SDEV_CANCEL and call scsi_mpath_remove_device(). This sets
> scsi_mpath_dev->scsi_mpath_head to NULL.
> 
> (Note: An earlier patch in this series appears to introduce an identical
> lockless pattern in scsi_device_block() checking sdev->scsi_mpath_dev.)

Who is calling scsi_internal_device_unblock() and __scsi_remove_device() 
in parallel?

> 
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> [ ... ]
>> @@ -231,6 +231,14 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>>   	return 0;
>>   }
>>   
>> +void scsi_mpath_revalidate_paths(struct scsi_mpath_device *scsi_mpath_dev)
>> +{
>> +       struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;
>> +       struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
>> +
>> +       mpath_revalidate_paths(mpath_head);
>> +}
> 
> [Severity: High]
> Does this code safely handle a NULL scsi_mpath_head?
> 
> If the race mentioned above occurs and a concurrent teardown clears
> scsi_mpath_head to NULL, would this assignment dereference the NULL pointer
> when attempting to resolve mpath_head?
>