Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Nick Desaulniers]

On Thu, Nov 12, 2020 at 11:59 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Alexandre,
>
> [FYI, it's a private test report for your RFC patch.]
> [auto build test WARNING on tip/x86/mm]
> [also build test WARNING on v5.10-rc3]
> [cannot apply to tip/x86/core tip/x86/asm next-20201112]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/x86-pti-Defer-CR3-switch-to-C-code/20201109-225007
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1fcd009102ee02e217f2e7635ab65517d785da8e
> config: x86_64-randconfig-a011-20201109 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 09ec07827b1128504457a93dee80b2ceee1af600)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # https://github.com/0day-ci/linux/commit/1095b6a7acb8dc568c474cd3b3d1c8393fb49728
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Alexandre-Chartre/x86-pti-Defer-CR3-switch-to-C-code/20201109-225007
>         git checkout 1095b6a7acb8dc568c474cd3b3d1c8393fb49728
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> arch/x86/kernel/cpu/mce/core.c:2023:1: warning: unknown attribute 'optimize' ignored [-Wunknown-attributes]
>    DEFINE_IDTENTRY_MCE(exc_machine_check)
>    ^
>    arch/x86/include/asm/idtentry.h:539:30: note: expanded from macro 'DEFINE_IDTENTRY_MCE'
>    #define DEFINE_IDTENTRY_MCE             DEFINE_IDTENTRY_IST
>                                            ^
>    arch/x86/include/asm/idtentry.h:428:2: note: expanded from macro 'DEFINE_IDTENTRY_IST'
>            no_stack_protector DEFINE_IDTENTRY_RAW(func)
>            ^
>    arch/x86/include/asm/idtentry.h:110:18: note: expanded from macro 'no_stack_protector'
>            __attribute__ ((optimize("-O2,-fno-stack-protector,-fno-omit-frame-pointer")))
>                            ^

Hi Alexandre,
The `optimize` attribute is both non-portable across toolchains (hence
this warning) and a little quirky in GCC.  It's advised that you do
not use it.  If you can describe the issue you're trying to solve with
it, we likely have a better way to fix.

If it's removing a stack protector, -fno-stack-protector for the whole
translation unit is the only portable solution today. (GCC 10 just
gained the no_stack_protector attribute, but the kernel needs to
support older versions of GCC).

>    1 warning generated.
>
> vim +/optimize +2023 arch/x86/kernel/cpu/mce/core.c
>
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2020
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2021  #ifdef CONFIG_X86_64
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2022  /* MCE hit kernel mode */
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25 @2023  DEFINE_IDTENTRY_MCE(exc_machine_check)
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2024  {
> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2025        unsigned long dr7;
> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2026
> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2027        dr7 = local_db_save();
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2028        exc_machine_check_kernel(regs);
> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2029        local_db_restore(dr7);
> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2030  }
> 9eda8cb3ac2352 arch/x86/kernel/cpu/mcheck/mce.c Andi Kleen      2009-07-09  2031
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/202011131552.4kvOb9Id-lkp%40intel.com.



-- 
Thanks,
~Nick Desaulniers

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Alexandre Chartre]

