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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 11:45, sashiko-bot@kernel.org wrote:
> 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?

Ensuring that new devices are not added while the head is being removed 
is the job of the driver.

In the NVMe mainline case, in the nvme_remove_head_work() will 
atomically ensure that no new devices appear and then decide to delete 
the head list entry reference (so no new devices can reference the same 
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?

The driver needs to ensure that this is set.

It is hard to add a check, as if the driver is builtin then THIS_MODULE 
is NULL. Maybe that can be improved in the core code.

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

It should not be called multiple times. That is the job of the driver.

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

I don't think that this is a problem. The delayed removal secs should 
not be so big as to overflow.

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

Hmmm ... I don't get it. When the delayed work is active, then only the 
delayed timer callback will remove the mpath_head

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

ok, I'll make sec as 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?

unsetting MPATH_HEAD_QUEUE_IF_NO_PATH has that effect

> 
> 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);
>