Re: [PATCH v4 06/28] libmultipath: Add delayed removal support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Concurrent path addition and delayed removal execution causes a use-after-free by destroying an active multipath head.
- [High] Missing synchronous cancellation of `remove_work` during `mpath_head` teardown leaks the delayed work.
- [Medium] Module reference count leak in `mpath_can_remove_head()` when called multiple times.
- [Medium] Passing a signed `int *` to `kstrtouint()` causes sign-extension and massively inflated timeouts.
--

--- Patch [6]: [PATCH v4 06/28] libmultipath: Add delayed removal support ---
commit e89d624119c3a1b6148c925347d05a0886a18e1d
Author: John Garry <john.garry@linux.dev>

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. A removal work callback is
supplied by the driver.

> diff --git a/include/linux/multipath.h b/include/linux/multipath.h
> index 6915c2a979642..44e26c16b3dc4 100644
> --- a/include/linux/multipath.h
> +++ b/include/linux/multipath.h

[ ... ]

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 20c633ad7e1df..1fa4bd6677051 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -65,6 +65,9 @@ int mpath_add_device(struct mpath_device *mpath_device,
>  	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]
Could this lead to a use-after-free if the delayed work is already executing?

When mpath_add_device() calls cancel_delayed_work():

lib/multipath.c:mpath_add_device() {
    ...
    if (cancel_delayed_work(&mpath_head->remove_work))
        module_put(mpath_head->drv_module);
    return 0;
}

If the timer has already expired and mpath_remove_head_work() is actively
running, cancel_delayed_work() returns false and this function proceeds to
add the device and return success. 

Concurrently, mpath_remove_head_work() executes mpdt->remove_head(mpath_head)
without taking mpath_head->lock, which destroys the multipath head while we
are adding a new path to it.

>  	return 0;
>  }

[ ... ]

> @@ -480,6 +493,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)) {
> +
> +		mod_delayed_work(mpath_wq, &mpath_head->remove_work,
> +				mpath_head->delayed_removal_secs * HZ);

[Severity: Medium]
Is there a module reference leak if this is called multiple times while the
timer is already pending?

If mpath_can_remove_head() is called multiple times in succession,
try_module_get() acquires a new reference each time. However, if the work
is already queued, mod_delayed_work() returns true but does not queue a
duplicate work item.

When the timer expires, mpath_remove_head_work() executes only once and calls
module_put() only once, leaking the additional references acquired during
the subsequent calls.

> +	} else {
> +		remove = true;
> +	}

[ ... ]

> @@ -524,6 +570,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]
Should there be a matching cancellation of remove_work during the teardown
path?

The delayed work is initialized here, but mpath_put_disk() only flushes
requeue_work and partition_scan_work before putting the disk. If the
multipath device is torn down while the delayed removal timeout is still
pending, does this leak the active timer and result in a use-after-free
when mpath_remove_head_work() eventually executes on the freed memory?

>  	set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state);
>  
>  	return 0;

[ ... ]

> @@ -567,6 +616,47 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>  }
>  EXPORT_SYMBOL_GPL(mpath_device_set_live);

[ ... ]

> +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: Medium]
Does passing a signed integer to kstrtouint() cause sign-extension issues
for large values?

The variable sec is declared as a signed int, but kstrtouint() expects an
unsigned int pointer. If a user writes a value greater than or equal to
2147483648, kstrtouint() will parse it correctly, but it will be stored as
a negative value in the signed int.

When sec is later assigned to mpath_head->delayed_removal_secs (which is
an unsigned long), it will undergo sign extension on 64-bit platforms,
resulting in a massive positive timeout value.

> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&mpath_head->lock);
> +	mpath_head->delayed_removal_secs = sec;

[ ... ]

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