Re: [PATCH v4 02/28] libmultipath: Add basic gendisk support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mpath_alloc_head_disk() leaves an ERR_PTR in mpath_head->disk on allocation failure, causing a kernel panic during cleanup.
- [High] mpath_head_init() fails to initialize multiple work items (remove_work, requeue_work), leading to kernel warnings and crashes.
- [High] The mpath_ops block_device_operations struct lacks a .submit_bio callback, causing device_add_disk() to fail unconditionally.
- [High] mpath_head_uninit() uses a synchronous wait for userspace file descriptors to close, deadlocking kernel unplug paths.
- [Medium] mpath_put_disk() claims to clean up pending bios but fails to schedule or flush requeue_work, leaking bios.
--

--- Patch [2]: [PATCH v4 02/28] libmultipath: Add basic gendisk support ---
commit 8b695b95ebd074db05fc7130622e9ffd48786c4d
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add basic gendisk support

This patch adds support to allocate and free a multipath gendisk. It
introduces the mpath_head_template to provide custom multipath functionality.
NVMe-like functions are added to manage multipath structures.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 9cc398d266adf..916d01a285284 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -35,6 +35,102 @@ void mpath_head_uninit(struct mpath_head *mpath_head)
>  }
>  EXPORT_SYMBOL_GPL(mpath_head_uninit);

[Severity: High]
Does mpath_head_uninit() use a synchronous wait for userspace file
descriptors to close?

It appears this function contains:
wait_var_event(&mpath_head->refcount,
    refcount_read(&mpath_head->refcount) == 1);

If userspace holds the block device open (for example via udev or
monitoring tools), this regression might block the kernel thread executing
the device removal forever in TASK_UNINTERRUPTIBLE, leading to a deadlock.

[ ... ]

> +const struct block_device_operations mpath_ops = {
> +	.owner          = THIS_MODULE,
> +	.open		= mpath_bdev_open,
> +	.release	= mpath_bdev_release,
> +};

[Severity: High]
Is it intentional to omit the .submit_bio callback here?

When mpath_device_set_live() is called, it triggers device_add_disk(),
which uses the block layer's __add_disk() function. This function verifies
that bio-based disks have a valid ->submit_bio pointer.

Since it is missing, this regression might unconditionally fail with
-EINVAL, preventing the multipath disk from being added to the system.

[ ... ]

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

[Severity: Medium]
Will pending bios actually be cleaned up without flushing requeue_work?

The comment indicates pending bios are handled, but drivers using the
requeue functionality might have bios stranded in mpath_head->requeue_list
unless mpath_head->requeue_work is also scheduled and flushed here.

[ ... ]

> +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]
Can this assignment leave an error pointer in mpath_head->disk on failure?

If blk_alloc_disk() fails, it returns an error pointer which is stored here
and not cleared before returning. Later, when standard teardown paths call
mpath_put_disk():

void mpath_put_disk(struct mpath_head *mpath_head)
{
    if (!mpath_head->disk)
        return;

The check will fail to catch the error pointer, which is then passed to
put_disk(). Could this regression result in a kernel panic when dereferenced?

[ ... ]

> +int mpath_head_init(struct mpath_head *mpath_head)
> +{
> +	memset(mpath_head, 0, sizeof(*mpath_head));
> +	mutex_init(&mpath_head->lock);
> +	refcount_set(&mpath_head->refcount, 1);
> +
> +	INIT_WORK(&mpath_head->partition_scan_work,
> +		multipath_partition_scan_work);

[Severity: High]
Are there missing work initializations in this function?

While partition_scan_work is initialized, remove_work and requeue_work
from the mpath_head structure appear to be omitted.

When mpath_add_device() is called, it executes:
cancel_delayed_work(&mpath_head->remove_work)

Passing an uninitialized, zero-filled delayed_work structure into the
workqueue APIs can violate their contracts. Could this regression trigger
debug warnings or crashes in timer management code?

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