Re: [PATCH v3 03/13] libmultipath: Add path selection support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `mpath_round_robin_path` enters an infinite loop if the current cached path (`old`) is concurrently removed.
- [High] Unsafe usage of `list_is_singular()` on an RCU list leads to data races and routes I/O to a removed path.
- [High] `mpath_numa_path` fails to verify if the cached path is disabled before returning it, potentially routing I/O to a disabled path.
- [High] `mpath_alloc_head_disk` leaves an ERR_PTR in `mpath_head->disk` on failure, causing cleanup paths to crash.
- [Medium] NULL pointer dereference in `mpath_queue_depth_path` if a device does not provide `nr_active`.
- [Medium] The `.get_access_state` callback is missing from `struct mpath_head_template`, violating the documented API intent.
--

commit e941f797884516dd9568e0b668984eb3c895248a
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. The motivation here is
>     so that SCSI ALUA can be used. Callbacks .is_disabled, .is_optimized,
>     .get_access_state are added to get the path access state.

[Severity: Medium]
The commit message mentions adding a .get_access_state callback, but it appears
to be missing from struct mpath_head_template. Is this intentional, or should
it be included in the struct definition to avoid requiring drivers to directly
modify the access_state field?

[ ... ]
> diff --git a/lib/multipath.c b/lib/multipath.c
> index 79be84d3d4f75..21f7ffdb22d60 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
[ ... ]
> +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: High]
Could using list_is_singular() here lead to data races? Since this list is
managed with RCU, and list_is_singular() evaluates head->next == head->prev,
it might read inconsistent prev pointers during concurrent updates. Also, if old
was concurrently removed and only one valid path remains, would this return
the removed old path instead of the new remaining path?

> +
> +	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]
Does this loop terminate safely if old has been concurrently removed from the list?
If old is unlinked via list_del_rcu(), mpath_device != old will always be true.
Could this result in an infinite loop wrapping around the active elements
indefinitely, leading to a lockup if no path is MPATH_STATE_OPTIMIZED?

> +
> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
> +			continue;
[ ... ]
> +static struct mpath_device *mpath_queue_depth_path(
> +				struct mpath_head *mpath_head)
> +{
> +	struct mpath_device *best_opt = NULL, *mpath_device;
> +	struct mpath_device *best_nonopt = NULL;
> +	unsigned int min_depth_opt = UINT_MAX, min_depth_nonopt = UINT_MAX;
> +	unsigned int depth;
> +
> +	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;
> +
> +		depth = atomic_read(mpath_device->nr_active);

[Severity: Medium]
Could this dereference a NULL pointer if a device was added without providing
an nr_active atomic counter during mpath_add_device()? If userspace sets the
I/O policy to 'queue-depth' via sysfs, would it crash here?

> +
> +		switch (mpath_device->access_state) {
[ ... ]
> +static struct mpath_device *mpath_numa_path(struct mpath_head *mpath_head)
> +{
> +	int node = numa_node_id();
> +	struct mpath_device *mpath_device;
> +
> +	mpath_device = srcu_dereference(mpath_head->current_path[node],
> +					&mpath_head->srcu);
> +	if (unlikely(!mpath_device))
> +		return __mpath_find_path(mpath_head, node);
> +	if (unlikely(!mpath_path_is_optimized(mpath_head, mpath_device)))
> +		return __mpath_find_path(mpath_head, node);
> +	return mpath_device;
> +}

[Severity: High]
Does this correctly validate the cached path before returning it? Unlike other
path selection functions, this omits checking mpath_path_is_disabled().
If a path transitions to a disabled state but remains marked as optimized, could
mpath_numa_path() incorrectly continue to route I/O to it?

[ ... ]
> @@ -103,7 +339,8 @@ EXPORT_SYMBOL_GPL(mpath_put_disk);
>  int mpath_alloc_head_disk(struct mpath_head *mpath_head,
>  			struct queue_limits *lim, int numa_node)
>  {
> -	if (!mpath_head->disk_groups || !mpath_head->parent)
> +	if (!mpath_head->disk_groups || !mpath_head->parent ||
> +	    !mpath_head->iopolicy)
>  		return -EINVAL;
>  
>  	mpath_head->disk = blk_alloc_disk(lim, numa_node);

[Severity: High]
If blk_alloc_disk() fails, it returns an ERR_PTR. If this is assigned directly
to mpath_head->disk and an error is returned without setting mpath_head->disk
back to NULL, could this cause subsequent cleanup paths (like mpath_put_disk())
to crash when they check if (mpath_head->disk) and attempt to put an ERR_PTR?

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