Re: [PATCH v3 02/13] libmultipath: Add basic gendisk support

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 11:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] `mpath_alloc_head_disk()` leaves `ERR_PTR` in `mpath_head->disk` on failure, causing a kernel panic during cleanup or state updates.
> - [High] A TOCTOU race between `mpath_device_set_live()` and `mpath_remove_disk()` permanently leaks the gendisk.
> - [High] Synchronous wait in `mpath_head_uninit()` allows unprivileged openers to block device teardown indefinitely.
> --
> 
> commit bba05c007c446cdb19dbac857c9ce717ca5f2a08
> Author: John Garry <john.g.garry@oracle.com>
> Subject: libmultipath: Add basic gendisk support
> 
> This patch adds support to allocate and free a multipath gendisk. It also
> 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
>> index 009d4bb875c6f..79be84d3d4f75 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
>> @@ -44,12 +44,111 @@ 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);
>> +}
> 
> [Severity: High]
> Does this increment the refcount on behalf of userspace openers?
> 
> If unprivileged users open the block device, this calls mpath_get_head()
> which increments mpath_head->refcount. If the admin later removes the device,
> the driver calls mpath_head_uninit() which synchronously waits for the
> refcount to hit zero. Since the user can hold the file descriptor open,
> couldn't this allow unprivileged openers to permanently hang the kernel
> thread handling device removal?

Userspace can still issue a syscall which can reference the mpath_head 
structure, so we need to guard against that.

> 
> [ ... ]
> 
>> +void mpath_remove_disk(struct mpath_head *mpath_head)
>> +{
>> +	if (test_and_clear_bit(MPATH_HEAD_DISK_LIVE, &mpath_head->flags)) {
>> +		struct gendisk *disk = mpath_head->disk;
>> +
>> +		del_gendisk(disk);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(mpath_remove_disk);
> 
> [Severity: High]
> Is there a race here with mpath_device_set_live() that could permanently
> leak the gendisk?
> 
> If Thread A calls mpath_device_set_live() and sets MPATH_HEAD_DISK_LIVE
> locklessly via test_and_set_bit(), and Thread B concurrently calls
> mpath_remove_disk(), 

That should not happen. The driver needs to take care of concurrent 
calls. In the NVMe example, we remove the NS head structure from the 
list of NS heads before we call mpath_remove_disk(), so no new NS can 
call mpath_device_set_live() for the same mpath_head once the disk is 
being removed.

>Thread B will clear the flag and call del_gendisk().
> 
> Because device_add_disk() in Thread A hasn't completed and set GD_ADDED
> yet, del_gendisk() observes the disk is not live and returns early without
> doing anything.
> 
> Thread A then completes device_add_disk() making the disk live, but the
> flag is now 0. Any subsequent mpath_remove_disk() will do nothing, leaking
> the active block device.
> 
>> +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);
>> +}
>> +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)
>> +		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, does this leave an ERR_PTR in mpath_head->disk?
> 
> When the driver cleans up on error and calls mpath_put_disk(), the
> if (!mpath_head->disk) check will pass because the ERR_PTR is not NULL.
> Will this result in a kernel panic when put_disk() attempts to dereference
> the invalid pointer?

We don't call mpath_put_disk() for a failure there. In this case the 
caller should tear down the mpath_head structure.

>