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

[Sagi Grimberg <sagi@grimberg.me>]

>       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,
--