[PATCH V2 0/2] nvme: dhchap_secret code cleanup

[PATCH V2 0/2] nvme: dhchap_secret code cleanup

[Chaitanya Kulkarni]

Hi,

Refactor code to avoid duplication and improve maintainability:

Consolidate the shared code between the functions
nvme_ctrl_dhchap_secret_store() and
nvme_ctrl_dhchap_ctrl_secret_store(). This duplication not only
increases the likelihood of bugs but also requires additional effort for 
maintenance and testing.

Introduce a new generic helper function called
nvme_dhchap_secret_store_common() to handle the storage of the
dhchap secret. This helper function will be used by both
nvme_ctrl_dhchap_secret_store() and
nvme_ctrl_dhchap_ctrl_secret_store().

Lastly create a macro to define dhchap attr to remove attribute code
duplication altogether for dhchap secret and dhchap ctrl secret.

Below are the blktests results.

Full disclosure checkpatch has following warning :-

WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
#62: FILE: drivers/nvme/host/sysfs.c:485:

I purposly did not fix it in this series, I'll send out a separate
fix for that if there is no objection.

-ck

V2:- Merge patch 1/2. (Sagi)

Chaitanya Kulkarni (2):
  nvme: add generic helper to store secret
  nvme-core: use macro defination to define dev attr

 drivers/nvme/host/sysfs.c | 145 +++++++++++++++-----------------------
 1 file changed, 55 insertions(+), 90 deletions(-)

blktests (master) # ./check nvme

nvme/002 (create many subsystems and test discovery)         [passed]
    runtime    ...  20.419s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.090s  ...  10.111s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.156s  ...  1.465s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.221s  ...  1.810s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.059s  ...  0.058s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.042s  ...  0.034s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.171s  ...  1.465s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.143s  ...  1.432s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  102.360s  ...  96.545s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  56.073s  ...  68.359s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  83.236s  ...  84.890s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  66.492s  ...  70.922s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  3.964s  ...  4.239s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.507s  ...  3.763s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime    ...  12.382s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime    ...  12.338s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.117s  ...  1.440s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.128s  ...  1.451s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.125s  ...  1.430s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.125s  ...  1.434s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.171s  ...  1.756s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.150s  ...  1.449s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.134s  ...  1.414s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.122s  ...  1.431s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.126s  ...  1.435s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.131s  ...  1.438s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.127s  ...  1.433s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.262s  ...  1.579s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.140s  ...  0.211s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  0.894s  ...  3.968s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.015s  ...  0.013s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.171s  ...  7.909s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.460s  ...  0.741s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  2.891s  ...  4.897s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  0.724s  ...  3.126s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.241s  ...  1.854s
nvme/045 (Test re-authentication)                            [passed]
    runtime  3.804s  ...  4.087s
nvme/047 (test different queue types for fabric transports)  [not run]
    runtime  1.849s  ...  
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    runtime  6.254s  ...  
    nvme_trtype=loop is not supported in this test

-- 
2.40.0

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

[Chaitanya Kulkarni]

Refactor code to avoid duplication and improve maintainability:

Consolidate the shared code between the functions
nvme_ctrl_dhchap_secret_store() and
nvme_ctrl_dhchap_ctrl_secret_store(). This duplication not only
increases the likelihood of bugs but also requires additional effort for
maintenance and testing.

Introduce a new generic helper function called
nvme_dhchap_secret_store_common() to handle the storage of the
dhchap secret. Update nvme_ctrl_dhchap_secret_store() and
nvme_ctrl_dhchap_ctrl_secret_store() to use newly introduced helper.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/sysfs.c | 96 ++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 57 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 796e1d373b7c..98becb7a7453 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -418,43 +418,53 @@ static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
 	return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
 }
 
