| Message ID | 20201117133214.29114-1-ardb@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: aegis128 enhancements | expand |
On Tue, Nov 17, 2020 at 02:32:10PM +0100, Ard Biesheuvel wrote: > This series supersedes [0] '[PATCH] crypto: aegis128/neon - optimize tail > block handling', which is included as patch #3 here, but hasn't been > modified substantially. > > Patch #1 should probably go to -stable, even though aegis128 does not appear > to be widely used. > > Patches #2 and #3 improve the SIMD code paths. > > Patch #4 enables fuzz testing for the SIMD code by registering the generic > code as a separate driver if the SIMD code path is enabled. > > Changes since v2: > - add Ondrej's ack to #1 > - fix an issue spotted by Ondrej in #4 where the generic code path would still > use some of the SIMD helpers > > Cc: Ondrej Mosnacek <omosnacek@gmail.com> > Cc: Eric Biggers <ebiggers@kernel.org> > > [0] https://lore.kernel.org/linux-crypto/20201107195516.13952-1-ardb@kernel.org/ > > Ard Biesheuvel (4): > crypto: aegis128 - wipe plaintext and tag if decryption fails > crypto: aegis128/neon - optimize tail block handling > crypto: aegis128/neon - move final tag check to SIMD domain > crypto: aegis128 - expose SIMD code path as separate driver > > crypto/aegis128-core.c | 245 ++++++++++++++------ > crypto/aegis128-neon-inner.c | 122 ++++++++-- > crypto/aegis128-neon.c | 21 +- > 3 files changed, 287 insertions(+), 101 deletions(-) All applied. Thanks.
Hi Ard, On Tue, Nov 17, 2020 at 2:38 PM Ard Biesheuvel <ardb@kernel.org> wrote: > This series supersedes [0] '[PATCH] crypto: aegis128/neon - optimize tail > block handling', which is included as patch #3 here, but hasn't been > modified substantially. > > Patch #1 should probably go to -stable, even though aegis128 does not appear > to be widely used. > > Patches #2 and #3 improve the SIMD code paths. > > Patch #4 enables fuzz testing for the SIMD code by registering the generic > code as a separate driver if the SIMD code path is enabled. > > Changes since v2: > - add Ondrej's ack to #1 > - fix an issue spotted by Ondrej in #4 where the generic code path would still > use some of the SIMD helpers > > Cc: Ondrej Mosnacek <omosnacek@gmail.com> > Cc: Eric Biggers <ebiggers@kernel.org> > > [0] https://lore.kernel.org/linux-crypto/20201107195516.13952-1-ardb@kernel.org/ > > Ard Biesheuvel (4): > crypto: aegis128 - wipe plaintext and tag if decryption fails > crypto: aegis128/neon - optimize tail block handling > crypto: aegis128/neon - move final tag check to SIMD domain crypto/aegis128-core.c: In function ‘crypto_aegis128_decrypt’: crypto/aegis128-core.c:454:40: error: passing argument 2 of ‘crypto_aegis128_process_crypt’ from incompatible pointer type [-Werror=incompatible-pointer-types] 454 | crypto_aegis128_process_crypt(NULL, req, &walk, | ^~~ | | | struct aead_request * crypto/aegis128-core.c:335:29: note: expected ‘struct skcipher_walk *’ but argument is of type ‘struct aead_request *’ 335 | struct skcipher_walk *walk, | ~~~~~~~~~~~~~~~~~~~~~~^~~~ crypto/aegis128-core.c:454:45: error: passing argument 3 of ‘crypto_aegis128_process_crypt’ from incompatible pointer type [-Werror=incompatible-pointer-types] 454 | crypto_aegis128_process_crypt(NULL, req, &walk, | ^~~~~ | | | struct skcipher_walk * crypto/aegis128-core.c:336:14: note: expected ‘void (*)(struct aegis_state *, u8 *, const u8 *, unsigned int)’ {aka ‘void (*)(struct aegis_state *, unsigned char *, const unsigned char *, unsigned int)’} but argument is of type ‘struct skcipher_walk *’ 336 | void (*crypt)(struct aegis_state *state, | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 337 | u8 *dst, const u8 *src, | ~~~~~~~~~~~~~~~~~~~~~~~ 338 | unsigned int size)) | ~~~~~~~~~~~~~~~~~~ crypto/aegis128-core.c:454:4: error: too many arguments to function ‘crypto_aegis128_process_crypt’ 454 | crypto_aegis128_process_crypt(NULL, req, &walk, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ crypto/aegis128-core.c:334:5: note: declared here 334 | int crypto_aegis128_process_crypt(struct aegis_state *state, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors make[1]: *** [scripts/Makefile.build:283: crypto/aegis128-core.o] Error 1 > crypto: aegis128 - expose SIMD code path as separate driver Fixes the above, but causes ERROR: modpost: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! as reported by noreply@ellerman.id.au for m68k/defconfig and m68k/sun3_defconfig. (neon depends on arm). > crypto/aegis128-core.c | 245 ++++++++++++++------ > crypto/aegis128-neon-inner.c | 122 ++++++++-- > crypto/aegis128-neon.c | 21 +- > 3 files changed, 287 insertions(+), 101 deletions(-) Gr{oetje,eeting}s, Geert
On Mon, 30 Nov 2020 at 10:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Ard, > > On Tue, Nov 17, 2020 at 2:38 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > This series supersedes [0] '[PATCH] crypto: aegis128/neon - optimize tail > > block handling', which is included as patch #3 here, but hasn't been > > modified substantially. > > > > Patch #1 should probably go to -stable, even though aegis128 does not appear > > to be widely used. > > > > Patches #2 and #3 improve the SIMD code paths. > > > > Patch #4 enables fuzz testing for the SIMD code by registering the generic > > code as a separate driver if the SIMD code path is enabled. > > > > Changes since v2: > > - add Ondrej's ack to #1 > > - fix an issue spotted by Ondrej in #4 where the generic code path would still > > use some of the SIMD helpers > > > > Cc: Ondrej Mosnacek <omosnacek@gmail.com> > > Cc: Eric Biggers <ebiggers@kernel.org> > > > > [0] https://lore.kernel.org/linux-crypto/20201107195516.13952-1-ardb@kernel.org/ > > > > Ard Biesheuvel (4): > > crypto: aegis128 - wipe plaintext and tag if decryption fails > > crypto: aegis128/neon - optimize tail block handling > > crypto: aegis128/neon - move final tag check to SIMD domain > > crypto/aegis128-core.c: In function ‘crypto_aegis128_decrypt’: > crypto/aegis128-core.c:454:40: error: passing argument 2 of > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > [-Werror=incompatible-pointer-types] > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > | ^~~ > | | > | struct aead_request * > crypto/aegis128-core.c:335:29: note: expected ‘struct skcipher_walk *’ > but argument is of type ‘struct aead_request *’ > 335 | struct skcipher_walk *walk, > | ~~~~~~~~~~~~~~~~~~~~~~^~~~ > crypto/aegis128-core.c:454:45: error: passing argument 3 of > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > [-Werror=incompatible-pointer-types] > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > | ^~~~~ > | | > | struct skcipher_walk * > crypto/aegis128-core.c:336:14: note: expected ‘void (*)(struct > aegis_state *, u8 *, const u8 *, unsigned int)’ {aka ‘void (*)(struct > aegis_state *, unsigned char *, const unsigned char *, unsigned int)’} > but argument is of type ‘struct skcipher_walk *’ > 336 | void (*crypt)(struct aegis_state *state, > | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 337 | u8 *dst, const u8 *src, > | ~~~~~~~~~~~~~~~~~~~~~~~ > 338 | unsigned int size)) > | ~~~~~~~~~~~~~~~~~~ > crypto/aegis128-core.c:454:4: error: too many arguments to function > ‘crypto_aegis128_process_crypt’ > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > crypto/aegis128-core.c:334:5: note: declared here > 334 | int crypto_aegis128_process_crypt(struct aegis_state *state, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors > make[1]: *** [scripts/Makefile.build:283: crypto/aegis128-core.o] Error 1 > > > crypto: aegis128 - expose SIMD code path as separate driver > > Fixes the above, but causes > > ERROR: modpost: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > > as reported by noreply@ellerman.id.au for m68k/defconfig and > m68k/sun3_defconfig. > (neon depends on arm). > Thanks for the report. It seems like GCC is not optimizing away calls to routines that are unreachable. Which GCC version are you using?
On Mon, 30 Nov 2020 at 10:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 30 Nov 2020 at 10:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > Hi Ard, > > > > On Tue, Nov 17, 2020 at 2:38 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > This series supersedes [0] '[PATCH] crypto: aegis128/neon - optimize tail > > > block handling', which is included as patch #3 here, but hasn't been > > > modified substantially. > > > > > > Patch #1 should probably go to -stable, even though aegis128 does not appear > > > to be widely used. > > > > > > Patches #2 and #3 improve the SIMD code paths. > > > > > > Patch #4 enables fuzz testing for the SIMD code by registering the generic > > > code as a separate driver if the SIMD code path is enabled. > > > > > > Changes since v2: > > > - add Ondrej's ack to #1 > > > - fix an issue spotted by Ondrej in #4 where the generic code path would still > > > use some of the SIMD helpers > > > > > > Cc: Ondrej Mosnacek <omosnacek@gmail.com> > > > Cc: Eric Biggers <ebiggers@kernel.org> > > > > > > [0] https://lore.kernel.org/linux-crypto/20201107195516.13952-1-ardb@kernel.org/ > > > > > > Ard Biesheuvel (4): > > > crypto: aegis128 - wipe plaintext and tag if decryption fails > > > crypto: aegis128/neon - optimize tail block handling > > > crypto: aegis128/neon - move final tag check to SIMD domain > > > > crypto/aegis128-core.c: In function ‘crypto_aegis128_decrypt’: > > crypto/aegis128-core.c:454:40: error: passing argument 2 of > > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > > [-Werror=incompatible-pointer-types] > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > | ^~~ > > | | > > | struct aead_request * > > crypto/aegis128-core.c:335:29: note: expected ‘struct skcipher_walk *’ > > but argument is of type ‘struct aead_request *’ > > 335 | struct skcipher_walk *walk, > > | ~~~~~~~~~~~~~~~~~~~~~~^~~~ > > crypto/aegis128-core.c:454:45: error: passing argument 3 of > > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > > [-Werror=incompatible-pointer-types] > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > | ^~~~~ > > | | > > | struct skcipher_walk * > > crypto/aegis128-core.c:336:14: note: expected ‘void (*)(struct > > aegis_state *, u8 *, const u8 *, unsigned int)’ {aka ‘void (*)(struct > > aegis_state *, unsigned char *, const unsigned char *, unsigned int)’} > > but argument is of type ‘struct skcipher_walk *’ > > 336 | void (*crypt)(struct aegis_state *state, > > | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 337 | u8 *dst, const u8 *src, > > | ~~~~~~~~~~~~~~~~~~~~~~~ > > 338 | unsigned int size)) > > | ~~~~~~~~~~~~~~~~~~ > > crypto/aegis128-core.c:454:4: error: too many arguments to function > > ‘crypto_aegis128_process_crypt’ > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > crypto/aegis128-core.c:334:5: note: declared here > > 334 | int crypto_aegis128_process_crypt(struct aegis_state *state, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > cc1: some warnings being treated as errors > > make[1]: *** [scripts/Makefile.build:283: crypto/aegis128-core.o] Error 1 > > > > > crypto: aegis128 - expose SIMD code path as separate driver > > > > Fixes the above, but causes > > > > ERROR: modpost: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > > > > as reported by noreply@ellerman.id.au for m68k/defconfig and > > m68k/sun3_defconfig. > > (neon depends on arm). > > > > Thanks for the report. > > It seems like GCC is not optimizing away calls to routines that are > unreachable. Which GCC version are you using? Also, mind checking whether the below works around this? diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c index 2b05f79475d3..89dc1c559689 100644 --- a/crypto/aegis128-core.c +++ b/crypto/aegis128-core.c @@ -89,7 +89,7 @@ static void crypto_aegis128_update_a(struct aegis_state *state, const union aegis_block *msg, bool do_simd) { - if (do_simd) { + if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) && do_simd) { crypto_aegis128_update_simd(state, msg); return; } @@ -101,7 +101,7 @@ static void crypto_aegis128_update_a(struct aegis_state *state, static void crypto_aegis128_update_u(struct aegis_state *state, const void *msg, bool do_simd) { - if (do_simd) { + if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) && do_simd) { crypto_aegis128_update_simd(state, msg); return; }
Hi Ard, On Mon, Nov 30, 2020 at 10:45 AM Ard Biesheuvel <ardb@kernel.org> wrote: > On Mon, 30 Nov 2020 at 10:43, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 30 Nov 2020 at 10:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Nov 17, 2020 at 2:38 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > This series supersedes [0] '[PATCH] crypto: aegis128/neon - optimize tail > > > > block handling', which is included as patch #3 here, but hasn't been > > > > modified substantially. > > > > > > > > Patch #1 should probably go to -stable, even though aegis128 does not appear > > > > to be widely used. > > > > > > > > Patches #2 and #3 improve the SIMD code paths. > > > > > > > > Patch #4 enables fuzz testing for the SIMD code by registering the generic > > > > code as a separate driver if the SIMD code path is enabled. > > > > > > > > Changes since v2: > > > > - add Ondrej's ack to #1 > > > > - fix an issue spotted by Ondrej in #4 where the generic code path would still > > > > use some of the SIMD helpers > > > > > > > > Cc: Ondrej Mosnacek <omosnacek@gmail.com> > > > > Cc: Eric Biggers <ebiggers@kernel.org> > > > > > > > > [0] https://lore.kernel.org/linux-crypto/20201107195516.13952-1-ardb@kernel.org/ > > > > > > > > Ard Biesheuvel (4): > > > > crypto: aegis128 - wipe plaintext and tag if decryption fails > > > > crypto: aegis128/neon - optimize tail block handling > > > > crypto: aegis128/neon - move final tag check to SIMD domain > > > > > > crypto/aegis128-core.c: In function ‘crypto_aegis128_decrypt’: > > > crypto/aegis128-core.c:454:40: error: passing argument 2 of > > > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > > > [-Werror=incompatible-pointer-types] > > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > > | ^~~ > > > | | > > > | struct aead_request * > > > crypto/aegis128-core.c:335:29: note: expected ‘struct skcipher_walk *’ > > > but argument is of type ‘struct aead_request *’ > > > 335 | struct skcipher_walk *walk, > > > | ~~~~~~~~~~~~~~~~~~~~~~^~~~ > > > crypto/aegis128-core.c:454:45: error: passing argument 3 of > > > ‘crypto_aegis128_process_crypt’ from incompatible pointer type > > > [-Werror=incompatible-pointer-types] > > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > > | ^~~~~ > > > | | > > > | struct skcipher_walk * > > > crypto/aegis128-core.c:336:14: note: expected ‘void (*)(struct > > > aegis_state *, u8 *, const u8 *, unsigned int)’ {aka ‘void (*)(struct > > > aegis_state *, unsigned char *, const unsigned char *, unsigned int)’} > > > but argument is of type ‘struct skcipher_walk *’ > > > 336 | void (*crypt)(struct aegis_state *state, > > > | ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > 337 | u8 *dst, const u8 *src, > > > | ~~~~~~~~~~~~~~~~~~~~~~~ > > > 338 | unsigned int size)) > > > | ~~~~~~~~~~~~~~~~~~ > > > crypto/aegis128-core.c:454:4: error: too many arguments to function > > > ‘crypto_aegis128_process_crypt’ > > > 454 | crypto_aegis128_process_crypt(NULL, req, &walk, > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > crypto/aegis128-core.c:334:5: note: declared here > > > 334 | int crypto_aegis128_process_crypt(struct aegis_state *state, > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > cc1: some warnings being treated as errors > > > make[1]: *** [scripts/Makefile.build:283: crypto/aegis128-core.o] Error 1 > > > > > > > crypto: aegis128 - expose SIMD code path as separate driver > > > > > > Fixes the above, but causes > > > > > > ERROR: modpost: "crypto_aegis128_update_simd" [crypto/aegis128.ko] undefined! > > > > > > as reported by noreply@ellerman.id.au for m68k/defconfig and > > > m68k/sun3_defconfig. > > > (neon depends on arm). > > > > > > > Thanks for the report. > > > > It seems like GCC is not optimizing away calls to routines that are The code is not unreachable. Both crypto_aegis128_encrypt_simd() and crypto_aegis128_decrypt_simd() call crypto_aegis128_process_ad(..., true); > > unreachable. Which GCC version are you using? I'm using 9.3.0, Kisskb is using 8.1.0. > Also, mind checking whether the below works around this? > > diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c > index 2b05f79475d3..89dc1c559689 100644 > --- a/crypto/aegis128-core.c > +++ b/crypto/aegis128-core.c > @@ -89,7 +89,7 @@ static void crypto_aegis128_update_a(struct > aegis_state *state, > const union aegis_block *msg, > bool do_simd) > { > - if (do_simd) { > + if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) && do_simd) { > crypto_aegis128_update_simd(state, msg); > return; > } > @@ -101,7 +101,7 @@ static void crypto_aegis128_update_a(struct > aegis_state *state, > static void crypto_aegis128_update_u(struct aegis_state *state, const > void *msg, > bool do_simd) > { > - if (do_simd) { > + if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) && do_simd) { > crypto_aegis128_update_simd(state, msg); > return; > } Thanks, that fixes the build for me. Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert