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

On 27/07/2026 16:00, [email protected] wrote:
> 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?

No, check sd_mpath_remove_head() which checks for new paths added

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

How is this even related or responsible bios already queued in requeue_list?

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