nvmet: implement valid sqhd values in completions

PATCH Superseded James Smart 3 participants 5 messages Sep 14, 2017 – Sep 18, 2017

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

James Smart

author

2017-09-14 17:58 UTC #0 lore raw

PATCH v2 1/1 nvmet: implement valid sqhd values in completions

To support sqhd, for initiators that are following the spec and paying attention to sqhd vs their sqtail values:
- add sqhd to struct nvmet_sq
- initialize sqhd to 0 in nvmet_sq_setup
- rather than propagate the 0's-based qsize value from the connect message which requires a +1 in every sqhd update, and as nothing else references it, convert to 1's-based value in nvmt_sq/cq_setup() calls.
- validate connect message sqsize being non-zero per spec.
- updated assign sqhd for every completion that goes back.

Signed-off-by James Smart <james.smart at broadcom.com>

---
v2: assignment corrected for endianness

 drivers/nvme/target/core.c        | 9 +++++----
 drivers/nvme/target/fabrics-cmd.c | 9 +++++++--
 drivers/nvme/target/nvmet.h       | 1 +
 3 files changed, 13 insertions(+), 6 deletions(-)

drivers/nvme/target/core.c +5 -4

index 7c23eaf8e563..764d46698b03 100644
@@ -390,8 +390,8 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
 	if (status)
 		nvmet_set_status(req, status);
 
-	/* XXX: need to fill in something useful for sq_head */
-	req->rsp->sq_head = 0;
+	req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
+	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
 	if (likely(req->sq)) /* may happen during early failure */
 		req->rsp->sq_id = cpu_to_le16(req->sq->qid);
 	req->rsp->command_id = req->cmd->common.command_id;

@@ -412,7 +412,7 @@ void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq,
 		u16 qid, u16 size)
 {
 	cq->qid = qid;
-	cq->size = size;
+	cq->size = size;	/* 1's-based number now ! */
 
 	ctrl->cqs[qid] = cq;
 }

@@ -420,8 +420,9 @@ void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq,
 void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq,
 		u16 qid, u16 size)
 {
+	sq->sqhd = 0;
 	sq->qid = qid;
-	sq->size = size;
+	sq->size = size;	/* 1's-based number now ! */
 
 	ctrl->sqs[qid] = sq;
 }

drivers/nvme/target/fabrics-cmd.c +7 -2

index 859a66725291..db3bf6b8bf9e 100644
@@ -109,9 +109,14 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 		pr_warn("queue already connected!\n");
 		return NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
 	}
+	if (!sqsize) {
+		pr_warn("queue size zero!\n");
+		return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
+	}
 
-	nvmet_cq_setup(ctrl, req->cq, qid, sqsize);
-	nvmet_sq_setup(ctrl, req->sq, qid, sqsize);
+	/* note: convert queue size from 0's-based value to 1's-based value */
+	nvmet_cq_setup(ctrl, req->cq, qid, sqsize + 1);
+	nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1);
 	return 0;
 }

drivers/nvme/target/nvmet.h +1 -0

index 7d261ab894f4..7b8e20adf760 100644
@@ -74,6 +74,7 @@ struct nvmet_sq {
 	struct percpu_ref	ref;
 	u16			qid;
 	u16			size;
+	u16			sqhd;
 	struct completion	free_done;
 	struct completion	confirm_done;
 };

Signature · 1 line

2.13.1

Christoph Hellwig

2017-09-15 15:43 UTC #1 lore raw

PATCH v2 1/1 nvmet: implement valid sqhd values in completions

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>

Max Gurtovoy

2017-09-16 10:04 UTC #2 lore raw

PATCH v2 1/1 nvmet: implement valid sqhd values in completions

In reply to James Smart

Quoted 25 lines · James Smart

To support sqhd, for initiators that are following the spec and paying attention to sqhd vs their sqtail values:
- add sqhd to struct nvmet_sq
- initialize sqhd to 0 in nvmet_sq_setup
- rather than propagate the 0's-based qsize value from the connect message which requires a +1 in every sqhd update, and as nothing else references it, convert to 1's-based value in nvmt_sq/cq_setup() calls.
- validate connect message sqsize being non-zero per spec.
- updated assign sqhd for every completion that goes back.
Signed-off-by James Smart <james.smart at broadcom.com>
---
v2: assignment corrected for endianness
  drivers/nvme/target/core.c        | 9 +++++----
  drivers/nvme/target/fabrics-cmd.c | 9 +++++++--
  drivers/nvme/target/nvmet.h       | 1 +
  3 files changed, 13 insertions(+), 6 deletions(-)
drivers/nvme/target/core.c +0 -0
index 7c23eaf8e563..764d46698b03 100644

drivers/nvme/target/core.c static void __nvmet_req_complete(struct nvmet_req *req, u16 status) · line 397

Quoted 10 lines

drivers/nvme/target/core.c +2 -2

@@ -390,8 +390,8 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
  	if (status)
  		nvmet_set_status(req, status);
 
-	/* XXX: need to fill in something useful for sq_head */
-	req->rsp->sq_head = 0;
+	req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
+	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
  	if (likely(req->sq)) /* may happen during early failure */
  		req->rsp->sq_id = cpu_to_le16(req->sq->qid);
  	req->rsp->command_id = req->cmd->common.command_id;

According to the "if (likely(req->sq)) /* may happen during early failure */" condition, we can get null deref here 2 lines above that...

James Smart

author

2017-09-18 16:00 UTC #3 lore raw

PATCH v2 1/1 nvmet: implement valid sqhd values in completions

Quoted 10 lines · Max Gurtovoy

Quoted 10 lines

drivers/nvme/target/core.c +0 -2

index 7c23eaf8e563..764d46698b03 100644
@@ -390,8 +390,8 @@ static void __nvmet_req_complete(struct nvmet_req 
 *req, u16 status)
 ????? if (status)
 ????????? nvmet_set_status(req, status);
-??? /* XXX: need to fill in something useful for sq_head */
-??? req->rsp->sq_head = 0;

Quoted 8 lines

Quoted 5 lines
drivers/nvme/target/core.c +2 -0
+??? req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
+??? req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
????? if (likely(req->sq)) /* may happen during early failure */ ????????? req->rsp->sq_id = cpu_to_le16(req->sq->qid); ????? req->rsp->command_id = req->cmd->common.command_id;
According to the "if (likely(req->sq)) /* may happen during early failure */" condition, we can get null deref here 2 lines above that...

I think the likely statement is erroneous. In order to get to this point the transport has to call nvmet_req_init(), and the first thing it does is assign the sq and there's never an unset. So any completion of the routine, even if failure to nvmet_req_init() has sq set. There's also several other cases in nvmet_req_init() where if the transport set it to NULL an oops would occur.

I'll send a revised patch that also removes the "if (likely())" test.

-- james

Christoph Hellwig

2017-09-18 16:05 UTC #4 lore raw

PATCH v2 1/1 nvmet: implement valid sqhd values in completions

Quoted 6 lines · James Smart

I think the likely statement is erroneous. In order to get to this point the transport has to call nvmet_req_init(), and the first thing it does is assign the sq and there's never an unset. So any completion of the routine, even if failure to nvmet_req_init() has sq set. There's also several other cases in nvmet_req_init() where if the transport set it to NULL an oops would occur.

That seems to be the case now - this check and comment looks like a leftover from earlier versions where this wasn't the case.

Quoted 1 line

I'll send a revised patch that also removes the "if (likely())" test.

Yes, please do.