On 11/13/20 7:59 PM, Nick Desaulniers wrote:
> On Thu, Nov 12, 2020 at 11:59 PM kernel test robot <lkp@intel.com> wrote:
>>
>> Hi Alexandre,
>>
>> [FYI, it's a private test report for your RFC patch.]
>> [auto build test WARNING on tip/x86/mm]
>> [also build test WARNING on v5.10-rc3]
>> [cannot apply to tip/x86/core tip/x86/asm next-20201112]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/x86-pti-Defer-CR3-switch-to-C-code/20201109-225007
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1fcd009102ee02e217f2e7635ab65517d785da8e
>> config: x86_64-randconfig-a011-20201109 (attached as .config)
>> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 09ec07827b1128504457a93dee80b2ceee1af600)
>> reproduce (this is a W=1 build):
>>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # install x86_64 cross compiling tool for clang build
>>          # apt-get install binutils-x86-64-linux-gnu
>>          # https://github.com/0day-ci/linux/commit/1095b6a7acb8dc568c474cd3b3d1c8393fb49728
>>          git remote add linux-review https://github.com/0day-ci/linux
>>          git fetch --no-tags linux-review Alexandre-Chartre/x86-pti-Defer-CR3-switch-to-C-code/20201109-225007
>>          git checkout 1095b6a7acb8dc568c474cd3b3d1c8393fb49728
>>          # save the attached .config to linux build tree
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All warnings (new ones prefixed by >>):
>>
>>>> arch/x86/kernel/cpu/mce/core.c:2023:1: warning: unknown attribute 'optimize' ignored [-Wunknown-attributes]
>>     DEFINE_IDTENTRY_MCE(exc_machine_check)
>>     ^
>>     arch/x86/include/asm/idtentry.h:539:30: note: expanded from macro 'DEFINE_IDTENTRY_MCE'
>>     #define DEFINE_IDTENTRY_MCE             DEFINE_IDTENTRY_IST
>>                                             ^
>>     arch/x86/include/asm/idtentry.h:428:2: note: expanded from macro 'DEFINE_IDTENTRY_IST'
>>             no_stack_protector DEFINE_IDTENTRY_RAW(func)
>>             ^
>>     arch/x86/include/asm/idtentry.h:110:18: note: expanded from macro 'no_stack_protector'
>>             __attribute__ ((optimize("-O2,-fno-stack-protector,-fno-omit-frame-pointer")))
>>                             ^
> 
> Hi Alexandre,
> The `optimize` attribute is both non-portable across toolchains (hence
> this warning) and a little quirky in GCC.  It's advised that you do
> not use it.  If you can describe the issue you're trying to solve with
> it, we likely have a better way to fix.

Yeah, I feared this wouldn't be portable. However, I am going to remove that
patch; I am making a change which doesn't require to disable stack protector
anymore.


> If it's removing a stack protector, -fno-stack-protector for the whole
> translation unit is the only portable solution today. (GCC 10 just
> gained the no_stack_protector attribute, but the kernel needs to
> support older versions of GCC).

I needed to disable stack protector for particular functions. Good to know
that GCC 10 will provide an attribute to do that. In the meantime, if I still
need it, I guess I would group functions into a file and disable stack protector
for that file.

Thanks.

alex.

>>     1 warning generated.
>>
>> vim +/optimize +2023 arch/x86/kernel/cpu/mce/core.c
>>
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2020
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2021  #ifdef CONFIG_X86_64
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2022  /* MCE hit kernel mode */
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25 @2023  DEFINE_IDTENTRY_MCE(exc_machine_check)
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2024  {
>> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2025        unsigned long dr7;
>> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2026
>> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2027        dr7 = local_db_save();
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2028        exc_machine_check_kernel(regs);
>> cd840e424f27fc arch/x86/kernel/cpu/mce/core.c   Peter Zijlstra  2020-05-29  2029        local_db_restore(dr7);
>> 4c0dcd8350a03c arch/x86/kernel/cpu/mce/core.c   Thomas Gleixner 2020-02-25  2030  }
>> 9eda8cb3ac2352 arch/x86/kernel/cpu/mcheck/mce.c Andi Kleen      2009-07-09  2031
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>>
>> --
>> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/202011131552.4kvOb9Id-lkp%40intel.com.
> 
> 
> 

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Nick Desaulniers]

