Subject: Re: [PATCH 10/12] nvmet: Implement basic In-Band Authentication
To:     Hannes Reinecke <hare@suse.de>
Cc:     Christoph Hellwig <hch@lst.de>, Keith Busch <keith.busch@wdc.com>,
        linux-nvme@lists.infradead.org,
        Herbert Xu <herberg@gondor.apana.org.au>,
        David Miller <davem@davemloft.org>,
        linux-crypto@vger.kernel.org
References: <20211122074727.25988-1-hare@suse.de>
 <20211122074727.25988-11-hare@suse.de>
 <762ce404-9035-30ca-078d-eb0b36223e4c@grimberg.me>
 <313b19d2-ea54-ce1c-f9cb-3f1fb6743787@suse.de>
 <891fe077-40d3-5bad-52fb-f0ee6f6107b6@grimberg.me>
 <f7011868-4542-bd09-6d44-3aaa192a9212@suse.de>
From:   Sagi Grimberg <sagi@grimberg.me>
Message-ID: <27432703-8e83-10c0-c428-090e6ec4b9af@grimberg.me>
Date:   Mon, 22 Nov 2021 16:31:13 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <f7011868-4542-bd09-6d44-3aaa192a9212@suse.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


>>>>> +    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.

OK, I see.

> Always possible that I messed up things somewhere.

Worth checking with kmemleak but it looks ok to me.