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

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.

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?

> +
> +	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.

> +		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.

> +
> +	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. Should this diverging
non-atomic pattern be replaced with test_and_clear_bit() to match the safe
pattern used in mpath_add_sysfs_link()?

> +}

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