Re: [PATCH 5.15 5.10 5.4 v2] kbuild: fix Build ID if CONFIG_MODVERSIONS

[Tom Saeger <tom.saeger@oracle.com>]

On Wed, Dec 21, 2022 at 11:56:33AM -0800, Nick Desaulniers wrote:
> Tom, thanks for pursuing this. Sorry I'm falling behind in reviews
> (going offline soon for the holidays).

Same :)

> Some additional questions:
> 
> 
> On Thu, Dec 15, 2022 at 3:18 PM Tom Saeger <tom.saeger@oracle.com> wrote:
> >
> > Backport of:
> > commit 0d362be5b142 ("Makefile: link with -z noexecstack --no-warn-rwx-segments")
> > breaks arm64 Build ID when CONFIG_MODVERSIONS=y for all kernels
> > from: commit e4484a495586 ("Merge tag 'kbuild-fixes-v5.0' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild")
> > until: commit df202b452fe6 ("Merge tag 'kbuild-v5.19' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild")
> 
> Just arm64? Why not other architectures?

I've only encountered this with arm64 and specifically NOT with x86.
I suspect other architectures may encounter this, but I haven't tried.

v1 cover has a simple example if someone has capability/time to adapt to
another architecture.

- enable CONFIG_MODVERSIONS
- build
- readelf -n vmlinux

> 
> >
> > Linus's tree doesn't have this issue since 0d362be5b142 was merged
> > after df202b452fe6 which included:
> > commit 7b4537199a4a ("kbuild: link symbol CRCs at final link, removing CONFIG_MODULE_REL_CRCS")
> >
> > This kernel's KBUILD CONFIG_MODVERSIONS tooling compiles and links .S targets
> > with relocatable (-r) and now (-z noexecstack)
> > which results in ld adding a .note.GNU-stack section to .o files.
> > Final linking of vmlinux should add a .NOTES segment containing the
> > Build ID, but does NOT (on some architectures like arm64) if a
> > .note.GNU-stack section is found in .o's supplied during link
> > of vmlinux.
> 
> Is that a bug in BFD?  That the behavior differs per target
> architecture is subtle.  If it's not documented behavior that you can
> link to, can you file a bug about your findings and cc me?
> https://sourceware.org/bugzilla/enter_bug.cgi?product=binutils

I've found:
https://sourceware.org/bugzilla/show_bug.cgi?id=16744
Comment 1: https://sourceware.org/bugzilla/show_bug.cgi?id=16744#c1

"the semantics of a .note.GNU-stack presence is target-dependent."

corresponding to this commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=76f0cad6f4e0fdfc4cfeee135b44b6a090919c60

So - I'm not entirely sure if this is a bug or expected behavior.

> 
> If it is a bug in BFD, then I'm not opposed to working around it, but
> it would be good to have as precise a report as possible in the commit
> message if we're going to do hijinks in a stable-only patch for
> existing tooling.
> 
> If it's a feature, having some explanation _why_ we get per-arch
> behavior like this may be helpful for us to link to in the future
> should this come up again.

While I agree - *I* don't have an explanation (despite digging), only
work-arounds.

> 
> >
> > DISCARD .note.GNU-stack sections of .S targets.  Final link of
> 
> That's going to give them an executable stack again.
> https://www.redhat.com/en/blog/linkers-warnings-about-executable-stacks-and-segments
> >> missing .note.GNU-stack section implies executable stack
> The intent of 0d362be5b142 is that we don't want translation units to
> have executable stacks, though I do note that assembler sources need
> to opt in.
> 
> Is it possible to force a build-id via linker flag `--build-id=sha1`?
That's an idea - I'll see if this works.

> 
> If not, can we just use `-z execstack` rather than concatenating a
> DISCARD section into a linker script?

so... something like v1 patch, but replace `-z noexecstack` with `-z
execstack`?  And for arm64 only?  I'll try this.


> Either command line flags feel
> cleaner than modifying a linker script at build time, if they work
> that is.

well... that entire linker script is generated at build-time.

> 
> > vmlinux then properly adds .NOTES segment containing Build ID that can
> > be read using tools like 'readelf -n'.
> >
> > Fixes: 0d362be5b142 ("Makefile: link with -z noexecstack --no-warn-rwx-segments")
> > Cc: <stable@vger.kernel.org> # 5.15, 5.10, 5.4
> > Cc: <linux-kbuild@vger.kernel.org>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Michal Marek <michal.lkml@markovi.net>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Signed-off-by: Tom Saeger <tom.saeger@oracle.com>
> > ---
> >
> > v2:
> >   - Changed approach to append DISCARD section to generated linker script.
> >     - ld no longer emits warning (which was intent of 0d362b35b142) this
> >       addresses Nick's v1 feedback.
> >     - this is applied to all arches, not just arm64
> >   - added commit refs and notes why this doesn't occur in Linus's tree
> >     to address Greg's v1 feedback.
> >   - added Fixes: 0d362b35b142 requested by Nick
> >   - added note to changelog for 7b4537199a4a requested by Nick
> >   - build tested on arm64 and x86
> >
> >    version           works(vmlinux contains Build ID)
> >    v4.14.302         x86, arm64
> >    v4.14.302.patched x86, arm64
> >    v4.19.269         x86, arm64
> >    v4.19.269.patched x86, arm64
> >    v5.4.227          x86
> >    v5.4.227.patched  x86, arm64
> >    v5.10.159         x86
> >    v5.10.159.patched x86, arm64
> >    v5.15.83          x86
> >    v5.15.83.patched  x86, arm64
> >
> > v1: https://lore.kernel.org/all/cover.1670358255.git.tom.saeger@oracle.com/
> >
> >
> >  scripts/Makefile.build | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 17aa8ef2d52a..e3939676eeb5 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -379,6 +379,8 @@ cmd_modversions_S =                                                         \
> >         if $(OBJDUMP) -h $@ | grep -q __ksymtab; then                           \
> >                 $(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))  \
> >                     > $(@D)/.tmp_$(@F:.o=.ver);                                 \
> > +               echo "SECTIONS { /DISCARD/ : { *(.note.GNU-stack) } }"          \
> > +               >> $(@D)/.tmp_$(@F:.o=.ver);                                    \
> >                                                                                 \
> >                 $(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@               \
> >                         -T $(@D)/.tmp_$(@F:.o=.ver);                            \
> >
> > base-commit: fd6d66840b4269da4e90e1ea807ae3197433bc66
> > --
> > 2.38.1
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers