* [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. @ 2019-09-04 21:45 Steve Wahl 2019-09-04 22:18 ` Nick Desaulniers 2019-09-05 9:15 ` Borislav Petkov 0 siblings, 2 replies; 12+ messages in thread From: Steve Wahl @ 2019-09-04 21:45 UTC (permalink / raw) To: LKML Cc: Nick Desaulniers, clang-built-linux, vaibhavrustagi, russ.anderson, dimitri.sivanich, mike.travis 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 <steve.wahl@hpe.com> --- arch/x86/purgatory/Makefile | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 8901a1f89cf5..9f0bfef1f5db 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -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) -- 2.12.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 21:45 [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors Steve Wahl @ 2019-09-04 22:18 ` Nick Desaulniers 2019-09-04 22:28 ` Vaibhav Rustagi ` (2 more replies) 2019-09-05 9:15 ` Borislav Petkov 1 sibling, 3 replies; 12+ messages in thread From: Nick Desaulniers @ 2019-09-04 22:18 UTC (permalink / raw) To: Steve Wahl, Thomas Gleixner Cc: LKML, clang-built-linux, Vaibhav Rustagi, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) + (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/) On Wed, Sep 4, 2019 at 2:45 PM Steve Wahl <steve.wahl@hpe.com> wrote: > > 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") > a kdump kernel. This change restores the appropriate flags, without > reverting to the former practice of resetting KBUILD_CFLAGS. > > Signed-off-by: Steve Wahl <steve.wahl@hpe.com> > --- > arch/x86/purgatory/Makefile | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > index 8901a1f89cf5..9f0bfef1f5db 100644 > --- a/arch/x86/purgatory/Makefile > +++ b/arch/x86/purgatory/Makefile > @@ -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 <ndesaulniers@google.com> 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)? > + > # 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 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 22:18 ` Nick Desaulniers @ 2019-09-04 22:28 ` Vaibhav Rustagi 2019-09-05 0:19 ` Vaibhav Rustagi 2019-09-04 22:53 ` Randy Dunlap 2019-09-05 5:34 ` Andreas Smas 2 siblings, 1 reply; 12+ messages in thread From: Vaibhav Rustagi @ 2019-09-04 22:28 UTC (permalink / raw) To: Nick Desaulniers Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > + (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/) > > On Wed, Sep 4, 2019 at 2:45 PM Steve Wahl <steve.wahl@hpe.com> wrote: > > > > 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") > > > a kdump kernel. This change restores the appropriate flags, without > > reverting to the former practice of resetting KBUILD_CFLAGS. > > > > Signed-off-by: Steve Wahl <steve.wahl@hpe.com> > > --- > > arch/x86/purgatory/Makefile | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > > index 8901a1f89cf5..9f0bfef1f5db 100644 > > --- a/arch/x86/purgatory/Makefile > > +++ b/arch/x86/purgatory/Makefile > > @@ -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 <ndesaulniers@google.com> > 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. > 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)? > > > + > > # 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 > > > > > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 22:28 ` Vaibhav Rustagi @ 2019-09-05 0:19 ` Vaibhav Rustagi 2019-09-05 0:23 ` Nick Desaulniers 0 siblings, 1 reply; 12+ messages in thread From: Vaibhav Rustagi @ 2019-09-05 0:19 UTC (permalink / raw) To: Nick Desaulniers Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Wed, Sep 4, 2019 at 3:28 PM Vaibhav Rustagi <vaibhavrustagi@google.com> wrote: > > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > + (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/) > > > > On Wed, Sep 4, 2019 at 2:45 PM Steve Wahl <steve.wahl@hpe.com> wrote: > > > > > > 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") > > > > > a kdump kernel. This change restores the appropriate flags, without > > > reverting to the former practice of resetting KBUILD_CFLAGS. > > > > > > Signed-off-by: Steve Wahl <steve.wahl@hpe.com> > > > --- > > > arch/x86/purgatory/Makefile | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > > > index 8901a1f89cf5..9f0bfef1f5db 100644 > > > --- a/arch/x86/purgatory/Makefile > > > +++ b/arch/x86/purgatory/Makefile > > > @@ -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 <ndesaulniers@google.com> > > 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. > > 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)? > > > > > + > > > # 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 > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-05 0:19 ` Vaibhav Rustagi @ 2019-09-05 0:23 ` Nick Desaulniers 0 siblings, 0 replies; 12+ messages in thread From: Nick Desaulniers @ 2019-09-05 0:23 UTC (permalink / raw) To: Vaibhav Rustagi Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Wed, Sep 4, 2019 at 5:19 PM 'Vaibhav Rustagi' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > On Wed, Sep 4, 2019 at 3:28 PM Vaibhav Rustagi > <vaibhavrustagi@google.com> wrote: > > > > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > 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 <vaibhavrustagi@google.com> (see other patches in `git log`) -- Thanks again, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 22:18 ` Nick Desaulniers 2019-09-04 22:28 ` Vaibhav Rustagi @ 2019-09-04 22:53 ` Randy Dunlap 2019-09-05 5:34 ` Andreas Smas 2 siblings, 0 replies; 12+ messages in thread From: Randy Dunlap @ 2019-09-04 22:53 UTC (permalink / raw) To: Nick Desaulniers, Steve Wahl, Thomas Gleixner Cc: LKML, clang-built-linux, Vaibhav Rustagi, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On 9/4/19 3:18 PM, Nick Desaulniers wrote: > + (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/) > > On Wed, Sep 4, 2019 at 2:45 PM Steve Wahl <steve.wahl@hpe.com> wrote: >> >> 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. -- ~Randy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 22:18 ` Nick Desaulniers 2019-09-04 22:28 ` Vaibhav Rustagi 2019-09-04 22:53 ` Randy Dunlap @ 2019-09-05 5:34 ` Andreas Smas 2019-09-05 18:19 ` Nick Desaulniers 2 siblings, 1 reply; 12+ messages in thread From: Andreas Smas @ 2019-09-05 5:34 UTC (permalink / raw) To: Nick Desaulniers Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, Vaibhav Rustagi, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > + (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/) > > On Wed, Sep 4, 2019 at 2:45 PM Steve Wahl <steve.wahl@hpe.com> wrote: > > > > 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") > > > a kdump kernel. This change restores the appropriate flags, without > > reverting to the former practice of resetting KBUILD_CFLAGS. > > > > Signed-off-by: Steve Wahl <steve.wahl@hpe.com> > > --- > > arch/x86/purgatory/Makefile | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > > index 8901a1f89cf5..9f0bfef1f5db 100644 > > --- a/arch/x86/purgatory/Makefile > > +++ b/arch/x86/purgatory/Makefile > > @@ -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. 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-05 5:34 ` Andreas Smas @ 2019-09-05 18:19 ` Nick Desaulniers 2019-09-05 20:00 ` Andreas Smas 0 siblings, 1 reply; 12+ messages in thread From: Nick Desaulniers @ 2019-09-05 18:19 UTC (permalink / raw) To: Andreas Smas Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, Vaibhav Rustagi, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Wed, Sep 4, 2019 at 10:34 PM Andreas Smas <andreas@lonelycoder.com> wrote: > > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > 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. > > $ 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? > 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. > > 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. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-05 18:19 ` Nick Desaulniers @ 2019-09-05 20:00 ` Andreas Smas 0 siblings, 0 replies; 12+ messages in thread From: Andreas Smas @ 2019-09-05 20:00 UTC (permalink / raw) To: Nick Desaulniers Cc: Steve Wahl, Thomas Gleixner, LKML, clang-built-linux, Vaibhav Rustagi, russ.anderson, dimitri.sivanich, mike.travis, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) On Thu, Sep 5, 2019 at 11:20 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Sep 4, 2019 at 10:34 PM Andreas Smas <andreas@lonelycoder.com> wrote: > > > > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > 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. > > > > > $ 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. > > > 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 <andreas@lonelycoder.com> 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-04 21:45 [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors Steve Wahl 2019-09-04 22:18 ` Nick Desaulniers @ 2019-09-05 9:15 ` Borislav Petkov 2019-09-05 15:07 ` Steve Wahl 1 sibling, 1 reply; 12+ messages in thread From: Borislav Petkov @ 2019-09-05 9:15 UTC (permalink / raw) To: Steve Wahl Cc: LKML, Nick Desaulniers, clang-built-linux, vaibhavrustagi, russ.anderson, dimitri.sivanich, mike.travis On Wed, Sep 04, 2019 at 04:45:05PM -0500, Steve Wahl wrote: > 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? > 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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-05 9:15 ` Borislav Petkov @ 2019-09-05 15:07 ` Steve Wahl 2019-09-05 16:46 ` Borislav Petkov 0 siblings, 1 reply; 12+ messages in thread From: Steve Wahl @ 2019-09-05 15:07 UTC (permalink / raw) To: Borislav Petkov Cc: Steve Wahl, LKML, Nick Desaulniers, clang-built-linux, vaibhavrustagi, russ.anderson, dimitri.sivanich, mike.travis On Thu, Sep 05, 2019 at 11:15:14AM +0200, Borislav Petkov wrote: > On Wed, Sep 04, 2019 at 04:45:05PM -0500, Steve Wahl wrote: > > 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. > 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. > 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? > > 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 -- Steve Wahl, Hewlett Packard Enterprise ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors. 2019-09-05 15:07 ` Steve Wahl @ 2019-09-05 16:46 ` Borislav Petkov 0 siblings, 0 replies; 12+ messages in thread From: Borislav Petkov @ 2019-09-05 16:46 UTC (permalink / raw) To: Steve Wahl Cc: LKML, Nick Desaulniers, clang-built-linux, vaibhavrustagi, russ.anderson, dimitri.sivanich, mike.travis On Thu, Sep 05, 2019 at 10:07:38AM -0500, Steve Wahl wrote: > 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. > ... when loading the crash kernel. > > > 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... > and adds back -ffreestanding and > -fno-zero-initialized-in-bss, to match the previous flags. ... and that I saw. :) > -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. > 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.) Yes, a shorter version of that should be part of the commit message too. > 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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-05 20:00 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-04 21:45 [PATCH 1/1] x86/purgatory: Change compiler flags to avoid relocation errors Steve Wahl 2019-09-04 22:18 ` Nick Desaulniers 2019-09-04 22:28 ` Vaibhav Rustagi 2019-09-05 0:19 ` Vaibhav Rustagi 2019-09-05 0:23 ` Nick Desaulniers 2019-09-04 22:53 ` Randy Dunlap 2019-09-05 5:34 ` Andreas Smas 2019-09-05 18:19 ` Nick Desaulniers 2019-09-05 20:00 ` Andreas Smas 2019-09-05 9:15 ` Borislav Petkov 2019-09-05 15:07 ` Steve Wahl 2019-09-05 16:46 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).