Re: [PATCH v5 02/27] libmultipath: Add basic gendisk support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mpath_alloc_head_disk() leaves mpath_head->disk as an ERR_PTR on failure, leading to a kernel panic during cleanup in mpath_put_disk().
- [High] The allocated multipath disk unconditionally uses mpath_ops, which lacks a .submit_bio implementation, causing device_add_disk to fail and rendering the device incapable of processing I/O.
--

--- Patch [2]: [PATCH v5 02/27] libmultipath: Add basic gendisk support ---
commit 15c7ca249b473955f186aec1f0490e8ccef64672
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add basic gendisk support
    
Add support to allocate and free a multipath gendisk.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 9cc398d266adf..d2e334259f3c1 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> @@ -35,6 +35,104 @@ 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);
> +}
> +
> +static void mpath_bdev_release(struct gendisk *disk)
> +{
> +	struct mpath_head *mpath_head = disk->private_data;
> +
> +	mpath_put_head(mpath_head);
> +}
> +
> +const struct block_device_operations mpath_ops = {
> +	.owner          = THIS_MODULE,
> +	.open		= mpath_bdev_open,
> +	.release	= mpath_bdev_release,
> +};

[Severity: High]
Does this block device operations struct need a .submit_bio callback?

Since device_add_disk() in the block layer strictly enforces that bio-based
disks provide a .submit_bio callback, won't device_add_disk() unconditionally
return -EINVAL when mpath_device_set_live() is called?

Also, the helper is_mpath_disk() strictly requires disk->fops == &mpath_ops,
which prevents drivers from dynamically overriding the operations struct to
provide their own .submit_bio implementation.

> +EXPORT_SYMBOL_GPL(mpath_ops);

[ ... ]

> +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);
> +	mpath_head->disk = NULL;
> +}
> +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 ||
> +	    mpath_head->disk)
> +		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 and returns an ERR_PTR, it is assigned to
mpath_head->disk. Does this cause a crash during cleanup?

If the caller handles this failure by tearing down the object via
mpath_put_disk():

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

The (!mpath_head->disk) check will evaluate to false for an ERR_PTR,
allowing the error pointer to be passed to put_disk(), which will dereference
it. Should mpath_head->disk be cleared to NULL before returning the error?

> +
> +	mpath_head->disk->private_data = mpath_head;
> +	mpath_head->disk->fops = &mpath_ops;
> +
> +	set_bit(GD_SUPPRESS_PART_SCAN, &mpath_head->disk->state);
> +
> +	return 0;
> +}

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