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

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

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

nvmf_parse_options() currently combines option tokenization with string
allocation, validation, conversion, ownership transfer and error cleanup in
one large switch.  The DH-CHAP parse-failure leak showed how easy it is for
a match_strdup() allocation to escape cleanup when those lifetime rules are
spread across the main parser.

This series follows Christoph's suggestion to keep strdup allocations in
the scope that parses the corresponding option and to stop reusing the
pointer used for the option token.  It deliberately does not introduce a
generic parser framework and leaves integer, key and flag options alone.
The goal is only to make the existing string lifetimes local and easy to
audit without changing option semantics.

The series is split by parsing and lifetime semantics:

  1/4 separates tokenization from allocated string storage.  option is the
      current entry returned by strsep() and passed to match_token(); the
      existing p is left only as the temporary match_strdup() pointer and
      disappears as the following helpers are introduced.  Keeping this
      patch mechanical avoids renaming the same temporary variable only to
      delete it again later in the series.

  2/4 adds nvmf_parse_string_option() for the five direct owned-string
      replacements: transport, traddr, trsvcid, host_traddr and host_iface.
      Their complete lifetime is duplicate -> replace old value -> transfer
      ownership, so sharing one helper removes identical code without
      mixing in validation policy.

  3/4 handles non-sensitive identity strings that need additional parsing.
      nvmf_parse_subsysnqn() owns subsystem-NQN replacement, length checking
      and discovery-NQN state; nvmf_parse_hostnqn() validates a temporary
      host NQN and copies it to the host buffer; nvmf_parse_hostid()
      converts the temporary string to uuid_t and frees it on every exit
      path.

  4/4 adds nvmf_parse_dhchap_secret() for dhchap_secret and
      dhchap_ctrl_secret.  These values share DHHC-1 validation and
      sensitive cleanup on rejection, so their resource rules stay out of
      the generic string helper.

The resulting five helpers are intentionally small and self-describing:

  nvmf_parse_string_option()
  nvmf_parse_subsysnqn()
  nvmf_parse_hostnqn()
  nvmf_parse_hostid()
  nvmf_parse_dhchap_secret()

The first helper represents one common ownership operation; the remaining
helpers are named after the exact protocol/identity value they parse.  A
future string option can reuse nvmf_parse_string_option() only if it has the
same direct replacement semantics.  A value with new validation or
conversion rules should get a dedicated helper rather than adding flags to
a generic parser.

This keeps the helpers grouped by ownership and validation semantics rather
than adding one helper per token, avoiding duplication for identical direct
string replacement while still keeping validation and sensitive cleanup local
to the options that need them.

This series is intended to be applied after the recently accepted
"nvme-fabrics: fix DHCHAP secret leak on parse failure" change.

No functional changes are intended.

Xu Rao (4):
  nvme-fabrics: separate option tokenizer pointer
  nvme-fabrics: add helper for owned string options
  nvme-fabrics: add helpers for subsystem and host identity options
  nvme-fabrics: add helper for DH-CHAP secret options

 drivers/nvme/host/fabrics.c | 229 +++++++++++++++++++-----------------
 1 file changed, 121 insertions(+), 108 deletions(-)

-- 
2.50.1

[PATCH nvme-7.3 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().
The existing p variable is then used only for the duplicated string values
in this intermediate step and is removed as those allocations move into
helpers in the following patches.

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>
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 1/4] nvme-fabrics: separate option tokenizer pointer

[Christoph Hellwig]

Looks good:

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

[PATCH nvme-7.3 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.  Options
that need validation, conversion, or sensitive cleanup have different
lifetime rules and are handled separately in the following patches.

No functional change is intended.

Suggested-by: Christoph Hellwig <hch@lst.de>
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 2/4] nvme-fabrics: add helper for owned string options

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH nvme-7.3 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 3/4] nvme-fabrics: add helpers for subsystem and host identity options

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

The nqn, hostnqn and hostid options also start with match_strdup(), but
unlike the direct string replacements handled by nvmf_parse_string_option()
they perform option-specific validation or conversion before parsing is
complete.

