Re: [PATCH v4 03/28] libmultipath: Add path selection support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The round-robin path selector can enter an infinite loop (SRCU lockup) if the currently selected path is removed concurrently.
- [Medium] Calling `list_is_singular()` on an SRCU-protected list introduces data races and state inconsistencies.
- [Low] Missing `WRITE_ONCE()` when updating `iopolicy` causes a data race with lockless readers.
--

--- Patch [3]: [PATCH v4 03/28] libmultipath: Add path selection support ---
commit 84fab111a99eb088e0b8abed39b8163220a9318d
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add path selection support

Add code for path selection. NVMe ANA is abstracted into enum
mpath_access_state so that SCSI ALUA can be used. Path selection modes
round-robin, NUMA, and queue-depth are added.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 916d01a285284..20bb8cb0ea956 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> +int mpath_set_iopolicy(const char *str, enum mpath_iopolicy_e *iopolicy)
> +{
> +	int policy;
> +
> +	if (!str)
> +		return -EINVAL;
> +	policy = mpath_iopolicy_parse(str);
> +	if (policy < 0)
> +		return policy;
> +	*iopolicy = policy;

[Severity: Low]
Does this code need a WRITE_ONCE() for the assignment?

mpath_read_iopolicy() safely accesses this locklessly via READ_ONCE(), but
this plain store could cause a data race violating LKMM guidelines and
triggering KCSAN warnings.

> +
> +	return 0;
> +}

[ ... ]

> +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;
> +	}

[Severity: Medium]
Is it safe to use list_is_singular() on an SRCU-protected list here?

list_is_singular() reads head->prev, which is a writer-only pointer in
RCU-protected lists, introducing a data race.

Additionally, if old was just concurrently unlinked leaving exactly one other
active device in the list, list_is_singular() evaluates to true. Could this
cause the function to incorrectly return the unlinked device instead of the
actual remaining device?

> +
> +	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 become infinite if the currently selected path is removed
concurrently?

If a concurrent thread unlinks old from the list, mpath_next_dev() will never
reach mpath_device == old to break the loop. Since old is unlinked, traversal
might wrap around the circular list infinitely, causing a hard lockup within
the SRCU read-side critical section.

Unlike the NVMe equivalent nvme_round_robin_path(), which avoids this by
breaking early if it cycles back to a fallback node, this patch omits that
loop termination condition. If no other path is MPATH_STATE_OPTIMIZED, could
this run forever?

> +
> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
> +			continue;

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