-static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
+static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
+		const char *buf, size_t count, bool ctrl_secret)
 {
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct nvmf_ctrl_options *opts = ctrl->opts;
-	char *dhchap_secret;
+	struct nvme_dhchap_key **orig_key;
+	char **dhchap_secret;
+	char *new_dhchap_secret;
+
+	if (ctrl_secret) {
+		if (!ctrl->opts->dhchap_ctrl_secret)
+			return -EINVAL;
+		dhchap_secret = &ctrl->opts->dhchap_ctrl_secret;
+		orig_key = &ctrl->ctrl_key;
+	} else {
+		if (!ctrl->opts->dhchap_secret)
+			return -EINVAL;
+		dhchap_secret = &ctrl->opts->dhchap_secret;
+		orig_key = &ctrl->host_key;
+	}
 
-	if (!ctrl->opts->dhchap_secret)
-		return -EINVAL;
 	if (count < 7)
 		return -EINVAL;
 	if (memcmp(buf, "DHHC-1:", 7))
 		return -EINVAL;
 
-	dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
-	if (!dhchap_secret)
+	new_dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
+	if (!new_dhchap_secret)
 		return -ENOMEM;
-	memcpy(dhchap_secret, buf, count);
+	memcpy(new_dhchap_secret, buf, count);
 	nvme_auth_stop(ctrl);
-	if (strcmp(dhchap_secret, opts->dhchap_secret)) {
-		struct nvme_dhchap_key *key, *host_key;
+	if (strcmp(new_dhchap_secret, *dhchap_secret)) {
+		struct nvme_dhchap_key *new_key, *prev_key;
 		int ret;
 
-		ret = nvme_auth_generate_key(dhchap_secret, &key);
+		ret = nvme_auth_generate_key(new_dhchap_secret, &new_key);
 		if (ret) {
-			kfree(dhchap_secret);
+			kfree(new_dhchap_secret);
 			return ret;
 		}
-		kfree(opts->dhchap_secret);
-		opts->dhchap_secret = dhchap_secret;
-		host_key = ctrl->host_key;
+		kfree(*dhchap_secret);
+		*dhchap_secret = new_dhchap_secret;
+		prev_key = *orig_key;
 		mutex_lock(&ctrl->dhchap_auth_mutex);
-		ctrl->host_key = key;
+		*orig_key = new_key;
 		mutex_unlock(&ctrl->dhchap_auth_mutex);
-		nvme_auth_free_key(host_key);
+		nvme_auth_free_key(prev_key);
 	} else
-		kfree(dhchap_secret);
+		kfree(new_dhchap_secret);
 	/* Start re-authentication */
 	dev_info(ctrl->device, "re-authenticating controller\n");
 	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
@@ -462,6 +472,14 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 	return count;
 }
 
+static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
+}
+
 static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
 
@@ -480,44 +498,8 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct nvmf_ctrl_options *opts = ctrl->opts;
-	char *dhchap_secret;
 
-	if (!ctrl->opts->dhchap_ctrl_secret)
-		return -EINVAL;
-	if (count < 7)
-		return -EINVAL;
-	if (memcmp(buf, "DHHC-1:", 7))
-		return -EINVAL;
-
-	dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
-	if (!dhchap_secret)
-		return -ENOMEM;
-	memcpy(dhchap_secret, buf, count);
-	nvme_auth_stop(ctrl);
-	if (strcmp(dhchap_secret, opts->dhchap_ctrl_secret)) {
-		struct nvme_dhchap_key *key, *ctrl_key;
-		int ret;
-
-		ret = nvme_auth_generate_key(dhchap_secret, &key);
-		if (ret) {
-			kfree(dhchap_secret);
-			return ret;
-		}
-		kfree(opts->dhchap_ctrl_secret);
-		opts->dhchap_ctrl_secret = dhchap_secret;
-		ctrl_key = ctrl->ctrl_key;
-		mutex_lock(&ctrl->dhchap_auth_mutex);
-		ctrl->ctrl_key = key;
-		mutex_unlock(&ctrl->dhchap_auth_mutex);
-		nvme_auth_free_key(ctrl_key);
-	} else
-		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);
 }
 
 static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
-- 
2.40.0

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

[Hannes Reinecke]

