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

On 29/07/2026 15:54, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Calling `scsi_mpath_revalidate_paths()` outside `sdev->state_mutex` without a NULL check leads to TOCTOU races with device removal or allocation, causing a NULL pointer dereference.

Same as v5 response, this should not happen

> - [High] Direct callers of `scsi_internal_device_unblock_nowait` bypass path revalidation, which can leave queued bios indefinitely stalled.

can you name relevant situations?

> 
> Pre-existing issues:
> - [High] The patch calls `mpath_revalidate_paths()`, which schedules `requeue_work` on a background workqueue, but the teardown path fails to cancel this work, leading to a Use-After-Free.

this is done by (disk) sd driver through calling mpath_put_disk()

> --
> 
> --- Patch [17]: [PATCH v6 17/27] scsi-multipath: revalidate paths upon device unblock ---
> commit 6253cb1fc352f4ec6d0d42ac99c1827b35592ff4
> Author: John Garry <[email protected]>
> 
> scsi-multipath: revalidate paths upon device unblock
> 
> When a scsi_device is unblocked, we need revalidate paths which means that
> we clear paths and requeue any queued bios.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index a1ec11050090c..6a2d7dbfbe8c3 100644
>> --- 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);
> 
> [Severity: High]
> Is it safe to access sdev->scsi_mpath_dev and call scsi_mpath_revalidate_paths()
> outside of sdev->state_mutex?
> 
> It seems that concurrently, __scsi_remove_device() could nullify
> scsi_mpath_dev->scsi_mpath_head. A similar race might exist during device
> initialization before the head is populated.
> 
> When scsi_mpath_revalidate_paths() is then called, it blindly computes the
> offset to &scsi_mpath_head->mpath_head, which could lead to a NULL pointer
> dereference.
> 
> [Severity: High]
> Does placing this hook in the wrapper bypass path revalidation for some drivers?
> 
> Low-level drivers like mpt3sas and scsi_transport_srp call the core API
> scsi_internal_device_unblock_nowait() directly. Since the revalidation is
> only added to the scsi_internal_device_unblock() wrapper, won't devices managed
> by those drivers miss path revalidation when unblocked, potentially leaving
> queued bios permanently stalled?
> 
>>   
>>   	return ret;
>>   }
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index b23cd6f12c602..12750621a7fc0 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> [ ... ]
>> @@ -238,6 +238,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]
> This is a pre-existing issue, but by introducing this new asynchronous trigger,
> could we run into a use-after-free during device removal?
> 
> Calling mpath_revalidate_paths() queues mpath_head->requeue_work. If the device
> is subsequently removed, the teardown path in scsi_mpath_head_release() frees
> scsi_mpath_head via kfree():
> 
> drivers/scsi/scsi_multipath.c:scsi_mpath_head_release() {
>      ...
>      mpath_head_uninit(mpath_head);
>      kfree(scsi_mpath_head);
> }
> 
> Because cancel_work_sync(&mpath_head->requeue_work) is missing from the teardown
> logic, could the pending work execute on a freed mpath_head structure?
> 
>> +}
>> +
>