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

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?

[ ... ]

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

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

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

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?

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?

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