[PATCH nvme-7.3 v3 0/4] nvme-fabrics: localize string option parsing

[PATCH nvme-7.3 v3 0/4] nvme-fabrics: localize string option parsing

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

nvmf_parse_options() currently uses the same temporary pointer for option
tokenization and for strings returned by match_strdup().  It also contains
several copies of the same string replacement and DH-CHAP parsing logic.

This series keeps the changes deliberately small.  It separates the option
tokenizer pointer from allocated strings, factors only parsing operations
that are actually shared, and otherwise leaves option-specific code in
place.

The series is split as follows:

  1/4 separates the option tokenizer pointer from the temporary pointer
      used for match_strdup() results.

  2/4 adds nvmf_parse_string_option() for the five direct owned-string
      replacements: transport, traddr, trsvcid, host_traddr and host_iface.

  3/4 reuses that helper for the subsystem NQN replacement only.  The
      existing NQN length check and discovery-NQN update are left
      unchanged, and hostnqn and hostid are not modified.

  4/4 adds nvmf_parse_dhchap_secret() for dhchap_secret and
      dhchap_ctrl_secret.  It reuses nvmf_parse_string_option() for the
      common replacement step and keeps only DHHC-1 validation and
      sensitive rejection cleanup in the DH-CHAP helper.

This series is based on nvme-7.3 after the accepted
"nvme-fabrics: fix DHCHAP secret leak on parse failure" change.

Changes in v3:
  - Rework 4/4 in response to Sagi's feedback.  Reuse
    nvmf_parse_string_option() for the common allocation and ownership
    transfer in nvmf_parse_dhchap_secret().
  - On DH-CHAP validation failure, free the installed value with
    kfree_sensitive() and clear the pointer before returning an error so
    normal option cleanup can run safely.
  - Keep Sagi's Reviewed-by tags on 1/4, 2/4 and 3/4.  Do not carry the
    old 4/4 Reviewed-by because the helper implementation changed.

Changes in v2:
  - Rework 3/4 in response to Sagi's feedback.  Drop the dedicated
    identity helpers and limit the patch to reusing
    nvmf_parse_string_option() for the subsystem NQN replacement.
  - Leave hostnqn, hostid, nqnlen and the discovery-NQN code unchanged.
  - Keep p for the remaining option-specific match_strdup() users.
  - Keep the reviewed DH-CHAP helper logic unchanged; p remains declared
    because hostnqn and hostid still use it.
  - Add Sagi's Reviewed-by tags to 2/4 and 4/4.

Xu Rao (4):
  nvme-fabrics: separate option tokenizer pointer
  nvme-fabrics: add helper for owned string options
  nvme-fabrics: reuse string helper for subsystem NQN
  nvme-fabrics: add helper for DH-CHAP secret options

 drivers/nvme/host/fabrics.c | 118 ++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 65 deletions(-)

-- 
2.50.1

[PATCH nvme-7.3 v3 1/4] nvme-fabrics: separate option tokenizer pointer

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

nvmf_parse_options() currently uses p both for the option returned by
strsep() and for strings allocated by match_strdup().  The former points
into the temporary options buffer while the latter owns a separate
allocation, so the same variable represents two different lifetimes.

Introduce option for the current string token passed to match_token().
Leave p for temporary strings returned by match_strdup(), giving option
tokenization and allocated string storage distinct variables without
changing the parsing logic.

The name option is intentional: options remains the backing buffer for the
full request, option is one comma/newline-delimited entry, and token names
the integer result returned by match_token().

This is a mechanical preparation with no functional change.

Suggested-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 drivers/nvme/host/fabrics.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 59f823dfbbcc..1ec6da49167d 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -716,7 +716,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
 	substring_t args[MAX_OPT_ARGS];
-	char *options, *o, *p;
+	char *options, *o, *option, *p;
 	int token, ret = 0;
 	size_t nqnlen  = 0;
 	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO, key_id;
@@ -747,11 +747,11 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	uuid_copy(&hostid, &nvmf_default_host->id);
 	strscpy(hostnqn, nvmf_default_host->nqn, NVMF_NQN_SIZE);
 
-	while ((p = strsep(&o, ",\n")) != NULL) {
-		if (!*p)
+	while ((option = strsep(&o, ",\n")) != NULL) {
+		if (!*option)
 			continue;
 
-		token = match_token(p, opt_tokens, args);
+		token = match_token(option, opt_tokens, args);
 		opts->mask |= token;
 		switch (token) {
 		case NVMF_OPT_TRANSPORT:
@@ -1068,7 +1068,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
-				p);
+				option);
 			ret = -EINVAL;
 			goto out;
 		}
-- 
2.50.1

Re: [PATCH nvme-7.3 v3 1/4] nvme-fabrics: separate option tokenizer pointer

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH nvme-7.3 v3 2/4] nvme-fabrics: add helper for owned string options

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

Five string options -- transport, traddr, trsvcid, host_traddr and
host_iface -- have exactly the same parsing and ownership rule: duplicate
the matched value, free the previously stored string, and transfer the new
allocation to the corresponding struct nvmf_ctrl_options field.

Add nvmf_parse_string_option() for that common operation.  The helper owns
the match_strdup() result until it either fails or stores the new pointer
in the destination field, keeping this simple allocation lifetime in one
scope and removing five copies of the same sequence from
nvmf_parse_options().

The helper is intentionally limited to direct string replacement and does
not absorb option-specific validation, conversion, or sensitive cleanup.

No functional change is intended.

