Re: [PATCH v5 08/27] libmultipath: Add support for block device IOCTL

On 27/07/2026 16:08, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Holding the SRCU read lock across the underlying ioctl call causes an AB-BA deadlock if the ioctl triggers a synchronous path removal.
> - [High] Forwarding ioctls via part0 strips partition context, bypassing partition-specific capability checks in underlying drivers.
> - [Medium] Returning -EOPNOTSUPP instead of -ENOTTY for unimplemented underlying ioctls breaks userspace fallback mechanics.
> --
> 
> --- Patch [8]: [PATCH v5 08/27] libmultipath: Add support for block device IOCTL ---
> commit c97bc7070dfdd2d761dd1464c23d043908adfc68
> Author: John Garry <[email protected]>
> 
> libmultipath: Add support for block device IOCTL
>      
> Add mpath_bdev_ioctl() as a multipath block device IOCTL handler. This
> handler calls into the mpath_device bdev fops handler. The .compat_ioctl
> handler is given the standard handler.
> 
>> diff --git a/lib/multipath.c b/lib/multipath.c
>> index c78d46a446cf3..af90cf38a79d6 100644
>> --- a/lib/multipath.c
>> +++ b/lib/multipath.c
> 
> [ ... ]
> 
>> @@ -454,11 +454,47 @@ static void mpath_bdev_release(struct gendisk *disk)
>>   	mpath_put_head(mpath_head);
>>   }
>>   
>> +static int mpath_bdev_ioctl(struct block_device *bdev, blk_mode_t mode,
>> +		    unsigned int cmd, unsigned long arg)
>> +{
>> +	struct gendisk *disk = bdev->bd_disk;
>> +	struct mpath_head *mpath_head = mpath_gendisk_to_head(disk);
>> +	struct mpath_device *mpath_device;
>> +	int srcu_idx, err;
>> +
>> +	if (bdev_is_partition(bdev) && !capable(CAP_SYS_RAWIO))
>> +		return -ENOIOCTLCMD;
>> +
>> +	srcu_idx = srcu_read_lock(&mpath_head->srcu);
>> +	mpath_device = mpath_find_path(mpath_head);
>> +	if (!mpath_device) {
>> +		err = -EWOULDBLOCK;
>> +		goto out_unlock;
>> +	}
>> +
>> +	if (!mpath_device->disk->fops->ioctl) {
>> +		err = -EOPNOTSUPP;
>> +		goto out_unlock;
>> +	}
> 
> [Severity: Medium]
> Should this return -ENOTTY instead?

will fix

> 
> Returning -EOPNOTSUPP for unimplemented ioctls in mpath_bdev_ioctl() can break
> userspace fallback mechanisms in tools like smartctl or nvme-cli that
> explicitly check for -ENOTTY to determine if an ioctl is supported on the
> block device.
> 
>> +
>> +	err = mpath_device->disk->fops->ioctl(
>> +			mpath_device->disk->part0, mode, cmd, arg);
> 
> [Severity: High]
> Does forwarding ioctls to part0 in mpath_bdev_ioctl() bypass
> partition-specific security checks?

I added that check above

> 
> If an ioctl is invoked on a multipath partition, passing
> mpath_device->disk->part0 strips the partition context. Underlying drivers
> like nvme rely on bdev_is_partition() to enforce stricter security on
> partitions (e.g. requiring CAP_SYS_ADMIN). Because part0 represents the whole
> disk, this could allow users with CAP_SYS_RAWIO to issue unrestricted
> passthrough commands.
> 
> [Severity: High]
> Could holding the srcu read lock during the underlying ioctl cause a deadlock?
> 
> In mpath_bdev_ioctl(), if the ioctl triggers a synchronous path removal
> (like an nvme namespace management command), the underlying driver might

we don't support NVMe yet

> attempt to remove the namespace. If mpath_delete_device() calls
> synchronize_srcu(&mpath_head->srcu) during removal, the removal thread will
> block waiting for the srcu read lock to be released, while the ioctl thread
> holds the lock and waits for the removal to complete.
> 
>> +out_unlock:
>> +	srcu_read_unlock(&mpath_head->srcu, srcu_idx);
>> +	return err;
>> +}
>