On Fri, Nov 13, 2020 at 11:39 AM Alexandre Chartre
<alexandre.chartre@oracle.com> wrote:
>
>
>
> On 11/13/20 7:59 PM, Nick Desaulniers wrote:
> > Hi Alexandre,
> > The `optimize` attribute is both non-portable across toolchains (hence
> > this warning) and a little quirky in GCC.  It's advised that you do
> > not use it.  If you can describe the issue you're trying to solve with
> > it, we likely have a better way to fix.
>
> Yeah, I feared this wouldn't be portable. However, I am going to remove that
> patch; I am making a change which doesn't require to disable stack protector
> anymore.

Ah, good to hear.

>
>
> > If it's removing a stack protector, -fno-stack-protector for the whole
> > translation unit is the only portable solution today. (GCC 10 just
> > gained the no_stack_protector attribute, but the kernel needs to
> > support older versions of GCC).
>
> I needed to disable stack protector for particular functions. Good to know
> that GCC 10 will provide an attribute to do that. In the meantime, if I still
> need it, I guess I would group functions into a file and disable stack protector
> for that file.

Yep, that would be best if still needed.  Since I had the link handy
(was just discussing this internally with folks working on `perf`),
Ard found a nice link that summarizes the issues succinctly:
https://gcc.gnu.org/wiki/FAQ#optimize_attribute_broken.  We're about
to remove its use from abseil (C++ library).

>
> Thanks.
>
> alex.


-- 
Thanks,
~Nick Desaulniers

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Segher Boessenkool]

Hi!

On Fri, Nov 13, 2020 at 10:59:26AM -0800, Nick Desaulniers wrote:
> The `optimize` attribute is both non-portable across toolchains (hence
> this warning)

Like *all* GCC extensions.

> and a little quirky in GCC.

How so?  Don't spread FUD please, say what *is* wrong, then people can
decide for themselves whether they want it or not.

We (GCC) do document it as:

     Not every optimization option that starts with the -f prefix
     specified by the attribute necessarily has an effect on the
     function.  The 'optimize' attribute should be used for debugging
     purposes only.  It is not suitable in production code.

The optimize attribute is for setting/resetting flags on a function
granularity.  Not all flags can be flipped per function.  The SSP flags
work fine though, AFAIK.  But don't use it for production, there are no
guarantees.

Cheers,


Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Miguel Ojeda]

On Sat, Nov 14, 2020 at 12:48 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Like *all* GCC extensions.

They might not be standard, but they may be portable enough for the
compilers we care about.

In any case, files should not use attributes on their own.

Cheers,
Miguel

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Segher Boessenkool]

On Sat, Nov 14, 2020 at 01:01:42AM +0100, Miguel Ojeda wrote:
> On Sat, Nov 14, 2020 at 12:48 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > Like *all* GCC extensions.
> 
> They might not be standard, but they may be portable enough for the
> compilers we care about.

Sure, they may be portable enough for some specific purpose :-)

But you cannot limit yourself to the greatest common denominator: the
kernel wants to use many features only available on newer compiler
versions, too; this is no different.


Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Miguel Ojeda]

On Sat, Nov 14, 2020 at 1:27 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> But you cannot limit yourself to the greatest common denominator: the
> kernel wants to use many features only available on newer compiler
> versions, too; this is no different.

What we do is conditionally enable both new and differentiating
features. We don't break GCC or LLVM just because the other introduces
a new shiny feature, nor we break builds with old compilers (except
when raising the minimum supported version).

That is why we need a level of indirection in things like attributes
nowadays, and why patches like the above are not ideal anymore
(regardless of which attribute we are talking about).

Cheers,
Miguel

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Ard Biesheuvel]

On Sat, 14 Nov 2020 at 02:59, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Nov 14, 2020 at 1:27 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > But you cannot limit yourself to the greatest common denominator: the
> > kernel wants to use many features only available on newer compiler
> > versions, too; this is no different.
>
> What we do is conditionally enable both new and differentiating
> features. We don't break GCC or LLVM just because the other introduces
> a new shiny feature, nor we break builds with old compilers (except
> when raising the minimum supported version).
>
> That is why we need a level of indirection in things like attributes
> nowadays, and why patches like the above are not ideal anymore
> (regardless of which attribute we are talking about).
>

