Message-ID: <CAKwvOdmNwNwa6rMC27-QZq8VDrYdTQeQqss-bAwF1EMmnAHxdw@mail.gmail.com>
Date:   Fri, 17 Apr 2020 11:22:25 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Jakub Jelinek <jakub@...hat.com>,
        Sergei Trofimovich <slyfox@...too.org>,
        Michael Matz <matz@...e.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH v2] x86: fix early boot crash on gcc-10

On Fri, Apr 17, 2020 at 11:15 AM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> On Fri, Apr 17, 2020 at 2:09 AM Borislav Petkov <bp@...en8.de> wrote:
> >
> > On Fri, Apr 17, 2020 at 10:58:59AM +0200, Jakub Jelinek wrote:
> > > On Fri, Apr 17, 2020 at 10:42:24AM +0200, Borislav Petkov wrote:
> > > > On Fri, Apr 17, 2020 at 10:07:26AM +0200, Jakub Jelinek wrote:
> > > > > If you want minimal changes, you can as I said earlier either
> > > > > mark cpu_startup_entry noreturn (in the declaration in some header so that
> > > > > smpboot.c sees it), or you could add something after the cpu_startup_entry
> > > > > call to ensure it is not tail call optimized (e.g. just
> > > > >   /* Prevent tail call to cpu_startup_entry because the stack
> > > > >      protector guard has been changed in the middle of this function
> > > > >      and must not be checked before tail calling another function.  */
> > > > >   asm ("");
> > > >
> > > > That sounds ok-ish to me too.
> > > >
> > > > I know you probably can't tell the future :) but what stops gcc from
> > > > doing the tail-call optimization in the future?
> > > >
> > > > Or are optimization decisions behind an inline asm a no-no and will
> > > > pretty much always stay that way?
> > >
> > > GCC intentionally treats asm as a black box, the only thing which it does
>
> Yep, that's how I would describe how LLVM see's inline asm, too.
>
> > > with it is: non-volatile asm (but asm without outputs is implicitly
> > > volatile) can be CSEd, and if the compiler needs to estimate size, it
> > > uses some heuristics by counting ; and newlines.
> > > And it will stay this way.
>
> I recently implemented parsing support for `asm inline` in Clang; I
> could have sworn I saw code in LLVM parsing newlines for a size
> estimate years ago, but when implementing `asm inline`, I couldn't
> find it.  And test cases I wrote that used the C preprocessor to
> create thousand+ line inline asm strings would always be inlined,
> regardless of the `inline` asm qualifier.
>
> Not sure about implied volatility (...inner stock trader had a laugh
> at that...) for output-less asm statements.
>
> > >
> > > > And I hope the clang folks don't come around and say, err, nope, we're
> > > > much more aggressive here.
> > >
> > > Unlike GCC, I think clang uses the builtin assembler to parse the string,
> > > but don't know if it still treats the asms more like black boxes or not.
> > > Certainly there is a lot of code in the wild that uses inline asm
> > > as optimization barriers, so if it doesn't, then it would cause a lot of
> > > problems.
> > >
> > > Or go with the for (;;);, I don't think any compiler optimizes those away;
> > > GCC 10 for C++ can optimize away infinite loops that have some conditional
> > > exit because the language guarantees forward progress, but the C language
> > > rules are different and for unconditional infinite loops GCC doesn't
> > > optimize them away even if explicitly asked to -ffinite-loops.
> >
> > Lemme add Nick for clang for an opinion:
> >
> > Nick, we're discussing what would be the cleanest and future-proof
> > way to disable stack protector for the function in the kernel which
>
> Oh, this reminds me of commit d0a8d9378d16 ("x86/paravirt: Make
> native_save_fl() extern inline"), where the insertion of stack guards
> was also causing some pain.
>
> The cleanest solution would be to have function attributes that say
> "yes, I know I said -fstack-protector*, but for this one lone function
> I really need -fno-stack-protector.  I know what I'm doing and accept
> whatever the consequences are."  But maybe the attribute would be
> shorter than all that? :P
>
> Compared to playing games with each other's inlining heuristics, that

s/inlining/tail call/

> would be the cleanest and future-proof solution.  (Then we can even
> revert d0a8d9378d16, and use such a function attribute.  I somehow
> prefer gnu_inline's semantics to ISO C99's extern inline semantics,
> and simultaneously hate the problems for which it's used.)
>
> > generates the canary value as gcc10 ends up checking that value due to
> > tail-call optimizing the last function called by start_secondary()...
> > upthread are all the details.
> >
> > And question is, can Jakub's suggestions above prevent tail-call
> > optimization on clang too and how reliable and future proof would that
> > be if we end up going that way?
>
> Sorry, I don't quite follow.  The idea is that an empty asm statement
> in foo() should prevent foo() from being inlined into bar()?

s/inlined/tail called/

> https://godbolt.org/z/7xBRGY

-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists