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

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?

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7