x86/purgatory: Change compiler flags to avoid relocation errors.

PATCH Superseded Steve Wahl 6 participants 12 messages Sep 4, 2019 – Sep 5, 2019 x86

Share & tools

Steve Wahl

author

2019-09-04 21:45 UTC #0 lore raw

PATCH 1/1 x86/purgatory: Change compiler flags to avoid relocation errors.

The last change to this Makefile caused relocation errors when loading a kdump kernel. This change restores the appropriate flags, without reverting to the former practice of resetting KBUILD_CFLAGS.

Signed-off-by Steve Wahl <[email protected]>

---
 arch/x86/purgatory/Makefile | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

arch/x86/purgatory/Makefile +19 ~~-16~~

index 8901a1f89cf5..9f0bfef1f5db 100644
@@ -18,37 +18,40 @@ targets += purgatory.ro
 KASAN_SANITIZE	:= n
 KCOV_INSTRUMENT := n
 
+# These are adjustments to the compiler flags used for objects that
+# make up the standalone porgatory.ro
+
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss
+
 # 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.
 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)
+PURGATORY_CFLAGS_REMOVE		+= $(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
+PURGATORY_CFLAGS_REMOVE		+= -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
+PURGATORY_CFLAGS_REMOVE		+= -fstack-protector-strong
 endif
 
 ifdef CONFIG_RETPOLINE
-CFLAGS_REMOVE_sha256.o		+= $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_purgatory.o	+= $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_string.o		+= $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_kexec-purgatory.o	+= $(RETPOLINE_CFLAGS)
+PURGATORY_CFLAGS_REMOVE		+= $(RETPOLINE_CFLAGS)
 endif
 
+CFLAGS_REMOVE_purgatory.o	+= $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_purgatory.o		+= $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_sha256.o		+= $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_sha256.o			+= $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_string.o		+= $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_string.o			+= $(PURGATORY_CFLAGS)
+
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
 		$(call if_changed,ld)

Signature · 1 line

2.12.3

Nick Desaulniers

Reviewed-by on v1 1/1

2019-09-04 22:19 UTC #1 lore raw

+ (folks recommended by ./scripts/get_maintainer.pl <patchfile>) (See also, step 7:
https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/)

Quoted 2 lines · Steve Wahl

The last change to this Makefile caused relocation errors when loading

It's good to add a fixes tag like below when a patch fixes a regression, so that stable backports the fix as far back as the regression:

Fixes b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than

reset KBUILD_CFLAGS")

Quoted 13 lines

a kdump kernel. This change restores the appropriate flags, without reverting to the former practice of resetting KBUILD_CFLAGS.
Signed-off-by Steve Wahl <[email protected]>
---
 arch/x86/purgatory/Makefile | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)
arch/x86/purgatory/Makefile +0 -0
index 8901a1f89cf5..9f0bfef1f5db 100644

arch/x86/purgatory/Makefile targets += purgatory.ro · line 25

Quoted 8 lines

arch/x86/purgatory/Makefile +5 -0

@@ -18,37 +18,40 @@ targets += purgatory.ro
 KASAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 
+# These are adjustments to the compiler flags used for objects that
+# make up the standalone porgatory.ro
+
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss

Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.

Reviewed-by Nick Desaulniers <[email protected]>

Vaibhav, do you still have an environment setup to quickly test this again w/ Clang builds?
Tglx, we'll likely want to get this into 5.3 if it's not too late (I saw Miguel Ojeda mention there might be an -rc8)?

Quoted 51 lines

arch/x86/purgatory/Makefile +14 ~~-17~~

+
 # 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.
 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)
