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

[John Garry <john.garry@linux.dev>]

On 9/7/26 12:49, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Lockless and unrefcounted access to `sdev->scsi_mpath_dev` in `scsi_device_block()` leads to Use-After-Free and Null Pointer Dereferences during concurrent device initialization or removal.
> --
> 
> commit 352c419b223cf3635501b6ef6ac4632a3b017a6d
> Author: John Garry <john.garry@linux.dev>
> 
> scsi-multipath: clear path when device is blocked
> 
> This commit adds scsi_mpath_dev_clear_path() and calls it from
> scsi_device_block() when a device path becomes blocked.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 48aab0df30b76..56200a7f5c748 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
> [ ... ]
>> @@ -3009,6 +3010,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]
> Can this lockless access to sdev->scsi_mpath_dev lead to a use-after-free
> during concurrent device removal?
> 
> If __scsi_remove_device() drops the reference and clears scsi_mpath_head,
> and scsi_device_block() races with it, it drops state_mutex and then reads
> sdev->scsi_mpath_dev locklessly. Could it proceed to call
> scsi_mpath_dev_clear_path() and dereference freed pointers?
> 
>>   
>>   	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 68fe50f62158c..b16fe6a55fa73 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]
> Are these pointers safe to dereference during concurrent device
> initialization?
> 
> When scsi_mpath_dev_alloc() calls scsi_multipath_sdev_init(), it assigns
> sdev->scsi_mpath_dev early, before fully initializing the pointers.

Same response as v7

> 
> If a concurrent scsi_target_block() transitions the device to SDEV_BLOCK,
> could it call scsi_mpath_dev_clear_path() and dereference an uninitialized
> or null scsi_mpath_head here, resulting in a crash?
> 
>> +
>> +       if (mpath_clear_current_path(mpath_device))
>> +               mpath_synchronize(mpath_head);
>> +}
>