Re: [PATCH v7 03/27] libmultipath: Add path selection support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Infinite loop in mpath_round_robin_path() due to concurrent path removal and cache overwrite.
--

commit 5ea7b90698a3876a3c3c747f99ff79a9d52b1e32
Author: John Garry <john.garry@linux.dev>

libmultipath: Add path selection support

This commit abstracts NVMe ANA into enum mpath_access_state to support
SCSI ALUA. It also adds path selection modes for round-robin, NUMA, and
queue-depth.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 1cf20b0f62d07..26714116023a3 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> @@ -109,6 +231,146 @@ static struct mpath_device *mpath_next_dev(struct mpath_head *mpath_head,
> +static struct mpath_device *mpath_round_robin_path(
> +				struct mpath_head *mpath_head)
> +{
> +	struct mpath_device *mpath_device, *found = NULL;
> +	int node = numa_node_id();
> +	enum mpath_access_state access_state_old;
> +	struct mpath_device *old =
> +			srcu_dereference(mpath_head->current_path[node],
> +				&mpath_head->srcu);
> +
> +	if (unlikely(!old))
> +		return __mpath_find_path(mpath_head, node);
> +
> +	if (list_is_singular(&mpath_head->dev_list)) {
> +		if (mpath_path_is_disabled(mpath_head, old))
> +			return NULL;
> +		return old;
> +	}
> +
> +	for (mpath_device = mpath_next_dev(mpath_head, old);
> +	    mpath_device && mpath_device != old;
> +	    mpath_device = mpath_next_dev(mpath_head, mpath_device)) {

[Severity: High]
Could this loop run infinitely if `old` is unlinked from the list concurrently?

If Thread 1 enters mpath_round_robin_path(), loads `old = D` from 
`current_path[node]`, and is preempted, another thread could enter, find a 
fallback path Y, and overwrite `current_path[node] = Y`.

When a removal thread subsequently calls mpath_clear_current_path(D), it would 
return false because the cached path is now Y. If this causes the removal 
thread to skip mpath_synchronize() and directly unlink D via 
mpath_delete_device(D), the path is removed from the list while Thread 1 still 
holds a reference to it.

When Thread 1 resumes, `old` is no longer linked. If there are no 
MPATH_STATE_OPTIMIZED paths to trigger an early break, mpath_next_dev() would 
cycle infinitely between the remaining active paths, because 
`mpath_device != old` would never evaluate to false. 

Is there a way to ensure the loop termination condition remains valid even if 
`old` is concurrently replaced and removed?

> +
> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
> +			continue;
> +		if (mpath_device->access_state == MPATH_STATE_OPTIMIZED) {
> +			found = mpath_device;
> +			goto out;
> +		}

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