[PATCH 0/6] nvme ioctl cleanups

[PATCH 0/6] nvme ioctl cleanups

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Various non-functional changes in the nvme ioctl code organaization.

Keith Busch (6):
  nvme-ioctl: fold open_for_write into flags
  nvme-ioctl: use common type for user data addresses
  nvme-ioctl: fold 'vec' into flags
  nvme-ioctl: common user timeout setting
  nvme-ioctl: combine alloc and map
  nvme-ioctl: simplify parameters

 drivers/nvme/host/ioctl.c | 243 ++++++++++++++++++++------------------
 1 file changed, 129 insertions(+), 114 deletions(-)

-- 
2.43.5

[PATCH 1/6] nvme-ioctl: fold open_for_write into flags

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Helps to reduce the number of parameters passed around.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 84 ++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index e8930146847af..a3082414c7714 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -12,10 +12,11 @@
 enum {
 	NVME_IOCTL_VEC		= (1 << 0),
 	NVME_IOCTL_PARTITION	= (1 << 1),
+	NVME_IOCTL_WRITABLE	= (1 << 2),
 };
 
 static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
-		unsigned int flags, bool open_for_write)
+		unsigned int flags)
 {
 	u32 effects;
 
@@ -78,7 +79,7 @@ static bool nvme_cmd_allowed(struct nvme_ns *ns, struct nvme_command *c,
 	 * writing.
 	 */
 	if ((nvme_is_write(c) || (effects & NVME_CMD_EFFECTS_LBCC)) &&
-	    !open_for_write)
+	    !(flags & NVME_IOCTL_WRITABLE))
 		goto admin;
 
 	return true;
@@ -293,8 +294,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
 }
 
 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-		struct nvme_passthru_cmd __user *ucmd, unsigned int flags,
-		bool open_for_write)
+		struct nvme_passthru_cmd __user *ucmd, unsigned int flags)
 {
 	struct nvme_passthru_cmd cmd;
 	struct nvme_command c;
@@ -322,7 +322,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
-	if (!nvme_cmd_allowed(ns, &c, 0, open_for_write))
+	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
 	if (cmd.timeout_ms)
@@ -341,8 +341,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 }
 
 static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-		struct nvme_passthru_cmd64 __user *ucmd, unsigned int flags,
-		bool open_for_write)
+		struct nvme_passthru_cmd64 __user *ucmd, unsigned int flags)
 {
 	struct nvme_passthru_cmd64 cmd;
 	struct nvme_command c;
@@ -369,7 +368,7 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
 	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
-	if (!nvme_cmd_allowed(ns, &c, flags, open_for_write))
+	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
 	if (cmd.timeout_ms)
@@ -467,6 +466,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct request *req;
 	blk_opf_t rq_flags = REQ_ALLOC_CACHE;
 	blk_mq_req_flags_t blk_flags = 0;
+	unsigned int flags = 0;
 	int ret;
 
 	c.common.opcode = READ_ONCE(cmd->opcode);
@@ -490,7 +490,9 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.cdw14 = cpu_to_le32(READ_ONCE(cmd->cdw14));
 	c.common.cdw15 = cpu_to_le32(READ_ONCE(cmd->cdw15));
 
-	if (!nvme_cmd_allowed(ns, &c, 0, ioucmd->file->f_mode & FMODE_WRITE))
+	if (ioucmd->file->f_mode & FMODE_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
+	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
 	d.metadata = READ_ONCE(cmd->metadata);
@@ -538,13 +540,13 @@ static bool is_ctrl_ioctl(unsigned int cmd)
 }
 
 static int nvme_ctrl_ioctl(struct nvme_ctrl *ctrl, unsigned int cmd,
-		void __user *argp, bool open_for_write)
+		void __user *argp, unsigned int flags)
 {
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
-		return nvme_user_cmd(ctrl, NULL, argp, 0, open_for_write);
+		return nvme_user_cmd(ctrl, NULL, argp, flags);
 	case NVME_IOCTL_ADMIN64_CMD:
-		return nvme_user_cmd64(ctrl, NULL, argp, 0, open_for_write);
+		return nvme_user_cmd64(ctrl, NULL, argp, flags);
 	default:
 		return sed_ioctl(ctrl->opal_dev, cmd, argp);
 	}
