Message-ID: <CAFkk2KQ+ybTQF4U9eHqhi-Hg8ML1Z72DsXij2sM58S5HHg6C-Q@mail.gmail.com>
Date:   Mon, 12 Feb 2018 12:44:59 +0100
From:   Ulf Magnusson <ulfalizer@...il.com>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nicolas Pitre <nicolas.pitre@...aro.org>,
        "Luis R . Rodriguez" <mcgrof@...e.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Pavel Machek <pavel@....cz>,
        linux-s390 <linux-s390@...r.kernel.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Tejun Heo <tj@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        "Van De Ven, Arjan" <arjan.van.de.ven@...el.com>
Subject: Re: [RFC PATCH 4/7] kconfig: support new special property shell=

On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada
<yamada.masahiro@...ionext.com> wrote:
> 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <ulfalizer@...il.com>:
>> Looks to me like there's a few unrelated issues here:
>>
>>
>> 1. The stack protector support test scripts
>>
>> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
>> subtly broken kernel from being built.
>>
>> A few questions:
>>
>>     - How do things fail with a broken stack protector implementation?
>>
>>     - How common are those broken compilers?
>>
>>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>>       or would a simpler static test work in practice?
>>
>>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>>       Kconfig, but should make sure it's actually needed in any case.
>>
>>       The scripts are already split up as
>>
>>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>>
>>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>>       gcc-x86_64-has-stack-protector.sh exist.
>>
>>     - How old do you need to go with GCC for -fno-stack-protector to give an
>>       error (i.e., for not even the option to be recognized)? Is it still
>>       warranted to test for it?
>>
>> Adding some CCs who worked on the stack protector test scripts.
>>
>> And yeah, I was assuming that needing support scripts would be rare, and that
>> you'd usually just check whether gcc accepts the flag.
>>
>> When you Google "gcc broken stack protector", the top hits about are about the
>> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
>> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
>> -fno-PIE")).
>>
>>
>> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>>
>> Or equivalently, whether to automatically fall back on other stack protector
>> alternatives (including no stack protector) if the one specified in the .config
>> isn't available.
>>
>> I'll let you battle this one out. In any case, as a user, I'd want a
>> super-clear message telling me what to change if the build breaks because of
>> missing stack protector support.
>>
>>
>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>
>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>> tell how it looks to other people.
>>
>> I'd add some comments to explain the idea in the final version.
>>
>> @Kees:
>> I was assuming that the Makefiles would error out with a message if none of the
>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>>
>> You could offload part of that check to Kconfig with something like
>>
>>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>                 def_bool CC_STACKPROTECTOR_STRONG || \
>>                          CC_STACKPROTECTOR_REGULAR || \
>>                          CC_STACKPROTECTOR_NONE
>>
>> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
>> It has the advantage of making the constraint clear in the Kconfig file
>> at least.
>>
>> You could add some kind of assert feature to Kconfig too, but IMO it's not
>> warranted purely for one-offs like this at least.
>>
>> That's details though. I'd want to explain it with a comment in any case if we
>> go with something like this, since it's slightly kludgy and subtle
>> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
>> express it like that directly, since it's derived from other symbols).
>>
>>
>> Here's an overview of the current Kconfig layout by the way, assuming
>> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
>> implemented in Kconfig:
>>
>>         # Feature tests
>>         CC_HAS_STACKPROTECTOR_STRONG
>>         CC_HAS_STACKPROTECTOR_REGULAR
>>         CC_HAS_STACKPROTECTOR_NONE
>>
>>         # User request
>>         WANT_CC_STACKPROTECTOR_AUTO
>>         WANT_CC_STACKPROTECTOR_STRONG
>>         WANT_CC_STACKPROTECTOR_REGULAR
>>         WANT_CC_STACKPROTECTOR_NONE
>>
>>         # The actual "output" to the Makefiles
>>         CC_STACKPROTECTOR_STRONG
>>         CC_STACKPROTECTOR_REGULAR
>>         CC_STACKPROTECTOR_NONE
>>
>>         # Some possible output "nicities"
>>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>         CC_OPT_STACKPROTECTOR
>>
>> Does anyone have objections to the naming or other things? I saw some
>> references to "Santa's wish list" in messages of commits that dealt with other
>> variables named WANT_*, though I didn't look into those cases. ;)
>>
>
>
>
> I think Linus's comment was dismissed here.
>
>
> Linus said:
>
>> But yes, I also reacted to your earlier " It can't silently rewrite it
>> to _REGULAR because the compiler support for _STRONG regressed."
>> Because it damn well can. If the compiler doesn't support
>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>> Silently. No extra crazy complex logic for that either.
>
>
> If I understood his comment correctly,
> we do not need either WANT_* or _AUTO.
>
>
>
>
> Kees' comment:
>
>> In the stack-protector case, this becomes quite important, since the
>> goal is to record the user's selection regardless of compiler
>> capability. For example, if someone selects _REGULAR, it shouldn't
>> "upgrade" to _STRONG. (Similarly for _NONE.)
>
> No.  Kconfig will not do this silently.