In spite of the apparent difference of opinion here, there are two
irrefutable facts about __attribute__((optimize)) on GCC that can only
lead to the conclusion that we must never use it in Linux:
- the GCC developers refuse to rigorously define its behavior, so we
don't know what it actually does;
- it has been observed to nullify unrelated command line arguments in
unexpected and undocumented ways.

So it does not matter whether you call that quirky or something else,
there is no way we can make meaningful use of it, and therefore,
abstracting or parameterizing any of its uses should be avoided as
well.

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Kees Cook]

On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> On Sat, 14 Nov 2020 at 02:59, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Sat, Nov 14, 2020 at 1:27 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > >
> > > But you cannot limit yourself to the greatest common denominator: the
> > > kernel wants to use many features only available on newer compiler
> > > versions, too; this is no different.
> >
> > What we do is conditionally enable both new and differentiating
> > features. We don't break GCC or LLVM just because the other introduces
> > a new shiny feature, nor we break builds with old compilers (except
> > when raising the minimum supported version).
> >
> > That is why we need a level of indirection in things like attributes
> > nowadays, and why patches like the above are not ideal anymore
> > (regardless of which attribute we are talking about).
> >
> 
> In spite of the apparent difference of opinion here, there are two
> irrefutable facts about __attribute__((optimize)) on GCC that can only
> lead to the conclusion that we must never use it in Linux:
> - the GCC developers refuse to rigorously define its behavior, so we
> don't know what it actually does;
> - it has been observed to nullify unrelated command line arguments in
> unexpected and undocumented ways.
> 
> So it does not matter whether you call that quirky or something else,
> there is no way we can make meaningful use of it, and therefore,
> abstracting or parameterizing any of its uses should be avoided as
> well.

Perhaps it's worth adding this to CodingStyle or deprecated.rst?

-- 
Kees Cook

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Segher Boessenkool]

On Wed, Nov 25, 2020 at 11:56:40AM -0800, Kees Cook wrote:
> On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> > In spite of the apparent difference of opinion here, there are two
> > irrefutable facts about __attribute__((optimize)) on GCC that can only
> > lead to the conclusion that we must never use it in Linux:
> > - the GCC developers refuse to rigorously define its behavior, so we
> > don't know what it actually does;

This is because it isn't clear at all what it *should* do, for some
options.  For others it is obvious, and it works just fine for those.

(And we do not rigorously define the behaviour of almost *anything*, not
in the user manual anyway!)

The interface has huge usability problems.  We want to wean people off
of using this attribute.  But claiming all kinds of FUD about it is a
disservice to users: it works fine for where it does work, there is no
reason for people to hurriedly change their code (or change it at all).

> > So it does not matter whether you call that quirky or something else,
> > there is no way we can make meaningful use of it, and therefore,
> > abstracting or parameterizing any of its uses should be avoided as
> > well.
> 
> Perhaps it's worth adding this to CodingStyle or deprecated.rst?

Sure!  But please don't claim things that aren't broken are.


Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Ard Biesheuvel]

On Thu, 26 Nov 2020 at 00:03, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Nov 25, 2020 at 11:56:40AM -0800, Kees Cook wrote:
> > On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> > > In spite of the apparent difference of opinion here, there are two
> > > irrefutable facts about __attribute__((optimize)) on GCC that can only
> > > lead to the conclusion that we must never use it in Linux:
> > > - the GCC developers refuse to rigorously define its behavior, so we
> > > don't know what it actually does;
>
> This is because it isn't clear at all what it *should* do, for some
> options.  For others it is obvious, and it works just fine for those.
>

The problem is that the distinction of some vs. others is not
documented, and may change between architectures or GCC versions.


