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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Infinite loop in `mpath_round_robin_path()` if the current round-robin path is concurrently removed from the device list, causing an SRCU deadlock.
- [High] `mpath_round_robin_path` fast-path incorrectly returns an unlinked path, breaking failover.
- [Medium] Calling `list_is_singular()` on an RCU-protected list without holding the update lock causes a data race on `head->prev`.
- [Medium] Path selection functions bypass the `.is_optimized` callback, violating the API contract and causing inconsistent path evaluation.
- [Low] Missing bounds check on iopolicy index in `mpath_get_iopolicy`.

Pre-existing issues:
- [Medium] `mpath_device_set_live()` silently swallows setup failures, leaving the driver unaware of the aborted initialization.
--

commit 59f56cbf3b60a1ec3a1336f453b4cd772a005e7b
Author: John Garry <john.garry@linux.dev>

libmultipath: Add path selection support

This commit adds path selection support to libmultipath, providing
round-robin, NUMA, and queue-depth selection modes similar to NVMe. It also
abstracts NVMe ANA into enum mpath_access_state to allow SCSI ALUA usage
and introduces iopolicy management functions.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 574868ce53381..3541357d07f8d 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -7,8 +7,245 @@

[ ... ]

> +int mpath_get_iopolicy(char *buf, int iopolicy)
> +{
> +	return sprintf(buf, "%s\n", mpath_iopolicy_names[iopolicy]);
> +}

[Severity: Low]
Does this code overflow mpath_iopolicy_names[] if a caller provides an
uninitialized or out-of-bounds iopolicy value?

[ ... ]

> +static struct mpath_device *__mpath_find_path(struct mpath_head *mpath_head,
> +					int node)
> +{
> +	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
> +	struct mpath_device *found = NULL, *fallback = NULL, *mpath_device;
> +
> +	list_for_each_entry_srcu(mpath_device, &mpath_head->dev_list, siblings,
> +		srcu_read_lock_held(&mpath_head->srcu)) {
> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
> +			continue;

[ ... ]

> +		switch(mpath_device->access_state) {
> +		case MPATH_STATE_OPTIMIZED:

[Severity: Medium]
Does this code bypass the driver's dynamic state evaluation by accessing
access_state directly instead of invoking the provided
mpath_path_is_optimized() callback wrapper?

If a driver like SCSI ALUA dynamically changes state, this will read
stale data.

> +		    if (distance < found_distance) {
> +			    found_distance = distance;
> +			    found = mpath_device;
> +		    }
> +		    break;

[ ... ]

> +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: Medium]
Calling list_is_singular() on an RCU-protected list without holding the
update lock causes a data race on head->prev. In RCU-protected lists,
only next pointers are safely updated for lockless readers.

[Severity: High]
Does this fast-path blindly return old without verifying if it is actually
the element present in the singular list?

If the previously cached old path was concurrently removed from dev_list
and exactly one valid path remains, list_is_singular() evaluates to true,
causing the function to incorrectly return the unlinked path instead of
the remaining valid path.

> +		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]
Can this loop spin infinitely if old is concurrently removed from the
RCU-protected list?

The loop relies on reaching old again to terminate. If a concurrent writer
unlinks old using list_del_rcu(), it is removed from the circular list but
its next pointer remains intact. mpath_next_dev() follows old->next into
the remaining list elements, wrapping around and circling the remaining
elements indefinitely without ever evaluating mpath_device == old.

If all remaining paths are non-optimized or disabled, the loop never hits
the break condition, spinning forever under the SRCU read lock and stalling
any writer.

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

[Severity: Medium]
Similar to __mpath_find_path(), does this bypass the driver's dynamic state
evaluation by accessing access_state directly rather than calling the
mpath_path_is_optimized() callback wrapper?

> +			found = mpath_device;
> +			goto out;
> +		}
> +		if (mpath_device->access_state == MPATH_STATE_NONOPTIMIZED)
> +			found = mpath_device;
> +	}

[ ... ]

> @@ -133,6 +371,19 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>  		}
>  		queue_work(mpath_wq, &mpath_head->partition_scan_work);
>  	}

[Severity: Medium]
This is a pre-existing issue, but does mpath_device_set_live() silently
swallow setup failures earlier in this function?

If device_add_disk() fails, for example due to memory exhaustion, the
function clears the MPATH_HEAD_DISK_LIVE flag and returns early, masking
the failure from the caller and leaving the driver unaware of the aborted
initialization.

> +
> +	mutex_lock(&mpath_head->lock);
> +	if (mpath_path_is_optimized(mpath_head, mpath_device)) {
> +		int node, srcu_idx;
> +
> +		srcu_idx = srcu_read_lock(&mpath_head->srcu);
> +		for_each_online_node(node)
> +			__mpath_find_path(mpath_head, node);
> +		srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	}
> +	mutex_unlock(&mpath_head->lock);
> +
> +	mpath_synchronize(mpath_head);
>  }

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