(Playing devil's advocate...)

If the user selected _STRONG and it becomes unavailable later, it
seems to silently fall back on other options, even for oldnoconfig
(which just checks if there are any new symbols in the choice).

It would be possible to also check if the old user selection still
applies btw. I do that in Kconfiglib. It's arguable whether that
matches the intent of oldnoconfig.

>
>
> "make oldconfig" (or "make silentoldconfig" as well)
> always ask users about new symbols.
>
>
> For example, let's say your compiler supports -fstack-protector
> but not -fstack-protector-strong.
> CC_STACKPROTECTOR_REGULAR is the best you can choose.
>
> Then, let's say you upgrade your compiler and the new compiler supports
> -fstack-protector-strong as well.
>
> In this case, CC_STACKPROTECTOR_STRONG is a newly visible symbol,
> so Kconfig will ask you
> "Hey, you were previously using _REGULAR, but your new compiler
> also supports _STRONG.  Do you want to use it?"
>
>
> The "make oldconfig" will look like follows:
>
>
> masahiro@...ver:~/workspace/linux-kbuild$ make oldconfig
>   HOSTCC  scripts/kconfig/conf.o
>   HOSTLD  scripts/kconfig/conf
> scripts/kconfig/conf  --oldconfig Kconfig
> *
> * Restart config...
> *
> *
> * Linux Kernel Configuration
> *
> Stack Protector buffer overflow detection
>   1. Strong (CC_STACKPROTECTOR_STRONG) (NEW)
>> 2. Regular (CC_STACKPROTECTOR_REGULAR)
>   3. None (CC_STACKPROTECTOR_NONE)
> choice[1-3?]:
>
>
>
> Please notice the Strong is marked as "(NEW)".
>
> Kconfig handles this really nicely with Linus' suggestion.
>
> [1] Users can select only features supported by your compiler
>     - this makes sense.
>
> [2] If you upgrade your compiler and it provides more capability,
>     "make (silent)oldconfig" will ask you about new choices.
>      - this also makes sense.

If you switch to a compiler that provides less capability, it'll
silently forget the old user preference though.

>
>
>
> So, the following simple implementation works well enough,
> doesn't it?
>
> ---------------->8---------------
> choice
>         prompt "Stack Protector buffer overflow detection"
>
> config CC_STACKPROTECTOR_STRONG
>         bool "Strong"
>         depends on CC_HAS_STACKPROTECTOR_STRONG
>         select CC_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_REGULAR
>         bool "Regular"
>         depends on CC_HAS_STACKPROTECTOR
>         select CC_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_NONE
>         bool "None"
>
> endchoice
> ---------------->8-------------------------

Obviously much neater at least. :)

>
>
>
> BTW, do we need to use 'choice' ?
>
>
> The problem of using 'choice' is,
> it does not work well with allnoconfig.
>
>
> For all{yes,mod}config, we want to enable as many features as possible.
> For allnoconfig, we want to disable as many as possible.

Oh... right. For allnoconfig, it would always pick the default, so if
the default is "on", it's bad there.

>
> However, the default of 'choice' is always the first visible value.
>
>
> So, I can suggest to remove _REGULAR and _NONE.
>
> We have just two bool options, like follows.
>
> ------------------->8-----------------
> config CC_STACKPROTECTOR
>         bool "Use stack protector"
>         depends on CC_HAS_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_STRONG
>         bool "Use strong strong stack protector"
>         depends on CC_STACKPROTECTOR
>         depends on CC_HAS_STACKPROTECTOR_STRONG
> -------------------->8------------------

Even neater. Much easier to understand.

>
> This will work well for all{yes,mod,no}config.
>
> We will not have a case where
> -fstack-protector-strong is supported, but -fstack-protector is not.
>
>
> --
> Best Regards
> Masahiro Yamada

So there's just the caveat about possibly "forgetting" the user
preferences and automatically falling back on
CC_STACKPROTECTOR_REGULAR or CC_STACKPROTECTOR_NONE when the
environment changes, instead of erroring out.

When you have to think hard about cases where something might break,
it might be a sign that it's not a huge problem in practice, but I
can't really speak for the priorities here.

Cheers,
Ulf

Powered by blists - more mailing lists