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

On 27/07/2026 16:04, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

That's not an issue, but I can clear it

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