On 9/27/21 12:51 AM, Sagi Grimberg wrote:

quoted

+void nvmet_execute_auth_send(struct nvmet_req *req)
+{
+    struct nvmet_ctrl *ctrl = req->sq->ctrl;
+    struct nvmf_auth_dhchap_success2_data *data;
+    void *d;
+    u32 tl;
+    u16 status = 0;
+
+    if (req->cmd->auth_send.secp != 
NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
+        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+        req->error_loc =
+            offsetof(struct nvmf_auth_send_command, secp);
+        goto done;
+    }
+    if (req->cmd->auth_send.spsp0 != 0x01) {
+        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+        req->error_loc =
+            offsetof(struct nvmf_auth_send_command, spsp0);
+        goto done;
+    }
+    if (req->cmd->auth_send.spsp1 != 0x01) {
+        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+        req->error_loc =
+            offsetof(struct nvmf_auth_send_command, spsp1);
+        goto done;
+    }
+    tl = le32_to_cpu(req->cmd->auth_send.tl);
+    if (!tl) {
+        status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+        req->error_loc =
+            offsetof(struct nvmf_auth_send_command, tl);
+        goto done;
+    }
+    if (!nvmet_check_transfer_len(req, tl)) {
+        pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
+        return;
+    }
+
+    d = kmalloc(tl, GFP_KERNEL);
+    if (!d) {
+        status = NVME_SC_INTERNAL;
+        goto done;
+    }
+
+    status = nvmet_copy_from_sgl(req, 0, d, tl);
+    if (status) {
+        kfree(d);
+        goto done;
+    }
+
+    data = d;
+    pr_debug("%s: ctrl %d qid %d type %d id %d step %x\n", __func__,
+         ctrl->cntlid, req->sq->qid, data->auth_type, data->auth_id,
+         req->sq->dhchap_step);
+    if (data->auth_type != NVME_AUTH_COMMON_MESSAGES &&
+        data->auth_type != NVME_AUTH_DHCHAP_MESSAGES)
+        goto done_failure1;
+    if (data->auth_type == NVME_AUTH_COMMON_MESSAGES) {
+        if (data->auth_id == NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE) {
+            /* Restart negotiation */
+            pr_debug("%s: ctrl %d qid %d reset negotiation\n", __func__,
+                 ctrl->cntlid, req->sq->qid);

This is the point where you need to reset also auth config as this may
have changed and the host will not create a new controller but rather
re-authenticate on the existing controller.

i.e.

+                       if (!req->sq->qid) {
+                               nvmet_destroy_auth(ctrl);
+                               if (nvmet_setup_auth(ctrl) < 0) {
+                                       pr_err("Failed to setup 
re-authentication\n");
+                                       goto done_failure1;
+                               }
+                       }

Not sure. We have two paths how re-authentication can be triggered.
The one is from the host, which sends a 'negotiate' command to the 
controller (ie this path).  Then nothing on the controller has changed, 
and we just need to ensure that we restart negotiation.
IE we should _not_ reset the authentication (as that would also remove 
the controller keys, which haven't changed). We should just ensure that 
all ephemeral data is regenerated. But that should be handled in-line, 
and I _think_ I have covered all of that.
The other path to trigger re-authentication is when changing values on 
the controller via configfs. Then sure we need to reset the controller 
data, and trigger reauthentication.
And there I do agree, that path isn't fully implemented / tested.
But should be started whenever the configfs values change.

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: Felix Imendörffer