Move each lifetime into a helper whose name identifies the value being
parsed:

  nvmf_parse_subsysnqn() handles NVMF_OPT_NQN.  It replaces
  opts->subsysnqn, checks NVMF_NQN_SIZE and updates discovery_nqn.  Using
  subsysnqn in the helper name distinguishes the subsystem NQN from the
  host NQN at the call site.

  nvmf_parse_hostnqn() handles the hostnqn option.  It keeps the existing
  host-assignment check, validates the temporary NQN, copies it to the
  hostnqn buffer and frees the temporary allocation before returning.

  nvmf_parse_hostid() handles the hostid option.  It converts the temporary
  string to uuid_t and frees the duplicated string on both success and
  failure paths.

These three helpers are grouped because they parse non-sensitive identity
values that require validation or conversion rather than a simple owned
string replacement.  Preserve the existing validation and ownership
ordering while moving the code.

No functional change is intended.

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

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index aab3fd279d0e..120e57964cdd 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -725,13 +725,75 @@ static int nvmf_parse_string_option(substring_t *args, char **dst)
 	return 0;
 }
 
+static int nvmf_parse_subsysnqn(struct nvmf_ctrl_options *opts, substring_t *args)
+{
+	char *nqn;
+
+	nqn = match_strdup(args);
+	if (!nqn)
+		return -ENOMEM;
+
+	kfree(opts->subsysnqn);
+	opts->subsysnqn = nqn;
+	if (strlen(opts->subsysnqn) >= NVMF_NQN_SIZE) {
+		pr_err("%s needs to be < %d bytes\n",
+		       opts->subsysnqn, NVMF_NQN_SIZE);
+		return -EINVAL;
+	}
+
+	opts->discovery_nqn = !strcmp(opts->subsysnqn, NVME_DISC_SUBSYS_NAME);
+	return 0;
+}
+
+static int nvmf_parse_hostnqn(struct nvmf_ctrl_options *opts,
+			      substring_t *args, char *hostnqn)
+{
+	char *nqn;
+
+	if (opts->host) {
+		pr_err("hostnqn already user-assigned: %s\n", opts->host->nqn);
+		return -EADDRINUSE;
+	}
+
+	nqn = match_strdup(args);
+	if (!nqn)
+		return -ENOMEM;
+
+	if (strlen(nqn) >= NVMF_NQN_SIZE) {
+		pr_err("%s needs to be < %d bytes\n", nqn, NVMF_NQN_SIZE);
+		kfree(nqn);
+		return -EINVAL;
+	}
+
+	strscpy(hostnqn, nqn, NVMF_NQN_SIZE);
+	kfree(nqn);
+	return 0;
+}
+
+static int nvmf_parse_hostid(substring_t *args, uuid_t *hostid)
+{
+	char *id;
+	int ret;
+
+	id = match_strdup(args);
+	if (!id)
+		return -ENOMEM;
+
+	ret = uuid_parse(id, hostid);
+	if (ret) {
+		pr_err("Invalid hostid %s\n", id);
+		ret = -EINVAL;
+	}
+	kfree(id);
+	return ret;
+}
+
 static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *options, *o, *option, *p;
 	int token, ret = 0;
-	size_t nqnlen  = 0;
 	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO, key_id;
 	uuid_t hostid;
 	char hostnqn[NVMF_NQN_SIZE];
@@ -773,23 +835,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;
-				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",
-					opts->subsysnqn, NVMF_NQN_SIZE);
-				ret = -EINVAL;
+			ret = nvmf_parse_subsysnqn(opts, args);
+			if (ret)
 				goto out;
-			}
-			opts->discovery_nqn =
-				!(strcmp(opts->subsysnqn,
-					 NVME_DISC_SUBSYS_NAME));
 			break;
 		case NVMF_OPT_TRADDR:
 			ret = nvmf_parse_string_option(args, &opts->traddr);
@@ -873,27 +921,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			opts->fast_io_fail_tmo = token;
 			break;
 		case NVMF_OPT_HOSTNQN:
