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

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

[Steve Wahl]

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

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

[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/)

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

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

[Vaibhav Rustagi]

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

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

[Vaibhav Rustagi]

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

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

[Nick Desaulniers]

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

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

[Randy Dunlap]

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

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

[Andreas Smas]

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.

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

[Nick Desaulniers]

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

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

[Andreas Smas]

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.

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

[Borislav Petkov]

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

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

[Steve Wahl]

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

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

[Borislav Petkov]

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