Suggested-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 drivers/nvme/host/fabrics.c | 53 ++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 30 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 1ec6da49167d..aab3fd279d0e 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -712,6 +712,19 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_ERR,			NULL			}
 };
 
+static int nvmf_parse_string_option(substring_t *args, char **dst)
+{
+	char *value;
+
+	value = match_strdup(args);
+	if (!value)
+		return -ENOMEM;
+
+	kfree(*dst);
+	*dst = value;
+	return 0;
+}
+
 static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
@@ -755,13 +768,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		opts->mask |= token;
 		switch (token) {
 		case NVMF_OPT_TRANSPORT:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->transport);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->transport);
-			opts->transport = p;
 			break;
 		case NVMF_OPT_NQN:
 			p = match_strdup(args);
@@ -783,22 +792,14 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 					 NVME_DISC_SUBSYS_NAME));
 			break;
 		case NVMF_OPT_TRADDR:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->traddr);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->traddr);
-			opts->traddr = p;
 			break;
 		case NVMF_OPT_TRSVCID:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->trsvcid);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->trsvcid);
-			opts->trsvcid = p;
 			break;
 		case NVMF_OPT_QUEUE_SIZE:
 			if (match_int(args, &token)) {
@@ -907,22 +908,14 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			opts->reconnect_delay = token;
 			break;
 		case NVMF_OPT_HOST_TRADDR:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->host_traddr);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->host_traddr);
-			opts->host_traddr = p;
 			break;
 		case NVMF_OPT_HOST_IFACE:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->host_iface);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->host_iface);
-			opts->host_iface = p;
 			break;
 		case NVMF_OPT_HOST_ID:
 			p = match_strdup(args);
-- 
2.50.1

Re: [PATCH nvme-7.3 v3 2/4] nvme-fabrics: add helper for owned string options

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH nvme-7.3 v3 3/4] nvme-fabrics: reuse string helper for subsystem NQN

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

The subsystem NQN starts with the same owned-string replacement sequence
as the direct string options handled by nvmf_parse_string_option():
duplicate the matched value, free the previously stored string, and
transfer ownership of the new allocation.

Reuse nvmf_parse_string_option() for that replacement only.  Leave the
existing NQN length validation and discovery-NQN update unchanged.  The
hostnqn and hostid parsing paths are also left untouched.

This removes another copy of the match_strdup(), free and
ownership-transfer sequence without introducing a dedicated helper or
changing option-specific logic.

No functional change is intended.

Suggested-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 drivers/nvme/host/fabrics.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index aab3fd279d0e..24385e777307 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -773,13 +773,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				goto out;
 			break;
 		case NVMF_OPT_NQN:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
+			ret = nvmf_parse_string_option(args, &opts->subsysnqn);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->subsysnqn);
-			opts->subsysnqn = p;
 			nqnlen = strlen(opts->subsysnqn);
 			if (nqnlen >= NVMF_NQN_SIZE) {
 				pr_err("%s needs to be < %d bytes\n",
-- 
2.50.1

[PATCH nvme-7.3 v3 4/4] nvme-fabrics: add helper for DH-CHAP secret options

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

The dhchap_secret and dhchap_ctrl_secret options share the same string
replacement and DHHC-1 validation rules.  The replacement step can reuse
nvmf_parse_string_option(), leaving only the DH-CHAP-specific validation
in a dedicated helper.

Add nvmf_parse_dhchap_secret() to reuse nvmf_parse_string_option() for the
common allocation and ownership handling, then perform the DH-CHAP-specific
validation.  If validation fails, discard the installed value with
kfree_sensitive() and clear the field so the normal options cleanup can
safely run after nvmf_parse_options() returns an error.

Both secret options share this helper because their parsing and validation
rules are identical.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 drivers/nvme/host/fabrics.c | 47 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 24385e777307..207b05ade022 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -725,6 +725,24 @@ static int nvmf_parse_string_option(substring_t *args, char **dst)
 	return 0;
 }
 
+static int nvmf_parse_dhchap_secret(substring_t *args, char **secret)
+{
+	int ret;
+
+	ret = nvmf_parse_string_option(args, secret);
+	if (ret)
+		return ret;
+
+	if (strlen(*secret) < 11 || strncmp(*secret, "DHHC-1:", 7)) {
+		pr_err("Invalid DH-CHAP secret %s\n", *secret);
+		kfree_sensitive(*secret);
+		*secret = NULL;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
@@ -1010,34 +1028,15 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			opts->discovery_nqn = true;
 			break;
 		case NVMF_OPT_DHCHAP_SECRET:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
-				goto out;
-			}
-			if (strlen(p) < 11 || strncmp(p, "DHHC-1:", 7)) {
-				pr_err("Invalid DH-CHAP secret %s\n", p);
-				kfree_sensitive(p);
-				ret = -EINVAL;
+			ret = nvmf_parse_dhchap_secret(args, &opts->dhchap_secret);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->dhchap_secret);
-			opts->dhchap_secret = p;
 			break;
 		case NVMF_OPT_DHCHAP_CTRL_SECRET:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
-				goto out;
-			}
-			if (strlen(p) < 11 || strncmp(p, "DHHC-1:", 7)) {
-				pr_err("Invalid DH-CHAP secret %s\n", p);
-				kfree_sensitive(p);
-				ret = -EINVAL;
+			ret = nvmf_parse_dhchap_secret(args,
+						       &opts->dhchap_ctrl_secret);
+			if (ret)
 				goto out;
-			}
-			kfree(opts->dhchap_ctrl_secret);
-			opts->dhchap_ctrl_secret = p;
 			break;
 		case NVMF_OPT_TLS:
 			if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
-- 
2.50.1