Re: [PATCH 6/9] nvme: allow to pass in key description as dhchap secret

[Sagi Grimberg <sagi@grimberg.me>]

On 28/05/2025 17:05, Hannes Reinecke wrote:
> In order to use pre-populated keys update the parsing for
> DH-HMAC-CHAP secret to allow for a key description instead
> of an encoded binary secret.

Would it be possible to paste an example "dhchap_secret=%s"
for before and after?

>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
>   drivers/nvme/common/auth.c     | 15 ++++++---
>   drivers/nvme/host/auth.c       | 17 ++++++++--
>   drivers/nvme/host/fabrics.c    | 24 ++++++++++----
>   drivers/nvme/host/fabrics.h    |  4 +++
>   drivers/nvme/host/nvme.h       |  2 ++
>   drivers/nvme/host/sysfs.c      | 60 ++++++++++++++++++++++------------
>   drivers/nvme/target/auth.c     | 20 ++++++++++--
>   drivers/nvme/target/configfs.c | 10 +++---
>   drivers/nvme/target/nvmet.h    |  2 ++
>   include/linux/nvme-auth.h      |  3 +-
>   10 files changed, 116 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index 8c2ccbfb9986..cbf35a7c3105 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -156,14 +156,21 @@ size_t nvme_auth_hmac_hash_len(u8 hmac_id)
>   EXPORT_SYMBOL_GPL(nvme_auth_hmac_hash_len);
>   
>   struct key *nvme_auth_extract_key(struct key *keyring, const u8 *secret,
> -				  size_t secret_len)
> +				  size_t secret_len, bool *generated)
>   {
>   	struct key *key;
>   
> +	key = nvme_dhchap_psk_lookup(keyring, secret);
> +	if (!IS_ERR(key)) {
> +		*generated = false;
> +		return key;
> +	}
>   	key = nvme_dhchap_psk_refresh(keyring, secret, secret_len);
> -	if (!IS_ERR(key))
> -		pr_debug("generated dhchap key %08x\n",
> -			 key_serial(key));
> +	if (!IS_ERR(key)) {
> +		*generated = true;

Can you explain what exactly is being "generated" here?

Is this intermediate key you are generating is because the
user passed in a legacy dhchap_secret?

If this is the case, can we please wrap this call in something like:
nvme_auth_extract_key_legacy() and head it with some documentation?
also the patch description should discuss this. It is not easy to understand
from simply reading the code.

> +		pr_debug("generated dhchap key %s\n",
> +			 key->description);
> +	}
>   	return key;
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_extract_key);
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 0e8a5b544f63..0464e23b2a21 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -1057,19 +1057,29 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
>   static void nvme_auth_clear_key(struct nvme_ctrl *ctrl, bool is_ctrl)
>   {
>   	struct key *key;
> +	bool generated;
>   
>   	if (is_ctrl) {
>   		key = ctrl->ctrl_key;
>   		ctrl->ctrl_key = NULL;
> +		generated = ctrl->ctrl_key_generated;
> +		ctrl->ctrl_key_generated = false;
>   	} else {
>   		key = ctrl->host_key;
>   		ctrl->host_key = NULL;
> +		generated = ctrl->host_key_generated;
> +		ctrl->host_key_generated = false;
>   	}
>   	if (key) {
> -		dev_dbg(ctrl->device, "%s: revoke%s key %08x\n",
> +		if (generated) {
> +			dev_dbg(ctrl->device, "%s: revoke%s key %08x\n",
> +				__func__, is_ctrl ? " ctrl" : "",
> +				key_serial(key));
> +			key_revoke(key);
> +		}
> +		dev_dbg(ctrl->device, "%s: drop%s key %08x\n",
>   			__func__, is_ctrl ? " ctrl" : "",
>   			key_serial(key));
> -		key_revoke(key);
>   		key_put(key);
>   	}
>   }
> @@ -1099,6 +1109,7 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
>   			 key_serial(ctrl->opts->dhchap_key));
>   		return -ENOKEY;
>   	}
> +	ctrl->host_key_generated = ctrl->opts->dhchap_key_generated;

Why do we have this in both ctrl and opts? this is very very confusing.
  I think I mentioned this before, I really don't think that we should 
be adding state to opts.
opts is intended to store user opts and state should remain under ctrl.