Re: [PATCH v4 4/5] nvme: wire-up uring-cmd support for io-passthru on char-device.

Jens Axboe
2022-05-05T13:38:46Z

On 5/5/22 12:06 AM, Kanchan Joshi wrote:
> +static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> + struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> +{
> + struct nvme_uring_cmd *cmd =
> + (struct nvme_uring_cmd *)ioucmd->cmd;
> + struct request_queue *q = ns ? ns->queue : ctrl->admin_q;
> + struct nvme_command c;
> + struct request *req;
> + unsigned int rq_flags = 0;
> + blk_mq_req_flags_t blk_flags = 0;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> + if (cmd->flags)
> + return -EINVAL;
> + if (!nvme_validate_passthru_nsid(ctrl, ns, cmd->nsid))
> + return -EINVAL;
> +
> + if (issue_flags & IO_URING_F_NONBLOCK) {
> + rq_flags = REQ_NOWAIT;
> + blk_flags = BLK_MQ_REQ_NOWAIT;
> + }
> + memset(&c, 0, sizeof(c));
> + c.common.opcode = cmd->opcode;
> + c.common.flags = cmd->flags;
> + c.common.nsid = cpu_to_le32(cmd->nsid);
> + c.common.cdw2[0] = cpu_to_le32(cmd->cdw2);
> + c.common.cdw2[1] = cpu_to_le32(cmd->cdw3);
> + c.common.cdw10 = cpu_to_le32(cmd->cdw10);
> + c.common.cdw11 = cpu_to_le32(cmd->cdw11);
> + c.common.cdw12 = cpu_to_le32(cmd->cdw12);
> + c.common.cdw13 = cpu_to_le32(cmd->cdw13);
> + c.common.cdw14 = cpu_to_le32(cmd->cdw14);
> + c.common.cdw15 = cpu_to_le32(cmd->cdw15);
> +
> + req = nvme_alloc_user_request(q, &c, nvme_to_user_ptr(cmd->addr),
> + cmd->data_len, nvme_to_user_ptr(cmd->metadata),
> + cmd->metadata_len, 0, cmd->timeout_ms ?
> + msecs_to_jiffies(cmd->timeout_ms) : 0, 0, rq_flags,
> + blk_flags);

You need to be careful with reading/re-reading the shared memory. For
example, you do:

if (!nvme_validate_passthru_nsid(ctrl, ns, cmd->nsid))
return -EINVAL;

but then later read it again:

c.common.nsid = cpu_to_le32(cmd->nsid);

What happens if this changes in between the validation and assigning it
here? Either this needs to be a single read and validation, or the
validation doesn't really matter. I'd make this:

c.common.opcode = READ_ONCE(cmd->opcode);
c.common.flags = READ_ONCE(cmd->flags);
c.common.nsid = cpu_to_le32(READ_ONCE(cmd->nsid));

if (!nvme_validate_passthru_nsid(ctrl, ns, le32_to_cpu(c.common.nsid)));
return -EINVAL;

c.common.cdw2[0] = cpu_to_le32(READ_ONCE(cmd->cdw2));
c.common.cdw2[1] = cpu_to_le32(READ_ONCE(cmd->cdw3));
c.common.metadata = 0;
memset(&c.common.dptr, 0, sizeof(c.common.dptr));
c.common.cdw10 = cpu_to_le32(READ_ONCE(cmd->cdw10));
c.common.cdw11 = cpu_to_le32(READ_ONCE(cmd->cdw11));
c.common.cdw12 = cpu_to_le32(READ_ONCE(cmd->cdw12));
c.common.cdw13 = cpu_to_le32(READ_ONCE(cmd->cdw13));
c.common.cdw14 = cpu_to_le32(READ_ONCE(cmd->cdw14));
c.common.cdw15 = cpu_to_le32(READ_ONCE(cmd->cdw15));

and then consider the ones passed in to nvme_alloc_user_request() as
well.

--
Jens Axboe