[PATCH 0/2] nvme: restrict authentication to the admin queue

[PATCH 0/2] nvme: restrict authentication to the admin queue

[Hannes Reinecke]

Hi all,

it might be an idea to restrict authentication to the admin queue only,
as authentication on I/O queues brings only so much additional benefit,
and the entire authentication is noticeably faster.
This patchsets enables the host to handle this situation, and adds a
new configfs entry 'dhchap_admin_only' for the target to enable this
behaviour.
Patches are on top of my 'secure-concat.v5' branch on kernel.org.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  nvme: Do not re-authenticate queues with no prior authentication
  nvmet: Implement 'admin_only' authentication

 drivers/nvme/host/auth.c               | 17 ++++++++++++++++-
 drivers/nvme/target/auth.c             | 11 +++++++----
 drivers/nvme/target/configfs.c         | 24 ++++++++++++++++++++++++
 drivers/nvme/target/fabrics-cmd-auth.c |  7 +++++++
 drivers/nvme/target/fabrics-cmd.c      |  7 +++++--
 drivers/nvme/target/nvmet.h            |  2 ++
 6 files changed, 61 insertions(+), 7 deletions(-)

-- 
2.35.3

[PATCH 1/2] nvme: Do not re-authenticate queues with no prior authentication

[Hannes Reinecke]

When sending 'connect' the queues can figure out whether authentication
is required. But reauthentication doesn't disconnect the queues, so this
check is not available. Rather we need to check whether the queue had
been authenticated initially to figure out if we need to reauthenticate.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/auth.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 23ff3df27e80..66828742586c 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -31,6 +31,7 @@ struct nvme_dhchap_queue_context {
 	u32 s1;
 	u32 s2;
 	bool bi_directional;
+	bool authenticated;
 	u16 transaction;
 	u8 status;
 	u8 dhgroup_id;
@@ -915,6 +916,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		goto fail2;
 	if (chap->qid || !ctrl->opts->concat) {
 		chap->error = 0;
+		chap->authenticated = true;
 		return;
 	}
 	ret = nvme_auth_secure_concat(ctrl, chap);
@@ -923,8 +925,10 @@ static void nvme_queue_auth_work(struct work_struct *work)
 			 "%s: qid %d failed to enable secure concatenation\n",
 			 __func__, chap->qid);
 		chap->error = ret;
-	} else
+	} else {
 		chap->error = 0;
+		chap->authenticated = true;
+	}
 	return;
 
 fail2:
@@ -940,6 +944,8 @@ static void nvme_queue_auth_work(struct work_struct *work)
 	 */
 	if (ret && !chap->error)
 		chap->error = ret;
+	if (chap->error)
+		chap->authenticated = false;
 }
 
 int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
@@ -1011,6 +1017,14 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 		return;
 
 	for (q = 1; q < ctrl->queue_count; q++) {
+		/*
+		 * Skip re-authentication if the queue had
+		 * not been authenticated initially.
+		 */
+		if (!ctrl->dhchap_ctxs[q].authenticated) {
+			ctrl->dhchap_ctxs[q].error = 0;
+			continue;
+		}
 		ret = nvme_auth_negotiate(ctrl, q);
 		if (ret) {
 			dev_warn(ctrl->device,
@@ -1064,6 +1078,7 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
 		chap = &ctrl->dhchap_ctxs[i];
 		chap->qid = i;
 		chap->ctrl = ctrl;
+		chap->authenticated = false;
 		INIT_WORK(&chap->auth_work, nvme_queue_auth_work);
 	}
 
-- 
2.35.3

[PATCH 2/2] nvmet: Implement 'admin_only' authentication

[Hannes Reinecke]

It might be sufficient to run authentication on the admin queue only,
as this speeds up reconnection quite significantly. So add a configfs
attribute 'dhchap_admin_only' for the 'host' configfs entry to enable
this mode.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/auth.c             | 11 +++++++----
 drivers/nvme/target/configfs.c         | 24 ++++++++++++++++++++++++
 drivers/nvme/target/fabrics-cmd-auth.c |  7 +++++++
 drivers/nvme/target/fabrics-cmd.c      |  7 +++++--
 drivers/nvme/target/nvmet.h            |  2 ++
 5 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 560321df5bf6..d5aae8d8cc92 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -189,6 +189,8 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 		ctrl->shash_id = host->dhchap_hash_id;
 	}
 
+	ctrl->dh_admin_only = host->dhchap_admin_only;
+
 	/* Skip the 'DHHC-1:XX:' prefix */
 	nvme_auth_free_key(ctrl->host_key);
 	ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
@@ -279,10 +281,11 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
 
 bool nvmet_check_auth_status(struct nvmet_req *req)
 {
-	if (req->sq->ctrl->host_key &&
-	    !req->sq->authenticated)
-		return false;
-	return true;
+	if (!req->sq->ctrl->host_key)
+		return true;
+	if (req->sq->qid && req->sq->ctrl->dh_admin_only)
+		return true;
+	return req->sq->authenticated;
 }
 
 int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index bd87dfd173a4..807b0904ea88 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -2134,11 +2134,34 @@ static ssize_t nvmet_host_dhchap_dhgroup_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_host_, dhchap_dhgroup);
 
