[PATCHv2 0/2] nvme: honor DNR status for authentication

[PATCHv2 0/2] nvme: honor DNR status for authentication

[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

[PATCH 1/2] nvme-auth: retry command if DNR bit is not set

[Hannes Reinecke]

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

[PATCH 2/2] nvme-auth: set the DNR bit if authentication failed

[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

Re: [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set

[Sagi Grimberg]

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...

Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set

[Hannes Reinecke]

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

Re: [PATCH 1/2] nvme-auth: retry command if DNR bit is not set

[Christoph Hellwig]

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.

Re: [PATCH 2/2] nvme-auth: set the DNR bit if authentication failed

[Christoph Hellwig]

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.