On 6/5/23 08:51, Chaitanya Kulkarni wrote:
> Refactor code to avoid duplication and improve maintainability:
> 
> Consolidate the shared code between the functions
> nvme_ctrl_dhchap_secret_store() and
> nvme_ctrl_dhchap_ctrl_secret_store(). This duplication not only
> increases the likelihood of bugs but also requires additional effort for
> maintenance and testing.
> 
> Introduce a new generic helper function called
> nvme_dhchap_secret_store_common() to handle the storage of the
> dhchap secret. Update nvme_ctrl_dhchap_secret_store() and
> nvme_ctrl_dhchap_ctrl_secret_store() to use newly introduced helper.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/sysfs.c | 96 ++++++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 57 deletions(-)
> 
Hmm. I distinctly remember having it coded that way once, but later
have it split off into two functions.
But anyway.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Sagi Grimberg]

On 6/5/23 09:51, Chaitanya Kulkarni wrote:
> Refactor code to avoid duplication and improve maintainability:
> 
> Consolidate the shared code between the functions
> nvme_ctrl_dhchap_secret_store() and
> nvme_ctrl_dhchap_ctrl_secret_store(). This duplication not only
> increases the likelihood of bugs but also requires additional effort for
> maintenance and testing.
> 
> Introduce a new generic helper function called
> nvme_dhchap_secret_store_common() to handle the storage of the
> dhchap secret. Update nvme_ctrl_dhchap_secret_store() and
> nvme_ctrl_dhchap_ctrl_secret_store() to use newly introduced helper.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/sysfs.c | 96 ++++++++++++++++-----------------------
>   1 file changed, 39 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 796e1d373b7c..98becb7a7453 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -418,43 +418,53 @@ static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
>   	return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
>   }
>   
> -static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t count)
> +static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
> +		const char *buf, size_t count, bool ctrl_secret)
>   {
> -	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	struct nvmf_ctrl_options *opts = ctrl->opts;
> -	char *dhchap_secret;
> +	struct nvme_dhchap_key **orig_key;
> +	char **dhchap_secret;
> +	char *new_dhchap_secret;
> +
> +	if (ctrl_secret) {
> +		if (!ctrl->opts->dhchap_ctrl_secret)
> +			return -EINVAL;
> +		dhchap_secret = &ctrl->opts->dhchap_ctrl_secret;
> +		orig_key = &ctrl->ctrl_key;
> +	} else {
> +		if (!ctrl->opts->dhchap_secret)
> +			return -EINVAL;
> +		dhchap_secret = &ctrl->opts->dhchap_secret;
> +		orig_key = &ctrl->host_key;
> +	}
>   
> -	if (!ctrl->opts->dhchap_secret)
> -		return -EINVAL;
>   	if (count < 7)
>   		return -EINVAL;
>   	if (memcmp(buf, "DHHC-1:", 7))
>   		return -EINVAL;
>   
> -	dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> -	if (!dhchap_secret)
> +	new_dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> +	if (!new_dhchap_secret)
>   		return -ENOMEM;
> -	memcpy(dhchap_secret, buf, count);
> +	memcpy(new_dhchap_secret, buf, count);
>   	nvme_auth_stop(ctrl);
> -	if (strcmp(dhchap_secret, opts->dhchap_secret)) {
> -		struct nvme_dhchap_key *key, *host_key;
> +	if (strcmp(new_dhchap_secret, *dhchap_secret)) {
> +		struct nvme_dhchap_key *new_key, *prev_key;
>   		int ret;
>   
> -		ret = nvme_auth_generate_key(dhchap_secret, &key);
> +		ret = nvme_auth_generate_key(new_dhchap_secret, &new_key);
>   		if (ret) {
> -			kfree(dhchap_secret);
> +			kfree(new_dhchap_secret);
>   			return ret;
>   		}
> -		kfree(opts->dhchap_secret);
> -		opts->dhchap_secret = dhchap_secret;
> -		host_key = ctrl->host_key;
> +		kfree(*dhchap_secret);
> +		*dhchap_secret = new_dhchap_secret;
> +		prev_key = *orig_key;
>   		mutex_lock(&ctrl->dhchap_auth_mutex);
> -		ctrl->host_key = key;
> +		*orig_key = new_key;
>   		mutex_unlock(&ctrl->dhchap_auth_mutex);
> -		nvme_auth_free_key(host_key);
> +		nvme_auth_free_key(prev_key);
>   	} else
> -		kfree(dhchap_secret);
> +		kfree(new_dhchap_secret);
>   	/* Start re-authentication */
>   	dev_info(ctrl->device, "re-authenticating controller\n");
>   	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
> @@ -462,6 +472,14 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>   	return count;
>   }
>   
> +static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
> +}
> +
>   static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
>   	nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
>   
> @@ -480,44 +498,8 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	struct nvmf_ctrl_options *opts = ctrl->opts;
> -	char *dhchap_secret;
>   
> -	if (!ctrl->opts->dhchap_ctrl_secret)
> -		return -EINVAL;
> -	if (count < 7)
> -		return -EINVAL;
> -	if (memcmp(buf, "DHHC-1:", 7))
> -		return -EINVAL;
> -
> -	dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> -	if (!dhchap_secret)
> -		return -ENOMEM;
> -	memcpy(dhchap_secret, buf, count);
> -	nvme_auth_stop(ctrl);
> -	if (strcmp(dhchap_secret, opts->dhchap_ctrl_secret)) {
> -		struct nvme_dhchap_key *key, *ctrl_key;
> -		int ret;
> -
> -		ret = nvme_auth_generate_key(dhchap_secret, &key);
> -		if (ret) {
> -			kfree(dhchap_secret);
> -			return ret;
> -		}
> -		kfree(opts->dhchap_ctrl_secret);
> -		opts->dhchap_ctrl_secret = dhchap_secret;
> -		ctrl_key = ctrl->ctrl_key;
> -		mutex_lock(&ctrl->dhchap_auth_mutex);
> -		ctrl->ctrl_key = key;
> -		mutex_unlock(&ctrl->dhchap_auth_mutex);
> -		nvme_auth_free_key(ctrl_key);
> -	} else
> -		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?