+static ssize_t nvmet_host_dhchap_admin_only_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_host *host = to_host(item);
+
+	return sprintf(page, "%d\n", host->dhchap_admin_only);
+}
+
+static ssize_t nvmet_host_dhchap_admin_only_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_host *host = to_host(item);
+	bool val;
+
+	if (kstrtobool(page, &val))
+		return -EINVAL;
+	host->dhchap_admin_only = val;
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_host_, dhchap_admin_only);
+
 static struct configfs_attribute *nvmet_host_attrs[] = {
 	&nvmet_host_attr_dhchap_key,
 	&nvmet_host_attr_dhchap_ctrl_key,
 	&nvmet_host_attr_dhchap_hash,
 	&nvmet_host_attr_dhchap_dhgroup,
+	&nvmet_host_attr_dhchap_admin_only,
 	NULL,
 };
 #endif /* CONFIG_NVME_TARGET_AUTH */
@@ -2178,6 +2201,7 @@ static struct config_group *nvmet_hosts_make_group(struct config_group *group,
 #ifdef CONFIG_NVME_TARGET_AUTH
 	/* Default to SHA256 */
 	host->dhchap_hash_id = NVME_AUTH_HASH_SHA256;
+	host->dhchap_admin_only = false;
 #endif
 
 	config_group_init_type_name(&host->group, name, &nvmet_host_type);
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 4c392488c451..4af10a78ce69 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -62,6 +62,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
 		}
 		ctrl->concat = true;
+		ctrl->dh_admin_only = true;
 	}
 
 	if (data->napd != 1)
@@ -248,6 +249,12 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			offsetof(struct nvmf_auth_send_command, tl);
 		goto done;
 	}
