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

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?

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?

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

> +	}

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3