Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201117133214.29114-1-ardb@kernel.org> <CAMuHMdUzFLk=oYo1aK80d0H-qZ1_1BcdSULkYCxBnLWT_qUR2A@mail.gmail.com>
 <CAMj1kXGuxuB2iv-k60PCSkmec-=_+kKRnE3XSXeqNLihqrH9vQ@mail.gmail.com> <CAMj1kXH5SCe-v37BJrv3m2T6sHFkRfB6fPPk_+tvTtCZEZSkxQ@mail.gmail.com>
In-Reply-To: <CAMj1kXH5SCe-v37BJrv3m2T6sHFkRfB6fPPk_+tvTtCZEZSkxQ@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 30 Nov 2020 13:14:45 +0100
Message-ID: <CAMuHMdWRA6fSxnY7D1kYvxXPy0UkA7=1iJU+hYxJFM-5zOtf9g@mail.gmail.com>
Subject: Re: [PATCH v3 0/4] crypto: aegis128 enhancements
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Eric Biggers <ebiggers@kernel.org>,
        Ondrej Mosnacek <omosnacek@gmail.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux-Next <linux-next@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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

-- 
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