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(-)
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
+ (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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[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)?
> +
> # 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
On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[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.
> 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
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 <[email protected]> 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
On Wed, Sep 4, 2019 at 3:28 PM Vaibhav Rustagi
<[email protected]> wrote:
>
> On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[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.
>
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
On Wed, Sep 4, 2019 at 5:19 PM 'Vaibhav Rustagi' via Clang Built Linux
<[email protected]> wrote:
>
> On Wed, Sep 4, 2019 at 3:28 PM Vaibhav Rustagi
> <[email protected]> wrote:
> >
> > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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 <[email protected]>
(see other patches in `git log`)
--
Thanks again,
~Nick Desaulniers
On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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.
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
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
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
On Wed, Sep 4, 2019 at 10:34 PM Andreas Smas <[email protected]> wrote:
>
> On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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
On Thu, Sep 5, 2019 at 11:20 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Wed, Sep 4, 2019 at 10:34 PM Andreas Smas <[email protected]> wrote:
> >
> > On Wed, Sep 4, 2019 at 3:19 PM Nick Desaulniers <[email protected]> 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 <[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.