[PATCH 0/6] nvme-auth: use xarray and minor fixes

[PATCH 0/6] nvme-auth: use xarray and minor fixes

[Hannes Reinecke]

Hi all,

this patchset moves the dhchap context allocation to an xarray to avoid
locking issues uncovered by blktests. But during review of the original patch
several issues were pointed out (like allocating the transaction buffer
even if the transaction is finished), and I've found several other issues, too.
So I've decided to wrap them all up in a patchset.

So as usual, comments and reviews are welcome.

Hannes Reinecke (6):
  nvme-auth: allocate authentication buffer only during transaction
  nvme-auth: do not queue authentication if the queue is not live
  nvme-auth: use xarray instead of linked list
  nvme-auth: return real error instead of NVME_SC_AUTH_REQUIRED
  nvme-auth: set DNR bit on non-retryable errors
  nvme-auth: use a define for chap buffer size

 drivers/nvme/host/auth.c    | 199 +++++++++++++++++++-----------------
 drivers/nvme/host/fabrics.c |   2 -
 drivers/nvme/host/nvme.h    |   2 +-
 3 files changed, 107 insertions(+), 96 deletions(-)

-- 
2.35.3

[PATCH 1/6] nvme-auth: allocate authentication buffer only during transaction

[Hannes Reinecke]

The authentication buffer is only used during the authentication
transaction, so no need to keep it around.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c | 49 +++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 3b63aa155beb..b68fb2c764f6 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -667,8 +667,6 @@ static void __nvme_auth_reset(struct nvme_dhchap_queue_context *chap)
 	kfree_sensitive(chap->sess_key);
 	chap->sess_key = NULL;
 	chap->sess_key_len = 0;
-	chap->status = 0;
-	chap->error = 0;
 	chap->s1 = 0;
 	chap->s2 = 0;
 	chap->transaction = 0;
@@ -687,7 +685,6 @@ static void __nvme_auth_free(struct nvme_dhchap_queue_context *chap)
 	kfree_sensitive(chap->host_key);
 	kfree_sensitive(chap->sess_key);
 	kfree_sensitive(chap->host_response);
-	kfree(chap->buf);
 	kfree(chap);
 }
 
@@ -700,6 +697,19 @@ static void __nvme_auth_work(struct work_struct *work)
 	int ret = 0;
 
 	chap->transaction = ctrl->transaction++;
+	chap->status = 0;
+	chap->error = 0;
+
+	/*
+	 * Allocate a large enough buffer for the entire negotiation:
+	 * 4k should be enough to ffdhe8192.
+	 */
+	chap->buf_size = 4096;
+	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
+	if (!chap->buf) {
+		chap->error = -ENOMEM;
+		return;
+	}
 
 	/* DH-HMAC-CHAP Step 1: send negotiate */
 	dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
@@ -707,13 +717,13 @@ static void __nvme_auth_work(struct work_struct *work)
 	ret = nvme_auth_set_dhchap_negotiate_data(ctrl, chap);
 	if (ret < 0) {
 		chap->error = ret;
-		return;
+		goto out_free;
 	}
 	tl = ret;
 	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, tl, true);
 	if (ret) {
 		chap->error = ret;
-		return;
+		goto out_free;
 	}
 
 	/* DH-HMAC-CHAP Step 2: receive challenge */
@@ -727,14 +737,14 @@ static void __nvme_auth_work(struct work_struct *work)
 			 "qid %d failed to receive challenge, %s %d\n",
 			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
 		chap->error = ret;
-		return;
+		goto out_free;
 	}
 	ret = nvme_auth_receive_validate(ctrl, chap->qid, chap->buf, chap->transaction,
 					 NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE);
 	if (ret) {
 		chap->status = ret;
 		chap->error = NVME_SC_AUTH_REQUIRED;
-		return;
+		goto out_free;
 	}
 
 	ret = nvme_auth_process_dhchap_challenge(ctrl, chap);
@@ -790,7 +800,7 @@ static void __nvme_auth_work(struct work_struct *work)
 			 "qid %d failed to receive success1, %s %d\n",
 			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
 		chap->error = ret;
