Re: [PATCH v4 20/28] scsi-multipath: provide callbacks for path state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ALUA access state is not synchronized with the core multipath library, causing path selection to incorrectly treat non-optimized paths as optimal and resulting in an infinite path re-evaluation loop.
- [High] Missing ALUA path activation and state change notifications cause indefinite I/O hangs when only STANDBY, TRANSITIONING, or LBA-dependent paths are available.
- [Medium] Missing READ_ONCE() on concurrent sdev->sdev_state access can cause TOCTOU vulnerabilities via compiler reloads.
--

--- Patch [20]: [PATCH v4 20/28] scsi-multipath: provide callbacks for path state ---
commit 0d49283b5a1edcf6983ec33d9d54dad0023fe90c
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: provide callbacks for path state

This commit provides callbacks for .is_disabled, .is_optimized, and
.available_path. These all use scsi_device.sdev_state and
scsi_device.access_state, which will be driven by ALUA.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index a482b6ede21aa..e566d47199fcc 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -316,7 +316,56 @@ static struct bio *scsi_mpath_clone_bio(struct bio *bio)
>  	return clone;
>  }
>  
> +static bool scsi_mpath_is_disabled(struct mpath_device *mpath_device)
> +{
> +	struct scsi_mpath_device *scsi_mpath_dev =
> +				to_scsi_mpath_device(mpath_device);
> +	struct scsi_device *sdev = scsi_mpath_dev->sdev;
> +	unsigned char access_state = READ_ONCE(sdev->access_state) &
> +					SCSI_ACCESS_STATE_MASK;
> +
> +	if (sdev->sdev_state != SDEV_RUNNING)
> +		return true;

[Severity: Medium]
Should sdev->sdev_state be read with READ_ONCE() here to avoid compiler
reloads, similar to how access_state is read above?

> +
> +	if (access_state == SCSI_ACCESS_STATE_OPTIMAL ||
> +	    access_state == SCSI_ACCESS_STATE_ACTIVE)
> +		return false;
> +
> +	return true;

[Severity: High]
If a path is in ALUA STANDBY or TRANSITIONING state, it is treated as disabled
and skipped during path selection.

When all paths are in these states, scsi_mpath_available_path() will return
true since the state is SDEV_RUNNING, causing the block layer to requeue the
I/O.

Does this logic ever call scsi_dh_activate() or trigger an implicit ALUA
activation to transition these standby paths to optimal? Could this result in
an indefinite I/O hang with the requeue list looping forever?

> +}
> +
> +static bool scsi_mpath_is_optimized(struct mpath_device *mpath_device)
> +{
> +	struct scsi_mpath_device *scsi_mpath_dev =
> +				to_scsi_mpath_device(mpath_device);
> +	struct scsi_device *sdev = scsi_mpath_dev->sdev;
> +	unsigned char access_state = READ_ONCE(sdev->access_state) &
> +					SCSI_ACCESS_STATE_MASK;
> +
> +	if (sdev->sdev_state != SDEV_RUNNING)
> +		return false;

[Severity: Medium]
Should this unlocked read of sdev->sdev_state also use READ_ONCE()?

> +
> +	return access_state == SCSI_ACCESS_STATE_OPTIMAL;

[Severity: High]
Does this dynamic check of the ALUA state conflict with the statically
initialized access state in the core multipath library?

If an I/O is submitted to an ALUA Active/Non-Optimized path, won't the core
reject the cached path, fall back to __mpath_find_path() which uses the
static state, and continuously re-cache the exact same path? Could this
result in a perpetual cache miss and an infinite path re-evaluation loop on
every submission?

> +}
> +
> +static bool scsi_mpath_available_path(struct mpath_device *mpath_device)
> +{
> +	struct scsi_mpath_device *scsi_mpath_dev =
> +				to_scsi_mpath_device(mpath_device);
> +	struct scsi_device *sdev = scsi_mpath_dev->sdev;
> +	enum scsi_device_state sdev_state = sdev->sdev_state;
> +
> +	if (sdev_state == SDEV_RUNNING || sdev_state == SDEV_QUIESCE ||
> +	    sdev_state == SDEV_BLOCK || sdev_state == SDEV_CREATED_BLOCK)
> +		return true;

[Severity: Medium]
Since sdev_state is a local variable assigned via a plain read, can the
compiler reload it from memory for each condition in the if statement?

If the state changes concurrently between SDEV_BLOCK and SDEV_RUNNING, could
all checks evaluate to false even if the state is always one of the valid
values? Should this use READ_ONCE()?

> +
> +	return false;
> +}

[ ... ]

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