linux-kernel - Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu

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: <CAHk-=wjvHDp+oiC4UZxivF6fCjKWFAAzgBYZdng6qe+ED6rLTg@mail.gmail.com>
Date:   Thu, 19 Oct 2023 15:39:12 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Uros Bizjak <ubizjak@...il.com>
Cc:     peterz@...radead.org, Nadav Amit <namit@...are.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()

Unrelated question to the gcc people (well, related in the way that
this discussion made me *test* this).

Lookie here:

    int test(void)
    {
        unsigned int sum = 0;
        for (int i = 0; i < 4; i++) {
                unsigned int val;
    #if ONE
                asm("magic1 %0":"=r" (val): :"memory");
    #else
                asm volatile("magic2 %0":"=r" (val));
    #endif
                sum += val;
        }
        return sum;
    }

and now build this with

    gcc -O2 -S -DONE -funroll-all-loops t.c

and I get a *completely* nonsensical end result. What gcc generates is
literally insane.

What I *expected* to happen was that the two cases (with "-DONE" and
without) would generate the same code, since one has a "asm volatile",
and the other has a memory clobber.

IOW, neither really should be something that can be combined.

But no. The '-DONE" version is completely crazy with my gcc-13.2.1 setup.

First off, it does actually CSE all the asm's despite the memory
clobber. Which I find quite debatable, but whatever.

But not only does it CSE them, it then does *not* just multiply the
result by four. No. It generates this insanity:

        magic1 %eax
        movl    %eax, %edx
        addl    %eax, %eax
        addl    %edx, %eax
        addl    %edx, %eax
        ret

so it has apparently done the CSE _after_ the other optimizations.

Very strange.

Honestly, the CSE part looks like an obvious bug to me. The gcc
documentation states:

     The "memory" clobber tells the compiler that the assembly code
     performs memory reads or writes to items other than those listed in
     the input and output operands (for example, accessing the memory
     pointed to by one of the input parameters).

so CSE'ing any inline asm with a memory clobber sounds *very* dubious.
The asm literally told the compiler that it has side effects in
unrelated memory locations!

I don't think we actually care in the kernel (and yes, I think it
would always be safer to use "asm volatile" if there's some unrelated
memory locations that change), but since I was testing this and was
surprised, and since the obvious reading of the documented behavior of
a memory clobber really does scream "you can't combine those asms", I
thought I'd mention this.

Also, *without* the memory clobber, gcc obviously still does CSE the
asm, but also, gcc ends up doing just

        magic1 %eax
        sall    $2, %eax
        ret

so the memory clobber clearly does actually make a difference. Just
not a _sane_ one.

In testing, clang does *not* have this apparently buggy behavior (but
clang annoyingly actually checks the instruction mnemonics, so I had
to change "magic" into "strl" instead to make clang happy).

Hmm?

                  Linus