+PURGATORY_CFLAGS_REMOVE                += $(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
+PURGATORY_CFLAGS_REMOVE                += -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
+PURGATORY_CFLAGS_REMOVE                += -fstack-protector-strong
 endif
 
 ifdef CONFIG_RETPOLINE
-CFLAGS_REMOVE_sha256.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_purgatory.o      += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_string.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_kexec-purgatory.o        += $(RETPOLINE_CFLAGS)
+PURGATORY_CFLAGS_REMOVE                += $(RETPOLINE_CFLAGS)
 endif
 
+CFLAGS_REMOVE_purgatory.o      += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_purgatory.o             += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_sha256.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_sha256.o                        += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_string.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_string.o                        += $(PURGATORY_CFLAGS)
+
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
                $(call if_changed,ld)
 
--

2.12.3

Signature · 2 lines

Thanks,
~Nick Desaulniers

Vaibhav Rustagi

2019-09-04 22:28 UTC #2 lore raw

Quoted 41 lines · Nick Desaulniers

+ (folks recommended by ./scripts/get_maintainer.pl <patchfile>) (See also, step 7:
https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/)
Quoted 2 lines · Steve Wahl
The last change to this Makefile caused relocation errors when loading
It's good to add a fixes tag like below when a patch fixes a regression, so that stable backports the fix as far back as the regression:
Fixes b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than
reset KBUILD_CFLAGS")
Quoted 13 lines
a kdump kernel. This change restores the appropriate flags, without reverting to the former practice of resetting KBUILD_CFLAGS.
Signed-off-by Steve Wahl <[email protected]>
---
 arch/x86/purgatory/Makefile | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)
arch/x86/purgatory/Makefile +0 -0
index 8901a1f89cf5..9f0bfef1f5db 100644
arch/x86/purgatory/Makefile targets += purgatory.ro · line 25
Quoted 8 lines
arch/x86/purgatory/Makefile +5 -0
@@ -18,37 +18,40 @@ targets += purgatory.ro
 KASAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 
+# These are adjustments to the compiler flags used for objects that
+# make up the standalone porgatory.ro
+
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss
Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.

Quoted 3 lines

Reviewed-by Nick Desaulniers <[email protected]>
Vaibhav, do you still have an environment setup to quickly test this again w/ Clang builds?

I will setup the environment and will try the changes.

Quoted 59 lines

Tglx, we'll likely want to get this into 5.3 if it's not too late (I saw Miguel Ojeda mention there might be an -rc8)?

Quoted 49 lines

arch/x86/purgatory/Makefile +14 ~~-17~~

+
 # 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.
 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)
+PURGATORY_CFLAGS_REMOVE                += $(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
+PURGATORY_CFLAGS_REMOVE                += -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
+PURGATORY_CFLAGS_REMOVE                += -fstack-protector-strong
 endif
 
 ifdef CONFIG_RETPOLINE
-CFLAGS_REMOVE_sha256.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_purgatory.o      += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_string.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_kexec-purgatory.o        += $(RETPOLINE_CFLAGS)
+PURGATORY_CFLAGS_REMOVE                += $(RETPOLINE_CFLAGS)
 endif
 
+CFLAGS_REMOVE_purgatory.o      += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_purgatory.o             += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_sha256.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_sha256.o                        += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_string.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_string.o                        += $(PURGATORY_CFLAGS)
+
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
                $(call if_changed,ld)
 
--

Quoted 2 lines

2.12.3

--
Thanks,
~Nick Desaulniers

Vaibhav Rustagi

2019-09-05 00:19 UTC #3 lore raw

Quoted 47 lines · Vaibhav Rustagi

Quoted 41 lines · Nick Desaulniers
+ (folks recommended by ./scripts/get_maintainer.pl <patchfile>) (See also, step 7:
https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/)
Quoted 2 lines · Steve Wahl
The last change to this Makefile caused relocation errors when loading
It's good to add a fixes tag like below when a patch fixes a regression, so that stable backports the fix as far back as the regression:
Fixes b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than
reset KBUILD_CFLAGS")
Quoted 13 lines
a kdump kernel. This change restores the appropriate flags, without reverting to the former practice of resetting KBUILD_CFLAGS.
Signed-off-by Steve Wahl <[email protected]>
---
 arch/x86/purgatory/Makefile | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)
arch/x86/purgatory/Makefile +0 -0
index 8901a1f89cf5..9f0bfef1f5db 100644
arch/x86/purgatory/Makefile targets += purgatory.ro · line 25
Quoted 8 lines
arch/x86/purgatory/Makefile +5 -0
@@ -18,37 +18,40 @@ targets += purgatory.ro
 KASAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 
+# These are adjustments to the compiler flags used for objects that
+# make up the standalone porgatory.ro
+
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss
Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.
Quoted 3 lines
Reviewed-by Nick Desaulniers <[email protected]>
Vaibhav, do you still have an environment setup to quickly test this again w/ Clang builds?

Quoted 2 lines

I will setup the environment and will try the changes.

I tried the changes and kdump was working.

Quoted 59 lines

Tglx, we'll likely want to get this into 5.3 if it's not too late (I saw Miguel Ojeda mention there might be an -rc8)?

Quoted 49 lines

arch/x86/purgatory/Makefile +14 ~~-17~~

+
 # 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.
 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)
+PURGATORY_CFLAGS_REMOVE                += $(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
+PURGATORY_CFLAGS_REMOVE                += -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
+PURGATORY_CFLAGS_REMOVE                += -fstack-protector-strong
 endif
 
 ifdef CONFIG_RETPOLINE
-CFLAGS_REMOVE_sha256.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_purgatory.o      += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_string.o         += $(RETPOLINE_CFLAGS)
-CFLAGS_REMOVE_kexec-purgatory.o        += $(RETPOLINE_CFLAGS)
+PURGATORY_CFLAGS_REMOVE                += $(RETPOLINE_CFLAGS)
 endif
 
+CFLAGS_REMOVE_purgatory.o      += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_purgatory.o             += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_sha256.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_sha256.o                        += $(PURGATORY_CFLAGS)
+
+CFLAGS_REMOVE_string.o         += $(PURGATORY_CFLAGS_REMOVE)
+CFLAGS_string.o                        += $(PURGATORY_CFLAGS)
+
 $(obj)/purgatory.ro: $(PURGATORY_OBJS) FORCE
                $(call if_changed,ld)
 
--

Quoted 2 lines

2.12.3

--
Thanks,
~Nick Desaulniers

Nick Desaulniers

Tested-by Vaibhav Rustagi

2019-09-05 00:23 UTC #4 lore raw

Quoted 4 lines · Vaibhav Rustagi' via Clang Built Linux

Quoted 1 line · Vaibhav Rustagi

Quoted 8 lines

Quoted 6 lines · Nick Desaulniers
Quoted 2 lines
Vaibhav, do you still have an environment setup to quickly test this again w/ Clang builds?
I will setup the environment and will try the changes.
I tried the changes and kdump was working.

Great! Thanks for your help confirming the fix. If you put your "tested by tag" next time it may help some maintainers who use automation to collect patches. That way your help is immortalized in the source! Such a response would look like:

Tested-by Vaibhav Rustagi <[email protected]>

(see other patches in git log)

Signature · 2 lines

Thanks again,
~Nick Desaulniers

Randy Dunlap

2019-09-04 22:53 UTC #5 lore raw

In reply to Nick Desaulniers

Quoted 6 lines · Nick Desaulniers

+ (folks recommended by ./scripts/get_maintainer.pl <patchfile>) (See also, step 7:
https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/)
Quoted 1 line · Steve Wahl

Quoted 7 lines

Quoted 1 line
The last change to this Makefile caused relocation errors when loading
It's good to add a fixes tag like below when a patch fixes a regression, so that stable backports the fix as far back as the regression:
Fixes b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than
reset KBUILD_CFLAGS")

but don't split the Fixes: line (I did that once :).

from submitting-patches.rst:

If your patch fixes a bug in a specific commit, e.g. you found an issue using ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary. Do not split the tag across multiple lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify parsing scripts.

thnx.

Signature · 1 line

~Randy

Andreas Smas

2019-09-05 05:34 UTC #6 lore raw

In reply to Nick Desaulniers

Quoted 31 lines · Nick Desaulniers

+ (folks recommended by ./scripts/get_maintainer.pl <patchfile>) (See also, step 7:
https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patch-to-the-linux-kernel-and-responding-to-feedback/)
Quoted 2 lines · Steve Wahl
The last change to this Makefile caused relocation errors when loading
It's good to add a fixes tag like below when a patch fixes a regression, so that stable backports the fix as far back as the regression:
Fixes b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than
reset KBUILD_CFLAGS")
Quoted 16 lines
a kdump kernel. This change restores the appropriate flags, without reverting to the former practice of resetting KBUILD_CFLAGS.
Signed-off-by Steve Wahl <[email protected]>
---
 arch/x86/purgatory/Makefile | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)
arch/x86/purgatory/Makefile +0 -0
index 8901a1f89cf5..9f0bfef1f5db 100644
@@ -18,37 +18,40 @@ targets += purgatory.ro
 KASAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 

Quoted 10 lines

