* drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret'
@ 2022-12-23 11:03 Dan Carpenter
2022-12-23 15:47 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-12-23 11:03 UTC (permalink / raw)
To: oe-kbuild, Sagi Grimberg
Cc: lkp, oe-kbuild-all, linux-kernel, Christoph Hellwig
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 9d2f6060fe4c3b49d0cdc1dce1c99296f33379c8
commit: aa36d711e945e65fa87410927800f01878a8faed nvme-auth: convert dhchap_auth_list to an array
config: i386-randconfig-m021-20221219
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
smatch warnings:
drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret'
vim +/ret +950 drivers/nvme/host/auth.c
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 931 int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
f50fff73d620cd Hannes Reinecke 2022-06-27 932 {
aa36d711e945e6 Sagi Grimberg 2022-11-13 933 struct nvme_dhchap_queue_context *chap;
aa36d711e945e6 Sagi Grimberg 2022-11-13 934 int i, ret;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 935
f50fff73d620cd Hannes Reinecke 2022-06-27 936 mutex_init(&ctrl->dhchap_auth_mutex);
aa36d711e945e6 Sagi Grimberg 2022-11-13 937 INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
f50fff73d620cd Hannes Reinecke 2022-06-27 938 if (!ctrl->opts)
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 939 return 0;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 940 ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret,
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 941 &ctrl->host_key);
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 942 if (ret)
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 943 return ret;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 944 ret = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret,
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 945 &ctrl->ctrl_key);
aa36d711e945e6 Sagi Grimberg 2022-11-13 946 if (ret)
aa36d711e945e6 Sagi Grimberg 2022-11-13 947 goto err_free_dhchap_secret;
aa36d711e945e6 Sagi Grimberg 2022-11-13 948
aa36d711e945e6 Sagi Grimberg 2022-11-13 949 if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret)
aa36d711e945e6 Sagi Grimberg 2022-11-13 @950 return ret;
Please return a literal here. Either return -EINVAL or return 0;
aa36d711e945e6 Sagi Grimberg 2022-11-13 951
aa36d711e945e6 Sagi Grimberg 2022-11-13 952 ctrl->dhchap_ctxs = kvcalloc(ctrl_max_dhchaps(ctrl),
aa36d711e945e6 Sagi Grimberg 2022-11-13 953 sizeof(*chap), GFP_KERNEL);
aa36d711e945e6 Sagi Grimberg 2022-11-13 954 if (!ctrl->dhchap_ctxs) {
aa36d711e945e6 Sagi Grimberg 2022-11-13 955 ret = -ENOMEM;
aa36d711e945e6 Sagi Grimberg 2022-11-13 956 goto err_free_dhchap_ctrl_secret;
aa36d711e945e6 Sagi Grimberg 2022-11-13 957 }
aa36d711e945e6 Sagi Grimberg 2022-11-13 958
aa36d711e945e6 Sagi Grimberg 2022-11-13 959 for (i = 0; i < ctrl_max_dhchaps(ctrl); i++) {
aa36d711e945e6 Sagi Grimberg 2022-11-13 960 chap = &ctrl->dhchap_ctxs[i];
aa36d711e945e6 Sagi Grimberg 2022-11-13 961 chap->qid = i;
aa36d711e945e6 Sagi Grimberg 2022-11-13 962 chap->ctrl = ctrl;
aa36d711e945e6 Sagi Grimberg 2022-11-13 963 INIT_WORK(&chap->auth_work, nvme_queue_auth_work);
aa36d711e945e6 Sagi Grimberg 2022-11-13 964 }
aa36d711e945e6 Sagi Grimberg 2022-11-13 965
aa36d711e945e6 Sagi Grimberg 2022-11-13 966 return 0;
aa36d711e945e6 Sagi Grimberg 2022-11-13 967 err_free_dhchap_ctrl_secret:
aa36d711e945e6 Sagi Grimberg 2022-11-13 968 nvme_auth_free_key(ctrl->ctrl_key);
aa36d711e945e6 Sagi Grimberg 2022-11-13 969 ctrl->ctrl_key = NULL;
aa36d711e945e6 Sagi Grimberg 2022-11-13 970 err_free_dhchap_secret:
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 971 nvme_auth_free_key(ctrl->host_key);
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 972 ctrl->host_key = NULL;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 973 return ret;
f50fff73d620cd Hannes Reinecke 2022-06-27 974 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' 2022-12-23 11:03 drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' Dan Carpenter @ 2022-12-23 15:47 ` Christoph Hellwig 2022-12-23 16:21 ` Dan Carpenter 2022-12-25 10:36 ` Sagi Grimberg 0 siblings, 2 replies; 6+ messages in thread From: Christoph Hellwig @ 2022-12-23 15:47 UTC (permalink / raw) To: Dan Carpenter Cc: oe-kbuild, Sagi Grimberg, lkp, oe-kbuild-all, linux-kernel, Christoph Hellwig 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)) { + ret = PTR_ERR(ctrl->host_key); + goto out; + } + ctrl->ctrl_key = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret); + if (IS_ERR(ctrl->ctrl_key)) { + ret = PTR_ERR(ctrl->ctrl_key); goto err_free_dhchap_secret; + } - if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret) + if (!ctrl->host_key && !ctrl->ctrl_key) return ret; ctrl->dhchap_ctxs = kvcalloc(ctrl_max_dhchaps(ctrl), @@ -972,9 +975,10 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl) return 0; err_free_dhchap_ctrl_secret: nvme_auth_free_key(ctrl->ctrl_key); - ctrl->ctrl_key = NULL; err_free_dhchap_secret: + ctrl->ctrl_key = NULL; nvme_auth_free_key(ctrl->host_key); +out: ctrl->host_key = NULL; return ret; } diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index f72fc3bd07c348..6245f1c87c5ba8 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3751,11 +3751,10 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev, nvme_auth_stop(ctrl); if (strcmp(dhchap_secret, opts->dhchap_secret)) { struct nvme_dhchap_key *key, *host_key; - int ret; - ret = nvme_auth_generate_key(dhchap_secret, &key); - if (ret) - return ret; + key = nvme_auth_generate_key(dhchap_secret); + if (IS_ERR(key)) + return PTR_ERR(key); kfree(opts->dhchap_secret); opts->dhchap_secret = dhchap_secret; host_key = ctrl->host_key; @@ -3805,11 +3804,10 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev, nvme_auth_stop(ctrl); if (strcmp(dhchap_secret, opts->dhchap_ctrl_secret)) { struct nvme_dhchap_key *key, *ctrl_key; - int ret; - ret = nvme_auth_generate_key(dhchap_secret, &key); - if (ret) - return ret; + key = nvme_auth_generate_key(dhchap_secret); + if (IS_ERR(key)) + return PTR_ERR(key); kfree(opts->dhchap_ctrl_secret); opts->dhchap_ctrl_secret = dhchap_secret; ctrl_key = ctrl->ctrl_key; diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h index dcb8030062ddaf..df82d7950ee204 100644 --- a/include/linux/nvme-auth.h +++ b/include/linux/nvme-auth.h @@ -28,7 +28,7 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret, u8 key_hash); void nvme_auth_free_key(struct nvme_dhchap_key *key); u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn); -int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key); +struct nvme_dhchap_key *nvme_auth_generate_key(u8 *secret); int nvme_auth_augmented_challenge(u8 hmac_id, u8 *skey, size_t skey_len, u8 *challenge, u8 *aug, size_t hlen); int nvme_auth_gen_privkey(struct crypto_kpp *dh_tfm, u8 dh_gid); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' 2022-12-23 15:47 ` Christoph Hellwig @ 2022-12-23 16:21 ` Dan Carpenter 2022-12-23 16:42 ` Christoph Hellwig 2022-12-25 10:36 ` Sagi Grimberg 1 sibling, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2022-12-23 16:21 UTC (permalink / raw) To: Christoph Hellwig Cc: oe-kbuild, Sagi Grimberg, lkp, oe-kbuild-all, linux-kernel On Fri, Dec 23, 2022 at 04:47:54PM +0100, Christoph Hellwig wrote: > 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)) { > + ret = PTR_ERR(ctrl->host_key); > + goto out; > + } > + ctrl->ctrl_key = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret); > + if (IS_ERR(ctrl->ctrl_key)) { > + ret = PTR_ERR(ctrl->ctrl_key); > goto err_free_dhchap_secret; > + } > > - if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret) > + if (!ctrl->host_key && !ctrl->ctrl_key) > return ret; ret is uninitialized now. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' 2022-12-23 16:21 ` Dan Carpenter @ 2022-12-23 16:42 ` Christoph Hellwig 0 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2022-12-23 16:42 UTC (permalink / raw) To: Dan Carpenter Cc: Christoph Hellwig, oe-kbuild, Sagi Grimberg, lkp, oe-kbuild-all, linux-kernel On Fri, Dec 23, 2022 at 07:21:49PM +0300, Dan Carpenter wrote: > > - if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret) > > + if (!ctrl->host_key && !ctrl->ctrl_key) > > return ret; > > ret is uninitialized now. Yes. Should be a 'return 0'. We really need to turn the maybe uninitialized warnings back on. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' 2022-12-23 15:47 ` Christoph Hellwig 2022-12-23 16:21 ` Dan Carpenter @ 2022-12-25 10:36 ` Sagi Grimberg 2022-12-27 17:53 ` Dan Carpenter 1 sibling, 1 reply; 6+ messages in thread From: Sagi Grimberg @ 2022-12-25 10:36 UTC (permalink / raw) To: Christoph Hellwig, Dan Carpenter Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' 2022-12-25 10:36 ` Sagi Grimberg @ 2022-12-27 17:53 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2022-12-27 17:53 UTC (permalink / raw) To: Sagi Grimberg Cc: Christoph Hellwig, oe-kbuild, lkp, oe-kbuild-all, linux-kernel On Sun, Dec 25, 2022 at 12:36:07PM +0200, Sagi Grimberg wrote: > Although I think that for this specific report, we should do a simple > fix and then make the code look better. It turned out the code worked fine as-is. It just wasn't clear. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-27 17:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-23 11:03 drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' Dan Carpenter 2022-12-23 15:47 ` Christoph Hellwig 2022-12-23 16:21 ` Dan Carpenter 2022-12-23 16:42 ` Christoph Hellwig 2022-12-25 10:36 ` Sagi Grimberg 2022-12-27 17:53 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox