| Message ID | 20250110082744.457067-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: Strip runtime const RELA sections correctly | expand |
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
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.
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
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)
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.
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.
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 # ---------------------------------------------------------------------------