Re: [PATCH v9 07/29] libmultipath: Add delayed removal support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] libmultipath: race between mpath_add_device() and
  mpath_can_remove_head()
- [High] libmultipath: UAF from unsynchronized remove_work on head
  teardown
- [Medium] libmultipath: drv_module ref leak in mpath_can_remove_head()
- [Medium] libmultipath: missing requeue kick in
  mpath_delayed_removal_secs_store()
- [Medium] libmultipath: remove_work initialized in
  mpath_alloc_head_disk() instead of mpath_head_init()
- [Low] libmultipath: unchecked HZ overflow in
  mpath_delayed_removal_secs_store()
--

commit ac55fad5ec89b5f2112b89a2a217ff2d12ec50d8
Author: John Garry <john.garry@linux.dev>

libmultipath: Add delayed removal support

This commit adds delayed head removal support to libmultipath, keeping the
multipath disk present and requeuing I/O for a configurable duration when all
paths are lost.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index fd78218244472..cdf8fbc5105b3 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -67,6 +67,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;
>  }
>  EXPORT_SYMBOL_GPL(mpath_add_device);

[ ... ]

> @@ -482,6 +494,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) &&
> +	    try_module_get(mpath_head->drv_module)) {
> +		mod_delayed_work(mpath_wq, &mpath_head->remove_work, delay);

[Severity: Medium]
Does this leak a reference on mpath_head->drv_module if
mpath_head->remove_work is already pending?

try_module_get() increments the module refcount before mod_delayed_work() is
called.  When remove_work is already pending, mod_delayed_work() updates the
existing timer in place and returns true, but neither mpath_remove_head_work()
nor cancel_delayed_work() in mpath_add_device() will run more than once to
drop the extra reference, preventing the driver module from unloading.

> +	} else {
> +		remove = true;
> +	}
> +
> +	mutex_unlock(&mpath_head->lock);
> +	return remove;
> +}
> +EXPORT_SYMBOL_GPL(mpath_can_remove_head);

[Severity: High]
Can a race between mpath_add_device() and mpath_can_remove_head() remove an
active mpath_head while paths are present on dev_list, or cancel remove_work
after dev_list becomes empty?

mpath_delete_device() drops mpath_head->lock before calling
mpath_synchronize() and before the caller invokes mpath_can_remove_head(),
and neither mpath_can_remove_head() nor mpath_remove_head_work() rechecks
list_empty(&mpath_head->dev_list) under mpath_head->lock:

CPU0                                  CPU1
mpath_delete_device()
  mutex_lock(&mpath_head->lock)
  list_del_rcu(&dev1->siblings)
  last_path = true
  mutex_unlock(&mpath_head->lock)
                                      mpath_add_device()
                                        mutex_lock(&mpath_head->lock)
                                        list_add_tail_rcu(&dev2->siblings,
                                                          &mpath_head->dev_list)
                                        mutex_unlock(&mpath_head->lock)
                                        cancel_delayed_work(&remove_work)
  mpath_synchronize()
mpath_can_remove_head()
  mutex_lock(&mpath_head->lock)
  remove = true (or queues remove_work)
  mutex_unlock(&mpath_head->lock)
  --> removes head with dev2 on dev_list

Conversely, because mpath_add_device() calls cancel_delayed_work() after
releasing mpath_head->lock, if dev2 is deleted on another CPU and
mpath_can_remove_head() queues remove_work before CPU1 runs
cancel_delayed_work(), CPU1 cancels the newly armed remove_work even though
dev_list is now empty.

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

[Severity: High]
Can mpath_remove_head_work() cause a use-after-free on mpath_head during head
teardown or path flapping?

Unlike requeue_work and partition_scan_work, neither mpath_put_disk() nor
mpath_head_uninit() cancels or flushes mpath_head->remove_work, and
mpath_add_device() uses non-synchronous cancel_delayed_work():

CPU0 (mpath_wq)                       CPU1
mpath_remove_head_work()
  [work dequeued; PENDING cleared]
                                      mpath_add_device()
                                        cancel_delayed_work(&remove_work)
                                          --> returns false
                                      mpath_delete_device()
                                      mpath_can_remove_head()
                                        --> returns true (or re-arms timer)
                                      remove_head() frees mpath_head
  mpath_head->mpdt->remove_head()  <-- use-after-free

Similarly, if remove_work is armed and a subsequent path probe fails and tears
down the head before reaching mpath_add_device(), mpath_head can be freed
while remove_work is still pending in the timer wheel.

>  void mpath_remove_disk(struct mpath_head *mpath_head)
>  {
>  	if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
> @@ -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;
> +
>  	set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state);
>  
>  	return 0;

[Severity: Medium]
Should remove_work and delayed_removal_secs be initialized in
mpath_head_init() instead of mpath_alloc_head_disk()?

If mpath_alloc_head_disk() is called again on an existing mpath_head after
mpath_put_disk(), resetting delayed_removal_secs to 0 here without clearing
MPATH_HEAD_QUEUE_IF_NO_PATH leaves the flag out of sync with
delayed_removal_secs.  mpath_available_path() then continues returning true
when no paths exist, causing mpath_bdev_submit_bio() to requeue I/O
indefinitely even though delayed_removal_secs is 0.  Initializing remove_work
in mpath_head_init() alongside requeue_work and partition_scan_work also
ensures remove_work is initialized before mpath_add_device() can touch it.

[ ... ]

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

[Severity: Low]
On 32-bit architectures, can storing a large sec value set
MPATH_HEAD_QUEUE_IF_NO_PATH while mpath_can_remove_head() immediately removes
the head?

mpath_delayed_removal_secs_store() accepts any unsigned int and sets
MPATH_HEAD_QUEUE_IF_NO_PATH without checking whether sec * HZ overflows
unsigned long.  When sec > ULONG_MAX / HZ on 32-bit systems, I/O is requeued
on path loss, but check_mul_overflow() in mpath_can_remove_head() fails and
falls back to immediate head removal with zero delay.

> +
> +	/*
> +	 * Ensure that update to MPATH_HEAD_QUEUE_IF_NO_PATH is seen
> +	 * by its reader.
> +	 */
> +	mpath_synchronize(mpath_head);
> +
> +	return count;
> +}

[Severity: Medium]
When delayed_removal_secs is set to 0, does this leave previously queued BIOs
stalled on mpath_head->requeue_list?

Clearing MPATH_HEAD_QUEUE_IF_NO_PATH stops new BIOs from being queued when no
paths are available, but without calling
mpath_schedule_requeue_work(mpath_head) after mpath_synchronize(), BIOs that
were already added to requeue_list while MPATH_HEAD_QUEUE_IF_NO_PATH was set
are not kicked to fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=7