Re: [PATCH v5 16/27] scsi-multipath: clear path when device is blocked

On 27/07/2026 16:14, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `scsi_device_block()` due to lockless access to `sdev->scsi_mpath_dev` leads to NULL pointer dereferences and Use-After-Free vulnerabilities during device initialization, teardown, and initialization failure.
> --
> 
> --- Patch [16]: [PATCH v5 16/27] scsi-multipath: clear path when device is blocked ---
> commit 029c1109c722c76acac1333821c2190171087cb6
> Author: John Garry <[email protected]>
> 
> scsi-multipath: clear path when device is blocked
> 
> Add scsi_mpath_dev_clear_path() to clear a device path when it becomes
> blocked.
> 
> Signed-off-by: John Garry <[email protected]>
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index daeb3693fe559..a1ec11050090c 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
> 
> [ ... ]
> 
>> @@ -3008,6 +3009,8 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
>>   		scsi_stop_queue(sdev);
>>   
>>   	mutex_unlock(&sdev->state_mutex);
>> +	if ((err == 0) && sdev->scsi_mpath_dev)
>> +		scsi_mpath_dev_clear_path(sdev->scsi_mpath_dev);
> 
> [Severity: High]
> Since sdev->scsi_mpath_dev is read locklessly after dropping state_mutex,
> can this race with device teardown or initialization failures?
> 
> If a concurrent teardown or error path calls scsi_multipath_sdev_uninit()
> and frees sdev->scsi_mpath_dev, could this result in a use-after-free when
> evaluating or passing the pointer to scsi_mpath_dev_clear_path()?

scsi_multipath_sdev_uninit is called late in the sdev teardown - at this 
point scsi_device_block() should not be called

> 
>>   	WARN_ONCE(err, "%s: failed to block %s in state %d\n",
>>   		  __func__, dev_name(&sdev->sdev_gendev), state);
> 
> [ ... ]
> 
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index 5999c59bf7297..801d95652fc6b 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> 
> [ ... ]
> 
>> @@ -231,6 +231,16 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>>   	return 0;
>>   }
>>   
>> +void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
>> +{
>> +       struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;
>> +       struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;
>> +       struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
> 
> [Severity: High]
> Could scsi_mpath_dev->scsi_mpath_head be NULL here?
> 
> Looking at scsi_multipath_sdev_init(), sdev->scsi_mpath_dev is published
> globally before the inner scsi_mpath_head structure is fully initialized:
> 
> scsi_multipath_sdev_init() {
>      ...
>      scsi_mpath_dev->sdev = sdev;
>      sdev->scsi_mpath_dev = scsi_mpath_dev;
>      ...
> }
> 
> If a concurrent scsi_device_block() call sees the non-NULL wrapper, would
> this dereference a NULL scsi_mpath_head?

why would we see scsi_device_block() called for a device before we even 
call scsi_sysfs_add_sdev()?

> 
>> +
>> +       if (mpath_clear_current_path(mpath_device))
>> +               mpath_synchronize(mpath_head);
>> +}
>