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

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

On 9/1/26 12:02, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Calling synchronize_srcu() inside an iteration loop defeats bulk quiesce optimizations and stalls transport error recovery.
> - [High] Path clearing is missing for devices blocked natively by low-level drivers.
> - [High] TOCTOU race in `scsi_device_block` accessing `sdev->scsi_mpath_dev` without locking causes NULL pointer dereferences and Use-After-Free during device initialization or removal.
> --
> 
> commit a4233dac056f9047f69ee519256fc76b917ab456
> Author: John Garry <john.garry@linux.dev>
> 
> scsi-multipath: clear path when device is blocked
>      
> This commit adds scsi_mpath_dev_clear_path() and invokes it from
> scsi_device_block() to clear a device path when it becomes blocked.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index af27fd3df8d46..f02b3a50e20cd 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]
> Is there a TOCTOU race when accessing sdev->scsi_mpath_dev after releasing
> the state_mutex?
> 
> If __scsi_remove_device() runs concurrently on another CPU, it can acquire
> the state_mutex, change the state to SDEV_DEL, release the lock, and invoke
> scsi_mpath_remove_device(), which sets scsi_mpath_head to NULL.
> 
> Since err was cached as 0, this execution path could proceed to safely
> evaluate the if-statement, then call scsi_mpath_dev_clear_path(), resulting
> in a NULL pointer dereference on scsi_mpath_head.

That will not happen unless the sdev disappears, which should not happen 
when scsi_device_block() is being called.

> 
> 
> [Severity: High]
> Are low-level driver initiated blocks missing this path clearing logic?
> 
> When low-level drivers natively block devices due to hardware errors, they
> call scsi_internal_device_block_nowait(). This bypasses scsi_device_block()
> entirely and directly calls scsi_stop_queue(sdev), meaning multipath
> failover won't be triggered.
> 
> Should path clearing be added for native blocks as well?

No, as mpath_clear_current_path() -> mpath_synchronize() may block. Not 
clearing paths is not critical, as we should not use the path if it is 
disabled anyway.

> 
>>   
>>   	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;
>> +
>> +       if (mpath_clear_current_path(mpath_device))
>> +               mpath_synchronize(mpath_head);
> 
> [Severity: High]
> Does this code stall bulk quiesce operations during error recovery?
> 
> This function is called by scsi_device_block(), which is executed in a loop
> for all targets and LUNs during error recovery:
> 
> scsi_block_targets()
>    device_for_each_child() -> target_block()
>      starget_for_each_device() -> scsi_device_block()
> 
> Since mpath_synchronize() is a wrapper for synchronize_srcu(), waiting
> synchronously on an SRCU grace period here inside the loop could result in
> multi-second stalls, potentially cascading into command timeouts and
> unrecoverable host resets.
> 

We can address it if it becomes a real problem.