Re: [PATCH V2 1/2] nvme: add generic helper to store secret

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 6/5/2023 11:43 PM, Sagi Grimberg wrote:
> 
>>       kfree(dhchap_secret);
>>>> -    /* Start re-authentication */
>>>> -    dev_info(ctrl->device, "re-authenticating controller\n");
>>>> -    queue_work(nvme_wq, &ctrl->dhchap_auth_work);
>>>> -
>>>> -    return count;
>>>> +    return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
>>>
>>> Both call with ctrl_secret=false?
>>>
>>
>> this is wrong ..
>>
>> This is my mistake let me fix this and send V4.
>>
>>> I think it conveys the point that this is confusing...
>>
>> The reason I did this because original code had kfree() bug and that bug
>> was replicated in both the functions, with fixing the mistake, I believe
>> we can avoid such replication of code and potential bugs.
> 
> I understand, but at a point where the commonality becomes confusing
> it can get counter productive.
> 
> BTW while we're at it, I think that the extra check that the new
> secret matches the old secret is redundant, we already allocated
> the new secret, we can simply swap them (or compare before we allocate
> anything). Perhaps we are better off to fixing the reason we had a leak
> in the first place (convoluted ordering).
> 
> Lets do something like:
> -- 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 796e1d373b7c..51d980cdbb80 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -423,7 +423,9 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct 
> device *dev,
>   {
>          struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>          struct nvmf_ctrl_options *opts = ctrl->opts;
> +       struct nvme_dhchap_key *key, *host_key;
>          char *dhchap_secret;
> +       int ret;
> 
>          if (!ctrl->opts->dhchap_secret)
>                  return -EINVAL;
> @@ -431,35 +433,35 @@ static ssize_t 
> nvme_ctrl_dhchap_secret_store(struct device *dev,
>                  return -EINVAL;
>          if (memcmp(buf, "DHHC-1:", 7))
>                  return -EINVAL;
> +       if (!memcmp(buf, opts->dhchap_secret, count))
> +               return 0;
> 
> -       dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> +       dhchap_secret = kstrdup(buf, GFP_KERNEL);
>          if (!dhchap_secret)
>                  return -ENOMEM;
> -       memcpy(dhchap_secret, buf, count);
> +
> +       ret = nvme_auth_generate_key(dhchap_secret, &key);
> +       if (ret)
> +               goto err_key;
> +
>          nvme_auth_stop(ctrl);
> -       if (strcmp(dhchap_secret, opts->dhchap_secret)) {
> -               struct nvme_dhchap_key *key, *host_key;
> -               int ret;
> 
> -               ret = nvme_auth_generate_key(dhchap_secret, &key);
> -               if (ret) {
> -                       kfree(dhchap_secret);
> -                       return ret;
> -               }
> -               kfree(opts->dhchap_secret);
> -               opts->dhchap_secret = dhchap_secret;
> -               host_key = ctrl->host_key;
> -               mutex_lock(&ctrl->dhchap_auth_mutex);
> -               ctrl->host_key = key;
> -               mutex_unlock(&ctrl->dhchap_auth_mutex);
> -               nvme_auth_free_key(host_key);
> -       } else
> -               kfree(dhchap_secret);
> +       kfree(opts->dhchap_secret);
> +       opts->dhchap_secret = dhchap_secret;
> +       host_key = ctrl->host_key;
> +       mutex_lock(&ctrl->dhchap_auth_mutex);
> +       ctrl->host_key = key;
> +       mutex_unlock(&ctrl->dhchap_auth_mutex);
> +       nvme_auth_free_key(host_key);
> +
>          /* Start re-authentication */
>          dev_info(ctrl->device, "re-authenticating controller\n");
>          queue_work(nvme_wq, &ctrl->dhchap_auth_work);
> 
>          return count;
> +err_key:
> +       kfree(dhchap_secret);
> +       return ret;
>   }
> 
>   static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
> -- 

sounds good, Hannes are you okay with this ?

-ck