On 11/22/21 3:00 PM, Sagi Grimberg wrote:

On 11/22/21 3:21 PM, Hannes Reinecke wrote:

quoted

On 11/22/21 12:59 PM, Sagi Grimberg wrote:

quoted

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);
+            if (!req->sq->qid) {
+                status = nvmet_setup_auth(ctrl);

Aren't you leaking memory here?

I've checked, and I _think_ everything is in order.
Any particular concerns?
( 'd' is free at 'done_kfree', and we never exit without going through
it AFAICS).
Have I missed something?

You are calling nvmet_setup_auth for re-authentication, who is calling
nvmet_destroy_auth to free the controller auth stuff?

Don't you need something like:
-- 
        if (!req->sq->qid) {
            nvmet_destroy_auth(ctrl);
            status = nvmet_setup_auth(ctrl);
            ...
        }
-- 

nvme_setup_auth() should be re-entrant, ie it'll free old and reallocate
new keys as required. Hence no need to call nvmet_destroy_auth().
At least that's the plan.
Always possible that I messed up things somewhere.

Cheers,

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