[RFC COMPILE ONLY PATCH 0/2] nvme-core: dhchap_secret code cleanup

[RFC COMPILE ONLY PATCH 0/2] nvme-core: dhchap_secret code cleanup

[Chaitanya Kulkarni]

Hi,

Marking it compile only RFC since blktests are failing on current
nvme-6.4 so I was not able to complete the testing.

Bunch of code is repeated all over when defining nvme_ctrl_dhchap_secret
and nvme_ctrl_dhchap_ctrl_secret. Factor out a common function for
nvme_ctrl_secret_dhchap_store() and nvme_ctrl_secret_dhchap_ctrl_store()
into common function nvme_dhchap_secret_store_common().

Add a macro to define device attr in order to remove code duplication
for above mentioned attributes.

No functional changes in this patch-series.

-ck

Chaitanya Kulkarni (2):
  nvme-core: factor out common code into helper
  nvme-core: use macro defination to define dev attr

 drivers/nvme/host/core.c | 128 ++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 84 deletions(-)

-- 
2.40.0

[RFC PATCH 1/2] nvme-core: factor out common code into helper

[Chaitanya Kulkarni]

nvme_ctrl_dhchap_secrete_store() & nvme_ctrl_dhchap_ctrl_secret store()
share common code. Instead of repeating code into two functions factor
out into helper function.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c | 85 +++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 54 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 44408c0c1762..171ae1f2197a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3779,6 +3779,7 @@ 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)
 {
@@ -3790,41 +3791,40 @@ 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,
+		char **dhchap_secret, struct nvme_dhchap_key **orig_key,
+		const char *buf, size_t count)
 {
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct nvmf_ctrl_options *opts = ctrl->opts;
-	char *dhchap_secret;
+	char *new_dhchap_secret;
 
-	if (!ctrl->opts->dhchap_secret)
+	if (*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_host_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_host_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_host_key);
 	}
 	/* Start re-authentication */
 	dev_info(ctrl->device, "re-authenticating controller\n");
@@ -3832,6 +3832,16 @@ 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, &ctrl->opts->dhchap_secret,
+			&ctrl->host_key, buf, count);
+}
+
 static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
 
