* [PATCHv2 0/2] nvme: honor DNR status for authentication @ 2022-08-30 12:49 Hannes Reinecke 2022-08-30 12:49 ` [PATCH 1/2] nvme-auth: retry command if DNR bit is not set Hannes Reinecke 2022-08-30 12:49 ` [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed Hannes Reinecke 0 siblings, 2 replies; 8+ messages in thread From: Hannes Reinecke @ 2022-08-30 12:49 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke Hi all, when sending authentication commands we should be checking the DNR bit in the cqe status to figure out if a command should be retried. So the first patch will evaluate the DNR bit during _nvme_submit_sync_cmd() and initiate a retry, and the second patch will set the DNR bit in the nvme authentication code such that we don't attempt to retry the 'connect' operation. I have opened pull request #100 for blktests to validate this behaviour. As usual, comments and reviews are welcome. Hannes Reinecke (2): nvme-auth: retry command if DNR bit is not set nvme-auth: set the DNR bit if authentication failed drivers/nvme/host/auth.c | 22 +++++++++++----------- drivers/nvme/host/core.c | 20 ++++++++++++++++++-- drivers/nvme/host/fabrics.c | 4 ++-- include/linux/blk-mq.h | 2 ++ 4 files changed, 33 insertions(+), 15 deletions(-) -- 2.35.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] nvme-auth: retry command if DNR bit is not set 2022-08-30 12:49 [PATCHv2 0/2] nvme: honor DNR status for authentication Hannes Reinecke @ 2022-08-30 12:49 ` Hannes Reinecke 2022-09-05 12:05 ` Sagi Grimberg 2022-08-30 12:49 ` [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed Hannes Reinecke 1 sibling, 1 reply; 8+ messages in thread From: Hannes Reinecke @ 2022-08-30 12:49 UTC (permalink / raw) To: Christoph Hellwig Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke, Martin George If the cqe returns a status with the DNR bit not set we should retry the command; otherwise we might incur spurious failures. Reported-by: Martin George <marting@netapp.com> Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/host/auth.c | 2 +- drivers/nvme/host/core.c | 20 ++++++++++++++++++-- include/linux/blk-mq.h | 2 ++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c index c8a6db7c4498..4ca3f7d042f1 100644 --- a/drivers/nvme/host/auth.c +++ b/drivers/nvme/host/auth.c @@ -69,7 +69,7 @@ static int nvme_auth_submit(struct nvme_ctrl *ctrl, int qid, ret = __nvme_submit_sync_cmd(q, &cmd, NULL, data, data_len, qid == 0 ? NVME_QID_ANY : qid, - 0, flags); + 0, BLK_MQ_REQ_RETRY | flags); if (ret > 0) dev_warn(ctrl->device, "qid %d auth_send failed with status %d\n", qid, ret); diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 66ae23e9cb0e..a77f406512eb 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -355,7 +355,8 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req) if (likely(nvme_req(req)->status == 0)) return COMPLETE; - if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED) + if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED && + !(nvme_req(req)->status & NVME_SC_DNR)) return AUTHENTICATE; if (blk_noretry_request(req) || @@ -1037,15 +1038,30 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, if (IS_ERR(req)) return PTR_ERR(req); nvme_init_request(req, cmd); + if (flags & BLK_MQ_REQ_RETRY) + nvme_req(req)->retries = nvme_max_retries; if (buffer && bufflen) { ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL); if (ret) goto out; } - +retry: req->rq_flags |= RQF_QUIET; ret = nvme_execute_rq(req, at_head); + if (ret > 0) { + struct nvme_ctrl *ctrl = nvme_req(req)->ctrl; + + if (ctrl->kas) + ctrl->comp_seen = true; + + switch (nvme_decide_disposition(req)) { + case COMPLETE: + break; + default: + goto retry; + } + } if (result && ret >= 0) *result = nvme_req(req)->result; out: diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index effee1dc715a..acd833f37cd4 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -713,6 +713,8 @@ enum { BLK_MQ_REQ_RESERVED = (__force blk_mq_req_flags_t)(1 << 1), /* set RQF_PM */ BLK_MQ_REQ_PM = (__force blk_mq_req_flags_t)(1 << 2), + /* Retry reserved commands */ + BLK_MQ_REQ_RETRY = (__force blk_mq_req_flags_t)(1 << 3), }; struct request *blk_mq_alloc_request(struct request_queue *q, blk_opf_t opf, -- 2.35.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set 2022-08-30 12:49 ` [PATCH 1/2] nvme-auth: retry command if DNR bit is not set Hannes Reinecke @ 2022-09-05 12:05 ` Sagi Grimberg 2022-09-06 14:16 ` Hannes Reinecke 0 siblings, 1 reply; 8+ messages in thread From: Sagi Grimberg @ 2022-09-05 12:05 UTC (permalink / raw) To: Hannes Reinecke, Christoph Hellwig; +Cc: Keith Busch, linux-nvme, Martin George On 8/30/22 15:49, Hannes Reinecke wrote: > If the cqe returns a status with the DNR bit not set we should > retry the command; otherwise we might incur spurious failures. > > Reported-by: Martin George <marting@netapp.com> > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/nvme/host/auth.c | 2 +- > drivers/nvme/host/core.c | 20 ++++++++++++++++++-- > include/linux/blk-mq.h | 2 ++ > 3 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c > index c8a6db7c4498..4ca3f7d042f1 100644 > --- a/drivers/nvme/host/auth.c > +++ b/drivers/nvme/host/auth.c > @@ -69,7 +69,7 @@ static int nvme_auth_submit(struct nvme_ctrl *ctrl, int qid, > > ret = __nvme_submit_sync_cmd(q, &cmd, NULL, data, data_len, > qid == 0 ? NVME_QID_ANY : qid, > - 0, flags); > + 0, BLK_MQ_REQ_RETRY | flags); Surely there is a local way to have this than to leak a flag to blk-mq... > if (ret > 0) > dev_warn(ctrl->device, > "qid %d auth_send failed with status %d\n", qid, ret); > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 66ae23e9cb0e..a77f406512eb 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -355,7 +355,8 @@ static inline enum nvme_disposition nvme_decide_disposition(struct request *req) > if (likely(nvme_req(req)->status == 0)) > return COMPLETE; > > - if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED) > + if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED && > + !(nvme_req(req)->status & NVME_SC_DNR)) > return AUTHENTICATE; > > if (blk_noretry_request(req) || > @@ -1037,15 +1038,30 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, > if (IS_ERR(req)) > return PTR_ERR(req); > nvme_init_request(req, cmd); > + if (flags & BLK_MQ_REQ_RETRY) > + nvme_req(req)->retries = nvme_max_retries; Maybe just rename this to __nvme_submit_sync_cmd_retries() that accepts retries argument, and then __nvme_submit_sync_cmd becomes: int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, union nvme_result *result, void *buffer, unsigned bufflen, int qid, int at_head, blk_mq_req_flags_t flags) { return __nvme_submit_sync_cmd_retries(q, cmd, result, buffer, bufflen, qid, at_head, flags, 0) } And nvme_auth_submit() can call it with nvme_max_retries... > > if (buffer && bufflen) { > ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL); > if (ret) > goto out; > } > - > +retry: > req->rq_flags |= RQF_QUIET; > ret = nvme_execute_rq(req, at_head); > + if (ret > 0) { > + struct nvme_ctrl *ctrl = nvme_req(req)->ctrl; > + > + if (ctrl->kas) > + ctrl->comp_seen = true; > + > + switch (nvme_decide_disposition(req)) { > + case COMPLETE: > + break; > + default: > + goto retry; > + } > + } Why is this needed? isn't nvme_complete_rq called on this request? > if (result && ret >= 0) > *result = nvme_req(req)->result; > out: > diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h > index effee1dc715a..acd833f37cd4 100644 > --- a/include/linux/blk-mq.h > +++ b/include/linux/blk-mq.h > @@ -713,6 +713,8 @@ enum { > BLK_MQ_REQ_RESERVED = (__force blk_mq_req_flags_t)(1 << 1), > /* set RQF_PM */ > BLK_MQ_REQ_PM = (__force blk_mq_req_flags_t)(1 << 2), > + /* Retry reserved commands */ > + BLK_MQ_REQ_RETRY = (__force blk_mq_req_flags_t)(1 << 3), I don't think this is appropriate for what this is trying to do... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set 2022-09-05 12:05 ` Sagi Grimberg @ 2022-09-06 14:16 ` Hannes Reinecke 2022-09-07 6:44 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Hannes Reinecke @ 2022-09-06 14:16 UTC (permalink / raw) To: Sagi Grimberg, Christoph Hellwig; +Cc: Keith Busch, linux-nvme, Martin George On 9/5/22 14:05, Sagi Grimberg wrote: > > > On 8/30/22 15:49, Hannes Reinecke wrote: >> If the cqe returns a status with the DNR bit not set we should >> retry the command; otherwise we might incur spurious failures. >> >> Reported-by: Martin George <marting@netapp.com> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/nvme/host/auth.c | 2 +- >> drivers/nvme/host/core.c | 20 ++++++++++++++++++-- >> include/linux/blk-mq.h | 2 ++ >> 3 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c >> index c8a6db7c4498..4ca3f7d042f1 100644 >> --- a/drivers/nvme/host/auth.c >> +++ b/drivers/nvme/host/auth.c >> @@ -69,7 +69,7 @@ static int nvme_auth_submit(struct nvme_ctrl *ctrl, >> int qid, >> ret = __nvme_submit_sync_cmd(q, &cmd, NULL, data, data_len, >> qid == 0 ? NVME_QID_ANY : qid, >> - 0, flags); >> + 0, BLK_MQ_REQ_RETRY | flags); > > Surely there is a local way to have this than to leak a flag to > blk-mq... > Sigh. That was the first attempt I did. Which got rejected by Christoph, who wanted to have it generic. Personally I _did_ like the first version better, for precisely the reasons you mentioned. But if the maintainer requests a change, who am I to object ... So what to do now? Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set 2022-09-06 14:16 ` Hannes Reinecke @ 2022-09-07 6:44 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-09-07 6:44 UTC (permalink / raw) To: Hannes Reinecke Cc: Sagi Grimberg, Christoph Hellwig, Keith Busch, linux-nvme, Martin George On Tue, Sep 06, 2022 at 04:16:53PM +0200, Hannes Reinecke wrote: >>> ret = __nvme_submit_sync_cmd(q, &cmd, NULL, data, data_len, >>> qid == 0 ? NVME_QID_ANY : qid, >>> - 0, flags); >>> + 0, BLK_MQ_REQ_RETRY | flags); >> >> Surely there is a local way to have this than to leak a flag to >> blk-mq... >> > > Sigh. That was the first attempt I did. Which got rejected by Christoph, > who wanted to have it generic. I think you and Sagi are talking about difference means of "local". Your original version just retried in the caller. Which is a bad idea. The place where this code retries is the right one, and I think Sagi agrees. Abusing a blk-mq level flag for controlling this retry in core nvme code is not, and I agree with the suggestion form Sagi. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed 2022-08-30 12:49 [PATCHv2 0/2] nvme: honor DNR status for authentication Hannes Reinecke 2022-08-30 12:49 ` [PATCH 1/2] nvme-auth: retry command if DNR bit is not set Hannes Reinecke @ 2022-08-30 12:49 ` Hannes Reinecke 2022-09-05 11:52 ` Sagi Grimberg 2022-09-07 6:46 ` Christoph Hellwig 1 sibling, 2 replies; 8+ messages in thread From: Hannes Reinecke @ 2022-08-30 12:49 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke If authentication failed we should be setting the 'DNR' bit, as each retry will yield exactly the same result; we rather should change the parameters to 'nvme connect' to fixup the situation. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/host/auth.c | 20 ++++++++++---------- drivers/nvme/host/fabrics.c | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c index 4ca3f7d042f1..ad0decc5523f 100644 --- a/drivers/nvme/host/auth.c +++ b/drivers/nvme/host/auth.c @@ -158,7 +158,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl, "qid %d: invalid HASH ID %d\n", chap->qid, data->hashid); chap->status = NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE; - return NVME_SC_INVALID_FIELD; + return NVME_SC_DNR | NVME_SC_INVALID_FIELD; } if (chap->hash_id == data->hashid && chap->shash_tfm && @@ -194,7 +194,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl, crypto_free_shash(chap->shash_tfm); chap->shash_tfm = NULL; chap->status = NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE; - return NVME_SC_AUTH_REQUIRED; + return NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; } /* Reset host response if the hash had been changed */ @@ -216,7 +216,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl, chap->qid, data->dhgid); chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE; /* Leave previous dh_tfm intact */ - return NVME_SC_AUTH_REQUIRED; + return NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; } /* Clear host and controller key to avoid accidental reuse */ @@ -247,7 +247,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl, "qid %d: empty DH value\n", chap->qid); chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE; - return NVME_SC_INVALID_FIELD; + return NVME_SC_DNR | NVME_SC_INVALID_FIELD; } chap->dh_tfm = crypto_alloc_kpp(kpp_name, 0, 0); @@ -268,7 +268,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl, "qid %d: invalid DH value for NULL DH\n", chap->qid); chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD; - return NVME_SC_INVALID_FIELD; + return NVME_SC_DNR | NVME_SC_INVALID_FIELD; } chap->dhgroup_id = data->dhgid; @@ -357,7 +357,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl, "qid %d: invalid hash length %u\n", chap->qid, data->hl); chap->status = NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE; - return NVME_SC_INVALID_FIELD; + return NVME_SC_DNR | NVME_SC_INVALID_FIELD; } /* Just print out information for the admin queue */ @@ -381,7 +381,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl, "qid %d: controller authentication failed\n", chap->qid); chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED; - return NVME_SC_AUTH_REQUIRED; + return NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; } /* Just print out information for the admin queue */ @@ -733,7 +733,7 @@ static void __nvme_auth_work(struct work_struct *work) NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE); if (ret) { chap->status = ret; - chap->error = NVME_SC_AUTH_REQUIRED; + chap->error = NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; return; } @@ -797,7 +797,7 @@ static void __nvme_auth_work(struct work_struct *work) NVME_AUTH_DHCHAP_MESSAGE_SUCCESS1); if (ret) { chap->status = ret; - chap->error = NVME_SC_AUTH_REQUIRED; + chap->error = NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; return; } @@ -815,7 +815,7 @@ static void __nvme_auth_work(struct work_struct *work) ret = nvme_auth_process_dhchap_success1(ctrl, chap); if (ret) { /* Controller authentication failed */ - chap->error = NVME_SC_AUTH_REQUIRED; + chap->error = ret; goto fail2; } diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 83b505358859..633e5fe8e832 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -416,7 +416,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) if (ret) { dev_warn(ctrl->device, "qid 0: authentication setup failed\n"); - ret = NVME_SC_AUTH_REQUIRED; + ret = NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; goto out_free_data; } ret = nvme_auth_wait(ctrl, 0); @@ -492,7 +492,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid) if (ret) { dev_warn(ctrl->device, "qid %d: authentication setup failed\n", qid); - ret = NVME_SC_AUTH_REQUIRED; + ret = NVME_SC_DNR | NVME_SC_AUTH_REQUIRED; } else { ret = nvme_auth_wait(ctrl, qid); if (ret) -- 2.35.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed 2022-08-30 12:49 ` [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed Hannes Reinecke @ 2022-09-05 11:52 ` Sagi Grimberg 2022-09-07 6:46 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Sagi Grimberg @ 2022-09-05 11:52 UTC (permalink / raw) To: Hannes Reinecke, Christoph Hellwig; +Cc: Keith Busch, linux-nvme Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed 2022-08-30 12:49 ` [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed Hannes Reinecke 2022-09-05 11:52 ` Sagi Grimberg @ 2022-09-07 6:46 ` Christoph Hellwig 1 sibling, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2022-09-07 6:46 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, linux-nvme On Tue, Aug 30, 2022 at 02:49:25PM +0200, Hannes Reinecke wrote: > If authentication failed we should be setting the 'DNR' bit, as > each retry will yield exactly the same result; we rather should > change the parameters to 'nvme connect' to fixup the situation. Why is this code using nvme status codes in host codes? These codes only apply to on the wire communication and should never be used for local error reporting. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-07 6:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-30 12:49 [PATCHv2 0/2] nvme: honor DNR status for authentication Hannes Reinecke 2022-08-30 12:49 ` [PATCH 1/2] nvme-auth: retry command if DNR bit is not set Hannes Reinecke 2022-09-05 12:05 ` Sagi Grimberg 2022-09-06 14:16 ` Hannes Reinecke 2022-09-07 6:44 ` Christoph Hellwig 2022-08-30 12:49 ` [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed Hannes Reinecke 2022-09-05 11:52 ` Sagi Grimberg 2022-09-07 6:46 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.