[PATCH 0/3] nvme-core: dhchap_secret code cleanup

[PATCH 0/3] nvme-core: 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

RFC->V1:
1. don't use double pointer based approach use bool instead. (Hannes)

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

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

Following are the blktests results for nvme-loop and nvme-tcp :-
blktests (master) # ./check nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  18.996s  ...  19.272s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.085s  ...  10.107s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.453s  ...  1.440s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.803s  ...  1.773s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.056s  ...  0.060s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.034s  ...  0.032s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.452s  ...  1.464s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.425s  ...  1.461s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  88.076s  ...  87.321s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  71.957s  ...  63.395s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  74.016s  ...  82.918s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  74.105s  ...  63.881s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.089s  ...  4.088s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.703s  ...  3.725s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  12.407s  ...  12.628s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  12.318s  ...  12.614s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.411s  ...  1.429s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.434s  ...  1.449s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.435s  ...  1.424s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime    ...  1.421s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.351s  ...  1.748s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.314s  ...  1.440s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.284s  ...  1.425s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.281s  ...  1.422s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.282s  ...  1.426s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.284s  ...  1.439s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.278s  ...  1.445s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.605s  ...  1.558s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.287s  ...  0.216s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  1.458s  ...  4.031s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.050s  ...  0.013s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.402s  ...  7.902s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  1.148s  ...  0.747s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  7.215s  ...  4.824s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  1.338s  ...  7.028s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.513s  ...  1.858s
nvme/045 (Test re-authentication)                            [passed]
    runtime  5.721s  ...  4.062s
nvme/047 (test different queue types for fabric transports)  [not run]
    runtime  2.288s  ...  
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    runtime  5.590s  ...  
    nvme_trtype=loop is not supported in this test
blktests (master) # nvme_trtype=tcp; ./check nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  19.272s  ...  19.055s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.107s  ...  10.115s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.440s  ...  1.430s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.773s  ...  1.790s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.060s  ...  0.052s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.032s  ...  0.028s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.464s  ...  1.460s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.461s  ...  1.426s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  87.321s  ...  84.286s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  63.395s  ...  66.071s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  82.918s  ...  76.218s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  63.881s  ...  71.368s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.088s  ...  4.205s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.725s  ...  3.734s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  12.628s  ...  12.542s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  12.614s  ...  12.699s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.429s  ...  1.444s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.449s  ...  1.439s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.424s  ...  1.427s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.421s  ...  1.420s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.748s  ...  1.774s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.440s  ...  1.442s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.425s  ...  1.424s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.422s  ...  1.436s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.426s  ...  1.416s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.439s  ...  1.429s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.445s  ...  1.421s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.558s  ...  1.552s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.216s  ...  0.226s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  4.031s  ...  3.913s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.013s  ...  0.013s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.902s  ...  7.893s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.747s  ...  0.748s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.824s  ...  4.817s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  7.028s  ...  3.180s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.858s  ...  1.850s
nvme/045 (Test re-authentication)                            [passed]
    runtime  4.062s  ...  4.039s
nvme/047 (test different queue types for fabric transports)  [not run]
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    nvme_trtype=loop is not supported in this test
-- 
2.40.0

[PATCH 1/3] 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. This helper function will be used by both
nvme_ctrl_dhchap_secret_store() and
nvme_ctrl_dhchap_ctrl_secret_store().

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

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 796e1d373b7c..9ce3b16f06da 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_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);
 	} 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,15 @@ 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);
 
-- 
2.40.0

[PATCH 2/3] nvme: use generic helper to store ctrl secret

[Chaitanya Kulkarni]

Use generic helper to store the dhchap ctrl secret.

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

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 9ce3b16f06da..515baf8d6402 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -498,45 +498,7 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
 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

[PATCH 3/3] 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/sysfs.c | 66 +++++++++++++++------------------------
 1 file changed, 25 insertions(+), 41 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 515baf8d6402..e0a2a041e595 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,37 +461,32 @@ static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
 	return count;
 }
 
+#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 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)
-{
-	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
-}
-
-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 1/3] nvme: add generic helper to store secret

[Sagi Grimberg]

On 5/16/23 13:06, 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. This helper function will be used by both
> nvme_ctrl_dhchap_secret_store() and
> nvme_ctrl_dhchap_ctrl_secret_store().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/sysfs.c | 59 ++++++++++++++++++++++++++-------------
>   1 file changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 796e1d373b7c..9ce3b16f06da 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_host_key;

prev_host_key? or 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_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);
>   	} 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,15 @@ 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);
>   

Re: [PATCH 2/3] nvme: use generic helper to store ctrl secret

[Sagi Grimberg]

Can be squashed to the former.

On 5/16/23 13:06, Chaitanya Kulkarni wrote:
> Use generic helper to store the dhchap ctrl secret.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/sysfs.c | 40 +--------------------------------------
>   1 file changed, 1 insertion(+), 39 deletions(-)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 9ce3b16f06da..515baf8d6402 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -498,45 +498,7 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
>   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,

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

[Sagi Grimberg]

> 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/sysfs.c | 66 +++++++++++++++------------------------
>   1 file changed, 25 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 515baf8d6402..e0a2a041e595 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,37 +461,32 @@ static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>   	return count;
>   }
>   
> +#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)						\

This is where these macros can really take things out of context...
The code just lost readability at this point.

> +		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 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)
> -{
> -	return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
> -}
> -
> -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[] = {

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

[Chaitanya Kulkarni]

On 5/17/23 00:31, Sagi Grimberg wrote:
>
>
> On 5/16/23 13:06, 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. This helper function will be used by both
>> nvme_ctrl_dhchap_secret_store() and
>> nvme_ctrl_dhchap_ctrl_secret_store().
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/sysfs.c | 59 ++++++++++++++++++++++++++-------------
>>   1 file changed, 39 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 796e1d373b7c..9ce3b16f06da 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_host_key;
>
> prev_host_key? or prev_key?


will fix it in the next version ..

-ck