[PATCH] kbuild: Strip runtime const RELA sections correctly

[PATCH] kbuild: Strip runtime const RELA sections correctly

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Due to the fact that runtime const ELF sections are named without a
leading period or double underscore, the RSTRIP logic that removes the
static RELA sections from vmlinux fails to identify them. This results
in a situation like below, where some sections that were supposed to get
removed are left behind.

  [Nr] Name                              Type            Address          Off     Size   ES Flg Lk Inf Al

  [58] runtime_shift_d_hash_shift        PROGBITS        ffffffff83500f50 2900f50 000014 00   A  0   0  1
  [59] .relaruntime_shift_d_hash_shift   RELA            0000000000000000 55b6f00 000078 18   I 70  58  8
  [60] runtime_ptr_dentry_hashtable      PROGBITS        ffffffff83500f68 2900f68 000014 00   A  0   0  1
  [61] .relaruntime_ptr_dentry_hashtable RELA            0000000000000000 55b6f78 000078 18   I 70  60  8
  [62] runtime_ptr_USER_PTR_MAX          PROGBITS        ffffffff83500f80 2900f80 000238 00   A  0   0  1
  [63] .relaruntime_ptr_USER_PTR_MAX     RELA            0000000000000000 55b6ff0 000d50 18   I 70  62  8

So tweak the match expression to strip all sections starting with .rel.
While at it, consolidate the logic used by RISC-V, s390 and x86 into a
single shared Makefile library command.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-riscv@lists.infradead.org
Cc: linux-s390@vger.kernel.org
Link: https://lore.kernel.org/all/CAHk-=wjk3ynjomNvFN8jf9A1k=qSc=JFF591W00uXj-qqNUxPQ@mail.gmail.com/
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/riscv/Makefile.postlink | 9 +--------
 arch/s390/Makefile.postlink  | 5 -----
 arch/x86/Makefile.postlink   | 5 -----
 scripts/Makefile.lib         | 3 +++
 4 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/Makefile.postlink b/arch/riscv/Makefile.postlink
index 829b9abc91f6..65652fd6a252 100644
--- a/arch/riscv/Makefile.postlink
+++ b/arch/riscv/Makefile.postlink
@@ -19,13 +19,6 @@ ifdef CONFIG_RELOCATABLE
 quiet_cmd_cp_vmlinux_relocs = CPREL   vmlinux.relocs
 cmd_cp_vmlinux_relocs = cp vmlinux vmlinux.relocs
 
-quiet_cmd_relocs_strip = STRIPREL $@
-cmd_relocs_strip = $(OBJCOPY)   --remove-section='.rel.*'       \
-                                --remove-section='.rel__*'      \
-                                --remove-section='.rela.*'      \
-                                --remove-section='.rela__*' $@
-endif
-
 # `@true` prevents complaint when there is nothing to be done
 
 vmlinux: FORCE
@@ -33,7 +26,7 @@ vmlinux: FORCE
 ifdef CONFIG_RELOCATABLE
 	$(call if_changed,relocs_check)
 	$(call if_changed,cp_vmlinux_relocs)
-	$(call if_changed,relocs_strip)
+	$(call if_changed,strip_relocs)
 endif
 
 clean:
diff --git a/arch/s390/Makefile.postlink b/arch/s390/Makefile.postlink
index df82f5410769..008acf35c37e 100644
--- a/arch/s390/Makefile.postlink
+++ b/arch/s390/Makefile.postlink
@@ -19,11 +19,6 @@ quiet_cmd_relocs = RELOCS  $(OUT_RELOCS)/relocs.S
 	mkdir -p $(OUT_RELOCS); \
 	$(CMD_RELOCS) $@ > $(OUT_RELOCS)/relocs.S
 
-quiet_cmd_strip_relocs = RSTRIP  $@
-      cmd_strip_relocs = \
-	$(OBJCOPY) --remove-section='.rel.*' --remove-section='.rel__*' \
-		   --remove-section='.rela.*' --remove-section='.rela__*' $@
-
 vmlinux: FORCE
 	$(call cmd,relocs)
 	$(call cmd,strip_relocs)