> (And we do not rigorously define the behaviour of almost *anything*, not
> in the user manual anyway!)
>
> The interface has huge usability problems.  We want to wean people off
> of using this attribute.  But claiming all kinds of FUD about it is a
> disservice to users: it works fine for where it does work, there is no
> reason for people to hurriedly change their code (or change it at all).
>

What do you mean by all kinds of FUD? The kind of FUD appearing on the
GCC wiki? I'll quote it again here for everyone's convenience.


"""
Currently (2015), this attribute is known to have several critical
bugs (PR37565, PR63401, PR60580, PR50782). Using it may produce not
effect at all or lead to wrong-code.

Quoting one GCC maintainer: "I consider the optimize attribute code
seriously broken and unmaintained (but sometimes useful for debugging
- and only that)." source

Unfortunately, the people who added it are either not working on GCC
anymore or not interested in fixing it. Do not try to guess how it is
supposed to work by trial-and-error. There is not a list of options
that are safe to use or known to be broken. Bug reports about the
optimize attribute being broken will probably be closed as WONTFIX
(PR59262), thus it is not worth to open new ones. If it works for you
for a given version of GCC, it doesn't mean it will work on a
different machine or a different version.

The only realistic choices are to not use it, to use it and accept its
brokenness (current or future one, since it is unmaintained), or join
GCC and fix it (perhaps motivating other people along the way to join
your effort).

"""

The reason we have to change code in the kernel is because it actually
breaks stuff. For instance, functions using __attribute__((optimize))
to disable GCSE are suddenly compiled with or without stack protector
checks or frame pointers, even though the opposite option is set at
the compilation unit level.

I am not disputing that __attribute__((optimize)) is highly useful in
some cases, I am just arguing that such cases don't exist in a Linux
kernel running on a production system.


> > > So it does not matter whether you call that quirky or something else,
> > > there is no way we can make meaningful use of it, and therefore,
> > > abstracting or parameterizing any of its uses should be avoided as
> > > well.
> >
> > Perhaps it's worth adding this to CodingStyle or deprecated.rst?
>
> Sure!  But please don't claim things that aren't broken are.
>
>
> Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Segher Boessenkool]

On Thu, Nov 26, 2020 at 07:40:10AM +0100, Ard Biesheuvel wrote:
> On Thu, 26 Nov 2020 at 00:03, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Wed, Nov 25, 2020 at 11:56:40AM -0800, Kees Cook wrote:
> > > On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> > > > In spite of the apparent difference of opinion here, there are two
> > > > irrefutable facts about __attribute__((optimize)) on GCC that can only
> > > > lead to the conclusion that we must never use it in Linux:
> > > > - the GCC developers refuse to rigorously define its behavior, so we
> > > > don't know what it actually does;
> >
> > This is because it isn't clear at all what it *should* do, for some
> > options.  For others it is obvious, and it works just fine for those.
> 
> The problem is that the distinction of some vs. others is not
> documented, and may change between architectures or GCC versions.

And obvious is still obvious.

> > (And we do not rigorously define the behaviour of almost *anything*, not
> > in the user manual anyway!)
> >
> > The interface has huge usability problems.  We want to wean people off
> > of using this attribute.  But claiming all kinds of FUD about it is a
> > disservice to users: it works fine for where it does work, there is no
> > reason for people to hurriedly change their code (or change it at all).
> 
> What do you mean by all kinds of FUD? The kind of FUD appearing on the
> GCC wiki? I'll quote it again here for everyone's convenience.

<snip>

No, saying "ohnoes this feature (that always worked fine for many
purposes) is so broken that it is super dangerous to keep using it even
a minute longer".  _That_ FUD.

> The reason we have to change code in the kernel is because it actually
> breaks stuff.

That makes sense.  Then please write *that*?  :-)

> For instance, functions using __attribute__((optimize))
> to disable GCSE are suddenly compiled with or without stack protector
> checks or frame pointers, even though the opposite option is set at
> the compilation unit level.

