linux-kernel - Re: Heads up: gcc miscompiling initramfs zlib decompression code at -O3

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]

Date:   Mon, 3 May 2021 19:18:38 +0000
From:   Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Vineet Gupta <Vineet.Gupta1@...opsys.com>
CC:     Arnd Bergmann <arnd@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Jann Horn <jannh@...gle.com>,
        lkml <linux-kernel@...r.kernel.org>,
        arcml <linux-snps-arc@...ts.infradead.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: Re: Heads up: gcc miscompiling initramfs zlib decompression code at
 -O3

On 5/3/21 10:41 AM, Linus Torvalds wrote:
> On Fri, Apr 30, 2021 at 1:46 PM Vineet Gupta <Vineet.Gupta1@...opsys.com> wrote:
>> I've hit a mainline gcc 10.2 (also gcc 9.3) bug which triggers at -O3
>> causing wrong codegen.
> So it does seem to be a gcc bug or at least mis-feature where gcc gets
> the aliasing decision wrong when vectorizing the code.
>
> That said, the fact that gcc even tries to vectorize the code shows
> how pointless it was for us to (years ago) try to make it use 16-bit
> accesses in the first place.
>
> So can you try this patch that basically reverts some of those
> kernel-specific changes to zlib's inffast.c - and does so by just
> upgrading it to a newer version from a more modern zlib (which in this
> case still means "from 2017", but that's the most recent version there
> is).
>
> This is a fairly quick hack, and I really tried to keep it to just
> inffast.c and inftrees.c with a few minimal fixups to inflate.c
> itself.
>
> Most of the changes are for naming (which seems to be at least partly
> for C++ namespace reasons, ie "this" -> "here"), but there's some
> cleanup wrt maximum table sizes etc too.
>
> NOTE! I have not tested this patch in any other way than "it compiles
> with allmodconfig". The diff looks reasonable to me, but that's all
> I'm really going to say.
>
> Does this avoid the gcc -O3 problem for you?

Yes it does. I built the following:

2021-05-03 b93bedcf8fa4 Update zlib inffast code to more modern zlib
2021-02-26 ea680985468f ARC: fix CONFIG_HARDENED_USERCOPY
2021-04-23 f7118f8ada1b ARC: entry: fix off-by-one error in syscall 
number validation
2021-04-21 1cb7eefda7ed ARC: kgdb: add 'fallthrough' to prevent a warning
2021-03-22 163630b2d95b arc: Fix typos/spellos
2021-04-11 d434405aaab7 Linux 5.12-rc7

And it seems to be DTRT

| Linux version 5.12.0-rc7-00005-gb93bedcf8fa4 
(vineetg@...eetg-Latitude-7400) (arc-linux-gcc.br_real (Buildroot 
2021.02-6-g5e29ba7bf732) 10.2.0, GNU ld (GNU Binutils) 2.36.1) #678 
PREEMPT Mon May 3 11:29:32 PDT 2021
| Memory @ 80000000 [1024M]
| ...
|   with environment:
|     HOME=/
|     TERM=linux
| Starting syslogd: OK
| Starting klogd: OK
| Running sysctl: OK
| $
| $ zcat /proc/config.gz | egrep "(OPTIM|COMPRESSION_GZIP)"
| CONFIG_INITRAMFS_COMPRESSION_GZIP=y
| # CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is not set
| CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE_O3=y
|
| $ free -m
|               total        used        free      shared buff/cache   
available
| Mem:           1012           3         978          31 31         972
| Swap:
|

> It would be lovely if somebody also took a look at some of the other
> zlib code, like inflate.c itself. But some of that code has rather
> subtle changes for things like s390 hardware support, and we have
> thihngs like our fallthrough rules etc, so it gets a bit hairier.

I took a quick look, but there some to be subtle state machine changes 
in inflate.c which I'm not comfortable mucking around with.

> Which is why I did just this fairly minimal part.
>
> Note that the commit message has a "Not-yet-signed-off-by:" from me.
> That's purely because I wanted it to be obvious that this needs a lot
> more testing - I'm not comfy with this patch unless somebody takes it
> upon themselves to actually test it under different loads (and
> different architectures).

Maybe give it some time to bake in linux-next for a 5.14 inclusion ?
Thx again for jumping in and steering gcc folks to right conclusions.

-Vineet