diff --git a/arch/x86/Makefile.postlink b/arch/x86/Makefile.postlink
index fef2e977cc7d..8db8b7d338ad 100644
--- a/arch/x86/Makefile.postlink
+++ b/arch/x86/Makefile.postlink
@@ -20,11 +20,6 @@ quiet_cmd_relocs = RELOCS  $(OUT_RELOCS)/$@.relocs
 	$(CMD_RELOCS) $@ > $(OUT_RELOCS)/$@.relocs; \
 	$(CMD_RELOCS) --abs-relocs $@
 
-quiet_cmd_strip_relocs = RSTRIP  $@
-      cmd_strip_relocs = \
-	$(OBJCOPY) --remove-section='.rel.*' --remove-section='.rel__*' \
-		   --remove-section='.rela.*' --remove-section='.rela__*' $@
-
 # `@true` prevents complaint when there is nothing to be done
 
 vmlinux: FORCE
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 7395200538da..f604f51d23ca 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -374,6 +374,9 @@ quiet_cmd_ar = AR      $@
 quiet_cmd_objcopy = OBJCOPY $@
 cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@
 
+quiet_cmd_strip_relocs = RSTRIP  $@
+cmd_strip_relocs = $(OBJCOPY) --remove-section='.rel*' $@
+
 # Gzip
 # ---------------------------------------------------------------------------
 
-- 
2.47.1.688.g23fc6f90ad-goog

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Masahiro Yamada]

On Fri, Jan 10, 2025 at 5:28 PM Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> Due to the fact that runtime const ELF sections are named without a
> leading period or double underscore, the RSTRIP logic that removes the
> static RELA sections from vmlinux fails to identify them. This results
> in a situation like below, where some sections that were supposed to get
> removed are left behind.
>
>   [Nr] Name                              Type            Address          Off     Size   ES Flg Lk Inf Al
>
>   [58] runtime_shift_d_hash_shift        PROGBITS        ffffffff83500f50 2900f50 000014 00   A  0   0  1
>   [59] .relaruntime_shift_d_hash_shift   RELA            0000000000000000 55b6f00 000078 18   I 70  58  8
>   [60] runtime_ptr_dentry_hashtable      PROGBITS        ffffffff83500f68 2900f68 000014 00   A  0   0  1
>   [61] .relaruntime_ptr_dentry_hashtable RELA            0000000000000000 55b6f78 000078 18   I 70  60  8
>   [62] runtime_ptr_USER_PTR_MAX          PROGBITS        ffffffff83500f80 2900f80 000238 00   A  0   0  1
>   [63] .relaruntime_ptr_USER_PTR_MAX     RELA            0000000000000000 55b6ff0 000d50 18   I 70  62  8
>
> So tweak the match expression to strip all sections starting with .rel.
> While at it, consolidate the logic used by RISC-V, s390 and x86 into a
> single shared Makefile library command.

I do not know how this works because arch/*/Makefile.post
do not include scripts/Makefile.lib





-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Ard Biesheuvel]

On Fri, 10 Jan 2025 at 13:14, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Fri, Jan 10, 2025 at 5:28 PM Ard Biesheuvel <ardb+git@google.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Due to the fact that runtime const ELF sections are named without a
> > leading period or double underscore, the RSTRIP logic that removes the
> > static RELA sections from vmlinux fails to identify them. This results
> > in a situation like below, where some sections that were supposed to get
> > removed are left behind.
> >
> >   [Nr] Name                              Type            Address          Off     Size   ES Flg Lk Inf Al
> >
> >   [58] runtime_shift_d_hash_shift        PROGBITS        ffffffff83500f50 2900f50 000014 00   A  0   0  1
> >   [59] .relaruntime_shift_d_hash_shift   RELA            0000000000000000 55b6f00 000078 18   I 70  58  8
> >   [60] runtime_ptr_dentry_hashtable      PROGBITS        ffffffff83500f68 2900f68 000014 00   A  0   0  1
> >   [61] .relaruntime_ptr_dentry_hashtable RELA            0000000000000000 55b6f78 000078 18   I 70  60  8
> >   [62] runtime_ptr_USER_PTR_MAX          PROGBITS        ffffffff83500f80 2900f80 000238 00   A  0   0  1
> >   [63] .relaruntime_ptr_USER_PTR_MAX     RELA            0000000000000000 55b6ff0 000d50 18   I 70  62  8
> >
> > So tweak the match expression to strip all sections starting with .rel.
> > While at it, consolidate the logic used by RISC-V, s390 and x86 into a
> > single shared Makefile library command.
>
> I do not know how this works because arch/*/Makefile.post
> do not include scripts/Makefile.lib
>

