* [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
@ 2018-09-27 20:47 ndesaulniers
2018-09-27 21:40 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: ndesaulniers @ 2018-09-27 20:47 UTC (permalink / raw)
To: bp, mingo, tglx
Cc: Nick Desaulniers, H. Peter Anvin, x86, Kirill A. Shutemov,
Masahiro Yamada, Greg Kroah-Hartman, Matthias Kaehlcke,
Kees Cook, Cao jin, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
2018-09-27 20:47 [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO ndesaulniers
@ 2018-09-27 21:40 ` Kees Cook
2018-09-27 21:51 ` Borislav Petkov
2018-10-01 17:38 ` H. Peter Anvin
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2018-09-27 21:40 UTC (permalink / raw)
To: Nick Desaulniers, Masahiro Yamada
Cc: Borislav Petkov, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
X86 ML, Kirill A. Shutemov, Greg Kroah-Hartman,
Matthias Kaehlcke, Cao jin, LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
2018-09-27 20:47 [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO ndesaulniers
2018-09-27 21:40 ` Kees Cook
@ 2018-09-27 21:51 ` Borislav Petkov
2018-09-27 22:17 ` Nick Desaulniers
2018-10-01 17:38 ` H. Peter Anvin
2 siblings, 1 reply; 6+ messages in thread
From: Borislav Petkov @ 2018-09-27 21:51 UTC (permalink / raw)
To: ndesaulniers
Cc: mingo, tglx, H. Peter Anvin, x86, Kirill A. Shutemov,
Masahiro Yamada, Greg Kroah-Hartman, Matthias Kaehlcke,
Kees Cook, Cao jin, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
2018-09-27 21:51 ` Borislav Petkov
@ 2018-09-27 22:17 ` Nick Desaulniers
2018-10-01 17:32 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2018-09-27 22:17 UTC (permalink / raw)
To: bp
Cc: mingo, Thomas Gleixner, hpa, x86, Kirill A . Shutemov,
Masahiro Yamada, Greg KH, Matthias Kaehlcke, Kees Cook, Cao jin,
LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
2018-09-27 22:17 ` Nick Desaulniers
@ 2018-10-01 17:32 ` Borislav Petkov
0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2018-10-01 17:32 UTC (permalink / raw)
To: Nick Desaulniers
Cc: mingo, Thomas Gleixner, hpa, x86, Kirill A . Shutemov,
Masahiro Yamada, Greg KH, Matthias Kaehlcke, Kees Cook, Cao jin,
LKML
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO
2018-09-27 20:47 [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO ndesaulniers
2018-09-27 21:40 ` Kees Cook
2018-09-27 21:51 ` Borislav Petkov
@ 2018-10-01 17:38 ` H. Peter Anvin
2 siblings, 0 replies; 6+ messages in thread
From: H. Peter Anvin @ 2018-10-01 17:38 UTC (permalink / raw)
To: ndesaulniers, bp, mingo, tglx
Cc: x86, Kirill A. Shutemov, Masahiro Yamada, Greg Kroah-Hartman,
Matthias Kaehlcke, Kees Cook, Cao jin, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-01 17:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-27 20:47 [PATCH v2] x86/boot: define CC_HAVE_ASM_GOTO ndesaulniers
2018-09-27 21:40 ` Kees Cook
2018-09-27 21:51 ` Borislav Petkov
2018-09-27 22:17 ` Nick Desaulniers
2018-10-01 17:32 ` Borislav Petkov
2018-10-01 17:38 ` H. Peter Anvin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.