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 <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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
On Thu, Sep 27, 2018 at 1:47 PM, <[email protected]> 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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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 <[email protected]>
-Kees
--
Kees Cook
Pixel Security
On Thu, Sep 27, 2018 at 01:47:58PM -0700, [email protected] 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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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/[email protected]
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.
On Thu, Sep 27, 2018 at 2:51 PM Borislav Petkov <[email protected]> wrote:
>
> On Thu, Sep 27, 2018 at 01:47:58PM -0700, [email protected] 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 <[email protected]>
> > Signed-off-by: Nick Desaulniers <[email protected]>
> > ---
> > 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/[email protected]
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/[email protected]/).
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
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/[email protected]
> 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/[email protected]/).
>
> 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.
On 09/27/18 13:47, [email protected] 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