Apologies - it seems I didn't test this thoroughly.

The build happily completes without any errors, though - it appears
doing '$(call cmd,foo)' does not trigger an error when cmd_foo does
not exist.

I suppose this is a consequence of 8962b6b475bddc ("kbuild: print
short log in addition to the whole command with V=1") which introduced
an $(if ) where the else branch is simply ':' and so it always
succeeds.

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Linus Torvalds]

On Fri, 10 Jan 2025 at 10:11, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> I suppose this is a consequence of 8962b6b475bddc ("kbuild: print
> short log in addition to the whole command with V=1") which introduced
> an $(if ) where the else branch is simply ':' and so it always
> succeeds.

Hmm. Odd. I don't see why that part of the commit exists, and you're
right, that seems like a bad idea.

And removing that odd $(if..) and making it just do

   cmd = @set -e; $($(quiet)log_print) $(delete-on-interrupt) $(cmd_$(1))

doesn't seem to have any obvious negative effects.

I'm probably missing some obvious reason why that $(if..) was added,
it's been that way for two years now.

           Linus

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Masahiro Yamada]

On Sat, Jan 11, 2025 at 3:33 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, 10 Jan 2025 at 10:11, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > I suppose this is a consequence of 8962b6b475bddc ("kbuild: print
> > short log in addition to the whole command with V=1") which introduced
> > an $(if ) where the else branch is simply ':' and so it always
> > succeeds.
>
> Hmm. Odd. I don't see why that part of the commit exists, and you're
> right, that seems like a bad idea.
>
> And removing that odd $(if..) and making it just do
>
>    cmd = @set -e; $($(quiet)log_print) $(delete-on-interrupt) $(cmd_$(1))
>
> doesn't seem to have any obvious negative effects.

When cmd_foo is not defined, $(call if_changed,foo) will fail with this change,
but $(call cmd,foo) will succeed regardless.

In Makefile, the variable expansion works like the latter.
When X is not defined, $(X) is expanded to an empty string successfully.
This is useful.


> I'm probably missing some obvious reason why that $(if..) was added,
> it's been that way for two years now.

We do not need to guard both a definition and its callsite.

For example, the current code in arch/powerpc/Makefile.postlink:

vmlinux: FORCE
        @true
ifdef CONFIG_PPC64
        $(call cmd,head_check)
endif
ifdef CONFIG_RELOCATABLE
        $(call if_changed,relocs_check)
endif
ifdef CONFIG_FUNCTION_TRACER
ifndef CONFIG_PPC64_ELF_ABI_V1
        $(call cmd,ftrace_check)
endif
endif


... can be simplified into:

vmlinux: FORCE
        $(call cmd,head_check)
        $(call if_changed,relocs_check)
        $(call cmd,ftrace_check)






-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Masahiro Yamada]