Not that disabling GCSE makes much sense anyway (there are other passes
that do many of the same things, for example).  So why was this added?
To avoid some bug on some older compiler version?

> I am not disputing that __attribute__((optimize)) is highly useful in
> some cases, I am just arguing that such cases don't exist in a Linux
> kernel running on a production system.

And I am saying that before you can remove something, you probably
should look at why it was added in the first place, and see if you need
a replacement for that, etc.  Just destroying stuff you do not agree
with is iconoclasm.


Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Arvind Sankar]

On Thu, Nov 26, 2020 at 02:22:07PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 26, 2020 at 07:40:10AM +0100, Ard Biesheuvel wrote:
> > On Thu, 26 Nov 2020 at 00:03, Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Wed, Nov 25, 2020 at 11:56:40AM -0800, Kees Cook wrote:
> > > > On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> > > > > In spite of the apparent difference of opinion here, there are two
> > > > > irrefutable facts about __attribute__((optimize)) on GCC that can only
> > > > > lead to the conclusion that we must never use it in Linux:
> > > > > - the GCC developers refuse to rigorously define its behavior, so we
> > > > > don't know what it actually does;
> > >
> > > This is because it isn't clear at all what it *should* do, for some
> > > options.  For others it is obvious, and it works just fine for those.
> > 
> > The problem is that the distinction of some vs. others is not
> > documented, and may change between architectures or GCC versions.
> 
> And obvious is still obvious.
> 
> > > (And we do not rigorously define the behaviour of almost *anything*, not
> > > in the user manual anyway!)
> > >
> > > The interface has huge usability problems.  We want to wean people off
> > > of using this attribute.  But claiming all kinds of FUD about it is a
> > > disservice to users: it works fine for where it does work, there is no
> > > reason for people to hurriedly change their code (or change it at all).
> > 
> > What do you mean by all kinds of FUD? The kind of FUD appearing on the
> > GCC wiki? I'll quote it again here for everyone's convenience.
> 
> <snip>
> 
> No, saying "ohnoes this feature (that always worked fine for many
> purposes) is so broken that it is super dangerous to keep using it even
> a minute longer".  _That_ FUD.
> 

In the context of _this_ thread -- newly applying the attribute to an
exception entry vector, it is about stopping new users who may not be
aware of the attribute's pitfalls.  You've got to admit that "quirky" is
comparatively mild language relative to what's in GCC's own wiki.

The breakage we've been trying to reverse/avoid isn't even about the
bugs alluded to in the wiki. It's an impedance mismatch in what users
are expecting the attribute to do and what it actually does, and it
would be good to add a mention of this in the documentation. Eg, in this
case the programmer expects stack protector to be disabled, and no other
effects. But the attribute will actually cause, eg -fstrict-aliasing to
be enabled for the function even though it was disabled on the command
line. Even outside of any bugs in the implementation of the attribute,
this has potential to subtly break kernel code in unexpected ways
because most users aren't aware of what the attribute really does.

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Ard Biesheuvel]

On Thu, 26 Nov 2020 at 21:25, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Nov 26, 2020 at 07:40:10AM +0100, Ard Biesheuvel wrote:
> > On Thu, 26 Nov 2020 at 00:03, Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Wed, Nov 25, 2020 at 11:56:40AM -0800, Kees Cook wrote:
> > > > On Sat, Nov 14, 2020 at 11:20:17AM +0100, Ard Biesheuvel wrote:
> > > > > In spite of the apparent difference of opinion here, there are two
> > > > > irrefutable facts about __attribute__((optimize)) on GCC that can only
> > > > > lead to the conclusion that we must never use it in Linux:
> > > > > - the GCC developers refuse to rigorously define its behavior, so we
> > > > > don't know what it actually does;
> > >
> > > This is because it isn't clear at all what it *should* do, for some
> > > options.  For others it is obvious, and it works just fine for those.
> >
> > The problem is that the distinction of some vs. others is not
> > documented, and may change between architectures or GCC versions.
>
> And obvious is still obvious.
>

