| Message ID | 20200629073925.127538-6-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: clean up ARM/arm64 glue code for GHASH and GCM | expand |
On Mon, Jun 29, 2020 at 09:39:25AM +0200, Ard Biesheuvel wrote: > Of the two versions of GHASH that the ARM driver implements, only one > performs aggregation, and so the other one has no use for the powers > of H to be precomputed, or space to be allocated for them in the key > struct. So make the context size dependent on which version is being > selected, and while at it, use a static key to carry this decision, > and get rid of the function pointer. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm/crypto/ghash-ce-glue.c | 51 +++++++++----------- > 1 file changed, 24 insertions(+), 27 deletions(-) This introduces some new sparse warnings: ../arch/arm/crypto/ghash-ce-glue.c:67:65: warning: incorrect type in argument 4 (different modifiers) ../arch/arm/crypto/ghash-ce-glue.c:67:65: expected unsigned long long const [usertype] ( *h )[2] ../arch/arm/crypto/ghash-ce-glue.c:67:65: got unsigned long long [usertype] ( * )[2] ../arch/arm/crypto/ghash-ce-glue.c:69:64: warning: incorrect type in argument 4 (different modifiers) ../arch/arm/crypto/ghash-ce-glue.c:69:64: expected unsigned long long const [usertype] ( *h )[2] ../arch/arm/crypto/ghash-ce-glue.c:69:64: got unsigned long long [usertype] ( * )[2] Thanks,
On Thu, 9 Jul 2020 at 11:22, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Mon, Jun 29, 2020 at 09:39:25AM +0200, Ard Biesheuvel wrote: > > Of the two versions of GHASH that the ARM driver implements, only one > > performs aggregation, and so the other one has no use for the powers > > of H to be precomputed, or space to be allocated for them in the key > > struct. So make the context size dependent on which version is being > > selected, and while at it, use a static key to carry this decision, > > and get rid of the function pointer. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/arm/crypto/ghash-ce-glue.c | 51 +++++++++----------- > > 1 file changed, 24 insertions(+), 27 deletions(-) > > This introduces some new sparse warnings: > > ../arch/arm/crypto/ghash-ce-glue.c:67:65: warning: incorrect type in argument 4 (different modifiers) > ../arch/arm/crypto/ghash-ce-glue.c:67:65: expected unsigned long long const [usertype] ( *h )[2] > ../arch/arm/crypto/ghash-ce-glue.c:67:65: got unsigned long long [usertype] ( * )[2] > ../arch/arm/crypto/ghash-ce-glue.c:69:64: warning: incorrect type in argument 4 (different modifiers) > ../arch/arm/crypto/ghash-ce-glue.c:69:64: expected unsigned long long const [usertype] ( *h )[2] > ../arch/arm/crypto/ghash-ce-glue.c:69:64: got unsigned long long [usertype] ( * )[2] > That looks like a sparse bug to me. Since when is it not allowed to pass a non-const value as a const parameter? I.e., you can pass a u64[] to a function that takes a u64 const *, giving the caller the guarantee that their u64[] will not be modified during the call, even if it is passed by reference. Here, we are dealing with u64[][2], but the same reasoning holds. A const u64[][2] formal parameter (or u64 const (*)[2] which comes down to the same thing) does not require a const argument, it only tells the caller that the array will be left untouched. This is why the compiler is perfectly happy with this arrangement.
On Thu, Jul 09, 2020 at 11:51:10AM +0300, Ard Biesheuvel wrote: > > That looks like a sparse bug to me. Since when is it not allowed to > pass a non-const value as a const parameter? > > I.e., you can pass a u64[] to a function that takes a u64 const *, > giving the caller the guarantee that their u64[] will not be modified > during the call, even if it is passed by reference. > > Here, we are dealing with u64[][2], but the same reasoning holds. A > const u64[][2] formal parameter (or u64 const (*)[2] which comes down > to the same thing) does not require a const argument, it only tells > the caller that the array will be left untouched. This is why the > compiler is perfectly happy with this arrangement. You're right. Luc, here is the patch that triggers the bogus warning with sparse. Thanks,
On Thu, Jul 09, 2020 at 10:09:37PM +1000, Herbert Xu wrote: > On Thu, Jul 09, 2020 at 11:51:10AM +0300, Ard Biesheuvel wrote: > > > > That looks like a sparse bug to me. Since when is it not allowed to > > pass a non-const value as a const parameter? > > > > I.e., you can pass a u64[] to a function that takes a u64 const *, > > giving the caller the guarantee that their u64[] will not be modified > > during the call, even if it is passed by reference. > > > > Here, we are dealing with u64[][2], but the same reasoning holds. A > > const u64[][2] formal parameter (or u64 const (*)[2] which comes down > > to the same thing) does not require a const argument, it only tells > > the caller that the array will be left untouched. This is why the > > compiler is perfectly happy with this arrangement. > > You're right. Luc, here is the patch that triggers the bogus > warning with sparse. Thanks for the analysis and the bug report. A fix is under way and should be upstreamed in a few days. -- Luc
diff --git a/arch/arm/crypto/ghash-ce-glue.c b/arch/arm/crypto/ghash-ce-glue.c index a00fd329255f..f13401f3e669 100644 --- a/arch/arm/crypto/ghash-ce-glue.c +++ b/arch/arm/crypto/ghash-ce-glue.c @@ -16,6 +16,7 @@ #include <crypto/gf128mul.h> #include <linux/cpufeature.h> #include <linux/crypto.h> +#include <linux/jump_label.h> #include <linux/module.h> MODULE_DESCRIPTION("GHASH hash function using ARMv8 Crypto Extensions"); @@ -27,12 +28,8 @@ MODULE_ALIAS_CRYPTO("ghash"); #define GHASH_DIGEST_SIZE 16 struct ghash_key { - u64 h[2]; - u64 h2[2]; - u64 h3[2]; - u64 h4[2]; - be128 k; + u64 h[][2]; }; struct ghash_desc_ctx { @@ -46,16 +43,12 @@ struct ghash_async_ctx { }; asmlinkage void pmull_ghash_update_p64(int blocks, u64 dg[], const char *src, - struct ghash_key const *k, - const char *head); + u64 const h[][2], const char *head); asmlinkage void pmull_ghash_update_p8(int blocks, u64 dg[], const char *src, - struct ghash_key const *k, - const char *head); + u64 const h[][2], const char *head); -static void (*pmull_ghash_update)(int blocks, u64 dg[], const char *src, - struct ghash_key const *k, - const char *head); +static __ro_after_init DEFINE_STATIC_KEY_FALSE(use_p64); static int ghash_init(struct shash_desc *desc) { @@ -70,7 +63,10 @@ static void ghash_do_update(int blocks, u64 dg[], const char *src, { if (likely(crypto_simd_usable())) { kernel_neon_begin(); - pmull_ghash_update(blocks, dg, src, key, head); + if (static_branch_likely(&use_p64)) + pmull_ghash_update_p64(blocks, dg, src, key->h, head); + else + pmull_ghash_update_p8(blocks, dg, src, key->h, head); kernel_neon_end(); } else { be128 dst = { cpu_to_be64(dg[1]), cpu_to_be64(dg[0]) }; @@ -161,25 +157,26 @@ static int ghash_setkey(struct crypto_shash *tfm, const u8 *inkey, unsigned int keylen) { struct ghash_key *key = crypto_shash_ctx(tfm); - be128 h; if (keylen != GHASH_BLOCK_SIZE) return -EINVAL; /* needed for the fallback */ memcpy(&key->k, inkey, GHASH_BLOCK_SIZE); - ghash_reflect(key->h, &key->k); + ghash_reflect(key->h[0], &key->k); - h = key->k; - gf128mul_lle(&h, &key->k); - ghash_reflect(key->h2, &h); + if (static_branch_likely(&use_p64)) { + be128 h = key->k; - gf128mul_lle(&h, &key->k); - ghash_reflect(key->h3, &h); + gf128mul_lle(&h, &key->k); + ghash_reflect(key->h[1], &h); - gf128mul_lle(&h, &key->k); - ghash_reflect(key->h4, &h); + gf128mul_lle(&h, &key->k); + ghash_reflect(key->h[2], &h); + gf128mul_lle(&h, &key->k); + ghash_reflect(key->h[3], &h); + } return 0; } @@ -195,7 +192,7 @@ static struct shash_alg ghash_alg = { .base.cra_driver_name = "ghash-ce-sync", .base.cra_priority = 300 - 1, .base.cra_blocksize = GHASH_BLOCK_SIZE, - .base.cra_ctxsize = sizeof(struct ghash_key), + .base.cra_ctxsize = sizeof(struct ghash_key) + sizeof(u64[2]), .base.cra_module = THIS_MODULE, }; @@ -354,10 +351,10 @@ static int __init ghash_ce_mod_init(void) if (!(elf_hwcap & HWCAP_NEON)) return -ENODEV; - if (elf_hwcap2 & HWCAP2_PMULL) - pmull_ghash_update = pmull_ghash_update_p64; - else - pmull_ghash_update = pmull_ghash_update_p8; + if (elf_hwcap2 & HWCAP2_PMULL) { + ghash_alg.base.cra_ctxsize += 3 * sizeof(u64[2]); + static_branch_enable(&use_p64); + } err = crypto_register_shash(&ghash_alg); if (err)
Of the two versions of GHASH that the ARM driver implements, only one performs aggregation, and so the other one has no use for the powers of H to be precomputed, or space to be allocated for them in the key struct. So make the context size dependent on which version is being selected, and while at it, use a static key to carry this decision, and get rid of the function pointer. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm/crypto/ghash-ce-glue.c | 51 +++++++++----------- 1 file changed, 24 insertions(+), 27 deletions(-)