From: Hannes Reinecke <hare@kernel.org>

A failure in nvmet_setup_auth() does not mean that the NVMe
authentication command failed, so we should rather return a
protocol error with a 'failure1' response than an NVMe status.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/auth.c             | 18 +++++++-----------
 drivers/nvme/target/fabrics-cmd-auth.c | 11 ++++++-----
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 9afc28f1ffac..1079281a202e 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -131,7 +131,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 	int ret = 0;
 	struct nvmet_host_link *p;
 	struct nvmet_host *host = NULL;
-	const char *hash_name;
 
 	down_read(&nvmet_config_sem);
 	if (nvmet_is_disc_subsys(ctrl->subsys))
@@ -149,13 +148,16 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 	}
 	if (!host) {
 		pr_debug("host %s not found\n", ctrl->hostnqn);
-		ret = -EPERM;
+		ret = NVME_AUTH_DHCHAP_FAILURE_FAILED;
 		goto out_unlock;
 	}
 
 	ret = nvmet_setup_dhgroup(ctrl, host->dhchap_dhgroup_id);
-	if (ret < 0)
+	if (ret < 0) {
 		pr_warn("Failed to setup DH group");
+		ret = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
+		goto out_unlock;
+	}
 
 	if (!host->dhchap_secret) {
 		pr_debug("No authentication provided\n");
@@ -166,12 +168,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 		pr_debug("Re-use existing hash ID %d\n",
 			 ctrl->shash_id);
 	} else {
-		hash_name = nvme_auth_hmac_name(host->dhchap_hash_id);
-		if (!hash_name) {
-			pr_warn("Hash ID %d invalid\n", host->dhchap_hash_id);
-			ret = -EINVAL;
-			goto out_unlock;
-		}
 		ctrl->shash_id = host->dhchap_hash_id;
 	}
 
@@ -180,7 +176,7 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 	ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
 					       host->dhchap_key_hash);
 	if (IS_ERR(ctrl->host_key)) {
-		ret = PTR_ERR(ctrl->host_key);
+		ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
 		ctrl->host_key = NULL;
 		goto out_free_hash;
 	}
