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

[Jens Axboe <axboe@xxxxxxxxx>]

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