+	if (req->sq->qid && ctrl->dh_admin_only) {
+		pr_debug("%s: ctrl %d qid %d reject authentication on I/O queues\n",
+			 __func__, ctrl->cntlid, req->sq->qid);
+		status = NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
+		goto done;
+	}
 	if (!nvmet_check_transfer_len(req, tl)) {
 		pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
 		return;
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 1aabf55ef0a8..212fe271c8a4 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -203,8 +203,11 @@ static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 {
 	bool needs_auth = nvmet_has_auth(ctrl, req);
 
-	/* Do not authenticate I/O queues for secure concatenation */
-	if (ctrl->concat && req->sq->qid)
+	/*
+	 * Do not request authentication for I/O queues for secure concatenation
+	 * or when only the admin queue should be authenticated.
+	 */
+	if (req->sq->qid && (ctrl->concat || ctrl->dh_admin_only))
 		needs_auth = false;
 
 	pr_debug("%s: ctrl %d qid %d should %sauthenticate, tls psk %08x\n",
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 9486b43ab822..129b1cb8a4d1 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -249,6 +249,7 @@ struct nvmet_ctrl {
 	u8			dh_gid;
 	u8			*dh_key;
 	size_t			dh_keysize;
+	bool			dh_admin_only;
 #endif
 #ifdef CONFIG_NVME_TARGET_TCP_TLS
 	struct key		*tls_key;
@@ -325,6 +326,7 @@ struct nvmet_host {
 	u8			dhchap_ctrl_key_hash;
 	u8			dhchap_hash_id;
 	u8			dhchap_dhgroup_id;
+	bool			dhchap_admin_only;
 };
 
 static inline struct nvmet_host *to_host(struct config_item *item)
-- 
2.35.3

Re: [PATCH 2/2] nvmet: Implement 'admin_only' authentication

[Sagi Grimberg]

On 17/07/2024 14:03, Hannes Reinecke wrote:
> It might be sufficient to run authentication on the admin queue only,
> as this speeds up reconnection quite significantly.

By how much?

>   So add a configfs
> attribute 'dhchap_admin_only' for the 'host' configfs entry to enable
> this mode.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
>   drivers/nvme/target/auth.c             | 11 +++++++----
>   drivers/nvme/target/configfs.c         | 24 ++++++++++++++++++++++++
>   drivers/nvme/target/fabrics-cmd-auth.c |  7 +++++++
>   drivers/nvme/target/fabrics-cmd.c      |  7 +++++--
>   drivers/nvme/target/nvmet.h            |  2 ++
>   5 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index 560321df5bf6..d5aae8d8cc92 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -189,6 +189,8 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>   		ctrl->shash_id = host->dhchap_hash_id;
>   	}
>   
> +	ctrl->dh_admin_only = host->dhchap_admin_only;
> +
>   	/* Skip the 'DHHC-1:XX:' prefix */
>   	nvme_auth_free_key(ctrl->host_key);
>   	ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
> @@ -279,10 +281,11 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
>   
>   bool nvmet_check_auth_status(struct nvmet_req *req)
>   {
> -	if (req->sq->ctrl->host_key &&
> -	    !req->sq->authenticated)
> -		return false;
> -	return true;
> +	if (!req->sq->ctrl->host_key)
> +		return true;
> +	if (req->sq->qid && req->sq->ctrl->dh_admin_only)
> +		return true;
> +	return req->sq->authenticated;
>   }
>   
>   int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index bd87dfd173a4..807b0904ea88 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -2134,11 +2134,34 @@ static ssize_t nvmet_host_dhchap_dhgroup_store(struct config_item *item,
>   
>   CONFIGFS_ATTR(nvmet_host_, dhchap_dhgroup);
>   
> +static ssize_t nvmet_host_dhchap_admin_only_show(struct config_item *item,
> +		char *page)
> +{
> +	struct nvmet_host *host = to_host(item);
> +
> +	return sprintf(page, "%d\n", host->dhchap_admin_only);
> +}
> +
> +static ssize_t nvmet_host_dhchap_admin_only_store(struct config_item *item,
> +		const char *page, size_t count)
> +{
> +	struct nvmet_host *host = to_host(item);
> +	bool val;
> +
> +	if (kstrtobool(page, &val))
> +		return -EINVAL;
> +	host->dhchap_admin_only = val;
> +	return count;
> +}
> +
> +CONFIGFS_ATTR(nvmet_host_, dhchap_admin_only);
> +
>   static struct configfs_attribute *nvmet_host_attrs[] = {
>   	&nvmet_host_attr_dhchap_key,
>   	&nvmet_host_attr_dhchap_ctrl_key,
>   	&nvmet_host_attr_dhchap_hash,
>   	&nvmet_host_attr_dhchap_dhgroup,
> +	&nvmet_host_attr_dhchap_admin_only,
>   	NULL,
>   };
>   #endif /* CONFIG_NVME_TARGET_AUTH */
> @@ -2178,6 +2201,7 @@ static struct config_group *nvmet_hosts_make_group(struct config_group *group,
>   #ifdef CONFIG_NVME_TARGET_AUTH
>   	/* Default to SHA256 */
>   	host->dhchap_hash_id = NVME_AUTH_HASH_SHA256;
> +	host->dhchap_admin_only = false;
>   #endif
>   
>   	config_group_init_type_name(&host->group, name, &nvmet_host_type);
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index 4c392488c451..4af10a78ce69 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -62,6 +62,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
>   			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
>   		}
>   		ctrl->concat = true;
> +		ctrl->dh_admin_only = true;
>   	}
>   
>   	if (data->napd != 1)
> @@ -248,6 +249,12 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
>   			offsetof(struct nvmf_auth_send_command, tl);
>   		goto done;
>   	}
> +	if (req->sq->qid && ctrl->dh_admin_only) {
> +		pr_debug("%s: ctrl %d qid %d reject authentication on I/O queues\n",
> +			 __func__, ctrl->cntlid, req->sq->qid);
> +		status = NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
> +		goto done;
> +	}
>   	if (!nvmet_check_transfer_len(req, tl)) {
>   		pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
>   		return;
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 1aabf55ef0a8..212fe271c8a4 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -203,8 +203,11 @@ static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>   {
>   	bool needs_auth = nvmet_has_auth(ctrl, req);
>   
> -	/* Do not authenticate I/O queues for secure concatenation */
> -	if (ctrl->concat && req->sq->qid)
> +	/*
> +	 * Do not request authentication for I/O queues for secure concatenation
> +	 * or when only the admin queue should be authenticated.
> +	 */
> +	if (req->sq->qid && (ctrl->concat || ctrl->dh_admin_only))
>   		needs_auth = false;
>   
>   	pr_debug("%s: ctrl %d qid %d should %sauthenticate, tls psk %08x\n",
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 9486b43ab822..129b1cb8a4d1 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -249,6 +249,7 @@ struct nvmet_ctrl {
>   	u8			dh_gid;
>   	u8			*dh_key;
>   	size_t			dh_keysize;
> +	bool			dh_admin_only;
>   #endif
>   #ifdef CONFIG_NVME_TARGET_TCP_TLS
>   	struct key		*tls_key;
> @@ -325,6 +326,7 @@ struct nvmet_host {
>   	u8			dhchap_ctrl_key_hash;
>   	u8			dhchap_hash_id;
>   	u8			dhchap_dhgroup_id;
> +	bool			dhchap_admin_only;
>   };
>   
>   static inline struct nvmet_host *to_host(struct config_item *item)

Re: [PATCH 2/2] nvmet: Implement 'admin_only' authentication

[Hannes Reinecke]

On 7/18/24 00:40, Sagi Grimberg wrote:
> 
> 
> On 17/07/2024 14:03, Hannes Reinecke wrote:
>> It might be sufficient to run authentication on the admin queue only,
>> as this speeds up reconnection quite significantly.
> 
> By how much?
> 
Haven't really measured; the prime motivation for this patch was to
have a testbed for the patch to the initiator.
But I should be able to get some numbers.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/2] nvmet: Implement 'admin_only' authentication

[Sagi Grimberg]

On 18/07/2024 10:38, Hannes Reinecke wrote:
> On 7/18/24 00:40, Sagi Grimberg wrote:
>>
>>
>> On 17/07/2024 14:03, Hannes Reinecke wrote:
>>> It might be sufficient to run authentication on the admin queue only,
>>> as this speeds up reconnection quite significantly.
>>
>> By how much?
>>
> Haven't really measured; the prime motivation for this patch was to
> have a testbed for the patch to the initiator.
> But I should be able to get some numbers.

Cool, I agree that we want to have this.

Re: [PATCH 0/2] nvme: restrict authentication to the admin queue

[Hannes Reinecke]

On 7/17/24 13:03, Hannes Reinecke wrote:
> Hi all,
> 
> it might be an idea to restrict authentication to the admin queue only,
> as authentication on I/O queues brings only so much additional benefit,
> and the entire authentication is noticeably faster.
> This patchsets enables the host to handle this situation, and adds a
> new configfs entry 'dhchap_admin_only' for the target to enable this
> behaviour.
> Patches are on top of my 'secure-concat.v5' branch on kernel.org.
> 
> As usual, comments and reviews are welcome.
> 
> Hannes Reinecke (2):
>    nvme: Do not re-authenticate queues with no prior authentication
>    nvmet: Implement 'admin_only' authentication
> 
>   drivers/nvme/host/auth.c               | 17 ++++++++++++++++-
>   drivers/nvme/target/auth.c             | 11 +++++++----
>   drivers/nvme/target/configfs.c         | 24 ++++++++++++++++++++++++
>   drivers/nvme/target/fabrics-cmd-auth.c |  7 +++++++
>   drivers/nvme/target/fabrics-cmd.c      |  7 +++++--
>   drivers/nvme/target/nvmet.h            |  2 ++
>   6 files changed, 61 insertions(+), 7 deletions(-)
> 
Ping? What's the status here? Should I resend?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich