Re: [PATCH v3 06/13] libmultipath: Add delayed removal support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Repeated calls to `mpath_can_remove_head()` leak module references when removal work is already pending.
- [High] Race condition between `mpath_add_device()`, `mpath_can_remove_head()`, and `mpath_remove_head_work()` leads to use-after-free of mpath_head.
- [High] Missing cancellation of delayed removal work on `mpath_head` teardown results in Use-After-Free.
- [High] `mpath_head->drv_module` is never initialized by the subsystem API, leading to a silent failure to pin the driver module.
- [Medium] Disabling delayed removal via sysfs fails to cancel already pending removal work, leading to unexpected device removal.
- [Low] Incompatible pointer type passed to `kstrtouint()` causes compiler warnings.
- [Low] Integer overflow when calculating the delay for `mod_delayed_work()` results in severely shortened delays.
--

commit c00bb95222f826ebc3e8dbd55ff6884abf84858e
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add delayed removal support
    
Add support for delayed removal, same as exists for NVMe.
    
The purpose of this feature is to keep the multipath disk and cdev present
for intermittent periods of no available path.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 007aa34796569..78f88b0664c78 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -61,6 +61,8 @@ void mpath_add_device(struct mpath_device *mpath_device,
>  	mutex_lock(&mpath_head->lock);
>  	list_add_tail_rcu(&mpath_device->siblings, &mpath_head->dev_list);
>  	mutex_unlock(&mpath_head->lock);
> +	if (cancel_delayed_work(&mpath_head->remove_work))
> +		module_put(mpath_head->drv_module);
>  }

[Severity: High]
Does this code introduce a use-after-free race condition?

If the delayed removal timer is already executing, cancel_delayed_work()
returns false. 

mpath_add_device() will successfully add the device, but the
already-executing mpath_remove_head_work() unconditionally calls
mpath_head->mpdt->remove_head() without checking for newly added devices.

Could the newly added device access the destroyed mpath_head?

[ ... ]

> @@ -518,6 +530,39 @@ static void mpath_requeue_work(struct work_struct *work)
> +bool mpath_can_remove_head(struct mpath_head *mpath_head)
> +{
> +	bool remove = false;
> +
> +	mutex_lock(&mpath_head->lock);
> +	/*
> +	 * Ensure that no one could remove this module while the head
> +	 * remove work is pending.
> +	 */
> +	if (mpath_head_queue_if_no_path(mpath_head) &&
> +		try_module_get(mpath_head->drv_module)) {

[Severity: High]
Is mpath_head->drv_module initialized anywhere in the subsystem API?

If it remains uninitialized (e.g., NULL from zero-allocated structures),
try_module_get(NULL) will silently succeed. 

Could this lead to a failure to pin the driver module, potentially resulting
in a crash if the module is unloaded while the timer is pending?

> +
> +		mod_delayed_work(mpath_wq, &mpath_head->remove_work,
> +				mpath_head->delayed_removal_secs * HZ);

[Severity: High]
Does this unconditionally increment the module reference count even when the
timer is already pending?

If mpath_can_remove_head() is called multiple times, try_module_get()
will increment the reference count on each call. 

However, mod_delayed_work() merely modifies the existing timer. When the
timer eventually fires, mpath_remove_head_work() calls module_put() only
once, leaking the additional module references and preventing unloading.

[Severity: Low]
Does this multiplication overflow on 32-bit systems?

mpath_head->delayed_removal_secs is an unsigned int, and HZ is typically
an integer. 

Could their product overflow 32-bit arithmetic before being promoted to
unsigned long for mod_delayed_work(), leading to shortened delays?

> +	} else {
> +		remove = true;
> +	}
> +
> +	mutex_unlock(&mpath_head->lock);
> +	return remove;
> +}
> +EXPORT_SYMBOL_GPL(mpath_can_remove_head);
> +
> +static void mpath_remove_head_work(struct work_struct *work)
> +{
> +	struct mpath_head *mpath_head = container_of(to_delayed_work(work),
> +			struct mpath_head, remove_work);
> +	struct module *drv_module = mpath_head->drv_module;
> +
> +	mpath_head->mpdt->remove_head(mpath_head);
> +	module_put(drv_module);
> +}

[ ... ]

> @@ -562,6 +607,9 @@ int mpath_alloc_head_disk(struct mpath_head *mpath_head,
>  	mpath_head->disk->private_data = mpath_head;
>  	mpath_head->disk->fops = &mpath_ops;
>  
> +	INIT_DELAYED_WORK(&mpath_head->remove_work, mpath_remove_head_work);
> +	mpath_head->delayed_removal_secs = 0;
> +
>  	set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state);

[Severity: High]
Is the delayed removal work reliably canceled during final teardown?

During explicit device teardown (like a driver unbind), functions like
mpath_head_cleanup() are called to free resources. 

Since this patch doesn't add a cancel_delayed_work_sync() call to the
cleanup path, could an active timer fire after the mpath_head has been
freed, leading to a use-after-free?

[ ... ]

> @@ -605,6 +653,47 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
> +ssize_t mpath_delayed_removal_secs_store(struct mpath_head *mpath_head,
> +			const char *buf, size_t count)
> +{
> +	ssize_t ret;
> +	int sec;
> +
> +	ret = kstrtouint(buf, 0, &sec);

[Severity: Low]
Does passing an int pointer to kstrtouint() cause compiler warnings?

sec is declared as an int, but kstrtouint() strictly expects an unsigned
int *. 

Could this cause an incompatible pointer type warning and potential undefined
behavior on architectures with differing int/unsigned int layouts?

> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&mpath_head->lock);
> +	mpath_head->delayed_removal_secs = sec;
> +	if (sec)
> +		set_bit(MPATH_HEAD_QUEUE_IF_NO_PATH, &mpath_head->flags);
> +	else
> +		clear_bit(MPATH_HEAD_QUEUE_IF_NO_PATH, &mpath_head->flags);

[Severity: Medium]
Should the active timer be canceled when delayed removal is disabled?

When a user writes 0 to disable the feature, this code clears the flag but
misses a call to cancel_delayed_work(&mpath_head->remove_work). 

If the timer is already pending, will it continue to run and eventually
destroy the head, contrary to the user's configuration?

> +	mutex_unlock(&mpath_head->lock);

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