I think it conveys the point that this is confusing...

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

[Chaitanya Kulkarni]

     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.

-ck

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

[Sagi Grimberg]

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

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

[Chaitanya Kulkarni]

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

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

[hch]

On Tue, Jun 06, 2023 at 09:43:16AM +0300, Sagi Grimberg wrote:
> I understand, but at a point where the commonality becomes confusing
> it can get counter productive.

Agreed.  As-is I don't think the code has very sane structure after
the patch.  If we want to consolidate the code, I'd make the sectet and
key fields a two-element array, with an enum for the indices.  With
that might be possible to share the code without needing extra branches
for which case we're handling.

[PATCH V2 2/2] nvme-core: use macro defination to define dev attr

[Chaitanya Kulkarni]

Insated of duplicating the code for dhchap_secret & dhchap_ctrl_secret,
define a macro to register device attribute to create device attribute
that will also define store and show helpers.

Note that this is not newly invented but followed the same format
preasent in block layer and nvme source code.
nvme_show_str_function/nvme_show_int_function/
nvme_subsys_show_str_function/ nullb_device_##NAME##_show/
nullb_device_##NAME##_store/LOOP_ATTR_RO

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/sysfs.c | 67 +++++++++++++++------------------------
 1 file changed, 25 insertions(+), 42 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 98becb7a7453..ff72bcfb0ff8 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -407,17 +407,6 @@ static ssize_t dctype_show(struct device *dev,
 static DEVICE_ATTR_RO(dctype);
 
 #ifdef CONFIG_NVME_AUTH
-static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct nvmf_ctrl_options *opts = ctrl->opts;
-
-	if (!opts->dhchap_secret)
-		return sysfs_emit(buf, "none\n");
-	return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
-}
-
 static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
 		const char *buf, size_t count, bool ctrl_secret)
 {
@@ -472,38 +461,32 @@ static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
 	return count;
 }
 