Not really, due to the way it interacts with other options set on the
command line.

> > > (And we do not rigorously define the behaviour of almost *anything*, not
> > > in the user manual anyway!)
> > >
> > > The interface has huge usability problems.  We want to wean people off
> > > of using this attribute.  But claiming all kinds of FUD about it is a
> > > disservice to users: it works fine for where it does work, there is no
> > > reason for people to hurriedly change their code (or change it at all).
> >
> > What do you mean by all kinds of FUD? The kind of FUD appearing on the
> > GCC wiki? I'll quote it again here for everyone's convenience.
>
> <snip>
>
> No, saying "ohnoes this feature (that always worked fine for many
> purposes) is so broken that it is super dangerous to keep using it even
> a minute longer".  _That_ FUD.
>

It is. When we add -fno-stack-protector on the command line, we do
that for a reason. When we notice that __attribute__((optimize)) makes
GCC forget about that, we have a problem.

> > The reason we have to change code in the kernel is because it actually
> > breaks stuff.
>
> That makes sense.  Then please write *that*?  :-)
>

See above.

> > For instance, functions using __attribute__((optimize))
> > to disable GCSE are suddenly compiled with or without stack protector
> > checks or frame pointers, even though the opposite option is set at
> > the compilation unit level.
>
> Not that disabling GCSE makes much sense anyway (there are other passes
> that do many of the same things, for example).  So why was this added?
> To avoid some bug on some older compiler version?
>

No, it is related to x86 specific tooling (objtool) that exists in
Linux, which gets confused when code containing indirect gotos is
built with GCSE.

So the proposed fix was to use something like
__attribute__((optimize("-fno-gcse,-fno-stack-protector,-fomit-frame-pointer")))
because just disabling GCSE interferes with those other options as
well. I'm sure you can agree that this is not maintainable, and so
__attribute__((optimize)) is simply not suitable for our use.

But let's not get into objtool, that is another can of worms I'd
prefer to keep closed.

> > I am not disputing that __attribute__((optimize)) is highly useful in
> > some cases, I am just arguing that such cases don't exist in a Linux
> > kernel running on a production system.
>
> And I am saying that before you can remove something, you probably
> should look at why it was added in the first place, and see if you need
> a replacement for that, etc.  Just destroying stuff you do not agree
> with is iconoclasm.
>

I will point out again that this is not about whether the option
itself takes effect or not, it is about the fact that other,
completely unrelated options are affected as well, and those options
may affect code generation in a way that can cause crashes or other
catastrophic failures.

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Nick Desaulniers]

On Fri, Nov 13, 2020 at 3:49 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Fri, Nov 13, 2020 at 10:59:26AM -0800, Nick Desaulniers wrote:
> > The `optimize` attribute is both non-portable across toolchains (hence
> > this warning)
>
> Like *all* GCC extensions.
>
> > and a little quirky in GCC.
>
> How so?  Don't spread FUD please, say what *is* wrong, then people can
> decide for themselves whether they want it or not.

Spread FUD? Ard literally sent TO YOU:
https://lore.kernel.org/lkml/CAMj1kXHxX+u5-cN0v3SLdqZTSiKsWsFOvc2SC5=-ScaUZOu8Ng@mail.gmail.com/,
and it was referenced again in
https://lore.kernel.org/lkml/20201028081123.GT2628@hirez.programming.kicks-ass.net/.

Was it FUD when Ard sent it to you?  Did you even read the link you
dropped from my reply, which is what Ard quoted?
https://gcc.gnu.org/wiki/FAQ#optimize_attribute_broken

If it wasn't FUD when Ard sent it, why is it suddenly when I do?

