Re: [PATCH v3 05/10] nvme-multipath: add nvme_mpath_cdev_ioctl()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Accessing `ns->ctrl` in `_nvme_ns_chr_ioctl()` leads to a Use-After-Free because the SRCU read lock protecting the namespace is dropped by the caller.
- [Critical] `nvme_mpath_ioctl_begin()` fails to initialize `*data` for non-controller ioctls, causing a NULL pointer dereference or crash in `nvme_mpath_ioctl_finish()`.
--

commit 7c14abcc0f25bf0d710bcb67c0dbd7f7f3dc3774
Author: John Garry <john.g.garry@oracle.com>

nvme-multipath: add nvme_mpath_cdev_ioctl()

This commit adds nvme_mpath_cdev_ioctl() and related ioctl lifecycle
callbacks to handle SRCU read lock dropping during controller command
IOCTL handling for NVMe multipath character devices.

> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 664216eece4a6..14ac86b8b1a8a 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c

[ ... ]

> @@ -689,6 +695,29 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
>  	return 0;
>  }
>  #ifdef CONFIG_NVME_MULTIPATH
> +long nvme_mpath_cdev_ioctl(struct mpath_device *mpath_device, unsigned int cmd,
> +					unsigned long arg, bool open_for_write)
> +{
> +	return _nvme_ns_chr_ioctl(nvme_mpath_to_ns(mpath_device), cmd,
> +			arg, open_for_write);
> +}

[Severity: Critical]
Could this lead to a use-after-free on the namespace object?

When a userspace program issues a controller ioctl on a multipath char device,
the generic mpath_chr_ioctl() drops the SRCU read lock before calling
nvme_mpath_cdev_ioctl(). 

While nvme_mpath_ioctl_begin() takes a reference to the controller, the
namespace object itself is left unprotected during this window. If the
namespace is concurrently removed, passing the unprotected ns pointer down to
_nvme_ns_chr_ioctl() and dereferencing ns->ctrl could access freed memory.

> +
> +void nvme_mpath_ioctl_begin(struct mpath_device *mpath_device,
> +		unsigned int cmd, void **data)
> +{
> +	struct nvme_ctrl *ctrl = nvme_mpath_to_ns(mpath_device)->ctrl;
> +
> +	if (is_ctrl_ioctl(cmd)) {
> +		nvme_get_ctrl(ctrl);
> +		*data = ctrl;
> +	}
> +}

[Severity: Critical]
Does this leave *data uninitialized or unmodified for non-controller ioctls?

If a user issues a standard ioctl (like NVME_IOCTL_ID), is_ctrl_ioctl(cmd)
will evaluate to false, meaning *data is not assigned here.

> +
> +void nvme_mpath_ioctl_finish(void *opaque)
> +{
> +	nvme_put_ctrl(opaque);
> +}

[Severity: Critical]
Is it safe to unconditionally call nvme_put_ctrl() here?

If nvme_mpath_ioctl_begin() did not set *data (because it was not a controller
ioctl), opaque might be NULL or uninitialized garbage. 

Calling nvme_put_ctrl() unconditionally:

static inline void nvme_put_ctrl(struct nvme_ctrl *ctrl)
{
        put_device(ctrl->device);
}

will dereference ctrl->device, which could result in a panic if opaque is NULL
or an invalid pointer. Should there be a check before calling nvme_put_ctrl()?

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