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

[sashiko-bot@kernel.org]

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.


[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?

>  
>  	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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901104526.3850621-1-john.g.garry@oracle.com?part=16