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

[John Garry <john.garry@linux.dev>]

On 9/7/26 12:09, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> 
> New issues:
> - [High] Infinite loop in `mpath_round_robin_path()` if the current round-robin path is concurrently removed from the device list, causing an SRCU deadlock.
> - [High] `mpath_round_robin_path` fast-path incorrectly returns an unlinked path, breaking failover.
> - [Medium] Calling `list_is_singular()` on an RCU-protected list without holding the update lock causes a data race on `head->prev`.
> - [Medium] Path selection functions bypass the `.is_optimized` callback, violating the API contract and causing inconsistent path evaluation.
> - [Low] Missing bounds check on iopolicy index in `mpath_get_iopolicy`.
> 
> Pre-existing issues:
> - [Medium] `mpath_device_set_live()` silently swallows setup failures, leaving the driver unaware of the aborted initialization.
> --
> 
> commit 59f56cbf3b60a1ec3a1336f453b4cd772a005e7b
> Author: John Garry <john.garry@linux.dev>
> 
> libmultipath: Add path selection support
> 
> This commit adds path selection support to libmultipath, providing
> round-robin, NUMA, and queue-depth selection modes similar to NVMe. It also
> abstracts NVMe ANA into enum mpath_access_state to allow SCSI ALUA usage
> and introduces iopolicy management functions.
> 
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index 574868ce53381..3541357d07f8d 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
>> @@ -7,8 +7,245 @@
> 
> [ ... ]
> 
>> +int mpath_get_iopolicy(char *buf, int iopolicy)
>> +{
>> +	return sprintf(buf, "%s\n", mpath_iopolicy_names[iopolicy]);
>> +}
> 
> [Severity: Low]
> Does this code overflow mpath_iopolicy_names[] if a caller provides an
> uninitialized or out-of-bounds iopolicy value?

The caller should sanitize iopolicy

> 
> [ ... ]
> 
>> +static struct mpath_device *__mpath_find_path(struct mpath_head *mpath_head,
>> +					int node)
>> +{
>> +	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
>> +	struct mpath_device *found = NULL, *fallback = NULL, *mpath_device;
>> +
>> +	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;
> 
> [ ... ]
> 
>> +		switch(mpath_device->access_state) {
>> +		case MPATH_STATE_OPTIMIZED:
> 
> [Severity: Medium]
> Does this code bypass the driver's dynamic state evaluation by accessing
> access_state directly instead of invoking the provided
> mpath_path_is_optimized() callback wrapper?
> 
> If a driver like SCSI ALUA dynamically changes state, this will read
> stale data.

ALUA support will be added later

> 
>> +		    if (distance < found_distance) {
>> +			    found_distance = distance;
>> +			    found = mpath_device;
>> +		    }
>> +		    break;
> 
> [ ... ]
> 
>> +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: Medium]
> Calling list_is_singular() on an RCU-protected list without holding the
> update lock causes a data race on head->prev. In RCU-protected lists,
> only next pointers are safely updated for lockless readers.

locking will be added later

> 
> [Severity: High]
> Does this fast-path blindly return old without verifying if it is actually
> the element present in the singular list?

as above

> 
> If the previously cached old path was concurrently removed from dev_list
> and exactly one valid path remains, list_is_singular() evaluates to true,
> causing the function to incorrectly return the unlinked path instead of
> the remaining valid 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)) {
> 
> [Severity: High]
> Can this loop spin infinitely if old is concurrently removed from the
> RCU-protected list?

as above

> 
> The loop relies on reaching old again to terminate. If a concurrent writer
> unlinks old using list_del_rcu(), it is removed from the circular list but
> its next pointer remains intact. mpath_next_dev() follows old->next into
> the remaining list elements, wrapping around and circling the remaining
> elements indefinitely without ever evaluating mpath_device == old.
> 
> If all remaining paths are non-optimized or disabled, the loop never hits
> the break condition, spinning forever under the SRCU read lock and stalling
> any writer.
> 
>> +
>> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
>> +			continue;
>> +		if (mpath_device->access_state == MPATH_STATE_OPTIMIZED) {
> 
> [Severity: Medium]
> Similar to __mpath_find_path(), does this bypass the driver's dynamic state
> evaluation by accessing access_state directly rather than calling the
> mpath_path_is_optimized() callback wrapper?
> 
>> +			found = mpath_device;
>> +			goto out;
>> +		}
>> +		if (mpath_device->access_state == MPATH_STATE_NONOPTIMIZED)
>> +			found = mpath_device;
>> +	}
> 
> [ ... ]
> 
>> @@ -133,6 +371,19 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>>   		}
>>   		queue_work(mpath_wq, &mpath_head->partition_scan_work);
>>   	}
> 
> [Severity: Medium]
> This is a pre-existing issue, but does mpath_device_set_live() silently
> swallow setup failures earlier in this function?

Yes, this just minics what NVMe does

> 
> If device_add_disk() fails, for example due to memory exhaustion, the
> function clears the MPATH_HEAD_DISK_LIVE flag and returns early, masking
> the failure from the caller and leaving the driver unaware of the aborted
> initialization.
> 
>> +
>> +	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);
>>   }
>