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

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

On 9/1/26 12:04, sashiko-bot@kernel.org wrote:
> 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?
> 

locking is added later


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

as above

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

as above

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