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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Round-robin path selection can enter an infinite loop under the SRCU read lock if the current path is concurrently removed.
- [High] If `blk_alloc_disk()` fails, an `ERR_PTR` is left in `mpath_head->disk`, which will cause a kernel panic during cleanup.
- [High] Missing requeue list flushes in `mpath_device_set_live` and `mpath_remove_disk` cause IO stalls and bio leaks.
- [High] `mpath_round_robin_path` incorrectly returns a deleted path when the path list becomes singular.
--

--- Patch [3]: [PATCH v5 03/27] libmultipath: Add path selection support ---
commit 88605e74a8a44a0f658b820940be9c7cdbbebc25
Author: John Garry <john.garry@linux.dev>

libmultipath: Add path selection support

This commit adds path selection support for NVMe ANA, abstracting it into
enum mpath_access_state. It introduces round-robin, NUMA, and queue-depth
selection modes along with iopolicy configuration functions.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index d2e334259f3c1..2b22c74760868 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]
If the cached path (old) was concurrently unlinked from the list, is it
possible that list_is_singular() evaluates to true because there is exactly
one other valid path remaining? If so, does this mistakenly return the
unlinked old path (or NULL) instead of evaluating the single remaining valid
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]
If the currently cached path (old) is concurrently removed via
list_del_rcu(), it is unlinked from dev_list. Since mpath_next_dev()
iterates through the list, will this loop fail to encounter the unlinked
old pointer? If all remaining paths are non-optimized or disabled, could
this result in an infinite loop inside an SRCU read-side critical section?

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

[ ... ]

> @@ -75,6 +312,7 @@ void mpath_remove_disk(struct mpath_head *mpath_head)
>  	if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
>  		struct gendisk *disk = mpath_head->disk;
>  
> +		mpath_synchronize(mpath_head);
>  		del_gendisk(disk);
>  	}

[Severity: High]
When the disk is removed, are there potentially stranded bios left in the
mpath_head->requeue_list? Without a call to mpath_schedule_requeue_work()
here to flush the list, do these queued bios leak?

>  }

[ ... ]

> @@ -96,7 +334,7 @@ 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 ||
> -	    mpath_head->disk)
> +	     !mpath_head->iopolicy || mpath_head->disk)
>  		return -EINVAL;
>  
>  	mpath_head->disk = blk_alloc_disk(lim, numa_node);

[Severity: High]
If blk_alloc_disk() fails, does it leave an ERR_PTR in mpath_head->disk?
If the error pointer is not cleared here, would the cleanup path in
mpath_put_disk() mistakenly evaluate "if (!mpath_head->disk)" as false
and attempt to call put_disk() on the error pointer?

[ ... ]

> @@ -130,6 +368,19 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>  		}
>  		queue_work(mpath_wq, &mpath_head->partition_scan_work);
>  	}
> +
> +	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);
>  }

[Severity: High]
When a new path becomes available and transitions to live, does this function
need to call mpath_schedule_requeue_work()? If bios were held in the
requeue_list while no paths were available, could omitting this flush lead
to permanent I/O stalls?

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