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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing synchronization between path addition and delayed removal causes Use-After-Free.
- [High] Missing cancellation of `remove_work` during head teardown causes Use-After-Free.
- [High] Disabling delayed removal via sysfs leaves queued I/Os stuck indefinitely.
- [Medium] Module reference leak in `mpath_can_remove_head()` when delayed work is already pending.
- [Medium] 32-bit integer overflow in delayed removal seconds calculation.
--

--- Patch [6]: [PATCH v5 06/27] libmultipath: Add delayed removal support ---
commit c3a14a8d55791e59aaf6def4186888f9333f0985
Author: John Garry <[email protected]>

libmultipath: Add delayed removal support

This patch adds support for delayed removal of multipath devices, similar to
NVMe. It introduces a configurable delayed removal window to retain the disk
and cdev during transient path loss.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index f70049b6cede9..ede06e5e901c8 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -65,6 +65,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);
> +

[Severity: High]
If mpath_remove_head_work() is already executing when we call
cancel_delayed_work(), the cancellation will fail and return false. By
dropping the lock before this check, is it possible for mpath_remove_head_work()
to unconditionally tear down the mpath_head without realizing a new path was
just added?

This might leave the newly added path with dangling pointers to the freed
multipath head. Should we hold the lock or verify the execution state of the
work item?

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

[Severity: Medium]
If the delayed work is already pending from a previous path failure,
mod_delayed_work() will update the timer and return true, but it will not
queue a new execution of the work item.

Since try_module_get() unconditionally increments the reference count before
this, will subsequent calls to mpath_can_remove_head() leak module references
since mpath_remove_head_work() only executes once?

> +		unsigned long delay = mpath_head->delayed_removal_secs * HZ;

[Severity: Medium]
Could this multiplication overflow on 32-bit systems? Both
mpath_head->delayed_removal_secs (unsigned int) and HZ are 32-bit types, so the
multiplication is evaluated in 32-bit arithmetic before being promoted to
unsigned long.

If a user writes a large value to the sysfs attribute, could this result in an
unintended short delay or immediate removal?

> +
> +		mod_delayed_work(mpath_wq, &mpath_head->remove_work, delay);
> +	} else {
> +		remove = true;
> +	}
> +
> +	mutex_unlock(&mpath_head->lock);
> +	return remove;
> +}
> +EXPORT_SYMBOL_GPL(mpath_can_remove_head);

[ ... ]

> @@ -525,6 +571,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);

[Severity: High]
Since this work is initialized here, do we need to ensure it is cancelled
during head teardown?

Looking at mpath_put_disk(), it flushes requeue_work and partition_scan_work
but does not appear to call cancel_delayed_work_sync() for remove_work. Could
this leave the delayed work active in the workqueue after mpath_head is freed?

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

[ ... ]

> +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: High]
When delayed removal is disabled by writing 0, the MPATH_HEAD_QUEUE_IF_NO_PATH
flag is cleared. However, does this leave bios already queued in requeue_list
stuck indefinitely?

Should we call mpath_schedule_requeue_work() here to ensure previously queued
I/Os are flushed and failed?

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

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