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

On 29/07/2026 12:33, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Round-robin path selection behaves as reverse round-robin for non-optimized paths due to a missing loop break.
> - [High] Unsafe and logically flawed use of `list_is_singular` on an RCU-protected list routes I/O to a removed device.
> 
> Pre-existing issues:
> - [High] mpath_head_uninit() blocks indefinitely waiting for user-space to close the block device.
> --
> 
> --- Patch [3]: [PATCH v6 03/27] libmultipath: Add path selection support ---
> commit 70a41be1b0a598af8cbd34732b889f79118d17ac
> Author: John Garry <[email protected]>
> 
> libmultipath: Add path selection support
> 
> This commit adds code for path selection support in libmultipath. It abstracts
> NVMe ANA into mpath_access_state to allow SCSI ALUA usage and adds round-robin,
> NUMA, and queue-depth selection modes. It also introduces functions to set and
> get the default I/O policy.
> 
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index 1cf20b0f62d07..26714116023a3 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
> 
> [ ... ]
> 
>> @@ -7,8 +7,245 @@
>> +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)) {
> 
> [Severity: High]
> Does using list_is_singular() here on an SRCU-protected list introduce a data
> race on head->prev?

locking is added later

> 
> Additionally, if old is concurrently removed and exactly one other active
> device remains in the list, list_is_singular() evaluates to true. Will this
> cause the function to return the unlinked old path instead of the valid
> active path?

as above

> 
>> +		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)) {
>> +
>> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
>> +			continue;
>> +		if (mpath_device->access_state == MPATH_STATE_OPTIMIZED) {
>> +			found = mpath_device;
>> +			goto out;
>> +		}
>> +		if (mpath_device->access_state == MPATH_STATE_NONOPTIMIZED)
>> +			found = mpath_device;
> 
> [Severity: High]
> For the MPATH_STATE_NONOPTIMIZED case, found is overwritten unconditionally.
> Because there is no loop break or check like if (!found), will this
> evaluate multiple non-optimized paths and incorrectly select the last
> available one rather than the next available one?

I think so, but no one cares about non-optimized. Again, this is same as 
NVMe code.

> 
>> +	}
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential problem during
> teardown in mpath_head_uninit():
> 
> lib/multipath.c:mpath_head_uninit() {
>          ...
>          if (!refcount_dec_and_test(refcount))
>                  wait_var_event(refcount, !refcount_read(refcount));
> }
> 
> Since a reference is taken for every user-space open, can wait_var_event()
> block indefinitely if a user process holds the device open while the
> driver is unbound or the device is hot-unplugged?

yes, but that is just the way it is - userspace needs to drop the ref

>