Re: [PATCH v3 06/17] scsi-multipath: clear path when device is blocked

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 12:32, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [Critical] Unsafe lockless access to `scsi_mpath_head` in `scsi_mpath_dev_clear_path` races with device removal, causing a NULL pointer dereference or Use-After-Free.
> - [High] Adding a blocking synchronization call to the `nowait` context of `__scsi_internal_device_block_nowait` violates non-blocking guarantees.
> - [High] Publishing `sdev->scsi_mpath_dev` before `scsi_mpath_head` is initialized leads to NULL pointer dereferences during concurrent block requests.
> 
> Pre-existing issues:
> - [High] The error path in `scsi_mpath_alloc_head()` double-frees the `scsi_mpath_head` structure and its associated resources.
> - [High] The error path for `device_add()` failure in `scsi_mpath_dev_alloc()` causes a NULL pointer dereference and an invalid `device_unregister()`.
> --
> 
> commit 848d562af61faa57ddead6bc3e5e7a75ce63ea4e
> Author: John Garry <john.g.garry@oracle.com>
> Subject: scsi-multipath: clear path when device is blocked
> 
> This patch adds scsi_mpath_dev_clear_path() to clear a device path when it
> becomes blocked. It updates __scsi_internal_device_block_nowait() to call
> this new function when a path is available.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index daeb3693fe559..3c493068adb98 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
> [ ... ]
>> @@ -2923,6 +2924,8 @@ EXPORT_SYMBOL(scsi_target_resume);
>>   
>>   static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>   {
>> +	if (sdev->scsi_mpath_dev)
> 
> [Severity: High]
> Does this early access race with device probe?
> 
> If scsi_multipath_sdev_init() publishes sdev->scsi_mpath_dev before
> allocating scsi_mpath_head, could an asynchronous block request see this as
> true and invoke scsi_mpath_dev_clear_path(), leading to a NULL pointer
> dereference on the head?

this should not be a problem, but I will change to alloc head before 
publishing sdev->scsi_mpath_dev

> 
>> +		scsi_mpath_dev_clear_path(sdev->scsi_mpath_dev);
>>   	if (scsi_device_set_state(sdev, SDEV_BLOCK))
>>   		return scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
>>   
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index ca4ab720c19af..0f3f3f9fa5fae 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> [ ... ]
>> @@ -113,6 +113,17 @@ static ssize_t scsi_mpath_device_vpd_id_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR(vpd_id, S_IRUGO, scsi_mpath_device_vpd_id_show, NULL);
>>   
>> +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;
> 
> [Severity: Critical]
> Is this lockless access safe against concurrent device removal?
> 
> If scsi_mpath_remove_head() drops the head reference locklessly and sets:
>      scsi_mpath_dev->scsi_mpath_head = NULL;
> 
> Could a concurrent event hitting __scsi_internal_device_block_nowait()
> unconditionally call this and blindly dereference the NULL or freed
> scsi_mpath_head?

this should not happen as the sdev should be nearly removed when the 
scsi mpath head reference is put

> 
>> +	struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;
>> +
>> +	if (mpath_clear_current_path(mpath_device))
>> +		mpath_synchronize(mpath_head);
> 
> [Severity: High]
> Does this introduce a sleeping call into a nowait context?
> 
> __scsi_internal_device_block_nowait() is designed not to sleep to preserve
> batching optimizations. Since mpath_synchronize() typically sleeps to wait
> for grace periods or draining I/O, will calling it here break those
> optimizations or cause scheduling while atomic panics?

I need to check that

> 
>> +}
>> +EXPORT_SYMBOL_GPL(scsi_mpath_dev_clear_path);