-static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-
-	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
-}
-
-static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
-	nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
-
-static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct nvmf_ctrl_options *opts = ctrl->opts;
-
-	if (!opts->dhchap_ctrl_secret)
-		return sysfs_emit(buf, "none\n");
-	return sysfs_emit(buf, "%s\n", opts->dhchap_ctrl_secret);
-}
-
-static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-
-	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
-}
+#define NVME_AUTH_DEVICE_ATTR(NAME, cs)					\
+static ssize_t nvme_ctrl_##NAME##_show(struct device *dev,		\
+		struct device_attribute *attr, char *buf)		\
+{									\
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
+	struct nvmf_ctrl_options *opts = ctrl->opts;			\
+									\
+	if (!opts->NAME)						\
+		return sysfs_emit(buf, "none\n");			\
+	return sysfs_emit(buf, "%s\n", opts->NAME);			\
+}									\
+									\
+static ssize_t nvme_ctrl_##NAME##_store(struct device *dev,		\
+		struct device_attribute *attr, const char *buf,		\
+		size_t count)						\
+{									\
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
+									\
+	return nvme_dhchap_secret_store_common(ctrl, buf, count, cs);	\
+}									\
+									\
+static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,				\
+	nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store);		\
 
-static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
-	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
+NVME_AUTH_DEVICE_ATTR(dhchap_secret, false);
+NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret, true);
 #endif
 
 static struct attribute *nvme_dev_attrs[] = {
-- 
2.40.0

Re: [PATCH V2 2/2] nvme-core: use macro defination to define dev attr

[Hannes Reinecke]

On 6/5/23 08:51, Chaitanya Kulkarni wrote:
> Insated of duplicating the code for dhchap_secret & dhchap_ctrl_secret,
> define a macro to register device attribute to create device attribute
> that will also define store and show helpers.
> 
> Note that this is not newly invented but followed the same format
> preasent in block layer and nvme source code.
> nvme_show_str_function/nvme_show_int_function/
> nvme_subsys_show_str_function/ nullb_device_##NAME##_show/
> nullb_device_##NAME##_store/LOOP_ATTR_RO
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/sysfs.c | 67 +++++++++++++++------------------------
>   1 file changed, 25 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 98becb7a7453..ff72bcfb0ff8 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -407,17 +407,6 @@ static ssize_t dctype_show(struct device *dev,
>   static DEVICE_ATTR_RO(dctype);
>   
>   #ifdef CONFIG_NVME_AUTH
> -static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	struct nvmf_ctrl_options *opts = ctrl->opts;
> -
> -	if (!opts->dhchap_secret)
> -		return sysfs_emit(buf, "none\n");
> -	return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
> -}
> -
>   static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>   		const char *buf, size_t count, bool ctrl_secret)
>   {
> @@ -472,38 +461,32 @@ static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>   	return count;
>   }
>   
> -static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t count)
> -{
> -	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -
> -	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
> -}
> -
> -static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
> -	nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
> -
> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	struct nvmf_ctrl_options *opts = ctrl->opts;
> -
> -	if (!opts->dhchap_ctrl_secret)
> -		return sysfs_emit(buf, "none\n");
> -	return sysfs_emit(buf, "%s\n", opts->dhchap_ctrl_secret);
> -}
> -
> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t count)
> -{
> -	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -
> -	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
> -}
> +#define NVME_AUTH_DEVICE_ATTR(NAME, cs)					\
> +static ssize_t nvme_ctrl_##NAME##_show(struct device *dev,		\
> +		struct device_attribute *attr, char *buf)		\
> +{									\
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
> +	struct nvmf_ctrl_options *opts = ctrl->opts;			\
> +									\
> +	if (!opts->NAME)						\
> +		return sysfs_emit(buf, "none\n");			\
> +	return sysfs_emit(buf, "%s\n", opts->NAME);			\
> +}									\
> +									\
> +static ssize_t nvme_ctrl_##NAME##_store(struct device *dev,		\
> +		struct device_attribute *attr, const char *buf,		\
> +		size_t count)						\
> +{									\
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
> +									\
> +	return nvme_dhchap_secret_store_common(ctrl, buf, count, cs);	\
> +}									\
> +									\
> +static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,				\
> +	nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store);		\
>   
> -static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
> -	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
> +NVME_AUTH_DEVICE_ATTR(dhchap_secret, false);
> +NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret, true);
>   #endif
>   
>   static struct attribute *nvme_dev_attrs[] = {

Is it really worthwhile?
These are just two sysfs attributes which are generalized that way, and 
it's adding quite some boilerplate code to make it work.

Plus I'll plan to rewrite authentication key handling to leverage 
in-kernel keyrings once the TLS stuff has been accepted.

So I'd rather not have this one. But might be convinced if someone has a 
more compelling reason.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH V2 2/2] nvme-core: use macro defination to define dev attr