@@ -569,14 +571,14 @@ struct nvme_user_io32 {
 #endif /* COMPAT_FOR_U64_ALIGNMENT */
 
 static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
-		void __user *argp, unsigned int flags, bool open_for_write)
+		void __user *argp, unsigned int flags)
 {
 	switch (cmd) {
 	case NVME_IOCTL_ID:
 		force_successful_syscall_return();
 		return ns->head->ns_id;
 	case NVME_IOCTL_IO_CMD:
-		return nvme_user_cmd(ns->ctrl, ns, argp, flags, open_for_write);
+		return nvme_user_cmd(ns->ctrl, ns, argp, flags);
 	/*
 	 * struct nvme_user_io can have different padding on some 32-bit ABIs.
 	 * Just accept the compat version as all fields that are used are the
@@ -591,8 +593,7 @@ static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned int cmd,
 		flags |= NVME_IOCTL_VEC;
 		fallthrough;
 	case NVME_IOCTL_IO64_CMD:
-		return nvme_user_cmd64(ns->ctrl, ns, argp, flags,
-				       open_for_write);
+		return nvme_user_cmd64(ns->ctrl, ns, argp, flags);
 	default:
 		return -ENOTTY;
 	}
@@ -602,28 +603,32 @@ int nvme_ioctl(struct block_device *bdev, blk_mode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
 	struct nvme_ns *ns = bdev->bd_disk->private_data;
-	bool open_for_write = mode & BLK_OPEN_WRITE;
 	void __user *argp = (void __user *)arg;
 	unsigned int flags = 0;
 
+	if (mode & BLK_OPEN_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
 	if (bdev_is_partition(bdev))
 		flags |= NVME_IOCTL_PARTITION;
 
 	if (is_ctrl_ioctl(cmd))
-		return nvme_ctrl_ioctl(ns->ctrl, cmd, argp, open_for_write);
-	return nvme_ns_ioctl(ns, cmd, argp, flags, open_for_write);
+		return nvme_ctrl_ioctl(ns->ctrl, cmd, argp, flags);
+	return nvme_ns_ioctl(ns, cmd, argp, flags);
 }
 
 long nvme_ns_chr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct nvme_ns *ns =
 		container_of(file_inode(file)->i_cdev, struct nvme_ns, cdev);
-	bool open_for_write = file->f_mode & FMODE_WRITE;
 	void __user *argp = (void __user *)arg;
+	unsigned int flags = 0;
+
+	if (file->f_mode & FMODE_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
 
 	if (is_ctrl_ioctl(cmd))
-		return nvme_ctrl_ioctl(ns->ctrl, cmd, argp, open_for_write);
-	return nvme_ns_ioctl(ns, cmd, argp, 0, open_for_write);
+		return nvme_ctrl_ioctl(ns->ctrl, cmd, argp, flags);
+	return nvme_ns_ioctl(ns, cmd, argp, flags);
 }
 
 static int nvme_uring_cmd_checks(unsigned int issue_flags)
@@ -682,7 +687,7 @@ int nvme_ns_chr_uring_cmd_iopoll(struct io_uring_cmd *ioucmd,
 #ifdef CONFIG_NVME_MULTIPATH
 static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
 		void __user *argp, struct nvme_ns_head *head, int srcu_idx,
-		bool open_for_write)
+		unsigned int flags)
 	__releases(&head->srcu)
 {
 	struct nvme_ctrl *ctrl = ns->ctrl;
@@ -690,7 +695,7 @@ static int nvme_ns_head_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
 
 	nvme_get_ctrl(ns->ctrl);
 	srcu_read_unlock(&head->srcu, srcu_idx);
-	ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp, open_for_write);
+	ret = nvme_ctrl_ioctl(ns->ctrl, cmd, argp, flags);
 
 	nvme_put_ctrl(ctrl);
 	return ret;
@@ -700,12 +705,13 @@ int nvme_ns_head_ioctl(struct block_device *bdev, blk_mode_t mode,
 		unsigned int cmd, unsigned long arg)
 {
 	struct nvme_ns_head *head = bdev->bd_disk->private_data;
-	bool open_for_write = mode & BLK_OPEN_WRITE;
 	void __user *argp = (void __user *)arg;
 	struct nvme_ns *ns;
 	int srcu_idx, ret = -EWOULDBLOCK;
 	unsigned int flags = 0;
 
+	if (mode & BLK_OPEN_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
 	if (bdev_is_partition(bdev))
 		flags |= NVME_IOCTL_PARTITION;
 
@@ -721,9 +727,9 @@ int nvme_ns_head_ioctl(struct block_device *bdev, blk_mode_t mode,
 	 */
 	if (is_ctrl_ioctl(cmd))
 		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
-					       open_for_write);
+					       flags);
 
-	ret = nvme_ns_ioctl(ns, cmd, argp, flags, open_for_write);
+	ret = nvme_ns_ioctl(ns, cmd, argp, flags);
 out_unlock:
 	srcu_read_unlock(&head->srcu, srcu_idx);
 	return ret;
@@ -732,13 +738,16 @@ int nvme_ns_head_ioctl(struct block_device *bdev, blk_mode_t mode,
 long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg)
 {
-	bool open_for_write = file->f_mode & FMODE_WRITE;
 	struct cdev *cdev = file_inode(file)->i_cdev;
 	struct nvme_ns_head *head =
 		container_of(cdev, struct nvme_ns_head, cdev);
 	void __user *argp = (void __user *)arg;
 	struct nvme_ns *ns;
 	int srcu_idx, ret = -EWOULDBLOCK;
+	unsigned int flags = 0;
+
+	if (file->f_mode & FMODE_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
 
 	srcu_idx = srcu_read_lock(&head->srcu);
 	ns = nvme_find_path(head);
@@ -747,9 +756,9 @@ long nvme_ns_head_chr_ioctl(struct file *file, unsigned int cmd,
 
 	if (is_ctrl_ioctl(cmd))
 		return nvme_ns_head_ctrl_ioctl(ns, cmd, argp, head, srcu_idx,
-				open_for_write);
+					       flags);
 
-	ret = nvme_ns_ioctl(ns, cmd, argp, 0, open_for_write);
+	ret = nvme_ns_ioctl(ns, cmd, argp, flags);
 out_unlock:
 	srcu_read_unlock(&head->srcu, srcu_idx);
 	return ret;
@@ -799,7 +808,7 @@ int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
 }
 
 static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp,
-		bool open_for_write)
+		unsigned int flags)
 {
 	struct nvme_ns *ns;
 	int ret, srcu_idx;
@@ -826,7 +835,7 @@ static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp,
 	}
 	srcu_read_unlock(&ctrl->srcu, srcu_idx);
 