-		return;
+		goto out_free;
 	}
 	ret = nvme_auth_receive_validate(ctrl, chap->qid,
 					 chap->buf, chap->transaction,
@@ -798,7 +808,7 @@ static void __nvme_auth_work(struct work_struct *work)
 	if (ret) {
 		chap->status = ret;
 		chap->error = NVME_SC_AUTH_REQUIRED;
-		return;
+		goto out_free;
 	}
 
 	if (ctrl->ctrl_key) {
@@ -828,10 +838,7 @@ static void __nvme_auth_work(struct work_struct *work)
 		if (ret)
 			chap->error = ret;
 	}
-	if (!ret) {
-		chap->error = 0;
-		return;
-	}
+	goto out_free;
 
 fail2:
 	dev_dbg(ctrl->device, "%s: qid %d send failure2, status %x\n",
@@ -844,6 +851,9 @@ static void __nvme_auth_work(struct work_struct *work)
 	 */
 	if (ret && !chap->error)
 		chap->error = ret;
+out_free:
+	kfree(chap->buf);
+	chap->buf = NULL;
 }
 
 int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
@@ -863,7 +873,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
 	mutex_lock(&ctrl->dhchap_auth_mutex);
 	/* Check if the context is already queued */
 	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
-		WARN_ON(!chap->buf);
 		if (chap->qid == qid) {
 			dev_dbg(ctrl->device, "qid %d: re-using context\n", qid);
 			mutex_unlock(&ctrl->dhchap_auth_mutex);
@@ -881,18 +890,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
 	chap->qid = (qid == NVME_QID_ANY) ? 0 : qid;
 	chap->ctrl = ctrl;
 
-	/*
-	 * Allocate a large enough buffer for the entire negotiation:
-	 * 4k should be enough to ffdhe8192.
-	 */
-	chap->buf_size = 4096;
-	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
-	if (!chap->buf) {
-		mutex_unlock(&ctrl->dhchap_auth_mutex);
-		kfree(chap);
-		return -ENOMEM;
-	}
-
 	INIT_WORK(&chap->auth_work, __nvme_auth_work);
 	list_add(&chap->entry, &ctrl->dhchap_auth_list);
 	mutex_unlock(&ctrl->dhchap_auth_mutex);
-- 
2.35.3

[PATCH 2/6] nvme-auth: do not queue authentication if the queue is not live

[Hannes Reinecke]

If the queue is not connecting or live there's no point in trying
to start authentication as we won't be able to send commands.
So terminate the work functions directly if the controller is
in a wrong state.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index b68fb2c764f6..719c514363ee 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -696,6 +696,12 @@ static void __nvme_auth_work(struct work_struct *work)
 	size_t tl;
 	int ret = 0;
 
+	if (ctrl->state != NVME_CTRL_CONNECTING &&
+	    ctrl->state != NVME_CTRL_LIVE) {
+		chap->error = -ENOTCONN;
+		return;
+	}
+
 	chap->transaction = ctrl->transaction++;
 	chap->status = 0;
 	chap->error = 0;
@@ -937,6 +943,10 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
 		container_of(work, struct nvme_ctrl, dhchap_auth_work);
 	int ret, q;
 
+	if (ctrl->state != NVME_CTRL_CONNECTING &&
+	    ctrl->state != NVME_CTRL_LIVE)
+		return;
+
 	/* Authenticate admin queue first */
 	ret = nvme_auth_negotiate(ctrl, 0);
 	if (ret) {
-- 
2.35.3

[PATCH 3/6] nvme-auth: use xarray instead of linked list

[Hannes Reinecke]

The current design of holding the chap context is slightly awkward,
as the context is allocated on demand, and we have to lock the list
when looking up contexts as we wouldn't know if the context is
allocated.

This patch moves the allocation out of the chap context before starting
authentication and stores it into an xarray. With that we can do
away with the list traversal and access the context directly
via the queue number.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c | 85 +++++++++++++++++++++-------------------
 drivers/nvme/host/nvme.h |  2 +-
 2 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 719c514363ee..cece4f33e3a8 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -878,27 +878,35 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
 
 	mutex_lock(&ctrl->dhchap_auth_mutex);
 	/* Check if the context is already queued */
-	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
-		if (chap->qid == qid) {
-			dev_dbg(ctrl->device, "qid %d: re-using context\n", qid);
+	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
+	if (!chap) {
+		int ret;
+
+		chap = kzalloc(sizeof(*chap), GFP_NOIO);
+		if (!chap) {
 			mutex_unlock(&ctrl->dhchap_auth_mutex);
-			flush_work(&chap->auth_work);
-			__nvme_auth_reset(chap);
-			queue_work(nvme_wq, &chap->auth_work);
-			return 0;
+			dev_warn(ctrl->device,
+				 "qid %d: error allocation authentication", qid);
+			return -ENOMEM;
 		}
-	}
-	chap = kzalloc(sizeof(*chap), GFP_KERNEL);
-	if (!chap) {
+		chap->qid = qid;
+		chap->ctrl = ctrl;
+
+		INIT_WORK(&chap->auth_work, __nvme_auth_work);
+		ret = xa_insert(&ctrl->dhchap_auth_xa, qid, chap, GFP_NOIO);
 		mutex_unlock(&ctrl->dhchap_auth_mutex);
-		return -ENOMEM;
+		if (ret) {
+			dev_warn(ctrl->device,
+				 "qid %d: error %d inserting authentication",
+				 qid, ret);
+			kfree(chap);
+			return ret;
+		}
+	} else {
+		mutex_unlock(&ctrl->dhchap_auth_mutex);
+		flush_work(&chap->auth_work);
+		__nvme_auth_reset(chap);
 	}
-	chap->qid = (qid == NVME_QID_ANY) ? 0 : qid;
-	chap->ctrl = ctrl;
-
-	INIT_WORK(&chap->auth_work, __nvme_auth_work);
-	list_add(&chap->entry, &ctrl->dhchap_auth_list);
-	mutex_unlock(&ctrl->dhchap_auth_mutex);
 	queue_work(nvme_wq, &chap->auth_work);
 	return 0;
 }
@@ -907,33 +915,28 @@ EXPORT_SYMBOL_GPL(nvme_auth_negotiate);
 int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
 {
 	struct nvme_dhchap_queue_context *chap;
-	int ret;
 
-	mutex_lock(&ctrl->dhchap_auth_mutex);
-	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
-		if (chap->qid != qid)
-			continue;
-		mutex_unlock(&ctrl->dhchap_auth_mutex);
-		flush_work(&chap->auth_work);
-		ret = chap->error;
-		return ret;
+	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
+	if (!chap) {
+		dev_warn(ctrl->device,
+			 "qid %d: authentication not initialized!",
+			 qid);
+		return -ENOENT;
 	}
-	mutex_unlock(&ctrl->dhchap_auth_mutex);
-	return -ENXIO;
+	flush_work(&chap->auth_work);
+	return chap->error;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_wait);
 
 void nvme_auth_reset(struct nvme_ctrl *ctrl)
 {
 	struct nvme_dhchap_queue_context *chap;
+	unsigned long qid;
 
-	mutex_lock(&ctrl->dhchap_auth_mutex);
-	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
-		mutex_unlock(&ctrl->dhchap_auth_mutex);
+	xa_for_each(&ctrl->dhchap_auth_xa, qid, chap) {
 		flush_work(&chap->auth_work);
 		__nvme_auth_reset(chap);
 	}
-	mutex_unlock(&ctrl->dhchap_auth_mutex);
 }
 EXPORT_SYMBOL_GPL(nvme_auth_reset);
 
@@ -979,7 +982,7 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
 
 void nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
 {
-	INIT_LIST_HEAD(&ctrl->dhchap_auth_list);
+	xa_init_flags(&ctrl->dhchap_auth_xa, XA_FLAGS_ALLOC);
 	INIT_WORK(&ctrl->dhchap_auth_work, nvme_dhchap_auth_work);
 	mutex_init(&ctrl->dhchap_auth_mutex);
 	if (!ctrl->opts)
@@ -991,27 +994,27 @@ EXPORT_SYMBOL_GPL(nvme_auth_init_ctrl);
 
 void nvme_auth_stop(struct nvme_ctrl *ctrl)
 {
-	struct nvme_dhchap_queue_context *chap = NULL, *tmp;
+	struct nvme_dhchap_queue_context *chap;
+	unsigned long qid;
 
 	cancel_work_sync(&ctrl->dhchap_auth_work);
-	mutex_lock(&ctrl->dhchap_auth_mutex);
-	list_for_each_entry_safe(chap, tmp, &ctrl->dhchap_auth_list, entry)
+	xa_for_each(&ctrl->dhchap_auth_xa, qid, chap)
 		cancel_work_sync(&chap->auth_work);
-	mutex_unlock(&ctrl->dhchap_auth_mutex);
 }
 EXPORT_SYMBOL_GPL(nvme_auth_stop);
 
 void nvme_auth_free(struct nvme_ctrl *ctrl)
 {
-	struct nvme_dhchap_queue_context *chap = NULL, *tmp;
+	struct nvme_dhchap_queue_context *chap;
+	unsigned long qid;
 
 	mutex_lock(&ctrl->dhchap_auth_mutex);
-	list_for_each_entry_safe(chap, tmp, &ctrl->dhchap_auth_list, entry) {
-		list_del_init(&chap->entry);
-		flush_work(&chap->auth_work);
+	xa_for_each(&ctrl->dhchap_auth_xa, qid, chap) {
+		chap = xa_erase(&ctrl->dhchap_auth_xa, qid);
 		__nvme_auth_free(chap);
 	}
 	mutex_unlock(&ctrl->dhchap_auth_mutex);
+	xa_destroy(&ctrl->dhchap_auth_xa);
 	if (ctrl->host_key) {
 		nvme_auth_free_key(ctrl->host_key);
 		ctrl->host_key = NULL;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 32d9dc2d957e..9b31fb8c1b27 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -338,7 +338,7 @@ struct nvme_ctrl {
 
 #ifdef CONFIG_NVME_AUTH
 	struct work_struct dhchap_auth_work;
-	struct list_head dhchap_auth_list;
+	struct xarray dhchap_auth_xa;
 	struct mutex dhchap_auth_mutex;
 	struct nvme_dhchap_key *host_key;
 	struct nvme_dhchap_key *ctrl_key;
-- 
2.35.3

[PATCH 4/6] nvme-auth: return real error instead of NVME_SC_AUTH_REQUIRED

[Hannes Reinecke]

The caller should be equipped to handle combined nvme errors
(where a negative value indicates an error number and positive
ones an nvme status), so there is no need to return
NVME_SC_AUTH_REQUIRED on failure.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c    | 18 +++++++++++-------
 drivers/nvme/host/fabrics.c |  2 --
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index cece4f33e3a8..b4af8661c988 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -72,10 +72,12 @@ static int nvme_auth_submit(struct nvme_ctrl *ctrl, int qid,
 				     0, flags, nvme_max_retries);
 	if (ret > 0)
 		dev_warn(ctrl->device,
-			"qid %d auth_send failed with status %d\n", qid, ret);
+			"qid %d auth_%s failed with status %d\n",
+			 qid, auth_send ? "send" : "recv", ret);
 	else if (ret < 0)
 		dev_err(ctrl->device,
-			"qid %d auth_send failed with error %d\n", qid, ret);
+			"qid %d auth_%s failed with error %d\n",
+			qid, auth_send ? "send" : "recv", ret);
 	return ret;
 }
 
@@ -179,12 +181,14 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
 	chap->shash_tfm = crypto_alloc_shash(hmac_name, 0,
 					     CRYPTO_ALG_ALLOCATES_MEMORY);
 	if (IS_ERR(chap->shash_tfm)) {
+		int ret = PTR_ERR(chap->shash_tfm);
+
 		dev_warn(ctrl->device,
 			 "qid %d: failed to allocate hash %s, error %ld\n",
 			 chap->qid, hmac_name, PTR_ERR(chap->shash_tfm));
 		chap->shash_tfm = NULL;
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
-		return NVME_SC_AUTH_REQUIRED;
+		return ret;
 	}
 
 	if (crypto_shash_digestsize(chap->shash_tfm) != data->hl) {
@@ -259,7 +263,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
 				 chap->qid, ret, gid_name);
 			chap->status = NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE;
 			chap->dh_tfm = NULL;
-			return NVME_SC_AUTH_REQUIRED;
+			return ret;
 		}
 		dev_dbg(ctrl->device, "qid %d: selected DH group %s\n",
 			chap->qid, gid_name);
@@ -279,7 +283,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
 		chap->ctrl_key = kmalloc(dhvlen, GFP_KERNEL);
 		if (!chap->ctrl_key) {
 			chap->status = NVME_AUTH_DHCHAP_FAILURE_FAILED;
-			return NVME_SC_AUTH_REQUIRED;
+			return -ENOMEM;
 		}
 		chap->ctrl_key_len = dhvlen;
 		memcpy(chap->ctrl_key, data->cval + chap->hash_len,
@@ -831,7 +835,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;
 	}
 
@@ -960,7 +964,7 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
 	ret = nvme_auth_wait(ctrl, 0);
 	if (ret) {
 		dev_warn(ctrl->device,
-			 "qid 0: authentication failed\n");
+			 "qid 0: authentication failed with %d\n", ret);
 		return;
 	}
 
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index f4bfbaf733e9..7b64357ecd1d 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -416,7 +416,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);
@@ -492,7 +491,6 @@ 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)
-- 
2.35.3

[PATCH 5/6] nvme-auth: set DNR bit on non-retryable errors

[Hannes Reinecke]

If the negotiation fails due to protocol errors or unsupported
hash algorithms we need to set the DNR bit on the final status
to avoid retries.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index b4af8661c988..6ca0c2bb06c0 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -151,7 +151,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
 
 	if (chap->buf_size < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
-		return NVME_SC_INVALID_FIELD;
+		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;
 	}
 
 	hmac_name = nvme_auth_hmac_name(data->hashid);
@@ -160,7 +160,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 &&
@@ -198,7 +198,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 */
@@ -220,7 +220,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 */
@@ -251,7 +251,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);
@@ -272,7 +272,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;
 