[Chaitanya Kulkarni]

On 6/5/23 00:55, Hannes Reinecke wrote:
> On 6/5/23 08:51, Chaitanya Kulkarni wrote:
>> Insated of duplicating the code for dhchap_secret & dhchap_ctrl_secret,
>> define a macro to register device attribute to create device attribute
>> that will also define store and show helpers.
>>
>> Note that this is not newly invented but followed the same format
>> preasent in block layer and nvme source code.
>> nvme_show_str_function/nvme_show_int_function/
>> nvme_subsys_show_str_function/ nullb_device_##NAME##_show/
>> nullb_device_##NAME##_store/LOOP_ATTR_RO
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/sysfs.c | 67 +++++++++++++++------------------------
>>   1 file changed, 25 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 98becb7a7453..ff72bcfb0ff8 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -407,17 +407,6 @@ static ssize_t dctype_show(struct device *dev,
>>   static DEVICE_ATTR_RO(dctype);
>>     #ifdef CONFIG_NVME_AUTH
>> -static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
>> -        struct device_attribute *attr, char *buf)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -    struct nvmf_ctrl_options *opts = ctrl->opts;
>> -
>> -    if (!opts->dhchap_secret)
>> -        return sysfs_emit(buf, "none\n");
>> -    return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
>> -}
>> -
>>   static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>>           const char *buf, size_t count, bool ctrl_secret)
>>   {
>> @@ -472,38 +461,32 @@ static ssize_t 
>> nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>>       return count;
>>   }
>>   -static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>> -        struct device_attribute *attr, const char *buf, size_t count)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -
>> -    return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
>> -}
>> -
>> -static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
>> -    nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
>> -
>> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
>> -        struct device_attribute *attr, char *buf)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -    struct nvmf_ctrl_options *opts = ctrl->opts;
>> -
>> -    if (!opts->dhchap_ctrl_secret)
>> -        return sysfs_emit(buf, "none\n");
>> -    return sysfs_emit(buf, "%s\n", opts->dhchap_ctrl_secret);
>> -}
>> -
>> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
>> -        struct device_attribute *attr, const char *buf, size_t count)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -
>> -    return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
>> -}
>> +#define NVME_AUTH_DEVICE_ATTR(NAME, cs)                    \
>> +static ssize_t nvme_ctrl_##NAME##_show(struct device *dev,        \
>> +        struct device_attribute *attr, char *buf)        \
>> +{                                    \
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);            \
>> +    struct nvmf_ctrl_options *opts = ctrl->opts; \
>> +                                    \
>> +    if (!opts->NAME)                        \
>> +        return sysfs_emit(buf, "none\n");            \
>> +    return sysfs_emit(buf, "%s\n", opts->NAME);            \
>> +}                                    \
>> +                                    \
>> +static ssize_t nvme_ctrl_##NAME##_store(struct device *dev,        \
>> +        struct device_attribute *attr, const char *buf, \
>> +        size_t count)                        \
>> +{                                    \
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);            \
>> +                                    \
>> +    return nvme_dhchap_secret_store_common(ctrl, buf, count, cs);    \
>> +}                                    \
>> +                                    \
>> +static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,                \
>> +    nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store); \
>>   -static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
>> -    nvme_ctrl_dhchap_ctrl_secret_show, 
>> nvme_ctrl_dhchap_ctrl_secret_store);
>> +NVME_AUTH_DEVICE_ATTR(dhchap_secret, false);
>> +NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret, true);
>>   #endif
>>     static struct attribute *nvme_dev_attrs[] = {
>
> Is it really worthwhile?
> These are just two sysfs attributes which are generalized that way, 
> and it's adding quite some boilerplate code to make it work.
>
> Plus I'll plan to rewrite authentication key handling to leverage 
> in-kernel keyrings once the TLS stuff has been accepted.
>
> So I'd rather not have this one. But might be convinced if someone has 
> a more compelling reason.
>
> Cheers,
>
> Hannes

let's drop this then, and merge first one ...

-ck