-	ret = nvme_user_cmd(ctrl, ns, argp, 0, open_for_write);
+	ret = nvme_user_cmd(ctrl, ns, argp, flags);
 	nvme_put_ns(ns);
 	return ret;
 
@@ -838,17 +847,20 @@ static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp,
 long nvme_dev_ioctl(struct file *file, unsigned int cmd,
 		unsigned long arg)
 {
-	bool open_for_write = file->f_mode & FMODE_WRITE;
 	struct nvme_ctrl *ctrl = file->private_data;
 	void __user *argp = (void __user *)arg;
+	unsigned int flags = 0;
+
+	if (file->f_mode & FMODE_WRITE)
+		flags |= NVME_IOCTL_WRITABLE;
 
 	switch (cmd) {
 	case NVME_IOCTL_ADMIN_CMD:
-		return nvme_user_cmd(ctrl, NULL, argp, 0, open_for_write);
+		return nvme_user_cmd(ctrl, NULL, argp, flags);
 	case NVME_IOCTL_ADMIN64_CMD:
-		return nvme_user_cmd64(ctrl, NULL, argp, 0, open_for_write);
+		return nvme_user_cmd64(ctrl, NULL, argp, flags);
 	case NVME_IOCTL_IO_CMD:
-		return nvme_dev_user_cmd(ctrl, argp, open_for_write);
+		return nvme_dev_user_cmd(ctrl, argp, flags);
 	case NVME_IOCTL_RESET:
 		if (!capable(CAP_SYS_ADMIN))
 			return -EACCES;
-- 
2.43.5

[PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The data type is represented as a u64, but the metadata type as a __user
void *. These two fields represent the same thing, so treating them so
differently looks a bit odd. Just pick one consistent type of
representation.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index a3082414c7714..f1233a280d3b3 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -114,7 +114,7 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 }
 
 static int nvme_map_user_request(struct request *req, u64 ubuffer,
-		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
+		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
 		struct io_uring_cmd *ioucmd, unsigned int flags)
 {
 	struct request_queue *q = req->q;
@@ -161,7 +161,8 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		bio_set_dev(bio, bdev);
 
 	if (has_metadata) {
-		ret = blk_rq_integrity_map_user(req, meta_buffer, meta_len);
+		ret = blk_rq_integrity_map_user(req,
+				nvme_to_user_ptr(meta_buffer), meta_len);
 		if (ret)
 			goto out_unmap;
 	}
@@ -178,8 +179,8 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 
 static int nvme_submit_user_cmd(struct request_queue *q,
 		struct nvme_command *cmd, u64 ubuffer, unsigned bufflen,
-		void __user *meta_buffer, unsigned meta_len,
-		u64 *result, unsigned timeout, unsigned int flags)
+		u64 meta_buffer, unsigned meta_len, u64 *result,
+		unsigned timeout, unsigned int flags)
 {
 	struct nvme_ns *ns = q->queuedata;
 	struct nvme_ctrl *ctrl;
@@ -222,7 +223,6 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	struct nvme_user_io io;
 	struct nvme_command c;
 	unsigned length, meta_len;
-	void __user *metadata;
 
 	if (copy_from_user(&io, uio, sizeof(io)))
 		return -EFAULT;
@@ -246,13 +246,12 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 		 * Protection information is stripped/inserted by the
 		 * controller.
 		 */
-		if (nvme_to_user_ptr(io.metadata))
+		if (io.metadata)
 			return -EINVAL;
 		meta_len = 0;
-		metadata = NULL;
+		io.metadata = 0;
 	} else {
 		meta_len = (io.nblocks + 1) * ns->head->ms;
-		metadata = nvme_to_user_ptr(io.metadata);
 	}
 
 	if (ns->head->features & NVME_NS_EXT_LBAS) {
@@ -275,7 +274,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	c.rw.lbat = cpu_to_le16(io.apptag);
 	c.rw.lbatm = cpu_to_le16(io.appmask);
 
-	return nvme_submit_user_cmd(ns->queue, &c, io.addr, length, metadata,
+	return nvme_submit_user_cmd(ns->queue, &c, io.addr, length, io.metadata,
 			meta_len, NULL, 0, 0);
 }
 
@@ -329,8 +328,8 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
-			cmd.addr, cmd.data_len, nvme_to_user_ptr(cmd.metadata),
-			cmd.metadata_len, &result, timeout, 0);
+			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
+			&result, timeout, flags);
 
 	if (status >= 0) {
 		if (put_user(result, &ucmd->result))
@@ -375,8 +374,8 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
 
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
-			cmd.addr, cmd.data_len, nvme_to_user_ptr(cmd.metadata),
-			cmd.metadata_len, &cmd.result, timeout, flags);
+			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
+			&cmd.result, timeout, flags);
 
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
@@ -514,9 +513,8 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	req->timeout = d.timeout_ms ? msecs_to_jiffies(d.timeout_ms) : 0;
 
 	if (d.addr && d.data_len) {
-		ret = nvme_map_user_request(req, d.addr,
-			d.data_len, nvme_to_user_ptr(d.metadata),
-			d.metadata_len, ioucmd, vec);
+		ret = nvme_map_user_request(req, d.addr, d.data_len, d.metadata,
+					    d.metadata_len, ioucmd, vec);
 		if (ret)
 			return ret;
 	}
-- 
2.43.5

