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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Deadlock in `mpath_bdev_ioctl` due to holding `srcu_read_lock` while forwarding ioctls that can trigger synchronous path removal.
- [High] Bypass of underlying partition-specific IOCTL security policies because `mpath_bdev_ioctl` unconditionally passes `part0`.
--

--- Patch [8]: [PATCH v6 08/27] libmultipath: Add support for block device IOCTL ---
commit d2e45114ccfb93fccdccc60ed75e05084f9b9173
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.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 58eb9be5bebbe..f1758d1e60a60 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -454,11 +454,53 @@ 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;
> +
> +	/*
> +	 * This check is duplicated from sd_ioctl() as we don't pass the
> +	 * partition bdev to fops->ioctl. That is not yet possible as the
> +	 * per-path disk is hidden and, as such, does not have partitions
> +	 * scanned.
> +	 */
> +	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 = -ENOTTY;
> +		goto out_unlock;
> +	}
> +
> +	err = mpath_device->disk->fops->ioctl(
> +			mpath_device->disk->part0, mode, cmd, arg);

[Severity: High]
Does unconditionally passing part0 bypass partition-specific IOCTL security
policies in the underlying driver?

By passing the whole-disk block device (mpath_device->disk->part0),
bdev_is_partition() will evaluate to false in the underlying driver. For
drivers like NVMe, this strips the partition context and could bypass
requirements like CAP_SYS_ADMIN for passthrough commands on partitions,
allowing a user with only CAP_SYS_RAWIO to execute whole-disk commands.

[Severity: High]
Can holding the SRCU read lock across the underlying ioctl cause a deadlock?

If the underlying ioctl triggers a controller reset or device removal, the
teardown path could eventually wait on synchronize_srcu() for this exact
srcu instance. Since the ioctl is waiting for the operation to complete
while holding srcu_read_lock(), synchronize_srcu() would block forever.

> +out_unlock:
> +	srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	return err;
> +}

[ ... ]

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