Re: [PATCH v3 07/13] libmultipath: Add cdev support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Dropping the SRCU read lock during ioctl execution leaves the `mpath_device` pointer unprotected, causing a use-after-free when passed to `cdev_ioctl` or accessed.
- [High] Decoupled lifetime between the embedded `cdev` and `mpath_head` reference counting leads to Use-After-Free during VFS file open and release.
--

commit 9a6527083c29f0b9d58867aa52ca2ea700cf1168
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add cdev support

Add support to create a cdev multipath device. The functionality is much
the same as NVMe, where the cdev is created when a mpath device is set
live.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 78f88b0664c78..6d2e1186a10f8 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> +static int mpath_chr_open(struct inode *inode, struct file *file)
> +{
> +	struct cdev *cdev = file_inode(file)->i_cdev;
> +	struct mpath_head *mpath_head =
> +			container_of(cdev, struct mpath_head, cdev);
> +
> +	return mpath_get_head(mpath_head);
> +}

[Severity: High]
Is there a race condition with driver teardown here? 

If teardown occurs after the VFS has looked up the cdev but before this
function calls mpath_get_head() to increment the refcount, mpath_head could
be freed, potentially causing a use-after-free when this accesses the freed
memory.

> +
> +static int mpath_chr_release(struct inode *inode, struct file *file)
> +{
> +	struct cdev *cdev = file_inode(file)->i_cdev;
> +	struct mpath_head *mpath_head =
> +			container_of(cdev, struct mpath_head, cdev);
> +
> +	mpath_put_head(mpath_head);
> +	return 0;
> +}

[Severity: High]
Does this sequence cause the embedded cdev to be freed prematurely? 

When the last file descriptor is closed, VFS __fput() calls
mpath_chr_release(). If mpath_put_head() drops the mpath_head refcount to 0,
the mpath_head structure (and the embedded cdev) is freed. 

When this returns, VFS __fput() will subsequently call
cdev_put(inode->i_cdev). Could this result in a use-after-free on the now
freed cdev->kobj?

> +
> +static long mpath_chr_ioctl(struct file *file, unsigned int cmd,
> +		unsigned long arg)
> +{
> +	struct cdev *cdev = file_inode(file)->i_cdev;
> +	struct mpath_head *mpath_head =
> +			container_of(cdev, struct mpath_head, cdev);
> +	struct mpath_device *mpath_device;
> +	int srcu_idx, err = -EWOULDBLOCK;
> +	void *unlocked_ioctl_data = NULL;
> +
> +	srcu_idx = srcu_read_lock(&mpath_head->srcu);
> +	mpath_device = mpath_find_path(mpath_head);
> +	if (!mpath_device)
> +		goto out_unlock;
> +	if (mpath_head->mpdt->ioctl_begin)
> +		mpath_head->mpdt->ioctl_begin(mpath_device, cmd,
> +					&unlocked_ioctl_data);
> +	if (unlocked_ioctl_data)
> +		srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	err = mpath_head->mpdt->cdev_ioctl(mpath_device, cmd, arg,
> +					file->f_mode & FMODE_WRITE);

[Severity: Critical]
Are we exposing the mpath_device pointer to a use-after-free here?

If unlocked_ioctl_data is present, the SRCU lock is dropped just before
calling cdev_ioctl(). Since the lock is dropped, a concurrent hot-unplug
removal process calling synchronize_srcu() might not wait for this ioctl to
finish and could free the mpath_device.

This would leave the mpath_device pointer unprotected, which is then
immediately passed into cdev_ioctl(). 

Also, does mpath_bdev_ioctl() suffer from this exact same problem when it
calls mpath_device->disk->fops->ioctl() after potentially dropping its SRCU
lock?

> +	if (unlocked_ioctl_data) {
> +		mpath_head->mpdt->ioctl_finish(unlocked_ioctl_data);
> +		return err;
> +	}
> +
> +out_unlock:
> +	srcu_read_unlock(&mpath_head->srcu, srcu_idx);
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703102918.3723667-1-john.g.garry@oracle.com?part=7