>
> We (GCC) do document it as:
>
>      Not every optimization option that starts with the -f prefix
>      specified by the attribute necessarily has an effect on the
>      function.  The 'optimize' attribute should be used for debugging
>      purposes only.  It is not suitable in production code.
>
> The optimize attribute is for setting/resetting flags on a function
> granularity.  Not all flags can be flipped per function.  The SSP flags
> work fine though, AFAIK.  But don't use it for production, there are no
> guarantees.
>
> Cheers,
>
>
> Segher



--
Thanks,
~Nick Desaulniers

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Segher Boessenkool]

On Fri, Nov 13, 2020 at 04:11:41PM -0800, Nick Desaulniers wrote:
> On Fri, Nov 13, 2020 at 3:49 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Fri, Nov 13, 2020 at 10:59:26AM -0800, Nick Desaulniers wrote:
> > > The `optimize` attribute is both non-portable across toolchains (hence
> > > this warning)
> >
> > Like *all* GCC extensions.
> >
> > > and a little quirky in GCC.
> >
> > How so?  Don't spread FUD please, say what *is* wrong, then people can
> > decide for themselves whether they want it or not.
> 
> Spread FUD? Ard literally sent TO YOU:
> https://lore.kernel.org/lkml/CAMj1kXHxX+u5-cN0v3SLdqZTSiKsWsFOvc2SC5=-ScaUZOu8Ng@mail.gmail.com/,
> and it was referenced again in
> https://lore.kernel.org/lkml/20201028081123.GT2628@hirez.programming.kicks-ass.net/.
> 
> Was it FUD when Ard sent it to you?

He didn't say "this option is a little quirky".  He simply quoted our
wiki entry for it, which says "use this only for debugging" (just like
the user documentation btw).  The FAQ also goes on to explain the
attribute is very hard to use, it is not obvious at all what flags you
can and cannot set, it's a user interface disaster.  It explains what is
bad with it, it doesn't just say "ooh I don't understand it, do not use
it".  (It does say "no one really understands it, do not use it", there
is that ;-) )


Segher

Re: [RFC][PATCH 21/24] x86/entry: Disable stack-protector for IST entry C handlers

[Nick Desaulniers]

On Fri, Nov 13, 2020 at 4:45 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Nov 13, 2020 at 04:11:41PM -0800, Nick Desaulniers wrote:
> > On Fri, Nov 13, 2020 at 3:49 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Fri, Nov 13, 2020 at 10:59:26AM -0800, Nick Desaulniers wrote:
> > > > The `optimize` attribute is both non-portable across toolchains (hence
> > > > this warning)
> > >
> > > Like *all* GCC extensions.
> > >
> > > > and a little quirky in GCC.
> > >
> > > How so?  Don't spread FUD please, say what *is* wrong, then people can
> > > decide for themselves whether they want it or not.
> >
> > Spread FUD? Ard literally sent TO YOU:
> > https://lore.kernel.org/lkml/CAMj1kXHxX+u5-cN0v3SLdqZTSiKsWsFOvc2SC5=-ScaUZOu8Ng@mail.gmail.com/,
> > and it was referenced again in
> > https://lore.kernel.org/lkml/20201028081123.GT2628@hirez.programming.kicks-ass.net/.
> >
> > Was it FUD when Ard sent it to you?
>
> He didn't say "this option is a little quirky".  He simply quoted our
> wiki entry for it, which says "use this only for debugging" (just like
> the user documentation btw).  The FAQ also goes on to explain the
> attribute is very hard to use, it is not obvious at all what flags you
> can and cannot set, it's a user interface disaster.  It explains what is
> bad with it, it doesn't just say "ooh I don't understand it, do not use
> it".  (It does say "no one really understands it, do not use it", there
> is that ;-) )

Are we splitting hairs here?  I want both toolchains to be successful;
though maybe next time I see something like this patch/0day report go
by, I'll just keep my mouth shut and we can deal with the runtime bugs
later?
-- 
Thanks,
~Nick Desaulniers