arch/x86/purgatory/Makefile targets += purgatory.ro · line 25
Quoted 5 lines
arch/x86/purgatory/Makefile +5 -0
+# These are adjustments to the compiler flags used for objects that
+# make up the standalone porgatory.ro
+
+PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
+PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss
Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.

Without -ffreestanding this results in undefined symbols (as before this patch)

$ readelf -a arch/x86/purgatory/purgatory.ro|grep UND
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
    51: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND __stack_chk_fail

I just bumped into this issue as I discovered that kexec() no longer works after the x86/purgatory: Use CFLAGS_REMOVE rather than reset KBUILD_CFLAGS -commit was merged.

Nick Desaulniers

2019-09-05 18:20 UTC #7 lore raw

Quoted 8 lines · Andreas Smas

Quoted 4 lines · Nick Desaulniers
Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.
Without -ffreestanding this results in undefined symbols (as before this patch)

Thanks for the report and sorry for the breakage. Can you test Steve's patch and send your tested by tag? Steve will likely respin the final patch today with Boris' feedback, so now is the time to get on the train.

Quoted 3 lines

$ readelf -a arch/x86/purgatory/purgatory.ro|grep UND
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND

^ what's that? A <strikethrough>horse</strikethrough> symbol with no name?

Quoted 1 line

    51: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND __stack_chk_fail

^ so I would have expected the stackprotector changes in my and Steve commits to prevent compiler emission of that runtime-implemented symbol. ie. that -ffreestanding affects that and not removing the stackprotector flags begs another question. Without -ffreestanding and -fstack-protector (or -fstack-protector-strong), why would the compiler emit references to __stack_chk_fail? Which .o file that composes the .ro file did we fail to remove the -fstack-protector* flag from? -ffreestanding seems to be covering that up.

Quoted 4 lines

I just bumped into this issue as I discovered that kexec() no longer works after the x86/purgatory: Use CFLAGS_REMOVE rather than reset KBUILD_CFLAGS -commit was merged.

Signature · 2 lines

Thanks,
~Nick Desaulniers

Andreas Smas

Tested-by

2019-09-05 20:00 UTC #8 lore raw

Quoted 17 lines · Nick Desaulniers

Quoted 8 lines · Andreas Smas
Quoted 4 lines · Nick Desaulniers
Thanks for confirming the fix. While it sounds like -mcmodel=large is the only necessary change, I don't object to -ffreestanding of -fno-zero-initialized-in-bss being readded, especially since I think what you've done with PURGATORY_CFLAGS_REMOVE is more concise.
Without -ffreestanding this results in undefined symbols (as before this patch)
Thanks for the report and sorry for the breakage. Can you test Steve's patch and send your tested by tag? Steve will likely respin the final patch today with Boris' feedback, so now is the time to get on the train.
Quoted 1 line

Quoted 4 lines

Quoted 2 lines
$ readelf -a arch/x86/purgatory/purgatory.ro|grep UND
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
^ what's that? A <strikethrough>horse</strikethrough> symbol with no name?

No idea TBH. Not enough of an ELF-expert to explain that. It's also there with the -ffreestanding -patch (when kexec() works for me again)
so it doesn't seem to cause any harm.

Quoted 11 lines

Quoted 2 lines
    51: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND __stack_chk_fail
^ so I would have expected the stackprotector changes in my and Steve commits to prevent compiler emission of that runtime-implemented symbol. ie. that -ffreestanding affects that and not removing the stackprotector flags begs another question. Without -ffreestanding and -fstack-protector (or -fstack-protector-strong), why would the compiler emit references to __stack_chk_fail? Which .o file that composes the .ro file did we fail to remove the -fstack-protector* flag from? -ffreestanding seems to be covering that up.

So, I'm using

$ gcc --version
gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0

I think the problem is that stock ubuntu gcc defaults to -fstack-protector. I haven't figured out where to check how/where ubuntu configures gcc except an ancient discussion here: https://wiki.ubuntu.com/GccSsp.

Both -fno-stack-protector or -ffreestanding fixes the issue. I'm not sure which would be preferred? -ffreestanding sounds a bit better to me though, as that's really what we are dealing with here.

So,

Tested-by Andreas Smas <[email protected]>

FWIW, one of the offending functions is sha256_transform() where the u32 W[64]; triggers insert of a stack guard variable. (since -fstack-protector is default on)