@@ -198,7 +194,7 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 	ctrl->ctrl_key = nvme_auth_extract_key(host->dhchap_ctrl_secret + 10,
 					       host->dhchap_ctrl_key_hash);
 	if (IS_ERR(ctrl->ctrl_key)) {
-		ret = PTR_ERR(ctrl->ctrl_key);
+		ret = NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE;
 		ctrl->ctrl_key = NULL;
 		goto out_free_hash;
 	}
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index eb7785be0ca7..a95dc6606396 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -240,12 +240,13 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			pr_debug("%s: ctrl %d qid %d reset negotiation\n", __func__,
 				 ctrl->cntlid, req->sq->qid);
 			if (!req->sq->qid) {
-				if (nvmet_setup_auth(ctrl) < 0) {
-					status = NVME_SC_INTERNAL;
-					pr_err("ctrl %d qid 0 failed to setup"
-					       "re-authentication",
+				status = nvmet_setup_auth(ctrl);
+				if (status) {
+					pr_err("ctrl %d qid 0 failed to setup re-authentication\n",
 					       ctrl->cntlid);
-					goto done_failure1;
+					req->sq->dhchap_status = status;
+					req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_FAILURE1;
+					goto done_kfree;
 				}
 			}
 			req->sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
-- 
2.44.0

On Thu, Apr 04, 2024 at 05:44:58PM +0200, Daniel Wagner wrote:
> From: Hannes Reinecke <hare@kernel.org>
> 
> A failure in nvmet_setup_auth() does not mean that the NVMe
> authentication command failed, so we should rather return a
> protocol error with a 'failure1' response than an NVMe status.

Nit: try to use up the 73 characters available for commit logs, this
looks weirdly, um condensed.

> @@ -131,7 +131,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
>  	int ret = 0;
>  	struct nvmet_host_link *p;
>  	struct nvmet_host *host = NULL;
> -	const char *hash_name;
>  
>  	down_read(&nvmet_config_sem);
>  	if (nvmet_is_disc_subsys(ctrl->subsys))
> @@ -149,13 +148,16 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
>  	}
>  	if (!host) {
>  		pr_debug("host %s not found\n", ctrl->hostnqn);
> -		ret = -EPERM;
> +		ret = NVME_AUTH_DHCHAP_FAILURE_FAILED;
>  		goto out_unlock;

This is now returning returning random on the wire fields that aren't
even the NVMe status codes from a function otherwise returning Linux
errno values.  I can't see how this works or is maintainable long term.

On Fri, Apr 05, 2024 at 08:20:55AM +0200, Christoph Hellwig wrote:
> > @@ -131,7 +131,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
> >  	int ret = 0;
> >  	struct nvmet_host_link *p;
> >  	struct nvmet_host *host = NULL;
> > -	const char *hash_name;
> >  
> >  	down_read(&nvmet_config_sem);
> >  	if (nvmet_is_disc_subsys(ctrl->subsys))
> > @@ -149,13 +148,16 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
> >  	}
> >  	if (!host) {
> >  		pr_debug("host %s not found\n", ctrl->hostnqn);
> > -		ret = -EPERM;
> > +		ret = NVME_AUTH_DHCHAP_FAILURE_FAILED;
> >  		goto out_unlock;
> 
> This is now returning returning random on the wire fields that aren't
> even the NVMe status codes from a function otherwise returning Linux
> errno values.  I can't see how this works or is maintainable long term.

This is the target side and we generate the on wire return code here.
Are you sure I should map this to errno codes and the back to NVME
status codes? Sure, this is possible but don't really think it makes
sense.

On Fri, Apr 05, 2024 at 12:02:51PM +0200, Daniel Wagner wrote:
> > >  	if (!host) {
> > >  		pr_debug("host %s not found\n", ctrl->hostnqn);
> > > -		ret = -EPERM;
> > > +		ret = NVME_AUTH_DHCHAP_FAILURE_FAILED;
> > >  		goto out_unlock;
> > 
> > This is now returning returning random on the wire fields that aren't
> > even the NVMe status codes from a function otherwise returning Linux
> > errno values.  I can't see how this works or is maintainable long term.
> 
> This is the target side and we generate the on wire return code here.

True.

> Are you sure I should map this to errno codes and the back to NVME
> status codes? Sure, this is possible but don't really think it makes
> sense.

No, but we should not overload the return value.  Pass in the req
or sq, or add a new paramter for the auth fail reason.

On Fri, Apr 05, 2024 at 04:47:52PM +0200, Christoph Hellwig wrote:
> > Are you sure I should map this to errno codes and the back to NVME
> > status codes? Sure, this is possible but don't really think it makes
> > sense.
> 
> No, but we should not overload the return value.  Pass in the req
> or sq, or add a new paramter for the auth fail reason.

Or make sure all returns are the status codes, change the return
value to a u8 and clearly document what values it returns if that
works out.

And while you're at it please fix all the overly long lines.

On Fri, Apr 05, 2024 at 12:02:52PM +0200, Daniel Wagner wrote:
> On Fri, Apr 05, 2024 at 08:20:55AM +0200, Christoph Hellwig wrote:
> > > @@ -131,7 +131,6 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
> > >  	int ret = 0;
> > >  	struct nvmet_host_link *p;
> > >  	struct nvmet_host *host = NULL;
> > > -	const char *hash_name;
> > >  
> > >  	down_read(&nvmet_config_sem);
> > >  	if (nvmet_is_disc_subsys(ctrl->subsys))
> > > @@ -149,13 +148,16 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
> > >  	}
> > >  	if (!host) {
> > >  		pr_debug("host %s not found\n", ctrl->hostnqn);
> > > -		ret = -EPERM;
> > > +		ret = NVME_AUTH_DHCHAP_FAILURE_FAILED;
> > >  		goto out_unlock;
> > 
> > This is now returning returning random on the wire fields that aren't
> > even the NVMe status codes from a function otherwise returning Linux
> > errno values.  I can't see how this works or is maintainable long term.
> 
> This is the target side and we generate the on wire return code here.
> Are you sure I should map this to errno codes and the back to NVME
> status codes? Sure, this is possible but don't really think it makes
> sense.

I think I start to understand your feedback. I'll try to address it.