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

On 29/07/2026 12:20, [email protected] wrote:
> 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?

the caller module should call mpath_head_uninit(), which ensures all 
references are dropped

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

it's added later

> 
> [ ... ]
>> +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?

This is same as NVMe, so I won't change it now

>