* [PATCH 0/3] nvme-core: dhchap_secret code cleanup
@ 2023-05-16 10:06 Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 1/3] nvme: add generic helper to store secret Chaitanya Kulkarni
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Chaitanya Kulkarni @ 2023-05-16 10:06 UTC (permalink / raw)
To: hare; +Cc: kbusch, hch, sagi, linux-nvme, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] nvme: add generic helper to store secret
2023-05-16 10:06 [PATCH 0/3] nvme-core: dhchap_secret code cleanup Chaitanya Kulkarni
@ 2023-05-16 10:06 ` Chaitanya Kulkarni
2023-05-17 7:31 ` Sagi Grimberg
2023-05-16 10:06 ` [PATCH 2/3] nvme: use generic helper to store ctrl secret Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 3/3] nvme-core: use macro defination to define dev attr Chaitanya Kulkarni
2 siblings, 1 reply; 8+ messages in thread
From: Chaitanya Kulkarni @ 2023-05-16 10:06 UTC (permalink / raw)
To: hare; +Cc: kbusch, hch, sagi, linux-nvme, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] nvme: use generic helper to store ctrl secret
2023-05-16 10:06 [PATCH 0/3] nvme-core: dhchap_secret code cleanup Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 1/3] nvme: add generic helper to store secret Chaitanya Kulkarni
@ 2023-05-16 10:06 ` Chaitanya Kulkarni
2023-05-17 7:32 ` Sagi Grimberg
2023-05-16 10:06 ` [PATCH 3/3] nvme-core: use macro defination to define dev attr Chaitanya Kulkarni
2 siblings, 1 reply; 8+ messages in thread
From: Chaitanya Kulkarni @ 2023-05-16 10:06 UTC (permalink / raw)
To: hare; +Cc: kbusch, hch, sagi, linux-nvme, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] nvme-core: use macro defination to define dev attr
2023-05-16 10:06 [PATCH 0/3] nvme-core: dhchap_secret code cleanup Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 1/3] nvme: add generic helper to store secret Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 2/3] nvme: use generic helper to store ctrl secret Chaitanya Kulkarni
@ 2023-05-16 10:06 ` Chaitanya Kulkarni
2023-05-17 7:35 ` Sagi Grimberg
2 siblings, 1 reply; 8+ messages in thread
From: Chaitanya Kulkarni @ 2023-05-16 10:06 UTC (permalink / raw)
To: hare; +Cc: kbusch, hch, sagi, linux-nvme, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] nvme: add generic helper to store secret
2023-05-16 10:06 ` [PATCH 1/3] nvme: add generic helper to store secret Chaitanya Kulkarni
@ 2023-05-17 7:31 ` Sagi Grimberg
2023-05-17 21:05 ` Chaitanya Kulkarni
0 siblings, 1 reply; 8+ messages in thread
From: Sagi Grimberg @ 2023-05-17 7:31 UTC (permalink / raw)
To: Chaitanya Kulkarni, hare; +Cc: kbusch, hch, linux-nvme
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] nvme: use generic helper to store ctrl secret
2023-05-16 10:06 ` [PATCH 2/3] nvme: use generic helper to store ctrl secret Chaitanya Kulkarni
@ 2023-05-17 7:32 ` Sagi Grimberg
0 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2023-05-17 7:32 UTC (permalink / raw)
To: Chaitanya Kulkarni, hare; +Cc: kbusch, hch, linux-nvme
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,
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] nvme-core: use macro defination to define dev attr
2023-05-16 10:06 ` [PATCH 3/3] nvme-core: use macro defination to define dev attr Chaitanya Kulkarni
@ 2023-05-17 7:35 ` Sagi Grimberg
0 siblings, 0 replies; 8+ messages in thread
From: Sagi Grimberg @ 2023-05-17 7:35 UTC (permalink / raw)
To: Chaitanya Kulkarni, hare; +Cc: kbusch, hch, linux-nvme
> 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[] = {
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] nvme: add generic helper to store secret
2023-05-17 7:31 ` Sagi Grimberg
@ 2023-05-17 21:05 ` Chaitanya Kulkarni
0 siblings, 0 replies; 8+ messages in thread
From: Chaitanya Kulkarni @ 2023-05-17 21:05 UTC (permalink / raw)
To: Sagi Grimberg, Chaitanya Kulkarni, hare; +Cc: kbusch, hch, linux-nvme
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-17 21:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-16 10:06 [PATCH 0/3] nvme-core: dhchap_secret code cleanup Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 1/3] nvme: add generic helper to store secret Chaitanya Kulkarni
2023-05-17 7:31 ` Sagi Grimberg
2023-05-17 21:05 ` Chaitanya Kulkarni
2023-05-16 10:06 ` [PATCH 2/3] nvme: use generic helper to store ctrl secret Chaitanya Kulkarni
2023-05-17 7:32 ` Sagi Grimberg
2023-05-16 10:06 ` [PATCH 3/3] nvme-core: use macro defination to define dev attr Chaitanya Kulkarni
2023-05-17 7:35 ` Sagi Grimberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).