[PATCH 3/6] nvme-ioctl: fold 'vec' into flags

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The vec indicator is already a flag and used as such, so use that type
instead of implicitly relying on a bool to happen to align to the enum
value.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index f1233a280d3b3..3da014ccbe550 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -455,7 +455,8 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 }
 
 static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
-		struct io_uring_cmd *ioucmd, unsigned int issue_flags, bool vec)
+		struct io_uring_cmd *ioucmd, unsigned int issue_flags,
+		unsigned int flags)
 {
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 	const struct nvme_uring_cmd *cmd = io_uring_sqe_cmd(ioucmd->sqe);
@@ -465,7 +466,6 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct request *req;
 	blk_opf_t rq_flags = REQ_ALLOC_CACHE;
 	blk_mq_req_flags_t blk_flags = 0;
-	unsigned int flags = 0;
 	int ret;
 
 	c.common.opcode = READ_ONCE(cmd->opcode);
@@ -514,7 +514,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 
 	if (d.addr && d.data_len) {
 		ret = nvme_map_user_request(req, d.addr, d.data_len, d.metadata,
-					    d.metadata_len, ioucmd, vec);
+					    d.metadata_len, ioucmd, flags);
 		if (ret)
 			return ret;
 	}
@@ -651,10 +651,11 @@ static int nvme_ns_uring_cmd(struct nvme_ns *ns, struct io_uring_cmd *ioucmd,
 
 	switch (ioucmd->cmd_op) {
 	case NVME_URING_CMD_IO:
-		ret = nvme_uring_cmd_io(ctrl, ns, ioucmd, issue_flags, false);
+		ret = nvme_uring_cmd_io(ctrl, ns, ioucmd, issue_flags, 0);
 		break;
 	case NVME_URING_CMD_IO_VEC:
-		ret = nvme_uring_cmd_io(ctrl, ns, ioucmd, issue_flags, true);
+		ret = nvme_uring_cmd_io(ctrl, ns, ioucmd, issue_flags,
+					NVME_IOCTL_VEC);
 		break;
 	default:
 		ret = -ENOTTY;
@@ -793,10 +794,11 @@ int nvme_dev_uring_cmd(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
 
 	switch (ioucmd->cmd_op) {
 	case NVME_URING_CMD_ADMIN:
-		ret = nvme_uring_cmd_io(ctrl, NULL, ioucmd, issue_flags, false);
+		ret = nvme_uring_cmd_io(ctrl, NULL, ioucmd, issue_flags, 0);
 		break;
 	case NVME_URING_CMD_ADMIN_VEC:
-		ret = nvme_uring_cmd_io(ctrl, NULL, ioucmd, issue_flags, true);
+		ret = nvme_uring_cmd_io(ctrl, NULL, ioucmd, issue_flags,
+					NVME_IOCTL_VEC);
 		break;
 	default:
 		ret = -ENOTTY;
-- 
2.43.5

[PATCH 4/6] nvme-ioctl: common user timeout setting

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Most of the passthrough paths repeat the same request timeout setup, so
make it common with the request allocation.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 3da014ccbe550..1f28fc4b341dd 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -101,7 +101,7 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
 
 static struct request *nvme_alloc_user_request(struct request_queue *q,
 		struct nvme_command *cmd, blk_opf_t rq_flags,
-		blk_mq_req_flags_t blk_flags)
+		blk_mq_req_flags_t blk_flags, unsigned timeout_ms)
 {
 	struct request *req;
 
@@ -110,6 +110,7 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 		return req;
 	nvme_init_request(req, cmd);
 	nvme_req(req)->flags |= NVME_REQ_USERCMD;
+	req->timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;
 	return req;
 }
 
@@ -180,7 +181,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 static int nvme_submit_user_cmd(struct request_queue *q,
 		struct nvme_command *cmd, u64 ubuffer, unsigned bufflen,
 		u64 meta_buffer, unsigned meta_len, u64 *result,
-		unsigned timeout, unsigned int flags)
+		unsigned timeout_ms, unsigned int flags)
 {
 	struct nvme_ns *ns = q->queuedata;
 	struct nvme_ctrl *ctrl;
@@ -189,11 +190,10 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	u32 effects;
 	int ret;
 
-	req = nvme_alloc_user_request(q, cmd, 0, 0);
+	req = nvme_alloc_user_request(q, cmd, 0, 0, timeout_ms);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	req->timeout = timeout;
 	if (ubuffer && bufflen) {
 		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
 				meta_len, NULL, flags);
@@ -297,7 +297,6 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 {
 	struct nvme_passthru_cmd cmd;
 	struct nvme_command c;
-	unsigned timeout = 0;
 	u64 result;
 	int status;
 
@@ -324,12 +323,9 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
-	if (cmd.timeout_ms)
-		timeout = msecs_to_jiffies(cmd.timeout_ms);
-
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
-			&result, timeout, flags);
+			&result, cmd.timeout_ms, flags);
 
 	if (status >= 0) {
 		if (put_user(result, &ucmd->result))
@@ -344,7 +340,6 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 {
 	struct nvme_passthru_cmd64 cmd;
 	struct nvme_command c;
-	unsigned timeout = 0;
 	int status;
 
 	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
@@ -370,12 +365,9 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
-	if (cmd.timeout_ms)
-		timeout = msecs_to_jiffies(cmd.timeout_ms);
-
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
-			&cmd.result, timeout, flags);
+			&cmd.result, cmd.timeout_ms, flags);
 
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
@@ -507,10 +499,9 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (issue_flags & IO_URING_F_IOPOLL)
 		rq_flags |= REQ_POLLED;
 
-	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags);
+	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags, d.timeout_ms);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-	req->timeout = d.timeout_ms ? msecs_to_jiffies(d.timeout_ms) : 0;
 
 	if (d.addr && d.data_len) {
 		ret = nvme_map_user_request(req, d.addr, d.data_len, d.metadata,
-- 
2.43.5

[PATCH 5/6] nvme-ioctl: combine alloc and map

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The same pattern repeats, so combine it. Also, the existing mapping
leaks a request if a user requests to map a vectored fixed buffer, or
unsupportable metadata formts, so these get fixed here too.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 69 +++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 1f28fc4b341dd..016a21a3861e9 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -99,21 +99,6 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
 	return (void __user *)ptrval;
 }
 
-static struct request *nvme_alloc_user_request(struct request_queue *q,
-		struct nvme_command *cmd, blk_opf_t rq_flags,
-		blk_mq_req_flags_t blk_flags, unsigned timeout_ms)
-{
-	struct request *req;
-
-	req = blk_mq_alloc_request(q, nvme_req_op(cmd) | rq_flags, blk_flags);
-	if (IS_ERR(req))
-		return req;
-	nvme_init_request(req, cmd);
-	nvme_req(req)->flags |= NVME_REQ_USERCMD;
-	req->timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;
-	return req;
-}
-
 static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
 		struct io_uring_cmd *ioucmd, unsigned int flags)
