From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
Herbert Xu <herbert@gondor.apana.org.au>
Cc: Nathan Chancellor <nathan@kernel.org>,
Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
Vladis Dronov <vdronov@redhat.com>,
Simo Sorce <ssorce@redhat.com>,
Eric Biggers <ebiggers@kernel.org>,
"llvm@lists.linux.dev" <llvm@lists.linux.dev>,
kernel test robot <lkp@intel.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] crypto: api - Do not create test larvals if manager is disabled
Date: Thu, 4 Nov 2021 07:28:51 +0000 [thread overview]
Message-ID: <DM6PR04MB708155E447FD9A79AB89686DE78D9@DM6PR04MB7081.namprd04.prod.outlook.com> (raw)
In-Reply-To: alpine.DEB.2.22.394.2111021636040.2330984@ramsan.of.borg
On 2021/11/03 0:41, Geert Uytterhoeven wrote:
> Hi Herbert,
>
> On Tue, 19 Oct 2021, Herbert Xu wrote:
>> On Tue, Oct 05, 2021 at 07:33:28PM -0700, Nathan Chancellor wrote:
>>> I assume this is the diff you mean? This does not resolve the issue. My
>>> apologies if I am slow to respond, I am on vacation until the middle of
>>> next week.
>>
>> Sorry for the delay. The kernel robot figured out the problem
>> for me. It's the crypto_alg_tested call that causes api.c to
>> depend on algapi.c. This call is only invoked in the case where
>> the crypto manager is turned off. We could instead simply make
>> test larvals disappear in that case.
>>
>> ---8<---
>> The delayed boot-time testing patch created a dependency loop
>> between api.c and algapi.c because it added a crypto_alg_tested
>> call to the former when the crypto manager is disabled.
>>
>> We could instead avoid creating the test larvals if the crypto
>> manager is disabled. This avoids the dependency loop as well
>> as saving some unnecessary work, albeit in a very unlikely case.
>>
>> Reported-by: Nathan Chancellor <nathan@kernel.org>
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Fixes: adad556efcdd ("crypto: api - Fix built-in testing dependency failures")
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Thanks for your patch, which is now commit cad439fc040efe5f
> ("crypto: api - Do not create test larvals if manager is disabled").
>
> I have bisected a failure to mount the root file system on k210 to this
> commit.
>
> Dmesg before/after:
>
> mmcblk0: mmc0:0000 SA04G 3.68 GiB
> random: fast init done
> mmcblk0: p1
> -EXT4-fs (mmcblk0p1): mounted filesystem with ordered data mode. Opts: (null). Quota mode: disabled.
> -VFS: Mounted root (ext4 filesystem) readonly on device 179:1.
> +EXT4-fs (mmcblk0p1): Cannot load crc32c driver.
> +VFS: Cannot open root device "mmcblk0p1" or unknown-block(179,1): error -80
p1 exist as the message 2 lines above shows. And since the mount error is -80
(ELIBBAD), it is really all about crypto. Since the default k210 config compile
everything in-kernel (no modules), it should work. Was crc32c compiled as a
module ? If yes, then the k210 will need to be booted with U-Boot and use a real
initrd, which likely will all end-up in a no memory situation. ext4 in itself
will consume way too much memory...
> +Please append a correct "root=" boot option; here are the available partitions:
> +b300 3858432 mmcblk0
> + driver: mmcblk
> + b301 3854336 mmcblk0p1 00000000-01
>
>> --- a/crypto/algapi.c
>> +++ b/crypto/algapi.c
>> @@ -216,6 +216,32 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>> }
>> EXPORT_SYMBOL_GPL(crypto_remove_spawns);
>>
>> +static struct crypto_larval *crypto_alloc_test_larval(struct crypto_alg *alg)
>> +{
>> + struct crypto_larval *larval;
>> +
>> + if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER))
>> + return NULL;
>> +
>> + larval = crypto_larval_alloc(alg->cra_name,
>> + alg->cra_flags | CRYPTO_ALG_TESTED, 0);
>> + if (IS_ERR(larval))
>> + return larval;
>> +
>> + larval->adult = crypto_mod_get(alg);
>> + if (!larval->adult) {
>> + kfree(larval);
>> + return ERR_PTR(-ENOENT);
>> + }
>> +
>> + refcount_set(&larval->alg.cra_refcnt, 1);
>> + memcpy(larval->alg.cra_driver_name, alg->cra_driver_name,
>> + CRYPTO_MAX_ALG_NAME);
>> + larval->alg.cra_priority = alg->cra_priority;
>> +
>> + return larval;
>> +}
>> +
>> static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
>> {
>> struct crypto_alg *q;
>> @@ -250,31 +276,20 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
>> goto err;
>> }
>>
>> - larval = crypto_larval_alloc(alg->cra_name,
>> - alg->cra_flags | CRYPTO_ALG_TESTED, 0);
>> + larval = crypto_alloc_test_larval(alg);
>> if (IS_ERR(larval))
>> goto out;
>>
>> - ret = -ENOENT;
>> - larval->adult = crypto_mod_get(alg);
>> - if (!larval->adult)
>> - goto free_larval;
>> -
>> - refcount_set(&larval->alg.cra_refcnt, 1);
>> - memcpy(larval->alg.cra_driver_name, alg->cra_driver_name,
>> - CRYPTO_MAX_ALG_NAME);
>> - larval->alg.cra_priority = alg->cra_priority;
>> -
>> list_add(&alg->cra_list, &crypto_alg_list);
>> - list_add(&larval->alg.cra_list, &crypto_alg_list);
>> +
>> + if (larval)
>> + list_add(&larval->alg.cra_list, &crypto_alg_list);
>>
>> crypto_stats_init(alg);
>>
>> out:
>> return larval;
>>
>> -free_larval:
>> - kfree(larval);
>> err:
>> larval = ERR_PTR(ret);
>> goto out;
>> @@ -403,10 +418,11 @@ int crypto_register_alg(struct crypto_alg *alg)
>> down_write(&crypto_alg_sem);
>> larval = __crypto_register_alg(alg);
>> test_started = static_key_enabled(&crypto_boot_test_finished);
>> - larval->test_started = test_started;
>> + if (!IS_ERR_OR_NULL(larval))
>> + larval->test_started = test_started;
>> up_write(&crypto_alg_sem);
>>
>> - if (IS_ERR(larval))
>> + if (IS_ERR_OR_NULL(larval))
>> return PTR_ERR(larval);
>>
>> if (test_started)
>> @@ -616,8 +632,8 @@ int crypto_register_instance(struct crypto_template *tmpl,
>> larval = __crypto_register_alg(&inst->alg);
>> if (IS_ERR(larval))
>> goto unlock;
>> -
>> - larval->test_started = true;
>> + else if (larval)
>> + larval->test_started = true;
>>
>> hlist_add_head(&inst->list, &tmpl->instances);
>> inst->tmpl = tmpl;
>> @@ -626,7 +642,7 @@ int crypto_register_instance(struct crypto_template *tmpl,
>> up_write(&crypto_alg_sem);
>>
>> err = PTR_ERR(larval);
>> - if (IS_ERR(larval))
>> + if (IS_ERR_OR_NULL(larval))
>> goto err;
>>
>> crypto_wait_for_test(larval);
>> diff --git a/crypto/api.c b/crypto/api.c
>> index ee5991fe11f8..cf0869dd130b 100644
>> --- a/crypto/api.c
>> +++ b/crypto/api.c
>> @@ -167,11 +167,8 @@ void crypto_wait_for_test(struct crypto_larval *larval)
>> int err;
>>
>> err = crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult);
>> - if (err != NOTIFY_STOP) {
>> - if (WARN_ON(err != NOTIFY_DONE))
>> - goto out;
>> - crypto_alg_tested(larval->alg.cra_driver_name, 0);
>> - }
>> + if (WARN_ON_ONCE(err != NOTIFY_STOP))
>> + goto out;
>>
>> err = wait_for_completion_killable(&larval->completion);
>> WARN_ON(err);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
--
Damien Le Moal
Western Digital Research
next prev parent reply other threads:[~2021-11-04 7:28 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210913071251.GA15235@gondor.apana.org.au>
[not found] ` <20210917002619.GA6407@gondor.apana.org.au>
[not found] ` <YVNfqUVJ7w4Z3WXK@archlinux-ax161>
[not found] ` <20211001055058.GA6081@gondor.apana.org.au>
[not found] ` <YVdNFzs8HUQwHa54@archlinux-ax161>
[not found] ` <20211003002801.GA5435@gondor.apana.org.au>
[not found] ` <YV0K+EbrAqDdw2vp@archlinux-ax161>
[not found] ` <20211019132802.GA14233@gondor.apana.org.au>
2021-11-02 15:41 ` [PATCH] crypto: api - Do not create test larvals if manager is disabled Geert Uytterhoeven
2021-11-04 7:28 ` Damien Le Moal [this message]
2021-11-04 7:58 ` Geert Uytterhoeven
2021-11-04 8:05 ` Damien Le Moal
2021-11-04 12:16 ` Herbert Xu
2021-11-04 13:11 ` Geert Uytterhoeven
2021-11-04 13:30 ` Herbert Xu
2021-11-04 15:18 ` Ido Schimmel
2021-11-05 7:26 ` crypto: api - Fix boot-up crash when crypto " Herbert Xu
2021-11-05 14:33 ` Ido Schimmel
2021-11-05 18:00 ` Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR04MB708155E447FD9A79AB89686DE78D9@DM6PR04MB7081.namprd04.prod.outlook.com \
--to=damien.lemoal@wdc.com \
--cc=ebiggers@kernel.org \
--cc=geert@linux-m68k.org \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ssorce@redhat.com \
--cc=vdronov@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).