[v4,2/2] x86/purgatory: use CFLAGS_REMOVE rather than reset KBUILD_CFLAGS

Message ID 20190725200625.174838-2-ndesaulniers@google.com
State New
Headers show
Series
  • [v4,1/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset
Related show

Commit Message

Nick Desaulniers July 25, 2019, 8:06 p.m.
KBUILD_CFLAGS is very carefully built up in the top level Makefile,
particularly when cross compiling or using different build tools.
Resetting KBUILD_CFLAGS via := assignment is an antipattern.

The comment above the reset mentions that -pg is problematic.  Other
Makefiles use `CFLAGS_REMOVE_file.o = $(CC_FLAGS_FTRACE)` when
CONFIG_FUNCTION_TRACER is set. Prefer that pattern to wiping out all of
the important KBUILD_CFLAGS then manually having to re-add them. Seems
also that __stack_chk_fail references are generated when using
CONFIG_STACKPROTECTOR or CONFIG_STACKPROTECTOR_STRONG.

Fixes: 8fc5b4d4121c ("purgatory: core purgatory functionality")
Reported-by: Vaibhav Rustagi <vaibhavrustagi@google.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Tested-by: Vaibhav Rustagi <vaibhavrustagi@google.com>

---
Changes v3 -> v4:
* Use tabs to align flags (stylistic change only).
* Drop stable tag, patch 01/02 doesn't apply earlier than 5.1.
* Add tglx's suggested by tag for the tabs.
* Carry Vaibhav's tested by tag from v3 since v4 is simply stylistic.
Changes v2 -> v3:
* Prefer $(CC_FLAGS_FTRACE) which is exported to -pg.
* Also check CONFIG_STACKPROTECTOR and CONFIG_STACKPROTECTOR_STRONG.
* Cc stable.
Changes v1 -> v2:
Rather than manually add -mno-sse, -mno-mmx, -mno-sse2, prefer to filter
-pg flags.
 arch/x86/purgatory/Makefile | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

-- 
2.22.0.709.g102302147b-goog

Comments

Thomas Gleixner July 26, 2019, 8:54 a.m. | #1
On Thu, 25 Jul 2019, Nick Desaulniers wrote:

> KBUILD_CFLAGS is very carefully built up in the top level Makefile,

> particularly when cross compiling or using different build tools.

> Resetting KBUILD_CFLAGS via := assignment is an antipattern.

> 

> The comment above the reset mentions that -pg is problematic.  Other

> Makefiles use `CFLAGS_REMOVE_file.o = $(CC_FLAGS_FTRACE)` when

> CONFIG_FUNCTION_TRACER is set. Prefer that pattern to wiping out all of

> the important KBUILD_CFLAGS then manually having to re-add them. Seems

> also that __stack_chk_fail references are generated when using

> CONFIG_STACKPROTECTOR or CONFIG_STACKPROTECTOR_STRONG.


Looking at the resulting build flags. Most stuff looks correct but there
are a few which need to be looked at twice.

removes:

 -ffreestanding
 -fno-builtin
 -fno-zero-initialized-in-bss

changes:

 -mcmodel=large to -mcmodel=kernel

adds:

  -mindirect-branch-register
  -mindirect-branch=thunk-extern

The latter makes me nervous. That probably wants to have retpoline disabled
as well. It's not having an instance right now, but ...

Thanks,

	tglx
Thomas Gleixner Aug. 7, 2019, 1:17 p.m. | #2
On Fri, 26 Jul 2019, Thomas Gleixner wrote:

Ping...

> On Thu, 25 Jul 2019, Nick Desaulniers wrote:

> 

> > KBUILD_CFLAGS is very carefully built up in the top level Makefile,

> > particularly when cross compiling or using different build tools.

> > Resetting KBUILD_CFLAGS via := assignment is an antipattern.

> > 

> > The comment above the reset mentions that -pg is problematic.  Other

> > Makefiles use `CFLAGS_REMOVE_file.o = $(CC_FLAGS_FTRACE)` when

> > CONFIG_FUNCTION_TRACER is set. Prefer that pattern to wiping out all of

> > the important KBUILD_CFLAGS then manually having to re-add them. Seems

> > also that __stack_chk_fail references are generated when using

> > CONFIG_STACKPROTECTOR or CONFIG_STACKPROTECTOR_STRONG.

> 

> Looking at the resulting build flags. Most stuff looks correct but there

> are a few which need to be looked at twice.

> 

> removes:

> 

>  -ffreestanding

>  -fno-builtin

>  -fno-zero-initialized-in-bss

> 

> changes:

> 

>  -mcmodel=large to -mcmodel=kernel

> 

> adds:

> 

>   -mindirect-branch-register

>   -mindirect-branch=thunk-extern

> 

> The latter makes me nervous. That probably wants to have retpoline disabled

> as well. It's not having an instance right now, but ...

> 

> Thanks,

> 

> 	tglx

>

Patch

diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 91ef244026d2..940f043a4d97 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -20,11 +20,27 @@  KCOV_INSTRUMENT := n
 
 # Default KBUILD_CFLAGS can have -pg option set when FTRACE is enabled. That
 # in turn leaves some undefined symbols like __fentry__ in purgatory and not
-# sure how to relocate those. Like kexec-tools, use custom flags.
-
-KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes -fno-zero-initialized-in-bss -fno-builtin -ffreestanding -c -Os -mcmodel=large
-KBUILD_CFLAGS += -m$(BITS)
-KBUILD_CFLAGS += $(call cc-option,-fno-PIE)
+# sure how to relocate those.
+ifdef CONFIG_FUNCTION_TRACER
+CFLAGS_REMOVE_sha256.o		+= $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_purgatory.o	+= $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_string.o		+= $(CC_FLAGS_FTRACE)
+CFLAGS_REMOVE_kexec-purgatory.o	+= $(CC_FLAGS_FTRACE)
+endif
+
+ifdef CONFIG_STACKPROTECTOR
+CFLAGS_REMOVE_sha256.o		+= -fstack-protector
+CFLAGS_REMOVE_purgatory.o	+= -fstack-protector
+CFLAGS_REMOVE_string.o		+= -fstack-protector
+CFLAGS_REMOVE_kexec-purgatory.o	+= -fstack-protector
+endif
+
+ifdef CONFIG_STACKPROTECTOR_STRONG
+CFLAGS_REMOVE_sha256.o		+= -fstack-protector-strong
+CFLAGS_REMOVE_purgatory.o	+= -fstack-protector-strong
+CFLAGS_REMOVE_string.o		+= -fstack-protector-strong
+CFLAGS_REMOVE_kexec-purgatory.o	+= -fstack-protector-strong
+endif
 
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)