Re: [PATCH v6 07/27] libmultipath: Add sysfs helpers

On 29/07/2026 13:28, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The sysfs store helper `mpath_iopolicy_store` updates the `iopolicy` locklessly and without invalidating cached paths, creating a TOCTOU race on concurrent writes and leaving the system operating on stale paths.
> - [Medium] Inconsistent evaluation of `iopolicy` in `__mpath_find_path` due to reading the state inside the device loop.
> --
> 
> --- Patch [7]: [PATCH v6 07/27] libmultipath: Add sysfs helpers ---
> commit ceaf280cc3938326ee64a5bab20fde5b9a603a83
> Author: John Garry <[email protected]>
> 
> libmultipath: Add sysfs helpers
> 
> This commit adds helper functions for driver sysfs code to get/set the
> iopolicy, show device paths per NUMA node, and manage multipath attribute
> groups.
> 
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index fada92bf9754c..58eb9be5bebbe 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
> 
> [ ... ]
> 
>> @@ -620,6 +620,100 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
> 
> [ ... ]
> 
>> +static void mpath_iopolicy_update(enum mpath_iopolicy_e *iopolicy,
>> +		int new)
>> +{
>> +	int old = READ_ONCE(*iopolicy);
>> +
>> +	if (old == new)
>> +		return;
>> +
>> +	WRITE_ONCE(*iopolicy, new);
> 
> [Severity: High]
> Since kernfs does not serialize store callbacks, can this lockless
> read-compare-update sequence lead to TOCTOU races on concurrent writes?
> 
> If multiple threads execute this simultaneously, could they both read the
> same old state, overwrite it, and emit inconsistent logs?

Don't call it concurrently then

> 
> Additionally, does this policy update need a way to invalidate the cached
> paths? Because this helper only updates the policy and has no reference to
> mpath_head, it doesn't invalidate mpath_head->current_path.
> 
> Will the new policy continue using the stale path cached by the old policy?
> 
>> +
>> +	pr_info("iopolicy changed from %s to %s\n",
>> +		mpath_iopolicy_names[old],
>> +		mpath_iopolicy_names[new]);
>> +}
> 
> [Severity: Medium]
> Does introducing this concurrent sysfs update expose a TOCTOU race in
> __mpath_find_path()?
> 
> If __mpath_find_path() iterates over devices and reads the IO policy on each
> iteration via mpath_read_iopolicy(mpath_head):
> 
> 	if (mpath_device->numa_node != NUMA_NO_NODE &&
> 	    (mpath_read_iopolicy(mpath_head) ==
> 		MPATH_IOPOLICY_NUMA))
> 		distance = node_distance(node,
> 				mpath_device->numa_node);
> 	else
> 		distance = LOCAL_DISTANCE;
> 
> Could a concurrent change here transition the policy mid-loop, causing the
> function to mix incompatible node_distance metrics with LOCAL_DISTANCE and
> potentially corrupting the minimum-finding logic?
> 
> Should the policy be read exactly once before the loop?
> 

So we choose not the best/appropiate path once - it won't make a difference