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

On 29/07/2026 15:42, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unprotected, lockless access to `sdev->scsi_mpath_dev` in `scsi_device_block()` causes NULL pointer dereferences and Use-After-Free (UAF) bugs when racing with device initialization or teardown.
> --
> 

Same as v5, there should be not races

> --- Patch [16]: [PATCH v6 16/27] scsi-multipath: clear path when device is blocked ---
> commit 8f07d9f873129f9ecc9b8d3d02fee5bce667c745
> 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.
> 
>> 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]
> Is it safe to access sdev->scsi_mpath_dev without holding state_mutex?
> 
> When scsi_alloc_sdev() adds a new device to shost->__devices, it becomes
> globally visible. Then scsi_mpath_dev_alloc() sets sdev->scsi_mpath_dev but
> leaves scsi_mpath_head as NULL while issuing SCSI VPD queries.
> 
> If a concurrent error on the bus triggers scsi_device_block(), it could
> read the partially initialized sdev->scsi_mpath_dev here outside of any locks.
> 
> Additionally, if scsi_mpath_dev_alloc() encounters an error and frees
> sdev->scsi_mpath_dev, could reading it here cause a use-after-free?
> 
> During teardown, if __scsi_remove_device() is called concurrently with
> scsi_device_block(), the teardown thread might set scsi_mpath_head to NULL.
> Can this lead to a NULL pointer dereference when evaluating
> scsi_mpath_dev_clear_path()?
> 
>>   
>>   	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 0d439cac6447a..b23cd6f12c602 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> 
> [ ... ]
> 
>> @@ -238,6 +238,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]
> Does this code dereference a NULL pointer if it races with initialization or
> teardown?
> 
> If scsi_mpath_head is NULL due to the races mentioned above, deriving
> mpath_head from it will calculate an invalid pointer.
> 
>> +
>> +       if (mpath_clear_current_path(mpath_device))
> 
> [Severity: High]
> Could calling mpath_clear_current_path() with a partially initialized
> mpath_device lead to a NULL pointer dereference?
> 
> If mpath_device->mpath_head is NULL, it will trigger an immediate dereference
> in the multipath code:
> 
> lib/multipath.c:mpath_clear_current_path() {
>      ...
> 	struct mpath_head *mpath_head = mpath_device->mpath_head;
>      ...
> 		if (mpath_device ==
> 			rcu_access_pointer(mpath_head->current_path[node])) {
>      ...
> }
> 
>> +               mpath_synchronize(mpath_head);
>> +}
>