On 11/22/21 3:00 PM, Sagi Grimberg wrote:
>
>
> On 11/22/21 3:21 PM, Hannes Reinecke wrote:
>> On 11/22/21 12:59 PM, Sagi Grimberg wrote:
>>>
>>>> +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@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
