[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <594b768a-9311-da86-1619-5435d3f720f5@grimberg.me>
Date: Sun, 25 Dec 2022 12:36:07 +0200
From: Sagi Grimberg <sagi@...mberg.me>
To: Christoph Hellwig <hch@....de>, Dan Carpenter <error27@...il.com>
Cc: oe-kbuild@...ts.linux.dev, lkp@...el.com,
oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing
error code? 'ret'
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.
Powered by blists - more mailing lists