[PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[ndesaulniers]

Early prototypes of Clang with asm goto support produce 6 instances of
the following warning:

In file included from arch/x86/boot/compressed/misc.h:20:
In file included from ./include/linux/elf.h:5:
In file included from ./arch/x86/include/asm/elf.h:8:
In file included from ./include/linux/thread_info.h:38:
In file included from ./arch/x86/include/asm/thread_info.h:53:
./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
[-W#warnings]
your compiler arguments"
 ^

Since 6 files under arch/x86/boot/compressed/ include
arch/x86/boot/compressed/misc.h AND
arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
which set these variables in the top level MAKEFILE.

Suggested-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
v1 -> v2:
  Updated commit message to provide more context as per Borislav.

 arch/x86/boot/compressed/Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 28764dacf018..158c0b4e178a 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
 endif
 LDFLAGS_vmlinux := -T
 
+# check for 'asm goto'
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
+  CC_HAVE_ASM_GOTO := 1
+  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
+  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
+endif
+
 hostprogs-y	:= mkpiggy
 HOST_EXTRACFLAGS += -I$(srctree)/tools/include
 
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[Kees Cook]

On Thu, Sep 27, 2018 at 1:47 PM,  <ndesaulniers@google.com> wrote:
> Early prototypes of Clang with asm goto support produce 6 instances of
> the following warning:
>
> In file included from arch/x86/boot/compressed/misc.h:20:
> In file included from ./include/linux/elf.h:5:
> In file included from ./arch/x86/include/asm/elf.h:8:
> In file included from ./include/linux/thread_info.h:38:
> In file included from ./arch/x86/include/asm/thread_info.h:53:
> ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
> ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> [-W#warnings]
> your compiler arguments"
>  ^
>
> Since 6 files under arch/x86/boot/compressed/ include
> arch/x86/boot/compressed/misc.h AND
> arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
> which set these variables in the top level MAKEFILE.
>
> Suggested-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> v1 -> v2:
>   Updated commit message to provide more context as per Borislav.
>
>  arch/x86/boot/compressed/Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 28764dacf018..158c0b4e178a 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
>  endif
>  LDFLAGS_vmlinux := -T
>
> +# check for 'asm goto'
> +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
> +  CC_HAVE_ASM_GOTO := 1
> +  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> +  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
> +endif
> +
>  hostprogs-y    := mkpiggy
>  HOST_EXTRACFLAGS += -I$(srctree)/tools/include
>
> --
> 2.19.0.605.g01d371f741-goog
>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[Borislav Petkov]

On Thu, Sep 27, 2018 at 01:47:58PM -0700, ndesaulniers@google.com wrote:
> Early prototypes of Clang with asm goto support produce 6 instances of
> the following warning:
> 
> In file included from arch/x86/boot/compressed/misc.h:20:
> In file included from ./include/linux/elf.h:5:
> In file included from ./arch/x86/include/asm/elf.h:8:
> In file included from ./include/linux/thread_info.h:38:
> In file included from ./arch/x86/include/asm/thread_info.h:53:
> ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
> ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> [-W#warnings]
> your compiler arguments"
>  ^
> 
> Since 6 files under arch/x86/boot/compressed/ include
> arch/x86/boot/compressed/misc.h AND
> arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
> which set these variables in the top level MAKEFILE.
> 
> Suggested-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> v1 -> v2:
>   Updated commit message to provide more context as per Borislav.
> 
>  arch/x86/boot/compressed/Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 28764dacf018..158c0b4e178a 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
>  endif
>  LDFLAGS_vmlinux := -T
>  
> +# check for 'asm goto'
> +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
> +  CC_HAVE_ASM_GOTO := 1
> +  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> +  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
> +endif

I would still like to know why can't we do the -D_SETUP thing here:
https://lkml.kernel.org/r/20180926090841.GC5745@zn.tnic

instead of polluting this Makefile with defines which are not really
needed in the compressed kernel build, except to silence build warnings.

I mean, we can perpetuate that ugly hack and do:

#define __BPF_TRACING__

here in arch/x86/boot/compressed/misc.h which we could kill once clang
can do asm goto...

Hmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[Nick Desaulniers]

On Thu, Sep 27, 2018 at 2:51 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Sep 27, 2018 at 01:47:58PM -0700, ndesaulniers@google.com wrote:
> > Early prototypes of Clang with asm goto support produce 6 instances of
> > the following warning:
> >
> > In file included from arch/x86/boot/compressed/misc.h:20:
> > In file included from ./include/linux/elf.h:5:
> > In file included from ./arch/x86/include/asm/elf.h:8:
> > In file included from ./include/linux/thread_info.h:38:
> > In file included from ./arch/x86/include/asm/thread_info.h:53:
> > ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
> > ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> > [-W#warnings]
> > your compiler arguments"
> >  ^
> >
> > Since 6 files under arch/x86/boot/compressed/ include
> > arch/x86/boot/compressed/misc.h AND
> > arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
> > which set these variables in the top level MAKEFILE.
> >
> > Suggested-by: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > v1 -> v2:
> >   Updated commit message to provide more context as per Borislav.
> >
> >  arch/x86/boot/compressed/Makefile | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> > index 28764dacf018..158c0b4e178a 100644
> > --- a/arch/x86/boot/compressed/Makefile
> > +++ b/arch/x86/boot/compressed/Makefile
> > @@ -56,6 +56,13 @@ KBUILD_LDFLAGS += $(shell $(LD) --help 2>&1 | grep -q "\-z noreloc-overflow" \
> >  endif
> >  LDFLAGS_vmlinux := -T
> >
> > +# check for 'asm goto'
> > +ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
> > +  CC_HAVE_ASM_GOTO := 1
> > +  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> > +  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
> > +endif
>
> I would still like to know why can't we do the -D_SETUP thing here:
> https://lkml.kernel.org/r/20180926090841.GC5745@zn.tnic

That's another case that I look at and wonder "why does this exist?"
The _SETUP guard exists in only one place:
$ grep -rP 'ifdef\s+_SETUP'
arch/x86/boot/cpucheck.c:#ifdef _SETUP

which is already under arch/x86/boot/. arch/x86/boot/Makefile
unconditionally sets -D_SETUP, so what/who are we guarding against?
Looks like a guard that's ALWAYS true (and thus could be removed).

>
> instead of polluting this Makefile with defines which are not really
> needed in the compressed kernel build, except to silence build warnings.
>
> I mean, we can perpetuate that ugly hack and do:
>
> #define __BPF_TRACING__
>
> here in arch/x86/boot/compressed/misc.h which we could kill once clang
> can do asm goto...

Or, or... we don't redefine KBUILD_CFLAGS in arch/x86/boot/Makefile
(or any Makefile other than the top level one), and simply filter out
the flags we DONT want, a la:

drivers/firmware/efi/libstub/Makefile:
 16 cflags-$(CONFIG_ARM64)    := $(subst -pg,,$(KBUILD_CFLAGS)) ...

ie, using Make's subst function to copy KBUILD_CFLAGS, filter out
results, then use that for cflags-y.
https://www.gnu.org/software/make/manual/html_node/Text-Functions.html

I'm curious to know Masahiro's thoughts on this?  I can't help but
shake the feeling that reassigning KBUILD_CFLAGS should be considered
an anti-pattern and warned from checkpatch.pl.  For the reasons
enumerated above AND in v1:
https://lore.kernel.org/lkml/CAKwvOdmLSVH7EVGY1ExU1Fh_hvL=FUzhq-80snDfZ+QhCT2FOA@mail.gmail.com/
(though there may be additional context from hpa answering
https://lore.kernel.org/lkml/20180926090841.GC5745@zn.tnic/).

Relying on the compiler's default/implicit C standard (which changed
in gcc 5) for parts of the kernel but not others I feel like should be
a big red flag.

Shall I prototype up what such a change might look like (not
reassigning KBUILD_CFLAGS in arch/x86/boot/Makefile)?  Maybe it's
harder/uglier than I imagine?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[Borislav Petkov]

On Thu, Sep 27, 2018 at 03:17:41PM -0700, Nick Desaulniers wrote:
> That's another case that I look at and wonder "why does this exist?"
> The _SETUP guard exists in only one place:
> $ grep -rP 'ifdef\s+_SETUP'
> arch/x86/boot/cpucheck.c:#ifdef _SETUP
> 
> which is already under arch/x86/boot/. arch/x86/boot/Makefile
> unconditionally sets -D_SETUP, so what/who are we guarding against?
> Looks like a guard that's ALWAYS true (and thus could be removed).

Looks like cpucheck.c was used somewhere else before and that guard was
for when it is being built in arch/x86/boot/...

Also, hpa says the override is because some 64-bit flags fail the 32-bit
compile:

https://lkml.kernel.org/r/56442061-7f55-878d-5b26-7cdd14e901d2@zytor.com

> Or, or... we don't redefine KBUILD_CFLAGS in arch/x86/boot/Makefile
> (or any Makefile other than the top level one), and simply filter out
> the flags we DONT want, a la:
> 
> drivers/firmware/efi/libstub/Makefile:
>  16 cflags-$(CONFIG_ARM64)    := $(subst -pg,,$(KBUILD_CFLAGS)) ...
> 
> ie, using Make's subst function to copy KBUILD_CFLAGS, filter out
> results, then use that for cflags-y.
> https://www.gnu.org/software/make/manual/html_node/Text-Functions.html

Hmm, definitely sounds like an interesting idea to try...

> I'm curious to know Masahiro's thoughts on this?  I can't help but
> shake the feeling that reassigning KBUILD_CFLAGS should be considered
> an anti-pattern and warned from checkpatch.pl.  For the reasons
> enumerated above AND in v1:
> https://lore.kernel.org/lkml/CAKwvOdmLSVH7EVGY1ExU1Fh_hvL=FUzhq-80snDfZ+QhCT2FOA@mail.gmail.com/
> (though there may be additional context from hpa answering
> https://lore.kernel.org/lkml/20180926090841.GC5745@zn.tnic/).
> 
> Relying on the compiler's default/implicit C standard (which changed
> in gcc 5) for parts of the kernel but not others I feel like should be
> a big red flag.

I sure see your point. But then there's also the opposing argument where
having stuff leak from kernel proper into .../boot/ is simply breaking
the build.

But then we have headers including stuff from kernel proper so I guess
*that* last fact kinda wants us to not redefine KBUILD_CFLAGS ...

Oh boy.

> Shall I prototype up what such a change might look like (not
> reassigning KBUILD_CFLAGS in arch/x86/boot/Makefile)?  Maybe it's
> harder/uglier than I imagine?

Sounds to me like a good thing to try. If anything, we'll know more
whether it makes sense at all.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO

[H. Peter Anvin]

On 09/27/18 13:47, ndesaulniers@google.com wrote:
> Early prototypes of Clang with asm goto support produce 6 instances of
> the following warning:
> 
> In file included from arch/x86/boot/compressed/misc.h:20:
> In file included from ./include/linux/elf.h:5:
> In file included from ./arch/x86/include/asm/elf.h:8:
> In file included from ./include/linux/thread_info.h:38:
> In file included from ./arch/x86/include/asm/thread_info.h:53:
> ./arch/x86/include/asm/cpufeature.h:150:2: warning: "Compiler lacks
> ASM_GOTO support. Add -D __BPF_TRACING__ to your compiler arguments"
> [-W#warnings]
> your compiler arguments"
>  ^
> 
> Since 6 files under arch/x86/boot/compressed/ include
> arch/x86/boot/compressed/misc.h AND
> arch/x86/boot/compressed/Makefile happens to redefine KBUILD_CFLAGS,
> which set these variables in the top level MAKEFILE.
> 

The Right Thing[TM] would probably be to split the CFLAGS into those
that are independent of x86_64 vs i386. After all, there is no
fundamental reason we couldn't want to use asm goto in the setup or
real-mode code in the future.  In addition to BIOS entry code there is
the EFI32 code.

	-hpa