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

On 27/07/2026 16:03, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

locking is added later, which is being ignored

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

that is done elsewhere

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

already commented on

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

I don't think so

>