@@ -353,7 +353,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl,
 
 	if (chap->buf_size < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
-		return NVME_SC_INVALID_FIELD;
+		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;
 	}
 
 	if (data->hl != chap->hash_len) {
@@ -361,7 +361,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 */
@@ -385,7 +385,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 */
@@ -753,7 +753,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;
 		goto out_free;
 	}
 
@@ -817,7 +817,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;
 		goto out_free;
 	}
 
-- 
2.35.3

[PATCH 6/6] nvme-auth: use a define for chap buffer size

[Hannes Reinecke]

The chap buffer size is pretty much static, so we can make it
a define and save the variable.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 6ca0c2bb06c0..444f56399ffc 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -13,6 +13,8 @@
 #include "fabrics.h"
 #include <linux/nvme-auth.h>
 
+#define CHAP_BUF_SIZE 4096
+
 struct nvme_dhchap_queue_context {
 	struct list_head entry;
 	struct work_struct auth_work;
@@ -20,7 +22,6 @@ struct nvme_dhchap_queue_context {
 	struct crypto_shash *shash_tfm;
 	struct crypto_kpp *dh_tfm;
 	void *buf;
-	size_t buf_size;
 	int qid;
 	int error;
 	u32 s1;
@@ -114,7 +115,7 @@ static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl,
 	struct nvmf_auth_dhchap_negotiate_data *data = chap->buf;
 	size_t size = sizeof(*data) + sizeof(union nvmf_auth_protocol);
 
-	if (chap->buf_size < size) {
+	if (CHAP_BUF_SIZE < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
 		return -EINVAL;
 	}
@@ -149,7 +150,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
 	const char *gid_name = nvme_auth_dhgroup_name(data->dhgid);
 	const char *hmac_name, *kpp_name;
 
-	if (chap->buf_size < size) {
+	if (CHAP_BUF_SIZE < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
 		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;
 	}
@@ -306,7 +307,7 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
 	if (chap->host_key_len)
 		size += chap->host_key_len;
 
-	if (chap->buf_size < size) {
+	if (CHAP_BUF_SIZE < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
 		return -EINVAL;
 	}
@@ -351,7 +352,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl,
 	if (ctrl->ctrl_key)
 		size += chap->hash_len;
 
-	if (chap->buf_size < size) {
+	if (CHAP_BUF_SIZE < size) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
 		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;
 	}
@@ -711,11 +712,9 @@ static void __nvme_auth_work(struct work_struct *work)
 	chap->error = 0;
 
 	/*
-	 * Allocate a large enough buffer for the entire negotiation:
-	 * 4k should be enough to ffdhe8192.
+	 * Allocate a buffer large enough for the entire negotiation
 	 */
-	chap->buf_size = 4096;
-	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
+	chap->buf = kzalloc(CHAP_BUF_SIZE, GFP_KERNEL);
 	if (!chap->buf) {
 		chap->error = -ENOMEM;
 		return;
@@ -740,8 +739,8 @@ static void __nvme_auth_work(struct work_struct *work)
 	dev_dbg(ctrl->device, "%s: qid %d receive challenge\n",
 		__func__, chap->qid);
 
-	memset(chap->buf, 0, chap->buf_size);
-	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, chap->buf_size, false);
+	memset(chap->buf, 0, CHAP_BUF_SIZE);
+	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, CHAP_BUF_SIZE, false);
 	if (ret) {
 		dev_warn(ctrl->device,
 			 "qid %d failed to receive challenge, %s %d\n",
@@ -803,8 +802,8 @@ static void __nvme_auth_work(struct work_struct *work)
 	dev_dbg(ctrl->device, "%s: qid %d receive success1\n",
 		__func__, chap->qid);
 
-	memset(chap->buf, 0, chap->buf_size);
-	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, chap->buf_size, false);
+	memset(chap->buf, 0, CHAP_BUF_SIZE);
+	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, CHAP_BUF_SIZE, false);
 	if (ret) {
 		dev_warn(ctrl->device,
 			 "qid %d failed to receive success1, %s %d\n",
-- 
2.35.3

Re: [PATCH 5/6] nvme-auth: set DNR bit on non-retryable errors

[Christoph Hellwig]

On Wed, Nov 02, 2022 at 08:52:23AM +0100, Hannes Reinecke wrote:
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index b4af8661c988..6ca0c2bb06c0 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -151,7 +151,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
>  
>  	if (chap->buf_size < size) {
>  		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
> -		return NVME_SC_INVALID_FIELD;
> +		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;

Any use of NVME_SC_ for error purely in host software are bogus.  This
should be using Linux error codes.

Re: [PATCH 3/6] nvme-auth: use xarray instead of linked list

[Christoph Hellwig]

> @@ -907,33 +915,28 @@ EXPORT_SYMBOL_GPL(nvme_auth_negotiate);
>  int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
>  {
>  	struct nvme_dhchap_queue_context *chap;
> -	int ret;
>  
> +	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
> +	if (!chap) {
> +		dev_warn(ctrl->device,
> +			 "qid %d: authentication not initialized!",
> +			 qid);
> +		return -ENOENT;
>  	}
> +	flush_work(&chap->auth_work);

What protects chap from going away after the load, but before use?

Re: [PATCH 5/6] nvme-auth: set DNR bit on non-retryable errors

[Hannes Reinecke]

On 11/2/22 09:02, Christoph Hellwig wrote:
> On Wed, Nov 02, 2022 at 08:52:23AM +0100, Hannes Reinecke wrote:
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index b4af8661c988..6ca0c2bb06c0 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -151,7 +151,7 @@ static int nvme_auth_process_dhchap_challenge(struct nvme_ctrl *ctrl,
>>   
>>   	if (chap->buf_size < size) {
>>   		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
>> -		return NVME_SC_INVALID_FIELD;
>> +		return NVME_SC_DNR | NVME_SC_INVALID_FIELD;
> 
> Any use of NVME_SC_ for error purely in host software are bogus.  This
> should be using Linux error codes.

I knew you would be saying that.
I was worrying about retryable and non-retryable errors, but seeing that 
all errors are currently treated as non-retryable I'll be changing it to 
return error codes.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 3/6] nvme-auth: use xarray instead of linked list

[Hannes Reinecke]

On 11/2/22 09:03, Christoph Hellwig wrote:
>> @@ -907,33 +915,28 @@ EXPORT_SYMBOL_GPL(nvme_auth_negotiate);
>>   int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
>>   {
>>   	struct nvme_dhchap_queue_context *chap;
>> -	int ret;
>>   
>> +	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
>> +	if (!chap) {
>> +		dev_warn(ctrl->device,
>> +			 "qid %d: authentication not initialized!",
>> +			 qid);
>> +		return -ENOENT;
>>   	}
>> +	flush_work(&chap->auth_work);
> 
> What protects chap from going away after the load, but before use?
> 

The control flow.
Entries are only deallocated in nvme_auth_free(), which is called from 
nvme_free_ctrl().
By that time all processing has stopped, all workqueue entries are 
flushed, and no further commands are accepted.
Entries are allocated from two call sites; one via ops->create_ctrl(), 
and the other one via the workqueue function nvme_dhchap_auth_work().
'create_ctrl' is synchronous, so the controller will only be deallocated 
on the final put() of the create_ctrl() control flow. So we should be 
good there.
The workqueue function is flushed during nvme_free_ctrl(), so we're good 
there, too.

Hence I felt additional protection is not required.
Do correct me if I'm wrong.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH 3/6] nvme-auth: use xarray instead of linked list

[Christoph Hellwig]

On Wed, Nov 02, 2022 at 09:52:24AM +0100, Hannes Reinecke wrote:
> The control flow.
> Entries are only deallocated in nvme_auth_free(), which is called from 
> nvme_free_ctrl().
> By that time all processing has stopped, all workqueue entries are flushed, 
> and no further commands are accepted.
> Entries are allocated from two call sites; one via ops->create_ctrl(), and 
> the other one via the workqueue function nvme_dhchap_auth_work().
> 'create_ctrl' is synchronous, so the controller will only be deallocated on 
> the final put() of the create_ctrl() control flow. So we should be good 
> there.
> The workqueue function is flushed during nvme_free_ctrl(), so we're good 
> there, too.
>
> Hence I felt additional protection is not required.
> Do correct me if I'm wrong.

I'm not up to speed on the auth code.  But something like this
needs to be clearly documented in the commit log, and preferably
in the code as well.

Re: [PATCH 1/6] nvme-auth: allocate authentication buffer only during transaction

[Sagi Grimberg]

On 11/2/22 09:52, Hannes Reinecke wrote:
> The authentication buffer is only used during the authentication
> transaction, so no need to keep it around.

Patch is fine, but why do we need the chap context dynamically
allocated? It is always the number of queues and never ever changes.

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/auth.c | 49 +++++++++++++++++++---------------------
>   1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 3b63aa155beb..b68fb2c764f6 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -667,8 +667,6 @@ static void __nvme_auth_reset(struct nvme_dhchap_queue_context *chap)
>   	kfree_sensitive(chap->sess_key);
>   	chap->sess_key = NULL;
>   	chap->sess_key_len = 0;
> -	chap->status = 0;
> -	chap->error = 0;
>   	chap->s1 = 0;
>   	chap->s2 = 0;
>   	chap->transaction = 0;
> @@ -687,7 +685,6 @@ static void __nvme_auth_free(struct nvme_dhchap_queue_context *chap)
>   	kfree_sensitive(chap->host_key);
>   	kfree_sensitive(chap->sess_key);
>   	kfree_sensitive(chap->host_response);
> -	kfree(chap->buf);
>   	kfree(chap);
>   }
>   
> @@ -700,6 +697,19 @@ static void __nvme_auth_work(struct work_struct *work)
>   	int ret = 0;
>   
>   	chap->transaction = ctrl->transaction++;
> +	chap->status = 0;
> +	chap->error = 0;
> +
> +	/*
> +	 * Allocate a large enough buffer for the entire negotiation:
> +	 * 4k should be enough to ffdhe8192.
> +	 */
> +	chap->buf_size = 4096;
> +	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
> +	if (!chap->buf) {
> +		chap->error = -ENOMEM;
> +		return;
> +	}
>   
>   	/* DH-HMAC-CHAP Step 1: send negotiate */
>   	dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
> @@ -707,13 +717,13 @@ static void __nvme_auth_work(struct work_struct *work)
>   	ret = nvme_auth_set_dhchap_negotiate_data(ctrl, chap);
>   	if (ret < 0) {
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	tl = ret;
>   	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, tl, true);
>   	if (ret) {
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   
>   	/* DH-HMAC-CHAP Step 2: receive challenge */
> @@ -727,14 +737,14 @@ static void __nvme_auth_work(struct work_struct *work)
>   			 "qid %d failed to receive challenge, %s %d\n",
>   			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	ret = nvme_auth_receive_validate(ctrl, chap->qid, chap->buf, chap->transaction,
>   					 NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE);
>   	if (ret) {
>   		chap->status = ret;
>   		chap->error = NVME_SC_AUTH_REQUIRED;
> -		return;
> +		goto out_free;
>   	}
>   
>   	ret = nvme_auth_process_dhchap_challenge(ctrl, chap);
> @@ -790,7 +800,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   			 "qid %d failed to receive success1, %s %d\n",
>   			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	ret = nvme_auth_receive_validate(ctrl, chap->qid,
>   					 chap->buf, chap->transaction,
> @@ -798,7 +808,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   	if (ret) {
>   		chap->status = ret;
>   		chap->error = NVME_SC_AUTH_REQUIRED;
> -		return;
> +		goto out_free;
>   	}
>   
>   	if (ctrl->ctrl_key) {
> @@ -828,10 +838,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   		if (ret)
>   			chap->error = ret;
>   	}
> -	if (!ret) {
> -		chap->error = 0;
> -		return;
> -	}
> +	goto out_free;
>   
>   fail2:
>   	dev_dbg(ctrl->device, "%s: qid %d send failure2, status %x\n",
> @@ -844,6 +851,9 @@ static void __nvme_auth_work(struct work_struct *work)
>   	 */
>   	if (ret && !chap->error)
>   		chap->error = ret;
> +out_free:
> +	kfree(chap->buf);
> +	chap->buf = NULL;
>   }
>   
>   int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
> @@ -863,7 +873,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
>   	mutex_lock(&ctrl->dhchap_auth_mutex);
>   	/* Check if the context is already queued */
>   	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
> -		WARN_ON(!chap->buf);
>   		if (chap->qid == qid) {
>   			dev_dbg(ctrl->device, "qid %d: re-using context\n", qid);
>   			mutex_unlock(&ctrl->dhchap_auth_mutex);
> @@ -881,18 +890,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
>   	chap->qid = (qid == NVME_QID_ANY) ? 0 : qid;
>   	chap->ctrl = ctrl;
>   
> -	/*
> -	 * Allocate a large enough buffer for the entire negotiation:
> -	 * 4k should be enough to ffdhe8192.
> -	 */
> -	chap->buf_size = 4096;
> -	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
> -	if (!chap->buf) {
> -		mutex_unlock(&ctrl->dhchap_auth_mutex);
> -		kfree(chap);
> -		return -ENOMEM;
> -	}
> -
>   	INIT_WORK(&chap->auth_work, __nvme_auth_work);
>   	list_add(&chap->entry, &ctrl->dhchap_auth_list);
>   	mutex_unlock(&ctrl->dhchap_auth_mutex);

Re: [PATCH 2/6] nvme-auth: do not queue authentication if the queue is not live

[Sagi Grimberg]

> If the queue is not connecting or live there's no point in trying
> to start authentication as we won't be able to send commands.
> So terminate the work functions directly if the controller is
> in a wrong state.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/auth.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index b68fb2c764f6..719c514363ee 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -696,6 +696,12 @@ static void __nvme_auth_work(struct work_struct *work)
>   	size_t tl;
>   	int ret = 0;
>   
> +	if (ctrl->state != NVME_CTRL_CONNECTING &&
> +	    ctrl->state != NVME_CTRL_LIVE) {
> +		chap->error = -ENOTCONN;
> +		return;
> +	}

Why do you want this to run in CONNECTING at all? The connect itself
will authenticate...

> +
>   	chap->transaction = ctrl->transaction++;
>   	chap->status = 0;
>   	chap->error = 0;
> @@ -937,6 +943,10 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
>   		container_of(work, struct nvme_ctrl, dhchap_auth_work);
>   	int ret, q;
>   
> +	if (ctrl->state != NVME_CTRL_CONNECTING &&
> +	    ctrl->state != NVME_CTRL_LIVE)
> +		return;

Same here.

> +
>   	/* Authenticate admin queue first */
>   	ret = nvme_auth_negotiate(ctrl, 0);
>   	if (ret) {

Re: [PATCH 3/6] nvme-auth: use xarray instead of linked list

[Sagi Grimberg]

> The current design of holding the chap context is slightly awkward,
> as the context is allocated on demand, and we have to lock the list
> when looking up contexts as we wouldn't know if the context is
> allocated.
> 
> This patch moves the allocation out of the chap context before starting
> authentication and stores it into an xarray. With that we can do
> away with the list traversal and access the context directly
> via the queue number.

Why not just allocate the chap contexts in auth_init() ??
its always the number of queues and is mapped 1x1. It IS
an array effectively.

Re: [PATCH 6/6] nvme-auth: use a define for chap buffer size

[Sagi Grimberg]

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

Re: [PATCH 1/6] nvme-auth: allocate authentication buffer only during transaction

[Hannes Reinecke]

On 11/3/22 21:01, Sagi Grimberg wrote:
> 
> 
> On 11/2/22 09:52, Hannes Reinecke wrote:
>> The authentication buffer is only used during the authentication
>> transaction, so no need to keep it around.
> 
> Patch is fine, but why do we need the chap context dynamically
> allocated? It is always the number of queues and never ever changes.
> 
I didn't want to clutter the main nvme structure with the chap stuff, so 
I kept it as a private definition in auth.c, and did a private allocation.

Also there is no 'queue' definition in the nvme structures; they live 
only within the transport definitions. The common nvme structures only 
know about controllers, and I don't have anything where I could hook the 
dhchap context into.

But the most crucial part here is that the number of queues are 
calculated after the initial 'connect' command (it's calculated during 
nvme_enable_ctrl()), so I would be prone to over-allocating the number 
of contexts.
If that's fine, though, I can easily simplify the code by allocating the 
dhchap contexts up front, and saving quite a bit of hassle with locking 
and stuff.

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 2/6] nvme-auth: do not queue authentication if the queue is not live

[Hannes Reinecke]

On 11/3/22 22:19, Sagi Grimberg wrote:
> 
>> If the queue is not connecting or live there's no point in trying
>> to start authentication as we won't be able to send commands.
>> So terminate the work functions directly if the controller is
>> in a wrong state.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/auth.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index b68fb2c764f6..719c514363ee 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -696,6 +696,12 @@ static void __nvme_auth_work(struct work_struct 
>> *work)
>>       size_t tl;
>>       int ret = 0;
>> +    if (ctrl->state != NVME_CTRL_CONNECTING &&
>> +        ctrl->state != NVME_CTRL_LIVE) {
>> +        chap->error = -ENOTCONN;
>> +        return;
>> +    }
> 
> Why do you want this to run in CONNECTING at all? The connect itself
> will authenticate...
> 
Precisely. It will authenticate _using this function_ ...

>> +
>>       chap->transaction = ctrl->transaction++;
>>       chap->status = 0;
>>       chap->error = 0;
>> @@ -937,6 +943,10 @@ static void nvme_dhchap_auth_work(struct 
>> work_struct *work)
>>           container_of(work, struct nvme_ctrl, dhchap_auth_work);
>>       int ret, q;
>> +    if (ctrl->state != NVME_CTRL_CONNECTING &&
>> +        ctrl->state != NVME_CTRL_LIVE)
>> +        return;
> 
> Same here.
> 
Here, however, things are different, as this function is only called 
during re-authentication (ie if someone wrote into the sysfs attributes).
This really makes sense only when the controller is live.
Will be fixing it.

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/6] nvme-auth: allocate authentication buffer only during transaction

[Christoph Hellwig]

On Fri, Nov 04, 2022 at 07:49:41AM +0100, Hannes Reinecke wrote:
> I didn't want to clutter the main nvme structure with the chap stuff, so I 
> kept it as a private definition in auth.c, and did a private allocation.

I think keeping the struct private is fine.  But adding two pointers
to the nvme controller for it, that is one directly to the
nvme_dhchap_queue_context for the admin queue as we'll need that
before setting the number of queues, and one to an array of the
I/O queues seems like a perfectly reasonable tradeoff.

In fact that is the exact two pointers we need for a list_head :)

Re: [PATCH 3/6] nvme-auth: use xarray instead of linked list

[Hannes Reinecke]

On 11/3/22 22:20, Sagi Grimberg wrote:
> 
>> The current design of holding the chap context is slightly awkward,
>> as the context is allocated on demand, and we have to lock the list
>> when looking up contexts as we wouldn't know if the context is
>> allocated.
>>
>> This patch moves the allocation out of the chap context before starting
>> authentication and stores it into an xarray. With that we can do
>> away with the list traversal and access the context directly
>> via the queue number.
> 
> Why not just allocate the chap contexts in auth_init() ??
> its always the number of queues and is mapped 1x1. It IS
> an array effectively.

As the dhchap structure is rather large I was worried about pointless 
memory allocations; quite some nvme-of arrays expose only a small number 
of queues (say: 8), causing the number of queues to be decreased after 
the initial connect call.
But if you're fine with it I can easily move it into auth_init(); that 
certainly will make the implementation simpler.

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