@@ -146,7 +131,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		ret = io_uring_cmd_import_fixed(ubuffer, bufflen,
 				rq_data_dir(req), &iter, ioucmd);
 		if (ret < 0)
-			goto out;
+			return ret;
 		ret = blk_rq_map_user_iov(q, req, NULL, &iter, GFP_KERNEL);
 	} else {
 		ret = blk_rq_map_user_io(req, NULL, nvme_to_user_ptr(ubuffer),
@@ -155,7 +140,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 	}
 
 	if (ret)
-		goto out;
+		return ret;
 
 	bio = req->bio;
 	if (bdev)
@@ -173,9 +158,36 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 out_unmap:
 	if (bio)
 		blk_rq_unmap_user(bio);
+	return ret;
+}
+
+static struct request *nvme_alloc_user_request(struct request_queue *q,
+		struct nvme_command *cmd, blk_opf_t rq_flags,
+		blk_mq_req_flags_t blk_flags, unsigned timeout_ms, u64 ubuffer,
+		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
+		struct io_uring_cmd *ioucmd, unsigned int flags)
+{
+	struct request *req;
+	int ret;
+
+	req = blk_mq_alloc_request(q, nvme_req_op(cmd) | rq_flags, blk_flags);
+	if (IS_ERR(req))
+		return req;
+	nvme_init_request(req, cmd);
+	nvme_req(req)->flags |= NVME_REQ_USERCMD;
+	req->timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;
+
+	if (ubuffer && bufflen) {
+		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
+				meta_len, ioucmd, flags);
+		if (ret)
+			goto out;
+	}
+
+	return req;
 out:
 	blk_mq_free_request(req);
-	return ret;
+	return ERR_PTR(ret);
 }
 
 static int nvme_submit_user_cmd(struct request_queue *q,
@@ -190,17 +202,11 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	u32 effects;
 	int ret;
 
-	req = nvme_alloc_user_request(q, cmd, 0, 0, timeout_ms);
+	req = nvme_alloc_user_request(q, cmd, 0, 0, timeout_ms, ubuffer,
+		bufflen, meta_buffer, meta_len, NULL, flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	if (ubuffer && bufflen) {
-		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
-				meta_len, NULL, flags);
-		if (ret)
-			return ret;
-	}
-
 	bio = req->bio;
 	ctrl = nvme_req(req)->ctrl;
 
@@ -458,7 +464,6 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct request *req;
 	blk_opf_t rq_flags = REQ_ALLOC_CACHE;
 	blk_mq_req_flags_t blk_flags = 0;
-	int ret;
 
 	c.common.opcode = READ_ONCE(cmd->opcode);
 	c.common.flags = READ_ONCE(cmd->flags);
@@ -499,17 +504,11 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (issue_flags & IO_URING_F_IOPOLL)
 		rq_flags |= REQ_POLLED;
 
-	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags, d.timeout_ms);
+	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags, d.timeout_ms,
+		d.addr, d.data_len, d.metadata, d.metadata_len, ioucmd, flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-	if (d.addr && d.data_len) {
-		ret = nvme_map_user_request(req, d.addr, d.data_len, d.metadata,
-					    d.metadata_len, ioucmd, flags);
-		if (ret)
-			return ret;
-	}
-
 	/* to free bio on completion, as req->bio will be null at that time */
 	pdu->bio = req->bio;
 	pdu->req = req;
-- 
2.43.5

[PATCH 6/6] nvme-ioctl: simplify parameters

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The uring_cmd handler already defines a struct to group all the
parameters needed to submit a request. Reuse this throughout to simplfy
the function signature.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/ioctl.c | 87 ++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 37 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 016a21a3861e9..729dac2d6ee0a 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -99,8 +99,15 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
 	return (void __user *)ptrval;
 }
 
