| Message ID | YjJq0RLIHvN7YWaT@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: arm/aes-neonbs-cbc - Select generic cbc and aes | expand |
Hello Herbert, On Thu, Mar 17, 2022 at 10:55:13AM +1200, Herbert Xu wrote: > On Wed, Mar 16, 2022 at 05:37:19PM +0100, Uwe Kleine-König wrote: > > > > # CONFIG_CRYPTO_CBC is not set > > This was the issue. The failure occurs on registering __cbc_aes > and the reason is that the neonbs cbc-aes requirs a fallback which > isn't available due to CBC being disabled. > > I have no idea why this started occurring only with the testmgr > change though as this should have been fatal all along. Yesterday I wondered about that, too. I didn't check, but I guess 00b99ad2bac2 was introduced between the testmgr regression and its fix and the failure looks similar enough for me to not having noticed the difference? Or my config somehow changed somewhere there (though I used the same defconfig in each bisection step). > ---8<--- > The algorithm __cbc-aes-neonbs requires a fallback so we need > to select the config options for them or otherwise it will fail > to register on boot-up. > > Fixes: 00b99ad2bac2 ("crypto: arm/aes-neonbs - Use generic cbc...") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig > index 2b575792363e..e4dba5461cb3 100644 > --- a/arch/arm/crypto/Kconfig > +++ b/arch/arm/crypto/Kconfig > @@ -102,6 +102,8 @@ config CRYPTO_AES_ARM_BS > depends on KERNEL_MODE_NEON > select CRYPTO_SKCIPHER > select CRYPTO_LIB_AES > + select CRYPTO_AES > + select CRYPTO_CBC > select CRYPTO_SIMD > help > Use a faster and more secure NEON based implementation of AES in CBC, I tested your change and that indeed fixes booting for me. Thanks! However I think there are two problems involved here, and you only fix one of them with your patch. Your change makes simd_skcipher_create_compat() succeed, but aes_init() still has a broken error handling. So if I do diff --git a/arch/arm/crypto/aes-neonbs-glue.c b/arch/arm/crypto/aes-neonbs-glue.c index 5c6cd3c63cbc..ee9812ee33b7 100644 --- a/arch/arm/crypto/aes-neonbs-glue.c +++ b/arch/arm/crypto/aes-neonbs-glue.c @@ -546,6 +546,11 @@ static int __init aes_init(void) algname = aes_algs[i].base.cra_name + 2; drvname = aes_algs[i].base.cra_driver_name + 2; basename = aes_algs[i].base.cra_driver_name; + if (i == 1) { + /* simulate a problem to test the error path */ + err = -ENOENT; + goto unregister_simds; + } simd = simd_skcipher_create_compat(algname, drvname, basename); err = PTR_ERR(simd); if (IS_ERR(simd)) the BUG is back. Best regards Uwe
On Do, 2022-03-17 at 10:55 +1200, Herbert Xu wrote: > On Wed, Mar 16, 2022 at 05:37:19PM +0100, Uwe Kleine-König wrote: > > > > # CONFIG_CRYPTO_CBC is not set > > This was the issue. The failure occurs on registering __cbc_aes > and the reason is that the neonbs cbc-aes requirs a fallback which > isn't available due to CBC being disabled. > > I have no idea why this started occurring only with the testmgr > change though as this should have been fatal all along. I think this always failed and nobody that actually had CRYPTO_AES or CRYPTO_CBC disabled noticed that aes-neonbs-cbc did not register. What commit adad556efcdd caused was allowing the error path in late_initcall(aes_init) to be hit before late_initcall(crypto_algapi_init) would start the tests. regards Philipp
On Thu, Mar 17, 2022 at 10:16:16AM +0100, Philipp Zabel wrote: > > What commit adad556efcdd caused was allowing the error path in > late_initcall(aes_init) to be hit before > late_initcall(crypto_algapi_init) would start the tests. OK I know what's going on now. Yes the registration had always failed but it was silent so nobody noticed. What adad556efcdd did different was to create larvals pointing to the algorithms which will hang around until all tests complete and that is what triggers the crash during unregister (that bug during unregister has always existed too, it's just that it was pretty much impossible to trigger as usually there aren't any third parties allocating tfms during the init call). I'll continue to work on the unregister crash. Thanks,
diff --git a/arch/arm/crypto/Kconfig b/arch/arm/crypto/Kconfig index 2b575792363e..e4dba5461cb3 100644 --- a/arch/arm/crypto/Kconfig +++ b/arch/arm/crypto/Kconfig @@ -102,6 +102,8 @@ config CRYPTO_AES_ARM_BS depends on KERNEL_MODE_NEON select CRYPTO_SKCIPHER select CRYPTO_LIB_AES + select CRYPTO_AES + select CRYPTO_CBC select CRYPTO_SIMD help Use a faster and more secure NEON based implementation of AES in CBC,