Re: [PATCH v3 02/13] libmultipath: Add basic gendisk support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `mpath_alloc_head_disk()` leaves `ERR_PTR` in `mpath_head->disk` on failure, causing a kernel panic during cleanup or state updates.
- [High] A TOCTOU race between `mpath_device_set_live()` and `mpath_remove_disk()` permanently leaks the gendisk.
- [High] Synchronous wait in `mpath_head_uninit()` allows unprivileged openers to block device teardown indefinitely.
--

commit bba05c007c446cdb19dbac857c9ce717ca5f2a08
Author: John Garry <john.g.garry@oracle.com>
Subject: libmultipath: Add basic gendisk support

This patch adds support to allocate and free a multipath gendisk. It also
introduces struct mpath_head_template as a method for drivers to provide
custom multipath functionality.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 009d4bb875c6f..79be84d3d4f75 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -44,12 +44,111 @@ void mpath_head_uninit(struct mpath_head *mpath_head)
>  }
>  EXPORT_SYMBOL_GPL(mpath_head_uninit);
>  
> +static int mpath_bdev_open(struct gendisk *disk, blk_mode_t mode)
> +{
> +	struct mpath_head *mpath_head = disk->private_data;
> +
> +	return mpath_get_head(mpath_head);
> +}

[Severity: High]
Does this increment the refcount on behalf of userspace openers?

If unprivileged users open the block device, this calls mpath_get_head()
which increments mpath_head->refcount. If the admin later removes the device,
the driver calls mpath_head_uninit() which synchronously waits for the
refcount to hit zero. Since the user can hold the file descriptor open,
couldn't this allow unprivileged openers to permanently hang the kernel
thread handling device removal?

[ ... ]

> +void mpath_remove_disk(struct mpath_head *mpath_head)
> +{
> +	if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
> +		struct gendisk *disk = mpath_head->disk;
> +
> +		del_gendisk(disk);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(mpath_remove_disk);

[Severity: High]
Is there a race here with mpath_device_set_live() that could permanently
leak the gendisk?

If Thread A calls mpath_device_set_live() and sets MPATH_HEAD_DISK_LIVE
locklessly via test_and_set_bit(), and Thread B concurrently calls
mpath_remove_disk(), Thread B will clear the flag and call del_gendisk().

Because device_add_disk() in Thread A hasn't completed and set GD_ADDED
yet, del_gendisk() observes the disk is not live and returns early without
doing anything. 

Thread A then completes device_add_disk() making the disk live, but the
flag is now 0. Any subsequent mpath_remove_disk() will do nothing, leaking
the active block device.

> +void mpath_put_disk(struct mpath_head *mpath_head)
> +{
> +	if (!mpath_head->disk)
> +		return;
> +
> +	/* make sure all pending bios are cleaned up */
> +	flush_work(&mpath_head->partition_scan_work);
> +	put_disk(mpath_head->disk);
> +}
> +EXPORT_SYMBOL_GPL(mpath_put_disk);
> +
> +int mpath_alloc_head_disk(struct mpath_head *mpath_head,
> +			struct queue_limits *lim, int numa_node)
> +{
> +	if (!mpath_head->disk_groups || !mpath_head->parent)
> +		return -EINVAL;
> +
> +	mpath_head->disk = blk_alloc_disk(lim, numa_node);
> +	if (IS_ERR(mpath_head->disk))
> +		return PTR_ERR(mpath_head->disk);

[Severity: High]
If blk_alloc_disk() fails, does this leave an ERR_PTR in mpath_head->disk?

When the driver cleans up on error and calls mpath_put_disk(), the
if (!mpath_head->disk) check will pass because the ERR_PTR is not NULL.
Will this result in a kernel panic when put_disk() attempts to dereference
the invalid pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703102918.3723667-1-john.g.garry@oracle.com?part=2