* [PATCH v4 1/5] nvme: authentication error are always non-retryable
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
@ 2024-04-04 15:44 ` Daniel Wagner
2024-04-05 6:16 ` Christoph Hellwig
2024-04-04 15:44 ` [PATCH v4 2/5] nvmet: lock config semaphore when accessing DH-HMAC-CHAP key Daniel Wagner
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:44 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Daniel Wagner
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>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/core.c | 6 +++---
drivers/nvme/host/fabrics.c | 29 +++++++++++++++++------------
drivers/nvme/host/nvme.h | 2 +-
3 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 504dc352c458..66387bcca8ae 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -383,14 +383,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 1f0ea1f32d22..309a69c24995 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,14 +475,16 @@ 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_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
+ "qid 0: authentication failed, error %d\n",
+ ret);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+ } else
dev_info(ctrl->device,
"qid 0: authenticated\n");
}
@@ -542,7 +544,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 */
@@ -550,12 +552,15 @@ 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);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+ goto out_free_data;
+ }
+ ret = nvme_auth_wait(ctrl, qid);
+ if (ret) {
+ dev_warn(ctrl->device,
+ "qid %u: authentication failed, error %d\n",
+ qid, ret);
+ ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
}
}
out_free_data:
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d0ed64dc7380..882967365108 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1122,7 +1122,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.44.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] nvme: authentication error are always non-retryable
2024-04-04 15:44 ` [PATCH v4 1/5] nvme: authentication error are always non-retryable Daniel Wagner
@ 2024-04-05 6:16 ` Christoph Hellwig
2024-04-05 6:45 ` Daniel Wagner
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2024-04-05 6:16 UTC (permalink / raw
To: Daniel Wagner
Cc: James Smart, Keith Busch, Christoph Hellwig, Sagi Grimberg,
Hannes Reinecke, linux-nvme, linux-kernel
> --- 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;
This is still abusing on the wire status code for in-kernel return
codes. Can we please sort this out properly?
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] nvme: authentication error are always non-retryable
2024-04-05 6:16 ` Christoph Hellwig
@ 2024-04-05 6:45 ` Daniel Wagner
2024-04-05 6:49 ` Christoph Hellwig
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Wagner @ 2024-04-05 6:45 UTC (permalink / raw
To: Christoph Hellwig
Cc: James Smart, Keith Busch, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel
On Fri, Apr 05, 2024 at 08:16:24AM +0200, Christoph Hellwig wrote:
> > --- 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;
>
> This is still abusing on the wire status code for in-kernel return
> codes. Can we please sort this out properly?
Okay, though I am not really sure how to do it correctly.
So the current mapping is
ret < 0: kernel errors
ret = 0: all good
ret > 0: wire status incl DNR
In order to split the internal DNR away, we could attach it to the cmd.
Is this what you had in mind? Or do you mean we should not return
NVME_SC_AUTH_REQUIRED at all. Instead just a negative value and update
the error handling on the callers?
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 1/5] nvme: authentication error are always non-retryable
2024-04-05 6:45 ` Daniel Wagner
@ 2024-04-05 6:49 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2024-04-05 6:49 UTC (permalink / raw
To: Daniel Wagner
Cc: Christoph Hellwig, James Smart, Keith Busch, Sagi Grimberg,
Hannes Reinecke, linux-nvme, linux-kernel
On Fri, Apr 05, 2024 at 08:45:11AM +0200, Daniel Wagner wrote:
> > This is still abusing on the wire status code for in-kernel return
> > codes. Can we please sort this out properly?
>
> Okay, though I am not really sure how to do it correctly.
>
> So the current mapping is
>
> ret < 0: kernel errors
> ret = 0: all good
> ret > 0: wire status incl DNR
Yes.
> In order to split the internal DNR away, we could attach it to the cmd.
> Is this what you had in mind? Or do you mean we should not return
> NVME_SC_AUTH_REQUIRED at all. Instead just a negative value and update
> the error handling on the callers?
The latter.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/5] nvmet: lock config semaphore when accessing DH-HMAC-CHAP key
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
2024-04-04 15:44 ` [PATCH v4 1/5] nvme: authentication error are always non-retryable Daniel Wagner
@ 2024-04-04 15:44 ` Daniel Wagner
2024-04-05 6:16 ` Christoph Hellwig
2024-04-04 15:44 ` [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth() Daniel Wagner
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:44 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Hannes Reinecke, Daniel Wagner
From: Hannes Reinecke <hare@kernel.org>
When the DH-HMAC-CHAP key is accessed via configfs we need to take the
config semaphore as a reconnect might be running at the same time.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/target/auth.c | 2 ++
drivers/nvme/target/configfs.c | 22 +++++++++++++++++-----
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 3ddbc3880cac..9afc28f1ffac 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -44,6 +44,7 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
dhchap_secret = kstrdup(secret, GFP_KERNEL);
if (!dhchap_secret)
return -ENOMEM;
+ down_write(&nvmet_config_sem);
if (set_ctrl) {
kfree(host->dhchap_ctrl_secret);
host->dhchap_ctrl_secret = strim(dhchap_secret);
@@ -53,6 +54,7 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
host->dhchap_secret = strim(dhchap_secret);
host->dhchap_key_hash = key_hash;
}
+ up_write(&nvmet_config_sem);
return 0;
}
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 77a6e817b315..7c28b9c0ee57 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1990,11 +1990,17 @@ static struct config_group nvmet_ports_group;
static ssize_t nvmet_host_dhchap_key_show(struct config_item *item,
char *page)
{
- u8 *dhchap_secret = to_host(item)->dhchap_secret;
+ u8 *dhchap_secret;
+ ssize_t ret;
+ down_read(&nvmet_config_sem);
+ dhchap_secret = to_host(item)->dhchap_secret;
if (!dhchap_secret)
- return sprintf(page, "\n");
- return sprintf(page, "%s\n", dhchap_secret);
+ ret = sprintf(page, "\n");
+ else
+ ret = sprintf(page, "%s\n", dhchap_secret);
+ up_read(&nvmet_config_sem);
+ return ret;
}
static ssize_t nvmet_host_dhchap_key_store(struct config_item *item,
@@ -2018,10 +2024,16 @@ static ssize_t nvmet_host_dhchap_ctrl_key_show(struct config_item *item,
char *page)
{
u8 *dhchap_secret = to_host(item)->dhchap_ctrl_secret;
+ ssize_t ret;
+ down_read(&nvmet_config_sem);
+ dhchap_secret = to_host(item)->dhchap_ctrl_secret;
if (!dhchap_secret)
- return sprintf(page, "\n");
- return sprintf(page, "%s\n", dhchap_secret);
+ ret = sprintf(page, "\n");
+ else
+ ret = sprintf(page, "%s\n", dhchap_secret);
+ up_read(&nvmet_config_sem);
+ return ret;
}
static ssize_t nvmet_host_dhchap_ctrl_key_store(struct config_item *item,
--
2.44.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
2024-04-04 15:44 ` [PATCH v4 1/5] nvme: authentication error are always non-retryable Daniel Wagner
2024-04-04 15:44 ` [PATCH v4 2/5] nvmet: lock config semaphore when accessing DH-HMAC-CHAP key Daniel Wagner
@ 2024-04-04 15:44 ` Daniel Wagner
2024-04-05 6:20 ` Christoph Hellwig
2024-04-04 15:44 ` [PATCH v4 4/5] nvme-tcp: short-circuit reconnect retries Daniel Wagner
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:44 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Hannes Reinecke, Daniel Wagner
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-04 15:44 ` [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth() Daniel Wagner
@ 2024-04-05 6:20 ` Christoph Hellwig
2024-04-05 10:02 ` Daniel Wagner
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2024-04-05 6:20 UTC (permalink / raw
To: Daniel Wagner
Cc: James Smart, Keith Busch, Christoph Hellwig, Sagi Grimberg,
Hannes Reinecke, linux-nvme, linux-kernel, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-05 6:20 ` Christoph Hellwig
@ 2024-04-05 10:02 ` Daniel Wagner
2024-04-05 13:24 ` Daniel Wagner
2024-04-05 14:47 ` Christoph Hellwig
0 siblings, 2 replies; 16+ messages in thread
From: Daniel Wagner @ 2024-04-05 10:02 UTC (permalink / raw
To: Christoph Hellwig
Cc: James Smart, Keith Busch, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-05 10:02 ` Daniel Wagner
@ 2024-04-05 13:24 ` Daniel Wagner
2024-04-05 14:47 ` Christoph Hellwig
1 sibling, 0 replies; 16+ messages in thread
From: Daniel Wagner @ 2024-04-05 13:24 UTC (permalink / raw
To: Christoph Hellwig
Cc: James Smart, Keith Busch, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-05 10:02 ` Daniel Wagner
2024-04-05 13:24 ` Daniel Wagner
@ 2024-04-05 14:47 ` Christoph Hellwig
2024-04-05 14:50 ` Christoph Hellwig
1 sibling, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2024-04-05 14:47 UTC (permalink / raw
To: Daniel Wagner
Cc: Christoph Hellwig, James Smart, Keith Busch, Sagi Grimberg,
Hannes Reinecke, linux-nvme, linux-kernel, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth()
2024-04-05 14:47 ` Christoph Hellwig
@ 2024-04-05 14:50 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2024-04-05 14:50 UTC (permalink / raw
To: Daniel Wagner
Cc: Christoph Hellwig, James Smart, Keith Busch, Sagi Grimberg,
Hannes Reinecke, linux-nvme, linux-kernel, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/5] nvme-tcp: short-circuit reconnect retries
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
` (2 preceding siblings ...)
2024-04-04 15:44 ` [PATCH v4 3/5] nvmet: return DHCHAP status codes from nvmet_setup_auth() Daniel Wagner
@ 2024-04-04 15:44 ` Daniel Wagner
2024-04-04 15:45 ` [PATCH v4 5/5] nvme-rdma: " Daniel Wagner
2024-04-04 15:50 ` [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
5 siblings, 0 replies; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:44 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Chaitanya Kulkarni, Daniel Wagner
From: Hannes Reinecke <hare@suse.de>
Returning an nvme status from nvme_tcp_setup_ctrl() indicates that the
association was established and we have received a status from the
controller; consequently we should honour the DNR bit. If not any future
reconnect attempts will just return the same error, so we can
short-circuit the reconnect attempts and fail the connection directly.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/nvme.h | 17 +++++++++++++++++
drivers/nvme/host/tcp.c | 23 +++++++++++++++--------
2 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 882967365108..654d8d1a4d29 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -701,6 +701,23 @@ static inline bool nvme_is_path_error(u16 status)
return (status & 0x700) == 0x300;
}
+/*
+ * Evaluate the status information returned by the LLDD in order to
+ * decided if a reconnect attempt should be scheduled.
+ *
+ * There are two cases where no reconnect attempt should be attempted:
+ *
+ * 1) The LLDD reports an negative status. There was an error (e.g. no
+ * memory) on the host side and thus abort the operation.
+ * 2) The DNR bit is set and the specification states no further
+ * connect attempts with the same set of paramenters should be
+ * attempted.
+ */
+static inline bool nvme_ctrl_reconnect(int status)
+{
+ return status > 0 && (status & NVME_SC_DNR) ? false : true;
+}
+
/*
* Fill in the status and result information from the CQE, and then figure out
* if blk-mq will need to use IPI magic to complete the request, and if yes do
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index fdbcdcedcee9..7e25a96e9870 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2155,9 +2155,11 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
nvme_tcp_destroy_io_queues(ctrl, remove);
}
-static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
+static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl,
+ int status)
{
enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
+ bool recon = nvme_ctrl_reconnect(status);
/* If we are resetting/deleting then do nothing */
if (state != NVME_CTRL_CONNECTING) {
@@ -2165,13 +2167,14 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
return;
}
- if (nvmf_should_reconnect(ctrl)) {
+ if (recon && nvmf_should_reconnect(ctrl)) {
dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
ctrl->opts->reconnect_delay);
queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work,
ctrl->opts->reconnect_delay * HZ);
} else {
- dev_info(ctrl->device, "Removing controller...\n");
+ dev_info(ctrl->device, "Removing controller (%d)...\n",
+ status);
nvme_delete_ctrl(ctrl);
}
}
@@ -2252,10 +2255,12 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work),
struct nvme_tcp_ctrl, connect_work);
struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
+ int ret;
++ctrl->nr_reconnects;
- if (nvme_tcp_setup_ctrl(ctrl, false))
+ ret = nvme_tcp_setup_ctrl(ctrl, false);
+ if (ret)
goto requeue;
dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
@@ -2268,7 +2273,7 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
requeue:
dev_info(ctrl->device, "Failed reconnect attempt %d\n",
ctrl->nr_reconnects);
- nvme_tcp_reconnect_or_remove(ctrl);
+ nvme_tcp_reconnect_or_remove(ctrl, ret);
}
static void nvme_tcp_error_recovery_work(struct work_struct *work)
@@ -2295,7 +2300,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
return;
}
- nvme_tcp_reconnect_or_remove(ctrl);
+ nvme_tcp_reconnect_or_remove(ctrl, -ENOTCONN);
}
static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
@@ -2315,6 +2320,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
{
struct nvme_ctrl *ctrl =
container_of(work, struct nvme_ctrl, reset_work);
+ int ret;
nvme_stop_ctrl(ctrl);
nvme_tcp_teardown_ctrl(ctrl, false);
@@ -2328,14 +2334,15 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
return;
}
- if (nvme_tcp_setup_ctrl(ctrl, false))
+ ret = nvme_tcp_setup_ctrl(ctrl, false);
+ if (ret)
goto out_fail;
return;
out_fail:
++ctrl->nr_reconnects;
- nvme_tcp_reconnect_or_remove(ctrl);
+ nvme_tcp_reconnect_or_remove(ctrl, ret);
}
static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
--
2.44.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v4 5/5] nvme-rdma: short-circuit reconnect retries
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
` (3 preceding siblings ...)
2024-04-04 15:44 ` [PATCH v4 4/5] nvme-tcp: short-circuit reconnect retries Daniel Wagner
@ 2024-04-04 15:45 ` Daniel Wagner
2024-04-04 15:50 ` [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
5 siblings, 0 replies; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:45 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel, Chaitanya Kulkarni, Daniel Wagner
From: Hannes Reinecke <hare@suse.de>
Returning an nvme status from nvme_rdma_setup_ctrl() indicates that the
association was established and we have received a status from the
controller; consequently we should honour the DNR bit. If not any future
reconnect attempts will just return the same error, so we can
short-circuit the reconnect attempts and fail the connection directly.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/rdma.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 366f0bb4ebfc..53d51df26ae1 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -982,9 +982,11 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
kfree(ctrl);
}
-static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
+static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl,
+ int status)
{
enum nvme_ctrl_state state = nvme_ctrl_state(&ctrl->ctrl);
+ bool recon = nvme_ctrl_reconnect(status);
/* If we are resetting/deleting then do nothing */
if (state != NVME_CTRL_CONNECTING) {
@@ -992,12 +994,14 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
return;
}
- if (nvmf_should_reconnect(&ctrl->ctrl)) {
+ if (recon && nvmf_should_reconnect(&ctrl->ctrl)) {
dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
ctrl->ctrl.opts->reconnect_delay);
queue_delayed_work(nvme_wq, &ctrl->reconnect_work,
ctrl->ctrl.opts->reconnect_delay * HZ);
} else {
+ dev_info(ctrl->ctrl.device, "Removing controller (%d)...\n",
+ status);
nvme_delete_ctrl(&ctrl->ctrl);
}
}
@@ -1104,10 +1108,12 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
{
struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
struct nvme_rdma_ctrl, reconnect_work);
+ int ret;
++ctrl->ctrl.nr_reconnects;
- if (nvme_rdma_setup_ctrl(ctrl, false))
+ ret = nvme_rdma_setup_ctrl(ctrl, false);
+ if (ret)
goto requeue;
dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attempts)\n",
@@ -1120,7 +1126,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
requeue:
dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
ctrl->ctrl.nr_reconnects);
- nvme_rdma_reconnect_or_remove(ctrl);
+ nvme_rdma_reconnect_or_remove(ctrl, ret);
}
static void nvme_rdma_error_recovery_work(struct work_struct *work)
@@ -1145,7 +1151,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
return;
}
- nvme_rdma_reconnect_or_remove(ctrl);
+ nvme_rdma_reconnect_or_remove(ctrl, -ENOTCONN);
}
static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
@@ -2169,6 +2175,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
{
struct nvme_rdma_ctrl *ctrl =
container_of(work, struct nvme_rdma_ctrl, ctrl.reset_work);
+ int ret;
nvme_stop_ctrl(&ctrl->ctrl);
nvme_rdma_shutdown_ctrl(ctrl, false);
@@ -2179,14 +2186,15 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
return;
}
- if (nvme_rdma_setup_ctrl(ctrl, false))
+ ret = nvme_rdma_setup_ctrl(ctrl, false);
+ if (ret)
goto out_fail;
return;
out_fail:
++ctrl->ctrl.nr_reconnects;
- nvme_rdma_reconnect_or_remove(ctrl);
+ nvme_rdma_reconnect_or_remove(ctrl, ret);
}
static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
--
2.44.0
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries
2024-04-04 15:44 [PATCH v4 0/5] nvme-fabrics: short-circuit connect retries Daniel Wagner
` (4 preceding siblings ...)
2024-04-04 15:45 ` [PATCH v4 5/5] nvme-rdma: " Daniel Wagner
@ 2024-04-04 15:50 ` Daniel Wagner
5 siblings, 0 replies; 16+ messages in thread
From: Daniel Wagner @ 2024-04-04 15:50 UTC (permalink / raw
To: James Smart
Cc: Keith Busch, Christoph Hellwig, Sagi Grimberg, Hannes Reinecke,
linux-nvme, linux-kernel
On Thu, Apr 04, 2024 at 05:44:55PM +0200, Daniel Wagner wrote:
> changes:
> v4:
> - rebased
> - added 'nvme: fixes for authentication errors' series
> https://lore.kernel.org/linux-nvme/20240301112823.132570-1-hare@kernel.org/
Please ignore v4 for now. I've forgot to update 'nvme: fixes for
authentication errors' series.
^ permalink raw reply [flat|nested] 16+ messages in thread