Re: [PATCH] [v2] crypto: aes-generic - build with -Os on gcc-7+

From: Herbert Xu
Date: Fri Jan 12 2018 - 07:25:58 EST


On Wed, Jan 03, 2018 at 11:39:27PM +0100, Arnd Bergmann wrote:
> While testing other changes, I discovered that gcc-7.2.1 produces badly
> optimized code for aes_encrypt/aes_decrypt. This is especially true when
> CONFIG_UBSAN_SANITIZE_ALL is enabled, where it leads to extremely
> large stack usage that in turn might cause kernel stack overflows:
>
> crypto/aes_generic.c: In function 'aes_encrypt':
> crypto/aes_generic.c:1371:1: warning: the frame size of 4880 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> crypto/aes_generic.c: In function 'aes_decrypt':
> crypto/aes_generic.c:1441:1: warning: the frame size of 4864 bytes is larger than 2048 bytes [-Wframe-larger-than=]
>
> I verified that this problem exists on all architectures that are
> supported by gcc-7.2, though arm64 in particular is less affected than
> the others. I also found that gcc-7.1 and gcc-8 do not show the extreme
> stack usage but still produce worse code than earlier versions for this
> file, apparently because of optimization passes that generally provide
> a substantial improvement in object code quality but understandably fail
> to find any shortcuts in the AES algorithm.
>
> Possible workarounds include
>
> a) disabling -ftree-pre and -ftree-sra optimizations, this was an earlier
> patch I tried, which reliably fixed the stack usage, but caused a
> serious performance regression in some versions, as later testing
> found.
>
> b) disabling UBSAN on this file or all ciphers, as suggested by Ard
> Biesheuvel. This would lead to massively better crypto performance in
> UBSAN-enabled kernels and avoid the stack usage, but there is a concern
> over whether we should exclude arbitrary files from UBSAN at all.
>
> c) Forcing the optimization level in a different way. Similar to a),
> but rather than deselecting specific optimization stages,
> this now uses "gcc -Os" for this file, regardless of the
> CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE/SIZE option. This is a reliable
> workaround for the stack consumption on all architecture, and I've
> retested the performance results now on x86, cycles/byte (lower is
> better) for cbc(aes-generic) with 256 bit keys:
>
> -O2 -Os
> gcc-6.3.1 14.9 15.1
> gcc-7.0.1 14.7 15.3
> gcc-7.1.1 15.3 14.7
> gcc-7.2.1 16.8 15.9
> gcc-8.0.0 15.5 15.6
>
> This implements the option c) by enabling forcing -Os on all compiler
> versions starting with gcc-7.1. As a workaround for PR83356, it would
> only be needed for gcc-7.2+ with UBSAN enabled, but since it also shows
> better performance on gcc-7.1 without UBSAN, it seems appropriate to
> use the faster version here as well.
>
> Side note: during testing, I also played with the AES code in libressl,
> which had a similar performance regression from gcc-6 to gcc-7.2,
> but was three times slower overall. It might be interesting to
> investigate that further and possibly port the Linux implementation
> into that.
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83651
> Cc: Richard Biener <rguenther@xxxxxxx>
> Cc: Jakub Jelinek <jakub@xxxxxxxxxxx>
> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

Patch applied. Thanks.
--
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt