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

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?

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?

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

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