On 11/23/21 2:11 PM, Sagi Grimberg wrote:

quoted

+int nvme_auth_generate_key(struct nvme_ctrl *ctrl, u8 *secret, bool
set_ctrl)

Didn't we agree to pass the key pointer? i.e.
int nvme_auth_generate_key(struct nvme_dhchap_key **key, u8 *secret)

Ah. That's what you had in mind.
Why, of course we can do that.

quoted

+{
+    struct nvme_dhchap_key *key;
+    u8 key_hash;
+
+    if (!secret)
+        return 0;
+
+    if (sscanf(secret, "DHHC-1:%hhd:%*s:", &key_hash) != 1)
+        return -EINVAL;
+
+    /* Pass in the secret without the 'DHHC-1:XX:' prefix */
+    key = nvme_auth_extract_key(secret + 10, key_hash);
+    if (IS_ERR(key)) {
+        dev_dbg(ctrl->device, "failed to extract key, error %ld\n",
+            PTR_ERR(key));

The print here is slightly redundant - you already have prints inside
nvme_auth_extract_key already.

Yeah; I really need to go through the code and remove the redundant
messages. Especially on the error paths.

quoted

+        return PTR_ERR(key);
+    }
+

Then we instead just do:
    *key = key;

quoted

+    if (set_ctrl)
+        ctrl->ctrl_key = key;
+    else
+        ctrl->host_key = key;
+
+    return 0;
+}

...

quoted

+EXPORT_SYMBOL_GPL(nvme_auth_generate_key);

diff --git a/drivers/nvme/host/auth.h b/drivers/nvme/host/auth.h
new file mode 100644
index 000000000000..16e3d893d54a
--- /dev/null
+++ b/drivers/nvme/host/auth.h

@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Hannes Reinecke, SUSE Software Solutions
+ */
+
+#ifndef _NVME_AUTH_H
+#define _NVME_AUTH_H
+
+#include <crypto/kpp.h>
+
+struct nvme_dhchap_key {
+    u8 *key;
+    size_t key_len;
+    u8 key_hash;

Why not just name it len and hash? don't think the key_
prefix is useful...

True.
Will do so.

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