On Sat, Jan 11, 2025 at 10:13 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Jan 11, 2025 at 3:33 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Fri, 10 Jan 2025 at 10:11, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > I suppose this is a consequence of 8962b6b475bddc ("kbuild: print
> > > short log in addition to the whole command with V=1") which introduced
> > > an $(if ) where the else branch is simply ':' and so it always
> > > succeeds.
> >
> > Hmm. Odd. I don't see why that part of the commit exists, and you're
> > right, that seems like a bad idea.
> >
> > And removing that odd $(if..) and making it just do
> >
> >    cmd = @set -e; $($(quiet)log_print) $(delete-on-interrupt) $(cmd_$(1))
> >
> > doesn't seem to have any obvious negative effects.
>
> When cmd_foo is not defined, $(call if_changed,foo) will fail with this change,
> but $(call cmd,foo) will succeed regardless.
>
> In Makefile, the variable expansion works like the latter.
> When X is not defined, $(X) is expanded to an empty string successfully.
> This is useful.
>
>
> > I'm probably missing some obvious reason why that $(if..) was added,
> > it's been that way for two years now.
>
> We do not need to guard both a definition and its callsite.
>
> For example, the current code in arch/powerpc/Makefile.postlink:
>
> vmlinux: FORCE
>         @true
> ifdef CONFIG_PPC64
>         $(call cmd,head_check)
> endif
> ifdef CONFIG_RELOCATABLE
>         $(call if_changed,relocs_check)
> endif
> ifdef CONFIG_FUNCTION_TRACER
> ifndef CONFIG_PPC64_ELF_ABI_V1
>         $(call cmd,ftrace_check)
> endif
> endif
>
>
> ... can be simplified into:
>
> vmlinux: FORCE
>         $(call cmd,head_check)
>         $(call if_changed,relocs_check)
>         $(call cmd,ftrace_check)
>

arch/riscv/Makefile.postlink is a better example.
CONFIG_RELOCATABLE for the callsite is redundant.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: Strip runtime const RELA sections correctly

[Masahiro Yamada]

On Fri, Jan 10, 2025 at 5:28 PM Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> Due to the fact that runtime const ELF sections are named without a
> leading period or double underscore, the RSTRIP logic that removes the
> static RELA sections from vmlinux fails to identify them. This results
> in a situation like below, where some sections that were supposed to get
> removed are left behind.
>
>   [Nr] Name                              Type            Address          Off     Size   ES Flg Lk Inf Al
>
>   [58] runtime_shift_d_hash_shift        PROGBITS        ffffffff83500f50 2900f50 000014 00   A  0   0  1
>   [59] .relaruntime_shift_d_hash_shift   RELA            0000000000000000 55b6f00 000078 18   I 70  58  8
>   [60] runtime_ptr_dentry_hashtable      PROGBITS        ffffffff83500f68 2900f68 000014 00   A  0   0  1
>   [61] .relaruntime_ptr_dentry_hashtable RELA            0000000000000000 55b6f78 000078 18   I 70  60  8
>   [62] runtime_ptr_USER_PTR_MAX          PROGBITS        ffffffff83500f80 2900f80 000238 00   A  0   0  1
>   [63] .relaruntime_ptr_USER_PTR_MAX     RELA            0000000000000000 55b6ff0 000d50 18   I 70  62  8
>
> So tweak the match expression to strip all sections starting with .rel.
> While at it, consolidate the logic used by RISC-V, s390 and x86 into a
> single shared Makefile library command.
>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-s390@vger.kernel.org
> Link: https://lore.kernel.org/all/CAHk-=wjk3ynjomNvFN8jf9A1k=qSc=JFF591W00uXj-qqNUxPQ@mail.gmail.com/
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/riscv/Makefile.postlink | 9 +--------
>  arch/s390/Makefile.postlink  | 5 -----
>  arch/x86/Makefile.postlink   | 5 -----
>  scripts/Makefile.lib         | 3 +++
>  4 files changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/Makefile.postlink b/arch/riscv/Makefile.postlink
> index 829b9abc91f6..65652fd6a252 100644
> --- a/arch/riscv/Makefile.postlink
> +++ b/arch/riscv/Makefile.postlink
> @@ -19,13 +19,6 @@ ifdef CONFIG_RELOCATABLE
>  quiet_cmd_cp_vmlinux_relocs = CPREL   vmlinux.relocs
>  cmd_cp_vmlinux_relocs = cp vmlinux vmlinux.relocs
>
> -quiet_cmd_relocs_strip = STRIPREL $@
> -cmd_relocs_strip = $(OBJCOPY)   --remove-section='.rel.*'       \
> -                                --remove-section='.rel__*'      \
> -                                --remove-section='.rela.*'      \
> -                                --remove-section='.rela__*' $@
> -endif
> -
>  # `@true` prevents complaint when there is nothing to be done
>
>  vmlinux: FORCE
> @@ -33,7 +26,7 @@ vmlinux: FORCE
>  ifdef CONFIG_RELOCATABLE
>         $(call if_changed,relocs_check)
>         $(call if_changed,cp_vmlinux_relocs)
> -       $(call if_changed,relocs_strip)
> +       $(call if_changed,strip_relocs)

BTW, when if_changed appears multiple times in the same target,
it is always a sign of a bug.

See these commits:

bb81955fd4a49fffdd86d50afd0c1f2eea044c05
92a4728608a8fd228c572bc8ff50dd98aa0ddf2a


Anyway, if_changed does not work in arch/*/Makefile.postlink,
and this is completely broken.








-- 
Best Regards
Masahiro Yamada