Re: [PATCH v6 06/27] libmultipath: Add delayed removal support

On 29/07/2026 13:08, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] mpath_remove_head_work() unconditionally removes the multipath head without verifying if paths have been concurrently added, leading to destruction of an active device.
> - [High] Missing cancellation of remove_work during multipath head teardown leads to a use-after-free when the delayed work expires.
> - [High] mpath_head->drv_module is never initialized, leading to silent failures or crashes on module unload.
> - [High] Disabling delayed removal via sysfs leaves queued I/Os stuck indefinitely.
> - [Medium] Module reference count leak when mpath_can_remove_head() is called multiple times.
> - [Medium] Large delayed removal values cause immediate removal instead of a long delay.
> --
> 
> --- Patch [6]: [PATCH v6 06/27] libmultipath: Add delayed removal support ---
> commit 87d6b13fc148098f06b11013c5f12523eb0d477f
> Author: John Garry <[email protected]>
> 
> libmultipath: Add delayed removal support
> 
> Add support for delayed removal, same as exists for NVMe, 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 6b8f6e846987a..fada92bf9754c 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
>> @@ -66,6 +66,9 @@ int mpath_add_device(struct mpath_device *mpath_device, struct gendisk *disk,
>>   	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);
>> +
>>   	return 0;
>>   }
> 
> [Severity: High]
> Can this race with the removal work?
> 
> If the delayed work is already executing, cancel_delayed_work() returns
> false, but mpath_add_device() still returns success. Meanwhile, could
> mpath_remove_head_work() execute without taking mpath_head->lock or
> verifying if the device list is empty, unintentionally destroying the
> multipath head while the driver believes a new path was successfully
> added?
> 

the removal callback should take care of this


> [ ... ]
> 
>> @@ -481,6 +493,39 @@ static void mpath_requeue_work(struct work_struct *work)
>>   	}
>>   }
>>   
>> +bool mpath_can_remove_head(struct mpath_head *mpath_head)
>> +{
>> +	unsigned long delay;
>> +	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) &&
>> +	    !check_mul_overflow(mpath_head->delayed_removal_secs, HZ, &delay) &&
> 
> [Severity: Medium]
> What happens if a user configures a very large delayed removal value?
> 
> If check_mul_overflow() detects an overflow (e.g., on 32-bit systems where
> a large sysfs input overflows unsigned long), it fails the condition and
> falls through to the else block, setting remove = true. Does this cause an
> immediate teardown instead of clamping the delay to a maximum value?

yes, so don't use a silly big value

> 
>> +	    try_module_get(mpath_head->drv_module)) {
>> +		mod_delayed_work(mpath_wq, &mpath_head->remove_work, delay);
> 
> [Severity: Medium]
> Could this lead to a module reference leak?
> 
> If mod_delayed_work() returns true (meaning the work was already pending
> and its timer was just modified), it doesn't queue a new work item.
> Since try_module_get() was called unconditionally beforehand, and the
> work callback only drops one module reference, do subsequent calls to
> mpath_can_remove_head() leak a module reference permanently?

it should not happen - that is the job of the caller to ensure

> 
>> +	} else {
>> +		remove = true;
>> +	}
>> +
>> +	mutex_unlock(&mpath_head->lock);
>> +	return remove;
>> +}
> 
> [ ... ]
> 
>> @@ -529,6 +574,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;
>> +
> 
> [Severity: High]
> Is drv_module properly initialized anywhere?

yes

> 
> It seems drv_module is added to struct mpath_head but never assigned in
> mpath_alloc_head_disk() or elsewhere in this patch. If it defaults to
> NULL, try_module_get(NULL) succeeds but pins no module. Could this allow
> the driver to unload while the delayed work is still pending, leading to
> a kernel panic when the timer fires?
> 
> [Severity: High]
> Is the remove_work guaranteed to be cancelled during object teardown?

it must be assigned

> 
> The standard teardown sequence calls mpath_put_disk() and
> mpath_head_uninit(). If cancel_delayed_work_sync(&mpath_head->remove_work)
> is omitted during teardown, could the timer remain active on the
> workqueue and execute on the freed mpath_head structure later?
> 
>>   	set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state);
>>   
>>   	return 0;
> 
> [ ... ]
> 
>> @@ -572,6 +620,47 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>>   }
>>   EXPORT_SYMBOL_GPL(mpath_device_set_live);
>>   
>> +ssize_t mpath_delayed_removal_secs_show(struct mpath_head *mpath_head,
>> +					char *buf)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&mpath_head->lock);
>> +	ret = sysfs_emit(buf, "%u\n", mpath_head->delayed_removal_secs);
>> +	mutex_unlock(&mpath_head->lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mpath_delayed_removal_secs_show);
>> +
>> +ssize_t mpath_delayed_removal_secs_store(struct mpath_head *mpath_head,
>> +			const char *buf, size_t count)
>> +{
>> +	unsigned int sec;
>> +	ssize_t ret;
>> +
>> +	ret = kstrtouint(buf, 0, &sec);
>> +	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);
>> +	mutex_unlock(&mpath_head->lock);
>> +
>> +	/*
>> +	 * Ensure that update to MPATH_HEAD_QUEUE_IF_NO_PATH is seen
>> +	 * by its reader.
>> +	 */
>> +	mpath_synchronize(mpath_head);
>> +
>> +	return count;
>> +}
> 
> [Severity: High]
> If a user disables delayed removal by setting delayed_removal_secs to 0 via
> sysfs while paths are missing, how do pending I/Os get handled?
> 
> We clear the flag and call mpath_synchronize(), but shouldn't we also
> call mpath_schedule_requeue_work() here to flush and fail the I/Os that
> are already parked in mpath_head->requeue_list?

I don't see how this is related to requeue work

>