Re: [PATCH v4 05/28] libmultipath: Add support for mpath_device management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mpath_delete_device fails to clear the device from the current_path array before calling mpath_synchronize, guaranteeing an RCU Use-After-Free.
- [Low] Duplicate function declaration of `mpath_synchronize`.
--

--- Patch [5]: [PATCH v4 05/28] libmultipath: Add support for mpath_device management ---
commit 729c9713b67dacb1c3c3c943a84db42ae105702d
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add support for mpath_device management

Add support to add or remove a mpath_device as a path. This implements
functionality similar to NVMe equivalents for path management.

> diff --git a/include/linux/multipath.h b/include/linux/multipath.h
> index 57d13eb1450cd..6915c2a979642 100644
> --- a/include/linux/multipath.h
> +++ b/include/linux/multipath.h
[ ... ]
> @@ -89,6 +92,17 @@ static inline enum mpath_iopolicy_e mpath_read_iopolicy(
>  void mpath_synchronize(struct mpath_head *mpath_head);
>  int mpath_set_iopolicy(const char *str, enum mpath_iopolicy_e *iopolicy);
>  int mpath_get_iopolicy(char *buf, int iopolicy);
> +bool mpath_clear_current_path(struct mpath_device *mpath_device);
> +void mpath_synchronize(struct mpath_head *mpath_head);

[Severity: Low]
This isn't a bug, but should we remove this duplicate declaration of
mpath_synchronize() since it is already declared just a few lines above?

> +int mpath_add_device(struct mpath_device *mpath_device,
[ ... ]
> diff --git a/lib/multipath.c b/lib/multipath.c
> index 448d879abb354..20c633ad7e1df 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
[ ... ]
> @@ -50,6 +50,87 @@ void mpath_synchronize(struct mpath_head *mpath_head)
[ ... ]
> +bool mpath_delete_device(struct mpath_device *mpath_device)
> +{
> +	bool empty;
> +
> +	mutex_lock(&mpath_device->mpath_head->lock);
> +	list_del_rcu(&mpath_device->siblings);
> +	empty = list_empty(&mpath_device->mpath_head->dev_list);
> +	mutex_unlock(&mpath_device->mpath_head->lock);
> +
> +	mpath_synchronize(mpath_device->mpath_head);

[Severity: High]
Does this leave a dangling pointer in the current_path array that could
lead to a use-after-free?

Since the deleted device isn't cleared from mpath_head->current_path[],
new I/O submissions starting after mpath_synchronize() returns could still
read the device from current_path and use it after it's been freed.

If a caller tries to avoid this by calling mpath_clear_current_path()
before mpath_delete_device(), concurrent I/O could potentially re-select
the device from dev_list and place it back into current_path before the
deletion acquires the list lock.

Should mpath_clear_current_path() be called here, exactly between
list_del_rcu() and mpath_synchronize()?

> +
> +	return empty;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723093627.2327456-1-john.g.garry@oracle.com?part=5