End of sha256_transform()

        /* clear any sensitive info... */
        a = b = c = d = e = f = g = h = t1 = t2 = 0;
        memset(W, 0, 64 * sizeof(u32));
}
    1aab:       48 8b 84 24 00 01 00    mov    0x100(%rsp),%rax
    1ab2:       00
    1ab3:       65 48 33 04 25 28 00    xor    %gs:0x28,%rax
    1aba:       00 00
        state[0] += a; state[1] += b; state[2] += c; state[3] += d;
    1abc:       44 89 37                mov    %r14d,(%rdi)
    1abf:       44 89 47 0c             mov    %r8d,0xc(%rdi)
        state[4] += e; state[5] += f; state[6] += g; state[7] += h;
    1ac3:       44 89 6f 10             mov    %r13d,0x10(%rdi)
    1ac7:       89 4f 14                mov    %ecx,0x14(%rdi)
    1aca:       89 5f 18                mov    %ebx,0x18(%rdi)
}
    1acd:       75 12                   jne    1ae1 <sha256_transform+0x1ae1>
    1acf:       48 81 c4 08 01 00 00    add    $0x108,%rsp
    1ad6:       5b                      pop    %rbx
    1ad7:       5d                      pop    %rbp
    1ad8:       41 5c                   pop    %r12
    1ada:       41 5d                   pop    %r13
    1adc:       41 5e                   pop    %r14
    1ade:       41 5f                   pop    %r15
    1ae0:       c3                      retq
    1ae1:       e8 00 00 00 00          callq  1ae6 <sha256_transform+0x1ae6>

.rela.text:

1ae2 001100000002 R_X86_64_PC32 __stack_chk_fail - 4

Same thing with this latest patch (ie, -ffreestanding)

        /* clear any sensitive info... */
        a = b = c = d = e = f = g = h = t1 = t2 = 0;
        memset(W, 0, 64 * sizeof(u32));
    1aa2:       ba 00 01 00 00          mov    $0x100,%edx
        state[4] += e; state[5] += f; state[6] += g; state[7] += h;
    1aa7:       89 47 1c                mov    %eax,0x1c(%rdi)
        state[0] += a; state[1] += b; state[2] += c; state[3] += d;
    1aaa:       44 89 47 0c             mov    %r8d,0xc(%rdi)
        memset(W, 0, 64 * sizeof(u32));
    1aae:       31 f6                   xor    %esi,%esi
        state[4] += e; state[5] += f; state[6] += g; state[7] += h;
    1ab0:       89 4f 14                mov    %ecx,0x14(%rdi)
        memset(W, 0, 64 * sizeof(u32));
    1ab3:       48 b8 00 00 00 00 00    movabs $0x0,%rax    <- &memset()
    1aba:       00 00 00
    1abd:       48 89 e7                mov    %rsp,%rdi
    1ac0:       ff d0                   callq  *%rax
}
    1ac2:       48 81 c4 00 01 00 00    add    $0x100,%rsp
    1ac9:       5b                      pop    %rbx
    1aca:       5d                      pop    %rbp
    1acb:       41 5c                   pop    %r12
    1acd:       41 5d                   pop    %r13
    1acf:       41 5e                   pop    %r14
    1ad1:       41 5f                   pop    %r15
    1ad3:       c3                      retq

1ab5 001100000001 R_X86_64_64 memset + 0

It's interesting / odd (?) that the memset() is eliminated when stack-guard is enabled.
I've no idea why this happens. But I suppose that's a separate thing.

Borislav Petkov

maintainer

2019-09-05 09:15 UTC #9 lore raw

In reply to Steve Wahl

Quoted 2 lines · Steve Wahl

The last change to this Makefile caused relocation errors when loading a kdump kernel.

How do those relocation errors look like?

What exactly caused those errors, the flags removal from kexec-purgatory.o?

Because this is the difference I can see from

b059f801a937 ("x86/purgatory: Use CFLAGS_REMOVE rather than reset KBUILD_CFLAGS")

Or is it something else?

Can we have the failure properly explained in the commit message pls?

Quoted 1 line

This change restores the appropriate flags, without

You don't have to say "This change" in the commit message - it is obvious which change you're talking about. Instead say: "Restore the appropriate... "

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Steve Wahl

author

