[PATCH] nvme: authentication error are always non-retryable

[PATCH] nvme: authentication error are always non-retryable

[hare]

From: Hannes Reinecke <hare@suse.de>

Any authentication errors which are generated internally are always
non-retryable, so set the DNR bit to ensure they are not retried.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c    |  6 +++---
 drivers/nvme/host/fabrics.c | 16 +++++++---------
 drivers/nvme/host/nvme.h    |  2 +-
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 5e9a0cf43636..0a8a595070c0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -374,14 +374,14 @@ 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)
-		return AUTHENTICATE;
-
 	if (blk_noretry_request(req) ||
 	    (nvme_req(req)->status & NVME_SC_DNR) ||
 	    nvme_req(req)->retries >= nvme_max_retries)
 		return COMPLETE;
 
+	if ((nvme_req(req)->status & 0x7ff) == NVME_SC_AUTH_REQUIRED)
+		return AUTHENTICATE;
+
 	if (req->cmd_flags & REQ_NVME_MPATH) {
 		if (nvme_is_path_error(nvme_req(req)->status) ||
 		    blk_queue_dying(req->q))
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index ab5ac219b70a..2b66fa5cd43f 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -467,7 +467,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
 			dev_warn(ctrl->device,
 				 "qid 0: secure concatenation is not supported\n");
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
 		/* Authentication required */
@@ -475,7 +475,6 @@ 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;
 			goto out_free_data;
 		}
 		ret = nvme_auth_wait(ctrl, 0);
@@ -541,7 +540,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
 			dev_warn(ctrl->device,
 				 "qid 0: secure concatenation is not supported\n");
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
 		/* Authentication required */
@@ -549,13 +548,12 @@ 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;
-		} else {
-			ret = nvme_auth_wait(ctrl, qid);
-			if (ret)
-				dev_warn(ctrl->device,
-					 "qid %u: authentication failed\n", qid);
+			goto out_free_data;
 		}
+		ret = nvme_auth_wait(ctrl, qid);
+		if (ret)
+			dev_warn(ctrl->device,
+				 "qid %u: authentication failed\n", qid);
 	}
 out_free_data:
 	kfree(data);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6ed2ca6b35e4..c08c220a68cf 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1123,7 +1123,7 @@ static inline int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
 }
 static inline int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
 {
-	return NVME_SC_AUTH_REQUIRED;
+	return NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 }
 static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
 #endif
-- 
2.35.3

Re: [PATCH] nvme: authentication error are always non-retryable

[Daniel Wagner]

On Fri, Feb 23, 2024 at 11:58:04AM +0100, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Any authentication errors which are generated internally are always
> non-retryable, so set the DNR bit to ensure they are not retried.

This still keeps nvme-fc in the retry loop. It fails with -126 which is
-ENOKEY:

[ 2267.185691] nvme nvme0: qid 0: no key
[ 2267.187596] nvme nvme0: qid 0: authentication setup failed
[ 2267.188866] nvme nvme0: NVME-FC{0}: create_assoc failed, assoc_id 2a3022dff0fe0000 ret -126

Re: [PATCH] nvme: authentication error are always non-retryable

[Daniel Wagner]

On Fri, Feb 23, 2024 at 02:29:02PM +0100, Daniel Wagner wrote:
> On Fri, Feb 23, 2024 at 11:58:04AM +0100, hare@kernel.org wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > Any authentication errors which are generated internally are always
> > non-retryable, so set the DNR bit to ensure they are not retried.
> 
> This still keeps nvme-fc in the retry loop. It fails with -126 which is
> -ENOKEY:
> 
> [ 2267.185691] nvme nvme0: qid 0: no key
> [ 2267.187596] nvme nvme0: qid 0: authentication setup failed
> [ 2267.188866] nvme nvme0: NVME-FC{0}: create_assoc failed, assoc_id 2a3022dff0fe0000 ret -126
> 

nvme_auth_negotiate() returns -ENOKEY. What's the idea to handle
negative return values? I though nvme_fc_reconnect_or_delete() should
stop the retry loop for for negative values too?

Re: [PATCH] nvme: authentication error are always non-retryable

[Hannes Reinecke]

On 2/23/24 14:29, Daniel Wagner wrote:
> On Fri, Feb 23, 2024 at 11:58:04AM +0100, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Any authentication errors which are generated internally are always
>> non-retryable, so set the DNR bit to ensure they are not retried.
> 
> This still keeps nvme-fc in the retry loop. It fails with -126 which is
> -ENOKEY:
> 
> [ 2267.185691] nvme nvme0: qid 0: no key
> [ 2267.187596] nvme nvme0: qid 0: authentication setup failed
> [ 2267.188866] nvme nvme0: NVME-FC{0}: create_assoc failed, assoc_id 2a3022dff0fe0000 ret -126
> 
Aw. Bugger.
Of course we do retry, as negative transport errors are considered 
retryable. Curiously enough, negative _NVMe_ errors are considered 
non-retryable. Hmph. We may want to work on that.

Cheers,

Hannes

Re: [PATCH] nvme: authentication error are always non-retryable

[Hannes Reinecke]

On 2/26/24 08:44, Hannes Reinecke wrote:
> On 2/23/24 14:29, Daniel Wagner wrote:
>> On Fri, Feb 23, 2024 at 11:58:04AM +0100, hare@kernel.org wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> Any authentication errors which are generated internally are always
>>> non-retryable, so set the DNR bit to ensure they are not retried.
>>
>> This still keeps nvme-fc in the retry loop. It fails with -126 which is
>> -ENOKEY:
>>
>> [ 2267.185691] nvme nvme0: qid 0: no key
>> [ 2267.187596] nvme nvme0: qid 0: authentication setup failed
>> [ 2267.188866] nvme nvme0: NVME-FC{0}: create_assoc failed, assoc_id 
>> 2a3022dff0fe0000 ret -126
>>
> Aw. Bugger.
> Of course we do retry, as negative transport errors are considered 
> retryable. Curiously enough, negative _NVMe_ errors are considered 
> non-retryable. Hmph. We may want to work on that.
> 
So we'll need this one on top:

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 2b66fa5cd43f..2b1bac54010c 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -475,13 +475,15 @@ 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 | NVME_SC_DNR;
                         goto out_free_data;
                 }
                 ret = nvme_auth_wait(ctrl, 0);
-               if (ret)
+               if (ret) {
                         dev_warn(ctrl->device,
                                  "qid 0: authentication failed\n");
-               else
+                       ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+               } else
                         dev_info(ctrl->device,
                                  "qid 0: authenticated\n");
         }

I'll send a proper patch.

Cheers,

Hannes

Re: Re: [PATCH] nvme: authentication error are always non-retryable

[Daniel Wagner]

On Mon, Feb 26, 2024 at 08:49:50AM +0100, Hannes Reinecke wrote:
> I'll send a proper patch.

Yes, this does the trick:

 nvme nvme0: qid 0: no key
 nvme nvme0: qid 0: authentication setup failed
 nvme nvme0: NVME-FC{0}: create_assoc failed, assoc_id 18da4ebfd6fa0000 ret 16785
 nvme nvme0: NVME-FC{0}: reset: Reconnect attempt failed (16785)
 nvme nvme0: NVME-FC{0}: reconnect failure
 nvme nvme0: Removing ctrl: NQN "blktests-subsystem-1"
 (NULL device *): {0:0} Association deleted