@@ -3850,43 +3860,10 @@ 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);
-	}
-	/* 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,
+			&ctrl->opts->dhchap_ctrl_secret, &ctrl->ctrl_key, buf,
+			count);
 }
 static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
-- 
2.40.0

[RFC PATCH 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.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c | 71 +++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 44 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 171ae1f2197a..f619158ea2fc 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3780,17 +3780,6 @@ 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,
 		char **dhchap_secret, struct nvme_dhchap_key **orig_key,
 		const char *buf, size_t count)
@@ -3833,40 +3822,34 @@ 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, &ctrl->opts->dhchap_secret,
-			&ctrl->host_key, buf, count);
-}
-
-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);
+#define NVME_AUTH_DEVICE_ATTR(NAME)					\
+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,			\
+			&ctrl->opts->NAME,				\
+			&ctrl->host_key, buf, count);			\
+}									\
+									\
+static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,				\
+	nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store);		\
 
-	return nvme_dhchap_secret_store_common(ctrl,
-			&ctrl->opts->dhchap_ctrl_secret, &ctrl->ctrl_key, buf,
-			count);
-}
-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);
+NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret);
 #endif
 
 static struct attribute *nvme_dev_attrs[] = {
-- 
2.40.0

Re: [RFC COMPILE ONLY PATCH 0/2] nvme-core: dhchap_secret code cleanup

[Chaitanya Kulkarni]

Hannes,

On 4/27/23 01:02, Chaitanya Kulkarni wrote:
> Hi,
>
> Marking it compile only RFC since blktests are failing on current
> nvme-6.4 so I was not able to complete the testing.
>
> Bunch of code is repeated all over when defining nvme_ctrl_dhchap_secret
> and nvme_ctrl_dhchap_ctrl_secret. Factor out a common function for
> nvme_ctrl_secret_dhchap_store() and nvme_ctrl_secret_dhchap_ctrl_store()
> into common function nvme_dhchap_secret_store_common().
>
> Add a macro to define device attr in order to remove code duplication
> for above mentioned attributes.
>
> No functional changes in this patch-series.
>
> -ck
>
> Chaitanya Kulkarni (2):
>    nvme-core: factor out common code into helper
>    nvme-core: use macro defination to define dev attr
>
>   drivers/nvme/host/core.c | 128 ++++++++++++++-------------------------
>   1 file changed, 44 insertions(+), 84 deletions(-)
>

it will be great if you can provide some feedback, it removes a lot of
duplicated code and potential bugs that are also duplicated,
so we can add this to next PR ...

-ck

Re: [RFC PATCH 1/2] nvme-core: factor out common code into helper

[Hannes Reinecke]

On 4/27/23 10:02, Chaitanya Kulkarni wrote:
> nvme_ctrl_dhchap_secrete_store() & nvme_ctrl_dhchap_ctrl_secret store()
> share common code. Instead of repeating code into two functions factor
> out into helper function.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/core.c | 85 +++++++++++++++-------------------------
>   1 file changed, 31 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 44408c0c1762..171ae1f2197a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3779,6 +3779,7 @@ 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)
>   {
> @@ -3790,41 +3791,40 @@ 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,
> +		char **dhchap_secret, struct nvme_dhchap_key **orig_key,
> +		const char *buf, size_t count)

Hmm. I'm _not_ a big fan of using double pointer in arguments; is always
feels to me like using the wrong abstraction.
Can't you just pass in the ctrl and use a 'bool' variable to switch 
between host and controller secret?
Interface is _far_ cleaner, and the resulting code will be, too, as we 
don't have to deal with pointer variables all the time ...

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: [RFC PATCH 2/2] nvme-core: use macro defination to define dev attr

[Hannes Reinecke]

On 4/27/23 10:02, 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.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/core.c | 71 +++++++++++++++-------------------------
>   1 file changed, 27 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 171ae1f2197a..f619158ea2fc 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3780,17 +3780,6 @@ 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,
>   		char **dhchap_secret, struct nvme_dhchap_key **orig_key,
>   		const char *buf, size_t count)
> @@ -3833,40 +3822,34 @@ 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, &ctrl->opts->dhchap_secret,
> -			&ctrl->host_key, buf, count);
> -}
> -
> -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);
> +#define NVME_AUTH_DEVICE_ATTR(NAME)					\
> +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,			\
> +			&ctrl->opts->NAME,				\
> +			&ctrl->host_key, buf, count);			\
> +}									\
> +									\
> +static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,				\
> +	nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store);		\
>   
> -	return nvme_dhchap_secret_store_common(ctrl,
> -			&ctrl->opts->dhchap_ctrl_secret, &ctrl->ctrl_key, buf,
> -			count);
> -}
> -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);
> +NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret);
>   #endif
>   
>   static struct attribute *nvme_dev_attrs[] = {

In principle ok, but obviously depends on the discussion on the first patch.

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: [RFC PATCH 1/2] nvme-core: factor out common code into helper

[Chaitanya Kulkarni]

On 5/1/23 23:11, Hannes Reinecke wrote:
> On 4/27/23 10:02, Chaitanya Kulkarni wrote:
>> nvme_ctrl_dhchap_secrete_store() & nvme_ctrl_dhchap_ctrl_secret store()
>> share common code. Instead of repeating code into two functions factor
>> out into helper function.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/core.c | 85 +++++++++++++++-------------------------
>>   1 file changed, 31 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 44408c0c1762..171ae1f2197a 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3779,6 +3779,7 @@ 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)
>>   {
>> @@ -3790,41 +3791,40 @@ 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,
>> +        char **dhchap_secret, struct nvme_dhchap_key **orig_key,
>> +        const char *buf, size_t count)
>
> Hmm. I'm _not_ a big fan of using double pointer in arguments; is always
> feels to me like using the wrong abstraction.
> Can't you just pass in the ctrl and use a 'bool' variable to switch 
> between host and controller secret?
> Interface is _far_ cleaner, and the resulting code will be, too, as we 
> don't have to deal with pointer variables all the time ...
>
> Cheers,
>
> Hannes


sounds good, I don't see the reason why bool will not work,
I'll sendout v1 with bool and removing double pointer..


thanks for taking a look ..

-ck