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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 11:45, sashiko-bot@kernel.org wrote:
> 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?

I will fix this. This text was not updated from an earlier series.

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

uh, since we hold the RCU read lock for traversal, this should not 
happen. Having said that, since in this patch there are no users of 
mpath_find_path() and the user does the RCU read lock.

So maybe I can merge this patch with another. Or the context analysis 
annotations could help.

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

As above

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

Well the driver needs to ensure that it is valid. However I can add code 
to test this.

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

Yeah, maybe we should check that also to be clear

> 
> [ ... ]
>> @@ -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?

I already commented on this in the response to another patch review.

>