Re: [PATCH v2 02/12] buildid: Add method to get running kernel's build ID

[Stephen Boyd]

Quoting Rasmus Villemoes (2021-03-24 02:24:27)
> On 24/03/2021 03.04, Stephen Boyd wrote:
> > Add vmlinux_build_id() so that callers can print a hex format string
> > representation of the running kernel's build ID. This will be used in
> > the kdump and dump_stack code so that developers can easily locate the
> > vmlinux debug symbols for a crash/stacktrace.
> > 
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Cc: Jessica Yu <jeyu@xxxxxxxxxx>
> > Cc: Evan Green <evgreen@xxxxxxxxxxxx>
> > Cc: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > Cc: Dave Young <dyoung@xxxxxxxxxx>
> > Cc: Baoquan He <bhe@xxxxxxxxxx>
> > Cc: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > Cc: <kexec@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> >  include/linux/buildid.h |  2 ++
> >  lib/buildid.c           | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index ebce93f26d06..2ff6b1b7cc9b 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -10,4 +10,6 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> >                  __u32 *size);
> >  int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
> >  
> > +const unsigned char *vmlinux_build_id(void);
> > +
> >  #endif
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 010ab0674cb9..fa1b6466b4b8 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -4,6 +4,7 @@
> >  #include <linux/elf.h>
> >  #include <linux/kernel.h>
> >  #include <linux/pagemap.h>
> > +#include <linux/string.h>
> >  
> >  #define BUILD_ID 3
> >  
> > @@ -171,3 +172,21 @@ int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size)
> >  {
> >       return parse_build_id_buf(build_id, NULL, buf, buf_size);
> >  }
> > +
> > +/**
> > + * vmlinux_build_id - Get the running kernel's build ID
> > + *
> > + * Return: Running kernel's build ID
> > + */
> > +const unsigned char *vmlinux_build_id(void)
> > +{
> > +     extern const void __start_notes __weak;
> > +     extern const void __stop_notes __weak;
> > +     unsigned int size = &__stop_notes - &__start_notes;
> > +     static unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
> > +
> > +     if (!memchr_inv(vmlinux_build_id, 0, BUILD_ID_SIZE_MAX))
> > +             build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
> > +
> > +     return vmlinux_build_id;
> > +}
> > 
> 
> Hm, is there any reason to do that initialization lazily and thus need
> an accessor? If the system is coming down hard, there's a (very very
> small) risk that one thread starts finding the build id, is in the
> middle of the memcpy, another thread also ends up wanting the vmlinux
> build id, sees some non-nul byte, and proceeds to using the partially
> written vmlinux_build_id.
> 
> Perhaps consider just exposing the vmlinux_build_id[] array itself,
> adding a init_vmlinux_build_id() call somewhere early in start_kernel().
> 
> It could then also be made __ro_after_init.
> 
> In any case, if you decide to keep the current way, please rename the
> local variable (just "build_id" is fine) so that it doesn't shadow the
> very function it resides in, that's very confusing.
> 

No particular reason to do it this way. I'll take that approach to
initialize it early in start_kernel() and then expose the array instead.
Thanks!