2019-09-05 15:07 UTC #10 lore raw

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Steve Wahl
The last change to this Makefile caused relocation errors when loading a kdump kernel.
How do those relocation errors look like?

kexec: Overflow in relocation type 11 value 0x11fffd000

... when loading the crash kernel.

Quoted 2 lines

What exactly caused those errors, the flags removal from kexec-purgatory.o?

No, it's the flags for compiling the other objects (purgatory.o, sha256.o, and string.o) that cause the problem. You may have missed the added initial values for PURGATORY_CFLAGS_REMOVE and PURGATORY_CFLAGS. This changes -mcmodel=kernel back to -mcmodel=large, and adds back -ffreestanding and -fno-zero-initialized-in-bss, to match the previous flags.

-mcmodel=kernel is the major cause of the relocation errors, as the code generated contained only 32 bit references to things that can be anywhere in 64 bit address space.

The remaining flag changes are appropriate for compiling a standalone module, which applies to 3 of the objects compiled from C files in this directory -- they contribute to a standalone piece of code that is not (technically) linked with the rest of the kernel.

(Fine line here: the standalone binary does not get any symbols resolved against the rest of the kernel; which is why I say it's not *linked* with it. The binary image of this standalone binary does get put into a character array that is pulled into the kernel object code, so it does become part of the kernel, but just as an array of bytes that kexec copies somewhere and eventually jumps to as a standalone program.)

kexec-purgatory.o, on the other hand, does get linked with the rest of the kernel and should be compiled with the usual flags, not standalone flags. That is why changes for it are a bit different, which you noticed.

Quoted 1 line

Can we have the failure properly explained in the commit message pls?

Is " 'kexec: Overflow in relocation type 11 value 0x11fffd000' when loading the crash kernel" sufficient, or would you like more?

Quoted 5 lines

Quoted 1 line
This change restores the appropriate flags, without
You don't have to say "This change" in the commit message - it is obvious which change you're talking about. Instead say: "Restore the appropriate... "

OK.

--> Steve

Signature · 1 line

Steve Wahl, Hewlett Packard Enterprise

Borislav Petkov

maintainer

2019-09-05 16:46 UTC #11 lore raw

Quoted 1 line · Steve Wahl

kexec: Overflow in relocation type 11 value 0x11fffd000

That looks like R_X86_64_32S which is:

"The linker must verify that the generated value for the R_X86_64_32 (R_X86_64_32S) relocation zero-extends (sign-extends) to the original 64-bit value."

Please add that to the commit message.

Quoted 10 lines

... when loading the crash kernel.
Quoted 2 lines
What exactly caused those errors, the flags removal from kexec-purgatory.o?
No, it's the flags for compiling the other objects (purgatory.o, sha256.o, and string.o) that cause the problem. You may have missed the added initial values for PURGATORY_CFLAGS_REMOVE and PURGATORY_CFLAGS. This changes -mcmodel=kernel back to -mcmodel=large,

That I missed...

Quoted 2 lines

and adds back -ffreestanding and -fno-zero-initialized-in-bss, to match the previous flags.

... and that I saw. :)

Quoted 3 lines

-mcmodel=kernel is the major cause of the relocation errors, as the code generated contained only 32 bit references to things that can be anywhere in 64 bit address space.

Needs to go into the commit message.

Quoted 5 lines

The remaining flag changes are appropriate for compiling a standalone module, which applies to 3 of the objects compiled from C files in this directory -- they contribute to a standalone piece of code that is not (technically) linked with the rest of the kernel.

Quoted 7 lines

(Fine line here: the standalone binary does not get any symbols resolved against the rest of the kernel; which is why I say it's not *linked* with it. The binary image of this standalone binary does get put into a character array that is pulled into the kernel object code, so it does become part of the kernel, but just as an array of bytes that kexec copies somewhere and eventually jumps to as a standalone program.)

Yes, a shorter version of that should be part of the commit message too.

Quoted 4 lines

kexec-purgatory.o, on the other hand, does get linked with the rest of the kernel and should be compiled with the usual flags, not standalone flags. That is why changes for it are a bit different, which you noticed.

Ok, thanks for explaining.

Now please add that more detailed explanation to your commit message so that people doing git archeology in the future can gather from it what the problem was and what the solution became and why.

Also, add the reviewed- and tested-by flags you got from people.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette