Re: [PATCH v6 05/27] libmultipath: Add support for mpath_device management

On 29/07/2026 12:57, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] RCU Use-After-Free Vulnerability in mpath_delete_device()
> - [High] NULL Pointer Dereference in sysfs link management
> - [High] Race condition causing sysfs link leak and dangling kobject references
> - [Medium] TOCTOU race in mpath_remove_sysfs_link
> --
> 
> --- Patch [5]: [PATCH v6 05/27] libmultipath: Add support for mpath_device management ---
> commit a32651ca51a97ca7e06ffd62321d0190b9e9cfa0
> Author: John Garry <[email protected]>
> 
> libmultipath: Add support for mpath_device management
>      
> Add support to add or remove a mpath_device as a path.
>      
> NVMe has almost like-for-like equivalents here:
> - nvme_mpath_clear_current_path() -> mpath_clear_current_path()
> - nvme_mpath_add_sysfs_link() -> mpath_add_sysfs_link()
> - nvme_mpath_remove_sysfs_link() -> mpath_remove_sysfs_link()
> - nvme_mpath_revalidate_paths() -> mpath_revalidate_paths()
> 
> The functionality in mpath_clear_paths() and mpath_synchronize() have the
> same pattern which is frequently used in the NVMe code.
> 
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index 93b82eea7d38b..6b8f6e846987a 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
> 
> [ ... ]
> 
>> @@ -51,6 +51,87 @@ void mpath_synchronize(struct mpath_head *mpath_head)
>>   }
>>   EXPORT_SYMBOL_GPL(mpath_synchronize);
>>   
> 
> [ ... ]
> 
>> +bool mpath_delete_device(struct mpath_device *mpath_device)
>> +{
>> +	bool empty;
>> +
>> +	mutex_lock(&mpath_device->mpath_head->lock);
>> +	list_del_rcu(&mpath_device->siblings);
>> +	empty = list_empty(&mpath_device->mpath_head->dev_list);
>> +	mutex_unlock(&mpath_device->mpath_head->lock);
>> +
>> +	mpath_synchronize(mpath_device->mpath_head);
> 
> [Severity: High]
> Does this sequence safely handle RCU readers accessing current_path?
> 
> Because mpath_delete_device() removes the device from dev_list and
> immediately calls mpath_synchronize() without clearing it from
> mpath_head->current_path, the RCU-protected current_path array may still
> hold a pointer to the device. Lockless readers can then dereference the
> freed device.

it is removed in sd_mpath_remove() -> mpath_clear_current_path()

> 
> If callers attempt to clear current_path before calling
> mpath_delete_device(), couldn't the pointer be repopulated from dev_list
> by concurrent readers during the race window? If cleared after, the
> synchronization has already passed, providing no protection against
> use-after-free.
> 
>> +
>> +	return empty;
>> +}
>> +EXPORT_SYMBOL_GPL(mpath_delete_device);
> 
> [ ... ]
> 
>> @@ -489,6 +572,77 @@ void mpath_device_set_live(struct mpath_device *mpath_device)
>>   }
>>   EXPORT_SYMBOL_GPL(mpath_device_set_live);
>>   
>> +void mpath_add_sysfs_link(struct mpath_head *mpath_head)
>> +{
>> +	struct device *target;
>> +	struct device *source;
>> +	int rc, srcu_idx;
>> +	struct kobject *mpath_gd_kobj;
>> +	struct mpath_device *mpath_device;
>> +
>> +	/*
>> +	 * Ensure head disk node is already added otherwise we may get invalid
>> +	 * kobj for head disk node
>> +	 */
>> +	if (!test_bit(GD_ADDED, &mpath_head->disk->state))
>> +		return;
> 
> [Severity: High]
> Is it possible for mpath_head->disk to be NULL here?
> 
> When a multi-path head disk is removed, mpath_put_disk() is called, which
> sets mpath_head->disk = NULL. If a path subsequently goes live,
> mpath_add_sysfs_link() appears to dereference it without checking for NULL.
> 
> Could this cause a deterministic oops when path teardown outlives the head,
> such as during an abrupt controller removal?

if a new device is going live, then mpath_head->disk has to be present 
as it has a reference to it

> 
>> +
>> +	mpath_gd_kobj = &disk_to_dev(mpath_head->disk)->kobj;
>> +	srcu_idx = srcu_read_lock(&mpath_head->srcu);
>> +
>> +	list_for_each_entry_srcu(mpath_device, &mpath_head->dev_list, siblings,
>> +				 srcu_read_lock_held(&mpath_head->srcu)) {
>> +		if (!test_bit(GD_ADDED, &mpath_device->disk->state))
>> +			continue;
>> +
>> +		if (test_and_set_bit(MPATH_DEVICE_SYSFS_ATTR_LINK,
>> +					&mpath_device->flags))
>> +			continue;
>> +
>> +		target = disk_to_dev(mpath_device->disk);
>> +		source = disk_to_dev(mpath_head->disk);
>> +		/*
>> +		 * Create sysfs link from head gendisk kobject @kobj to the
>> +		 * ns path gendisk kobject @target->kobj.
>> +		 */
> 
> [Severity: High]
> Could this re-create a sysfs link for a device that is in the middle of
> being deleted?
> 
> During path removal, mpath_remove_sysfs_link() clears the
> MPATH_DEVICE_SYSFS_ATTR_LINK bit before mpath_delete_device() removes
> the device from the RCU dev_list.
> 
> If a different path concurrently goes live, mpath_add_sysfs_link() iterates
> over dev_list, observes the cleared bit on the dying device, and might
> mistakenly re-create the sysfs link. When the dying device is finally
> removed and freed, the re-created sysfs link is permanently leaked and
> points to a freed block device kobject.

we call mpath_delete_device() before mpath_remove_sysfs_link() in 
sd_mpath_remove(), so this should not happen

> 
>> +		rc = sysfs_add_link_to_group(mpath_gd_kobj, "multipath",
>> +				&target->kobj, dev_name(target));
> 
> [ ... ]
> 
>> +void mpath_remove_sysfs_link(struct mpath_device *mpath_device)
>> +{
>> +	struct device *target;
>> +	struct kobject *mpath_gd_kobj;
>> +	struct mpath_head *mpath_head = mpath_device->mpath_head;
>> +
>> +	if (!test_bit(MPATH_DEVICE_SYSFS_ATTR_LINK, &mpath_device->flags))
>> +		return;
>> +
>> +	target = disk_to_dev(mpath_device->disk);
>> +	mpath_gd_kobj = &disk_to_dev(mpath_head->disk)->kobj;
> 
> [Severity: High]
> Does mpath_head->disk need to be checked for NULL before dereferencing it
> here?
> 
> Similar to mpath_add_sysfs_link(), if underlying paths are detached after
> mpath_put_disk() sets mpath_head->disk = NULL, this will cause a NULL
> pointer dereference during teardown.

the paths cannot be detached after mpath_put_disk()

> 
>> +
>> +	sysfs_remove_link_from_group(mpath_gd_kobj, "multipath",
>> +			dev_name(target));
>> +
>> +	clear_bit(MPATH_DEVICE_SYSFS_ATTR_LINK, &mpath_device->flags);
> 
> [Severity: Medium]
> Is there a race condition between test_bit() at the start of this function
> and clear_bit() here?
> 
> A concurrent execution for the same device could pass the test_bit() check
> and issue duplicate removals to the sysfs core. 

We don't call for the same device concurrently

> Should this diverging
> non-atomic pattern be replaced with test_and_clear_bit() to match the safe
> pattern used in mpath_add_sysfs_link()?
> 
>> +}
>