linux-kernel - Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANc+2y61_o80Wf4JAYB1xi3bb3=6oai77QskSVNjSvpnNZJP0Q@mail.gmail.com>
Date:   Thu, 21 Dec 2017 19:17:01 +0530
From:   PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>, Jakub Jelinek <jakub@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        James Morris <james.l.morris@...cle.com>,
        Richard Biener <rguenther@...e.de>,
        Jakub Jelinek <jakub@....gnu.org>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

Hi Ard,

On 21 December 2017 at 17:52, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
> On 21 December 2017 at 10:20, Arnd Bergmann <arnd@...db.de> wrote:
>> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek <jakub@...hat.com> wrote:
>>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
>>>> diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
>>>> index ca554d57d01e..35f973ba9878 100644
>>>> --- a/crypto/aes_generic.c
>>>> +++ b/crypto/aes_generic.c
>>>> @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
>>>>       f_rl(bo, bi, 3, k);     \
>>>>  } while (0)
>>>>
>>>> +#if __GNUC__ >= 7
>>>> +/*
>>>> + * Newer compilers try to optimize integer arithmetic more aggressively,
>>>> + * which generally improves code quality a lot, but in this specific case
>>>> + * ends up hurting more than it helps, in some configurations drastically
>>>> + * so. This turns off two optimization steps that have been shown to
>>>> + * lead to rather badly optimized code with gcc-7.
>>>> + *
>>>> + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>>>> + */
>>>> +#pragma GCC optimize("-fno-tree-pre")
>>>> +#pragma GCC optimize("-fno-tree-sra")
>>>
>>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>>> predefined macro for those (only for asan and tsan), but either the kernel
>>> does have something already, or could have something added in the
>>> corresponding Makefile.
>>
>> My original interpretation of the resulting object code suggested that disabling
>> those two optimizations produced better results for this particular
>> file even without
>> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
>> been better, but I did some measurements now as Ard suggested, showing
>> cycles/byte for AES256/CBC with 8KB blocks:
>>
>>
>>                default      ubsan         patched        patched+ubsan
>> gcc-4.3.6        14.9        ----           14.9         ----
>> gcc-4.6.4        15.0        ----           15.8         ----
>> gcc-4.9.4        15.5        20.7           15.9         20.9
>> gcc-5.5.0        15.6        47.3           86.4         48.8
>> gcc-6.3.1        14.6        49.4           94.3         50.9
>> gcc-7.1.1        13.5        54.6           15.2         52.0
>> gcc-7.2.1        16.8       124.7           92.0         52.2
>> gcc-8.0.0        15.0      no boot          15.3        no boot
>>
>> I checked that there are actually three significant digits on the measurements,
>> detailed output is available at https://pastebin.com/eFsWYjQp
>>
>> It seems that I was wrong about the interpretation that disabling
>> the optimization would be a win on gcc-7 and higher, it almost
>> always makes things worse even with UBSAN. Making that
>> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
>> would help here, or we could list the file as an exception for
>> UBSAN and never sanitize it.
>>
>> Looking at the 'default' column, I wonder if anyone would be interested
>> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>>
>
> Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
> appears the UBSAN code inserts runtime checks to ensure that certain
> u8 variables don't assume values <0 or >255, which seems like a rather
> pointless exercise to me. But even if it didn't, I think it is
> justified to disable UBSAN on all of the low-level cipher
> implementations, given that they are hardly ever modified, and
> typically don't suffer from the issues UBSAN tries to identify.
>
> So my vote is to disable UBSAN for all such cipher implementations:
> aes_generic, but also aes_ti, which has a similar 256 byte lookup
> table [although it does not seem to be affected by the same issue as
> aes_generic], and possibly others as well.
>
> Perhaps it makes sense to move core cipher code into a separate
> sub-directory, and disable UBSAN at the directory level?
>
> It would involve the following files
>
> crypto/aes_generic.c
> crypto/aes_ti.c
> crypto/anubis.c
> crypto/arc4.c
> crypto/blowfish_generic.c
> crypto/camellia_generic.c
> crypto/cast5_generic.c
> crypto/cast6_generic.c
> crypto/des_generic.c
> crypto/fcrypt.c
> crypto/khazad.c
> crypto/seed.c
> crypto/serpent_generic.c
> crypto/tea.c
> crypto/twofish_generic.c

As *SAN is enabled only on developer setup, is such a change required?
Looks like I am missing something here. Can you explain what value it
provides?

Regards,
PrasannaKumar