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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The block_device_operations struct `mpath_ops` lacks the mandatory `submit_bio` callback, causing `device_add_disk()` to unconditionally fail and preventing the multipath disk from functioning.
- [High] Re-using `gendisk` after `device_add_disk()` failure violates the driver model and causes kobject corruption.
- [High] `mpath_head_uninit()` synchronously blocks on open file descriptors, causing device removal to hang indefinitely if userspace holds the device open.
--

--- Patch [2]: [PATCH v6 02/27] libmultipath: Add basic gendisk support ---
commit 0995e161443bbd8d5024681ae6aeaebc37bafd47
Author: John Garry <[email protected]>

libmultipath: Add basic gendisk support

This commit adds support to allocate and free a multipath gendisk.
It 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
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
[ ... ]
> @@ -36,6 +36,107 @@ void mpath_head_uninit(struct mpath_head *mpath_head)
[ ... ]
> +const struct block_device_operations mpath_ops = {
> +	.owner          = THIS_MODULE,

[Severity: High]
Does setting the owner to THIS_MODULE (libmultipath) rather than the caller
module (such as nvme_core) introduce a regression where the caller module can
be unloaded while the disk is held open by userspace? If the caller's module
exit calls mpath_head_uninit(), could it hang indefinitely in an
uninterruptible wait on the refcount, permanently deadlocking module_mutex?

> +	.open		= mpath_bdev_open,
> +	.release	= mpath_bdev_release,
> +};

[Severity: High]
Is this regression missing a submit_bio callback in mpath_ops? For bio-based
queues, the block layer's __add_disk() mandates that disk->fops->submit_bio
is present. If it is absent, device_add_disk() unconditionally aborts and
returns -EINVAL, which prevents the multipath block device from registering.

[ ... ]
> +void mpath_device_set_live(struct mpath_device *mpath_device)
> +{
[ ... ]
> +	if (!test_and_set_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
> +		dev_set_drvdata(disk_to_dev(mpath_head->disk), mpath_head);
> +		ret = device_add_disk(mpath_head->parent, mpath_head->disk,
> +				mpath_head->disk_groups);
> +		if (ret) {
> +			clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags);
> +			return;
> +		}

[Severity: High]
If device_add_disk() fails, clearing the live flag without freeing the
disk leaves the object intact. When this function is called again for another
path, won't it retry device_add_disk() on the exact same gendisk object?
Because the driver model forbids calling device_add() more than once on the
same device object and requires dropping the device upon failure, could this
regression result in kobject BUGs or memory corruption?

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