-			if (opts->host) {
-				pr_err("hostnqn already user-assigned: %s\n",
-				       opts->host->nqn);
-				ret = -EADDRINUSE;
-				goto out;
-			}
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
-				goto out;
-			}
-			nqnlen = strlen(p);
-			if (nqnlen >= NVMF_NQN_SIZE) {
-				pr_err("%s needs to be < %d bytes\n",
-					p, NVMF_NQN_SIZE);
-				kfree(p);
-				ret = -EINVAL;
+			ret = nvmf_parse_hostnqn(opts, args, hostnqn);
+			if (ret)
 				goto out;
-			}
-			strscpy(hostnqn, p, NVMF_NQN_SIZE);
-			kfree(p);
 			break;
 		case NVMF_OPT_RECONNECT_DELAY:
 			if (match_int(args, &token)) {
@@ -918,19 +948,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 				goto out;
 			break;
 		case NVMF_OPT_HOST_ID:
-			p = match_strdup(args);
-			if (!p) {
-				ret = -ENOMEM;
-				goto out;
-			}
-			ret = uuid_parse(p, &hostid);
-			if (ret) {
-				pr_err("Invalid hostid %s\n", p);
-				ret = -EINVAL;
-				kfree(p);
+			ret = nvmf_parse_hostid(args, &hostid);
+			if (ret)
 				goto out;
-			}
-			kfree(p);
 			break;
 		case NVMF_OPT_DUP_CONNECT:
 			opts->duplicate_connect = true;
-- 
2.50.1

Re: [PATCH nvme-7.3 3/4] nvme-fabrics: add helpers for subsystem and host identity options

[Sagi Grimberg]

On 21/08/2026 9:25, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
>
> The nqn, hostnqn and hostid options also start with match_strdup(), but
> unlike the direct string replacements handled by nvmf_parse_string_option()
> they perform option-specific validation or conversion before parsing is
> complete.

Does this warrant that they get a special handler? I am not sure I see 
how this helps.

>
> Move each lifetime into a helper whose name identifies the value being
> parsed:
>
>    nvmf_parse_subsysnqn() handles NVMF_OPT_NQN.  It replaces
>    opts->subsysnqn, checks NVMF_NQN_SIZE and updates discovery_nqn.  Using
>    subsysnqn in the helper name distinguishes the subsystem NQN from the
>    host NQN at the call site.
>
>    nvmf_parse_hostnqn() handles the hostnqn option.  It keeps the existing
>    host-assignment check, validates the temporary NQN, copies it to the
>    hostnqn buffer and frees the temporary allocation before returning.
>
>    nvmf_parse_hostid() handles the hostid option.  It converts the temporary
>    string to uuid_t and frees the duplicated string on both success and
>    failure paths.
>
> These three helpers are grouped because they parse non-sensitive identity
> values that require validation or conversion rather than a simple owned
> string replacement.  Preserve the existing validation and ownership
> ordering while moving the code.
>
> No functional change is intended.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>   drivers/nvme/host/fabrics.c | 118 +++++++++++++++++++++---------------
>   1 file changed, 69 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index aab3fd279d0e..120e57964cdd 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -725,13 +725,75 @@ static int nvmf_parse_string_option(substring_t *args, char **dst)
>   	return 0;
>   }
>   
> +static int nvmf_parse_subsysnqn(struct nvmf_ctrl_options *opts, substring_t *args)
> +{
> +	char *nqn;
> +
> +	nqn = match_strdup(args);
> +	if (!nqn)
> +		return -ENOMEM;
> +
> +	kfree(opts->subsysnqn);
> +	opts->subsysnqn = nqn;

The above is nvmf_parse_string_option() no?
> +	if (strlen(opts->subsysnqn) >= NVMF_NQN_SIZE) {
> +		pr_err("%s needs to be < %d bytes\n",
> +		       opts->subsysnqn, NVMF_NQN_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	opts->discovery_nqn = !strcmp(opts->subsysnqn, NVME_DISC_SUBSYS_NAME);

Not sure that this helper existence is really needed, but ok.

Re: [PATCH nvme-7.3 3/4] nvme-fabrics: add helpers for subsystem and host identity options

[Christoph Hellwig]

On Sun, Aug 23, 2026 at 01:11:56AM +0300, Sagi Grimberg wrote:
>
>
> On 21/08/2026 9:25, raoxu wrote:
>> From: Xu Rao <raoxu@uniontech.com>
>>
>> The nqn, hostnqn and hostid options also start with match_strdup(), but
>> unlike the direct string replacements handled by nvmf_parse_string_option()
>> they perform option-specific validation or conversion before parsing is
>> complete.
>
> Does this warrant that they get a special handler? I am not sure I see how 
> this helps.

That doesn't.  But the fact that nvmf_parse_options is more than 400
lines in the current upstream tree is.  For non-trivial parsers
using switch statements, the code flow tends to be a lot nice when
every case is split out into a helper and not in the containing function.

Re: [PATCH nvme-7.3 3/4] nvme-fabrics: add helpers for subsystem and host identity options

[Christoph Hellwig]

On Fri, Aug 21, 2026 at 02:25:28PM +0800, raoxu wrote:
> +static int nvmf_parse_subsysnqn(struct nvmf_ctrl_options *opts, substring_t *args)
> +{
> +	char *nqn;
> +
> +	nqn = match_strdup(args);
> +	if (!nqn)
> +		return -ENOMEM;
> +
> +	kfree(opts->subsysnqn);
> +	opts->subsysnqn = nqn;
> +	if (strlen(opts->subsysnqn) >= NVMF_NQN_SIZE) {
> +		pr_err("%s needs to be < %d bytes\n",
> +		       opts->subsysnqn, NVMF_NQN_SIZE);
> +		return -EINVAL;
> +	}

I think we should only update opts->subsysnqn after the sanity check. The
current code doesn't do that, but we should use the chance to fix that
up as well.

> +	nqn = match_strdup(args);
> +	if (!nqn)
> +		return -ENOMEM;
> +
> +	if (strlen(nqn) >= NVMF_NQN_SIZE) {
> +		pr_err("%s needs to be < %d bytes\n", nqn, NVMF_NQN_SIZE);
> +		kfree(nqn);
> +		return -EINVAL;
> +	}
> +
> +	strscpy(hostnqn, nqn, NVMF_NQN_SIZE);
> +	kfree(nqn);

This already gets is right.  Also does anyone rememeber why we handled
the host vs subsys NQN so differently?

[PATCH nvme-7.3 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 have a distinct string
lifetime because their values are authentication material.  The temporary
string must be checked for the DHHC-1 representation and discarded with
kfree_sensitive() when validation fails before ownership is transferred.

Add nvmf_parse_dhchap_secret() to keep match_strdup(), DHHC-1 validation,
failure cleanup, replacement of the old value and successful ownership
transfer in one scope.  Both secret options use the same helper because
their parsing and ownership rules are identical.

Keep the existing replacement semantics for an already stored valid secret;
this patch only moves the parsing and temporary allocation lifetime into
the helper.

No functional change is intended.

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

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 120e57964cdd..c5ff03ab9e36 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -788,11 +788,30 @@ static int nvmf_parse_hostid(substring_t *args, uuid_t *hostid)
 	return ret;
 }
 
+static int nvmf_parse_dhchap_secret(substring_t *args, char **secret)
+{
+	char *value;
+
+	value = match_strdup(args);
+	if (!value)
+		return -ENOMEM;
+
+	if (strlen(value) < 11 || strncmp(value, "DHHC-1:", 7)) {
+		pr_err("Invalid DH-CHAP secret %s\n", value);
+		kfree_sensitive(value);
+		return -EINVAL;
+	}
+
+	kfree(*secret);
+	*secret = value;
+	return 0;
+}
+
 static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		const char *buf)
 {
 	substring_t args[MAX_OPT_ARGS];
-	char *options, *o, *option, *p;
+	char *options, *o, *option;
 	int token, ret = 0;
 	int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO, key_id;
 	uuid_t hostid;
@@ -1034,34 +1053,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

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

[Sagi Grimberg]

ok.

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[Christoph Hellwig]

Looks good:

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