Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret'

[Sagi Grimberg]

On 12/23/22 17:47, Christoph Hellwig wrote:
> Based on the code in nvme_auth_generate_key I assume this is intentional,
> but the code looks really confusing.
>
> Hannes, Sagi, what do you think of something like this:
>
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index d90e4f0c08b7b9..a07eb4cd9ce173 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -455,28 +455,18 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_gen_shared_secret);
>   
> -int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
> +struct nvme_dhchap_key *nvme_auth_generate_key(u8 *secret)
>   {
> -	struct nvme_dhchap_key *key;
>   	u8 key_hash;
>   
> -	if (!secret) {
> -		*ret_key = NULL;
> -		return 0;
> -	}
> +	if (!secret)
> +		return NULL;
>   
>   	if (sscanf(secret, "DHHC-1:%hhd:%*s:", &key_hash) != 1)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
>   	/* Pass in the secret without the 'DHHC-1:XX:' prefix */
> -	key = nvme_auth_extract_key(secret + 10, key_hash);
> -	if (IS_ERR(key)) {
> -		*ret_key = NULL;
> -		return PTR_ERR(key);
> -	}
> -
> -	*ret_key = key;
> -	return 0;
> +	return nvme_auth_extract_key(secret + 10, key_hash);
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
>   
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index bb0abbe4491cdc..c808652966a94f 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -943,16 +943,19 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
>   	INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
>   	if (!ctrl->opts)
>   		return 0;
> -	ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret,
> -			&ctrl->host_key);
> -	if (ret)
> -		return ret;
> -	ret = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret,
> -			&ctrl->ctrl_key);
> -	if (ret)
> +
> +	ctrl->host_key = nvme_auth_generate_key(ctrl->opts->dhchap_secret);
> +	if (IS_ERR(ctrl->host_key)) {

nvme_auth_generate_key can return NULL, so in this case we should avoid
calling it if the secret is null here.

Other than that, this looks good.
Although I think that for this specific report, we should do a simple
fix and then make the code look better.