-static int nvme_map_user_request(struct request *req, u64 ubuffer,
-		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
+struct nvme_user_data {
+	__u64	metadata;
+	__u64	addr;
+	__u32	data_len;
+	__u32	metadata_len;
+	__u32	timeout_ms;
+};
+
+static int nvme_map_user_request(struct request *req, struct nvme_user_data *d,
 		struct io_uring_cmd *ioucmd, unsigned int flags)
 {
 	struct request_queue *q = req->q;
@@ -108,7 +115,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 	struct block_device *bdev = ns ? ns->disk->part0 : NULL;
 	bool supports_metadata = bdev && blk_get_integrity(bdev->bd_disk);
 	struct nvme_ctrl *ctrl = nvme_req(req)->ctrl;
-	bool has_metadata = meta_buffer && meta_len;
+	bool has_metadata = d->metadata && d->metadata_len;
 	struct bio *bio = NULL;
 	int ret;
 
@@ -128,14 +135,14 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 		/* fixedbufs is only for non-vectored io */
 		if (WARN_ON_ONCE(flags & NVME_IOCTL_VEC))
 			return -EINVAL;
-		ret = io_uring_cmd_import_fixed(ubuffer, bufflen,
+		ret = io_uring_cmd_import_fixed(d->addr, d->data_len,
 				rq_data_dir(req), &iter, ioucmd);
 		if (ret < 0)
 			return ret;
 		ret = blk_rq_map_user_iov(q, req, NULL, &iter, GFP_KERNEL);
 	} else {
-		ret = blk_rq_map_user_io(req, NULL, nvme_to_user_ptr(ubuffer),
-				bufflen, GFP_KERNEL, flags & NVME_IOCTL_VEC, 0,
+		ret = blk_rq_map_user_io(req, NULL, nvme_to_user_ptr(d->addr),
+				d->data_len, GFP_KERNEL, flags & NVME_IOCTL_VEC, 0,
 				0, rq_data_dir(req));
 	}
 
@@ -148,7 +155,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 
 	if (has_metadata) {
 		ret = blk_rq_integrity_map_user(req,
-				nvme_to_user_ptr(meta_buffer), meta_len);
+				nvme_to_user_ptr(d->metadata), d->metadata_len);
 		if (ret)
 			goto out_unmap;
 	}
@@ -163,8 +170,7 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
 
 static struct request *nvme_alloc_user_request(struct request_queue *q,
 		struct nvme_command *cmd, blk_opf_t rq_flags,
-		blk_mq_req_flags_t blk_flags, unsigned timeout_ms, u64 ubuffer,
-		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
+		blk_mq_req_flags_t blk_flags, struct nvme_user_data *d,
 		struct io_uring_cmd *ioucmd, unsigned int flags)
 {
 	struct request *req;
@@ -175,11 +181,10 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 		return req;
 	nvme_init_request(req, cmd);
 	nvme_req(req)->flags |= NVME_REQ_USERCMD;
-	req->timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;
+	req->timeout = d->timeout_ms ? msecs_to_jiffies(d->timeout_ms) : 0;
 
-	if (ubuffer && bufflen) {
-		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
-				meta_len, ioucmd, flags);
+	if (d->addr && d->data_len) {
+		ret = nvme_map_user_request(req, d, ioucmd, flags);
 		if (ret)
 			goto out;
 	}
@@ -191,9 +196,8 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
 }
 
 static int nvme_submit_user_cmd(struct request_queue *q,
-		struct nvme_command *cmd, u64 ubuffer, unsigned bufflen,
-		u64 meta_buffer, unsigned meta_len, u64 *result,
-		unsigned timeout_ms, unsigned int flags)
+		struct nvme_command *cmd, struct nvme_user_data *d,
+		u64 *result, unsigned int flags)
 {
 	struct nvme_ns *ns = q->queuedata;
 	struct nvme_ctrl *ctrl;
@@ -202,8 +206,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	u32 effects;
 	int ret;
 
-	req = nvme_alloc_user_request(q, cmd, 0, 0, timeout_ms, ubuffer,
-		bufflen, meta_buffer, meta_len, NULL, flags);
+	req = nvme_alloc_user_request(q, cmd, 0, 0, d, NULL, flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
@@ -229,6 +232,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	struct nvme_user_io io;
 	struct nvme_command c;
 	unsigned length, meta_len;
+	struct nvme_user_data d;
 
 	if (copy_from_user(&io, uio, sizeof(io)))
 		return -EFAULT;
@@ -280,8 +284,13 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	c.rw.lbat = cpu_to_le16(io.apptag);
 	c.rw.lbatm = cpu_to_le16(io.appmask);
 
-	return nvme_submit_user_cmd(ns->queue, &c, io.addr, length, io.metadata,
-			meta_len, NULL, 0, 0);
+	d.addr = io.addr;
+	d.data_len = length;
+	d.metadata = io.metadata;
+	d.metadata_len = meta_len;
+	d.timeout_ms = 0;
+
+	return nvme_submit_user_cmd(ns->queue, &c, &d, NULL, 0);
 }
 
 static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
@@ -302,6 +311,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		struct nvme_passthru_cmd __user *ucmd, unsigned int flags)
 {
 	struct nvme_passthru_cmd cmd;
+	struct nvme_user_data d;
 	struct nvme_command c;
 	u64 result;
 	int status;
@@ -329,9 +339,14 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
-	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
-			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
-			&result, cmd.timeout_ms, flags);
+	d.addr = cmd.addr;
+	d.data_len = cmd.data_len;
+	d.metadata = cmd.metadata;
+	d.metadata_len = cmd.metadata_len;
+	d.timeout_ms = cmd.timeout_ms;
+
+	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c, &d,
+			&result, flags);
 
 	if (status >= 0) {
 		if (put_user(result, &ucmd->result))
@@ -345,6 +360,7 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		struct nvme_passthru_cmd64 __user *ucmd, unsigned int flags)
 {
 	struct nvme_passthru_cmd64 cmd;
+	struct nvme_user_data d;
 	struct nvme_command c;
 	int status;
 
@@ -371,9 +387,14 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (!nvme_cmd_allowed(ns, &c, flags))
 		return -EACCES;
 
-	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
-			cmd.addr, cmd.data_len, cmd.metadata, cmd.metadata_len,
-			&cmd.result, cmd.timeout_ms, flags);
+	d.addr = cmd.addr;
+	d.data_len = cmd.data_len;
+	d.metadata = cmd.metadata;
+	d.metadata_len = cmd.metadata_len;
+	d.timeout_ms = cmd.timeout_ms;
+
+	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c, &d,
+			&cmd.result, flags);
 
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
@@ -383,14 +404,6 @@ static int nvme_user_cmd64(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	return status;
 }
 
