| Message ID | ZsBKG0la0m69Dyq3@gondor.apana.org.au |
|---|---|
| State | Accepted |
| Commit | 3c44d31cb34ce4eb8311a2e73634d57702948230 |
| Headers | show |
| Series | [v3,1/3] crypto: api - Remove instance larval fulfilment | expand |
On Sat, Aug 17, 2024 at 02:58:35PM +0800, Herbert Xu wrote: > Algorithm registration is usually carried out during module init, > where as little work as possible should be carried out. The SIMD > code violated this rule by allocating a tfm, this then triggers a > full test of the algorithm which may dead-lock in certain cases. > > SIMD is only allocating the tfm to get at the alg object, which is > in fact already available as it is what we are registering. Use > that directly and remove the crypto_alloc_tfm call. > > Also remove some obsolete and unused SIMD API. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > arch/arm/crypto/aes-ce-glue.c | 2 +- > arch/arm/crypto/aes-neonbs-glue.c | 2 +- > crypto/simd.c | 76 ++++++------------------------- > include/crypto/internal/simd.h | 12 +---- > 4 files changed, 19 insertions(+), 73 deletions(-) > I'm getting a test failure with this series applied: [ 0.383128] alg: aead: failed to allocate transform for gcm_base(ctr(aes-generic),ghash-generic): -2 [ 0.383500] alg: self-tests for gcm(aes) using gcm_base(ctr(aes-generic),ghash-generic) failed (rc=-2) This is on x86_64 with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y. - Eric
On Tue, Aug 27, 2024 at 11:48:39AM -0700, Eric Biggers wrote: > On Sat, Aug 17, 2024 at 02:58:35PM +0800, Herbert Xu wrote: > > Algorithm registration is usually carried out during module init, > > where as little work as possible should be carried out. The SIMD > > code violated this rule by allocating a tfm, this then triggers a > > full test of the algorithm which may dead-lock in certain cases. > > > > SIMD is only allocating the tfm to get at the alg object, which is > > in fact already available as it is what we are registering. Use > > that directly and remove the crypto_alloc_tfm call. > > > > Also remove some obsolete and unused SIMD API. > > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > --- > > arch/arm/crypto/aes-ce-glue.c | 2 +- > > arch/arm/crypto/aes-neonbs-glue.c | 2 +- > > crypto/simd.c | 76 ++++++------------------------- > > include/crypto/internal/simd.h | 12 +---- > > 4 files changed, 19 insertions(+), 73 deletions(-) > > > > I'm getting a test failure with this series applied: > > [ 0.383128] alg: aead: failed to allocate transform for gcm_base(ctr(aes-generic),ghash-generic): -2 > [ 0.383500] alg: self-tests for gcm(aes) using gcm_base(ctr(aes-generic),ghash-generic) failed (rc=-2) > > This is on x86_64 with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y. Could you please send me your config file? Thanks,
On Wed, Aug 28, 2024 at 10:59:20AM +0800, Herbert Xu wrote: > On Tue, Aug 27, 2024 at 11:48:39AM -0700, Eric Biggers wrote: > > On Sat, Aug 17, 2024 at 02:58:35PM +0800, Herbert Xu wrote: > > > Algorithm registration is usually carried out during module init, > > > where as little work as possible should be carried out. The SIMD > > > code violated this rule by allocating a tfm, this then triggers a > > > full test of the algorithm which may dead-lock in certain cases. > > > > > > SIMD is only allocating the tfm to get at the alg object, which is > > > in fact already available as it is what we are registering. Use > > > that directly and remove the crypto_alloc_tfm call. > > > > > > Also remove some obsolete and unused SIMD API. > > > > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > > --- > > > arch/arm/crypto/aes-ce-glue.c | 2 +- > > > arch/arm/crypto/aes-neonbs-glue.c | 2 +- > > > crypto/simd.c | 76 ++++++------------------------- > > > include/crypto/internal/simd.h | 12 +---- > > > 4 files changed, 19 insertions(+), 73 deletions(-) > > > > > > > I'm getting a test failure with this series applied: > > > > [ 0.383128] alg: aead: failed to allocate transform for gcm_base(ctr(aes-generic),ghash-generic): -2 > > [ 0.383500] alg: self-tests for gcm(aes) using gcm_base(ctr(aes-generic),ghash-generic) failed (rc=-2) > > > > This is on x86_64 with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y. > > Could you please send me your config file? > > Thanks, Given below in defconfig form, use 'make olddefconfig' to apply. The failures are nondeterministic and sometimes there are different ones, for example: [ 0.358017] alg: skcipher: failed to allocate transform for cbc(twofish-generic): -2 [ 0.358365] alg: self-tests for cbc(twofish) using cbc(twofish-generic) failed (rc=-2) [ 0.358535] alg: skcipher: failed to allocate transform for cbc(camellia-generic): -2 [ 0.358918] alg: self-tests for cbc(camellia) using cbc(camellia-generic) failed (rc=-2) [ 0.371533] alg: skcipher: failed to allocate transform for xts(ecb(aes-generic)): -2 [ 0.371922] alg: self-tests for xts(aes) using xts(ecb(aes-generic)) failed (rc=-2) Modules are not enabled, maybe that matters (I haven't checked yet). CONFIG_SYSVIPC=y CONFIG_POSIX_MQUEUE=y CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y CONFIG_CGROUPS=y CONFIG_USER_NS=y CONFIG_BLK_DEV_INITRD=y CONFIG_SMP=y CONFIG_X86_X2APIC=y CONFIG_HYPERVISOR_GUEST=y CONFIG_PARAVIRT=y CONFIG_MCORE2=y CONFIG_NR_CPUS=48 CONFIG_NUMA=y CONFIG_HZ_300=y # CONFIG_RANDOMIZE_BASE is not set CONFIG_IA32_EMULATION=y CONFIG_JUMP_LABEL=y CONFIG_NET=y CONFIG_PACKET=y CONFIG_PACKET_DIAG=y CONFIG_UNIX=y CONFIG_UNIX_DIAG=y CONFIG_INET=y CONFIG_PCI=y CONFIG_PCI_MSI=y CONFIG_DEVTMPFS=y CONFIG_BLK_DEV_LOOP=y CONFIG_VIRTIO_BLK=y CONFIG_NETDEVICES=y CONFIG_VIRTIO_NET=y CONFIG_SERIAL_8250=y CONFIG_SERIAL_8250_CONSOLE=y CONFIG_SERIAL_8250_NR_UARTS=32 CONFIG_SERIAL_8250_RUNTIME_UARTS=32 CONFIG_HW_RANDOM_VIRTIO=y CONFIG_VIRT_DRIVERS=y CONFIG_VIRTIO_PCI=y CONFIG_VIRTIO_MMIO=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_EXT4_FS_SECURITY=y CONFIG_AUTOFS_FS=y CONFIG_TMPFS=y CONFIG_TMPFS_POSIX_ACL=y CONFIG_CRYPTO_USER=y # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y CONFIG_CRYPTO_PCRYPT=y CONFIG_CRYPTO_DH_RFC7919_GROUPS=y CONFIG_CRYPTO_ECDH=y CONFIG_CRYPTO_ECDSA=y CONFIG_CRYPTO_ECRDSA=y CONFIG_CRYPTO_CURVE25519=y CONFIG_CRYPTO_AES_TI=y CONFIG_CRYPTO_ANUBIS=y CONFIG_CRYPTO_BLOWFISH=y CONFIG_CRYPTO_CAMELLIA=y CONFIG_CRYPTO_DES=y CONFIG_CRYPTO_FCRYPT=y CONFIG_CRYPTO_KHAZAD=y CONFIG_CRYPTO_SEED=y CONFIG_CRYPTO_TEA=y CONFIG_CRYPTO_TWOFISH=y CONFIG_CRYPTO_ADIANTUM=y CONFIG_CRYPTO_ARC4=y CONFIG_CRYPTO_HCTR2=y CONFIG_CRYPTO_KEYWRAP=y CONFIG_CRYPTO_LRW=y CONFIG_CRYPTO_PCBC=y CONFIG_CRYPTO_AEGIS128=y CONFIG_CRYPTO_SEQIV=y CONFIG_CRYPTO_ECHAINIV=y CONFIG_CRYPTO_ESSIV=y CONFIG_CRYPTO_BLAKE2B=y CONFIG_CRYPTO_MD4=y CONFIG_CRYPTO_RMD160=y CONFIG_CRYPTO_SM3_GENERIC=y CONFIG_CRYPTO_VMAC=y CONFIG_CRYPTO_WP512=y CONFIG_CRYPTO_XXHASH=y CONFIG_CRYPTO_CRC32=y CONFIG_CRYPTO_DEFLATE=y CONFIG_CRYPTO_LZO=y CONFIG_CRYPTO_842=y CONFIG_CRYPTO_LZ4=y CONFIG_CRYPTO_LZ4HC=y CONFIG_CRYPTO_ZSTD=y CONFIG_CRYPTO_ANSI_CPRNG=y CONFIG_CRYPTO_DRBG_HASH=y CONFIG_CRYPTO_DRBG_CTR=y CONFIG_CRYPTO_USER_API_HASH=y CONFIG_CRYPTO_USER_API_SKCIPHER=y CONFIG_CRYPTO_USER_API_RNG=y CONFIG_CRYPTO_USER_API_RNG_CAVP=y CONFIG_CRYPTO_USER_API_AEAD=y CONFIG_CRYPTO_CURVE25519_X86=y CONFIG_CRYPTO_AES_NI_INTEL=y CONFIG_CRYPTO_BLOWFISH_X86_64=y CONFIG_CRYPTO_CAMELLIA_AESNI_AVX2_X86_64=y CONFIG_CRYPTO_CAST5_AVX_X86_64=y CONFIG_CRYPTO_CAST6_AVX_X86_64=y CONFIG_CRYPTO_DES3_EDE_X86_64=y CONFIG_CRYPTO_SERPENT_SSE2_X86_64=y CONFIG_CRYPTO_SERPENT_AVX2_X86_64=y CONFIG_CRYPTO_SM4_AESNI_AVX2_X86_64=y CONFIG_CRYPTO_TWOFISH_AVX_X86_64=y CONFIG_CRYPTO_ARIA_GFNI_AVX512_X86_64=y CONFIG_CRYPTO_CHACHA20_X86_64=y CONFIG_CRYPTO_AEGIS128_AESNI_SSE2=y CONFIG_CRYPTO_NHPOLY1305_SSE2=y CONFIG_CRYPTO_NHPOLY1305_AVX2=y CONFIG_CRYPTO_BLAKE2S_X86=y CONFIG_CRYPTO_POLYVAL_CLMUL_NI=y CONFIG_CRYPTO_POLY1305_X86_64=y CONFIG_CRYPTO_SHA1_SSSE3=y CONFIG_CRYPTO_SHA256_SSSE3=y CONFIG_CRYPTO_SHA512_SSSE3=y CONFIG_CRYPTO_SM3_AVX_X86_64=y CONFIG_CRYPTO_GHASH_CLMUL_NI_INTEL=y CONFIG_CRYPTO_CRC32C_INTEL=y CONFIG_CRYPTO_CRC32_PCLMUL=y CONFIG_CRYPTO_CRCT10DIF_PCLMUL=y CONFIG_CRYPTO_DEV_PADLOCK=y CONFIG_CRYPTO_DEV_PADLOCK_AES=y CONFIG_CRYPTO_DEV_PADLOCK_SHA=y CONFIG_CRYPTO_DEV_CCP=y CONFIG_CRYPTO_DEV_NITROX_CNN55XX=y CONFIG_CRYPTO_DEV_QAT_DH895xCC=y CONFIG_CRYPTO_DEV_QAT_C3XXX=y CONFIG_CRYPTO_DEV_QAT_C62X=y CONFIG_CRYPTO_DEV_QAT_4XXX=y CONFIG_CRYPTO_DEV_QAT_DH895xCCVF=y CONFIG_CRYPTO_DEV_QAT_C3XXXVF=y CONFIG_CRYPTO_DEV_QAT_C62XVF=y CONFIG_CRYPTO_DEV_VIRTIO=y CONFIG_CRYPTO_DEV_SAFEXCEL=y CONFIG_CRYPTO_DEV_AMLOGIC_GXL=y CONFIG_CRYPTO_DEV_AMLOGIC_GXL_DEBUG=y CONFIG_CRYPTO_LIB_CURVE25519=y CONFIG_CRYPTO_LIB_CHACHA20POLY1305=y CONFIG_CRC_CCITT=y CONFIG_CRC_T10DIF=y CONFIG_CRC64_ROCKSOFT=y CONFIG_CRC_ITU_T=y CONFIG_CRC32_SELFTEST=y CONFIG_CRC32_SLICEBY4=y CONFIG_CRC4=y CONFIG_CRC7=y CONFIG_LIBCRC32C=y CONFIG_PRINTK_TIME=y CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_FS=y CONFIG_PANIC_TIMEOUT=5 CONFIG_UNWINDER_FRAME_POINTER=y
diff --git a/arch/arm/crypto/aes-ce-glue.c b/arch/arm/crypto/aes-ce-glue.c index b668c97663ec..f5b66f4cf45d 100644 --- a/arch/arm/crypto/aes-ce-glue.c +++ b/arch/arm/crypto/aes-ce-glue.c @@ -711,7 +711,7 @@ 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; - simd = simd_skcipher_create_compat(algname, drvname, basename); + simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename); err = PTR_ERR(simd); if (IS_ERR(simd)) goto unregister_simds; diff --git a/arch/arm/crypto/aes-neonbs-glue.c b/arch/arm/crypto/aes-neonbs-glue.c index fd04f855b2f5..f6be80b5938b 100644 --- a/arch/arm/crypto/aes-neonbs-glue.c +++ b/arch/arm/crypto/aes-neonbs-glue.c @@ -491,7 +491,7 @@ 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; - simd = simd_skcipher_create_compat(algname, drvname, basename); + simd = simd_skcipher_create_compat(aes_algs + i, algname, drvname, basename); err = PTR_ERR(simd); if (IS_ERR(simd)) goto unregister_simds; diff --git a/crypto/simd.c b/crypto/simd.c index 2aa4f72e224f..b07721d1f3f6 100644 --- a/crypto/simd.c +++ b/crypto/simd.c @@ -136,27 +136,19 @@ static int simd_skcipher_init(struct crypto_skcipher *tfm) return 0; } -struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname, +struct simd_skcipher_alg *simd_skcipher_create_compat(struct skcipher_alg *ialg, + const char *algname, const char *drvname, const char *basename) { struct simd_skcipher_alg *salg; - struct crypto_skcipher *tfm; - struct skcipher_alg *ialg; struct skcipher_alg *alg; int err; - tfm = crypto_alloc_skcipher(basename, CRYPTO_ALG_INTERNAL, - CRYPTO_ALG_INTERNAL | CRYPTO_ALG_ASYNC); - if (IS_ERR(tfm)) - return ERR_CAST(tfm); - - ialg = crypto_skcipher_alg(tfm); - salg = kzalloc(sizeof(*salg), GFP_KERNEL); if (!salg) { salg = ERR_PTR(-ENOMEM); - goto out_put_tfm; + goto out; } salg->ialg_name = basename; @@ -195,30 +187,16 @@ struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname, if (err) goto out_free_salg; -out_put_tfm: - crypto_free_skcipher(tfm); +out: return salg; out_free_salg: kfree(salg); salg = ERR_PTR(err); - goto out_put_tfm; + goto out; } EXPORT_SYMBOL_GPL(simd_skcipher_create_compat); -struct simd_skcipher_alg *simd_skcipher_create(const char *algname, - const char *basename) -{ - char drvname[CRYPTO_MAX_ALG_NAME]; - - if (snprintf(drvname, CRYPTO_MAX_ALG_NAME, "simd-%s", basename) >= - CRYPTO_MAX_ALG_NAME) - return ERR_PTR(-ENAMETOOLONG); - - return simd_skcipher_create_compat(algname, drvname, basename); -} -EXPORT_SYMBOL_GPL(simd_skcipher_create); - void simd_skcipher_free(struct simd_skcipher_alg *salg) { crypto_unregister_skcipher(&salg->alg); @@ -246,7 +224,7 @@ int simd_register_skciphers_compat(struct skcipher_alg *algs, int count, algname = algs[i].base.cra_name + 2; drvname = algs[i].base.cra_driver_name + 2; basename = algs[i].base.cra_driver_name; - simd = simd_skcipher_create_compat(algname, drvname, basename); + simd = simd_skcipher_create_compat(algs + i, algname, drvname, basename); err = PTR_ERR(simd); if (IS_ERR(simd)) goto err_unregister; @@ -383,27 +361,19 @@ static int simd_aead_init(struct crypto_aead *tfm) return 0; } -struct simd_aead_alg *simd_aead_create_compat(const char *algname, - const char *drvname, - const char *basename) +static struct simd_aead_alg *simd_aead_create_compat(struct aead_alg *ialg, + const char *algname, + const char *drvname, + const char *basename) { struct simd_aead_alg *salg; - struct crypto_aead *tfm; - struct aead_alg *ialg; struct aead_alg *alg; int err; - tfm = crypto_alloc_aead(basename, CRYPTO_ALG_INTERNAL, - CRYPTO_ALG_INTERNAL | CRYPTO_ALG_ASYNC); - if (IS_ERR(tfm)) - return ERR_CAST(tfm); - - ialg = crypto_aead_alg(tfm); - salg = kzalloc(sizeof(*salg), GFP_KERNEL); if (!salg) { salg = ERR_PTR(-ENOMEM); - goto out_put_tfm; + goto out; } salg->ialg_name = basename; @@ -442,36 +412,20 @@ struct simd_aead_alg *simd_aead_create_compat(const char *algname, if (err) goto out_free_salg; -out_put_tfm: - crypto_free_aead(tfm); +out: return salg; out_free_salg: kfree(salg); salg = ERR_PTR(err); - goto out_put_tfm; + goto out; } -EXPORT_SYMBOL_GPL(simd_aead_create_compat); -struct simd_aead_alg *simd_aead_create(const char *algname, - const char *basename) -{ - char drvname[CRYPTO_MAX_ALG_NAME]; - - if (snprintf(drvname, CRYPTO_MAX_ALG_NAME, "simd-%s", basename) >= - CRYPTO_MAX_ALG_NAME) - return ERR_PTR(-ENAMETOOLONG); - - return simd_aead_create_compat(algname, drvname, basename); -} -EXPORT_SYMBOL_GPL(simd_aead_create); - -void simd_aead_free(struct simd_aead_alg *salg) +static void simd_aead_free(struct simd_aead_alg *salg) { crypto_unregister_aead(&salg->alg); kfree(salg); } -EXPORT_SYMBOL_GPL(simd_aead_free); int simd_register_aeads_compat(struct aead_alg *algs, int count, struct simd_aead_alg **simd_algs) @@ -493,7 +447,7 @@ int simd_register_aeads_compat(struct aead_alg *algs, int count, algname = algs[i].base.cra_name + 2; drvname = algs[i].base.cra_driver_name + 2; basename = algs[i].base.cra_driver_name; - simd = simd_aead_create_compat(algname, drvname, basename); + simd = simd_aead_create_compat(algs + i, algname, drvname, basename); err = PTR_ERR(simd); if (IS_ERR(simd)) goto err_unregister; diff --git a/include/crypto/internal/simd.h b/include/crypto/internal/simd.h index d2316242a988..be97b97a75dd 100644 --- a/include/crypto/internal/simd.h +++ b/include/crypto/internal/simd.h @@ -14,11 +14,10 @@ struct simd_skcipher_alg; struct skcipher_alg; -struct simd_skcipher_alg *simd_skcipher_create_compat(const char *algname, +struct simd_skcipher_alg *simd_skcipher_create_compat(struct skcipher_alg *ialg, + const char *algname, const char *drvname, const char *basename); -struct simd_skcipher_alg *simd_skcipher_create(const char *algname, - const char *basename); void simd_skcipher_free(struct simd_skcipher_alg *alg); int simd_register_skciphers_compat(struct skcipher_alg *algs, int count, @@ -32,13 +31,6 @@ void simd_unregister_skciphers(struct skcipher_alg *algs, int count, struct simd_aead_alg; struct aead_alg; -struct simd_aead_alg *simd_aead_create_compat(const char *algname, - const char *drvname, - const char *basename); -struct simd_aead_alg *simd_aead_create(const char *algname, - const char *basename); -void simd_aead_free(struct simd_aead_alg *alg); - int simd_register_aeads_compat(struct aead_alg *algs, int count, struct simd_aead_alg **simd_algs);
Algorithm registration is usually carried out during module init, where as little work as possible should be carried out. The SIMD code violated this rule by allocating a tfm, this then triggers a full test of the algorithm which may dead-lock in certain cases. SIMD is only allocating the tfm to get at the alg object, which is in fact already available as it is what we are registering. Use that directly and remove the crypto_alloc_tfm call. Also remove some obsolete and unused SIMD API. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- arch/arm/crypto/aes-ce-glue.c | 2 +- arch/arm/crypto/aes-neonbs-glue.c | 2 +- crypto/simd.c | 76 ++++++------------------------- include/crypto/internal/simd.h | 12 +---- 4 files changed, 19 insertions(+), 73 deletions(-)