-struct nvme_uring_data {
-	__u64	metadata;
-	__u64	addr;
-	__u32	data_len;
-	__u32	metadata_len;
-	__u32	timeout_ms;
-};
-
 /*
  * This overlays struct io_uring_cmd pdu.
  * Expect build errors if this grows larger than that.
@@ -459,7 +472,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
 	const struct nvme_uring_cmd *cmd = io_uring_sqe_cmd(ioucmd->sqe);
 	struct request_queue *q = ns ? ns->queue : ctrl->admin_q;
-	struct nvme_uring_data d;
+	struct nvme_user_data d;
 	struct nvme_command c;
 	struct request *req;
 	blk_opf_t rq_flags = REQ_ALLOC_CACHE;
@@ -504,8 +517,8 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	if (issue_flags & IO_URING_F_IOPOLL)
 		rq_flags |= REQ_POLLED;
 
-	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags, d.timeout_ms,
-		d.addr, d.data_len, d.metadata, d.metadata_len, ioucmd, flags);
+	req = nvme_alloc_user_request(q, &c, rq_flags, blk_flags, &d, ioucmd,
+		flags);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
 
-- 
2.43.5

Re: [PATCH 1/6] nvme-ioctl: fold open_for_write into flags

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Helps to reduce the number of parameters passed around.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/6] nvme-ioctl: fold open_for_write into flags

[Chaitanya Kulkarni]

On 2/24/25 10:21, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
>
> Helps to reduce the number of parameters passed around.
>
> Signed-off-by: Keith Busch<kbusch@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The data type is represented as a u64, but the metadata type as a __user
> void *. These two fields represent the same thing, so treating them so
> differently looks a bit odd. Just pick one consistent type of
> representation.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Maybe mention that were needed nvme_to_user_ptr() to not lose the __user attribute ?

Otherwise, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/6] nvme-ioctl: fold 'vec' into flags

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The vec indicator is already a flag and used as such, so use that type
> instead of implicitly relying on a bool to happen to align to the enum
> value.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/6] nvme-ioctl: common user timeout setting

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Most of the passthrough paths repeat the same request timeout setup, so
> make it common with the request allocation.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/ioctl.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 3da014ccbe550..1f28fc4b341dd 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -101,7 +101,7 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
>  
>  static struct request *nvme_alloc_user_request(struct request_queue *q,
>  		struct nvme_command *cmd, blk_opf_t rq_flags,
> -		blk_mq_req_flags_t blk_flags)
> +		blk_mq_req_flags_t blk_flags, unsigned timeout_ms)
>  {
>  	struct request *req;
>  
> @@ -110,6 +110,7 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
>  		return req;
>  	nvme_init_request(req, cmd);
>  	nvme_req(req)->flags |= NVME_REQ_USERCMD;
> +	req->timeout = timeout_ms ? msecs_to_jiffies(timeout_ms) : 0;

Should we check for overflows / MAX_JIFFY_OFFSET here ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Christoph Hellwig]

On Mon, Feb 24, 2025 at 10:21:24AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The data type is represented as a u64, but the metadata type as a __user
> void *. These two fields represent the same thing, so treating them so
> differently looks a bit odd. Just pick one consistent type of
> representation.

Yes, being consistent is good.  I would normally propagate the
proper pointer type as far as possible, but I can live with this
version if you add a little blurb why it's preferable.

Re: [PATCH 5/6] nvme-ioctl: combine alloc and map

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The same pattern repeats, so combine it. Also, the existing mapping
> leaks a request if a user requests to map a vectored fixed buffer, or
> unsupportable metadata formts, so these get fixed here too.

s/formts/formats

Maybe split this patch to have the fix come first with a Fixes tag ?

Other than that, Looks OK to me.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 6/6] nvme-ioctl: simplify parameters

[Damien Le Moal]

On 2/25/25 03:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The uring_cmd handler already defines a struct to group all the
> parameters needed to submit a request. Reuse this throughout to simplfy
> the function signature.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/6] nvme-ioctl: fold 'vec' into flags

[Christoph Hellwig]

On Mon, Feb 24, 2025 at 10:21:25AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The vec indicator is already a flag and used as such, so use that type
> instead of implicitly relying on a bool to happen to align to the enum
> value.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/6] nvme-ioctl: common user timeout setting

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/6] nvme-ioctl: combine alloc and map

[Christoph Hellwig]

On Mon, Feb 24, 2025 at 10:21:27AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The same pattern repeats, so combine it. Also, the existing mapping
> leaks a request if a user requests to map a vectored fixed buffer, or
> unsupportable metadata formts, so these get fixed here too.

That fix should probably be a separate backportable patch at the
beginning of the series.

Otherwise looks sane.

Re: [PATCH 6/6] nvme-ioctl: simplify parameters

[Christoph Hellwig]

On Mon, Feb 24, 2025 at 10:21:28AM -0800, Keith Busch wrote:
> +		ret = blk_rq_map_user_io(req, NULL, nvme_to_user_ptr(d->addr),
> +				d->data_len, GFP_KERNEL, flags & NVME_IOCTL_VEC, 0,

Overly long line here.

Otherwise this looks good.

Re: [PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Keith Busch]

On Mon, Feb 24, 2025 at 11:30:53PM +0100, Christoph Hellwig wrote:
> On Mon, Feb 24, 2025 at 10:21:24AM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The data type is represented as a u64, but the metadata type as a __user
> > void *. These two fields represent the same thing, so treating them so
> > differently looks a bit odd. Just pick one consistent type of
> > representation.
> 
> Yes, being consistent is good.  I would normally propagate the
> proper pointer type as far as possible, but I can live with this
> version if you add a little blurb why it's preferable.

I have a reason, but it's not very good :)

I settled on u64 simply because that's how 'struct nvme_uring_data'
represents both, so it makes for a tiny bit less churn in later patches. 

Re: [PATCH 1/6] nvme-ioctl: fold open_for_write into flags

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/6] nvme ioctl cleanups

[Chaitanya Kulkarni]

On 2/24/25 10:21, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> Various non-functional changes in the nvme ioctl code organaization.
>
> Keith Busch (6):
>    nvme-ioctl: fold open_for_write into flags
>    nvme-ioctl: use common type for user data addresses
>    nvme-ioctl: fold 'vec' into flags
>    nvme-ioctl: common user timeout setting
>    nvme-ioctl: combine alloc and map
>    nvme-ioctl: simplify parameters
>
>   drivers/nvme/host/ioctl.c | 243 ++++++++++++++++++++------------------
>   1 file changed, 129 insertions(+), 114 deletions(-)
>

For the whole series looks good :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Tested-by: Chaitanya Kulkarni <kch@nvidia.com>

Test result is below.

-ck

Re: [PATCH 1/6] nvme-ioctl: fold open_for_write into flags

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 3/6] nvme-ioctl: fold 'vec' into flags

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 4/6] nvme-ioctl: common user timeout setting

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 5/6] nvme-ioctl: combine alloc and map

[Kanchan Joshi]

On 2/24/2025 11:51 PM, Keith Busch wrote:
> +static struct request *nvme_alloc_user_request(struct request_queue *q,
> +		struct nvme_command *cmd, blk_opf_t rq_flags,
> +		blk_mq_req_flags_t blk_flags, unsigned timeout_ms, u64 ubuffer,
> +		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
> +		struct io_uring_cmd *ioucmd, unsigned int flags)

Back in 2022, nvme_alloc_user_request used to take 12 arguments.
For fixed-buffer support, I added two more and got 'too many arguments' 
complaint [*]. So I did alloc/map split to trim arguments.

This patch, in itself, is going back to 2022.
But the last patch neutralizes 'too many arguments'.


[*] 
https://lore.kernel.org/linux-nvme/26490329-5b51-7334-1e2a-44edfe75d8fa@nvidia.com/

Re: [PATCH 6/6] nvme-ioctl: simplify parameters

[Kanchan Joshi]

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCH 2/6] nvme-ioctl: use common type for user data addresses

[Nilay Shroff]

On 2/24/25 11:51 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The data type is represented as a u64, but the metadata type as a __user
> void *. These two fields represent the same thing, so treating them so
> differently looks a bit odd. Just pick one consistent type of
> representation.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/ioctl.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index a3082414c7714..f1233a280d3b3 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -114,7 +114,7 @@ static struct request *nvme_alloc_user_request(struct request_queue *q,
>  }
>  
>  static int nvme_map_user_request(struct request *req, u64 ubuffer,
> -		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
> +		unsigned bufflen, u64 meta_buffer, unsigned meta_len,
>  		struct io_uring_cmd *ioucmd, unsigned int flags)
>  {
>  	struct request_queue *q = req->q;
> @@ -161,7 +161,8 @@ static int nvme_map_user_request(struct request *req, u64 ubuffer,
>  		bio_set_dev(bio, bdev);
>  
>  	if (has_metadata) {
> -		ret = blk_rq_integrity_map_user(req, meta_buffer, meta_len);
> +		ret = blk_rq_integrity_map_user(req,
> +				nvme_to_user_ptr(meta_buffer), meta_len);
>  		if (ret)
>  			goto out_unmap;
>  	}
As we're stripping __user, for readability, can we name it as "umeta_buffer" or 
"user_meta_buffer"? The regular user data buffer is already named as "ubuffer". 
Otherwise this is good.

Thanks,
--Nilay

Re: [PATCH 0/6] nvme ioctl cleanups

[Keith Busch]

On Mon, Feb 24, 2025 at 10:21:22AM -0800, Keith Busch wrote:
> Various non-functional changes in the nvme ioctl code organaization.

Just fyi, I'm going to redo this series after the first nvme bits for
6.15 are merged upstream. Rebasing to a tree after that will make it
possible to avoid some minor merge conflicts.