Re: [PATCH v2][RFC] kdump: append kernel build-id string to VMCOREINFO

[Vijay Balakrishna]

Thanks Andrew.  See inline.

On 6/17/2020 12:45 PM, Andrew Morton wrote:
> On Wed, 10 Jun 2020 21:27:52 -0700 Vijay Balakrishna <vijayb@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > Make kernel GNU build-id available in VMCOREINFO.  Having
> > build-id in VMCOREINFO facilitates presenting appropriate kernel
> > namelist image with debug information file to kernel crash dump
> > analysis tools.  Currently VMCOREINFO lacks uniquely identifiable
> > key for crash analysis automation.
> >
> > Regarding if this patch is necessary or matching of linux_banner
> > and OSRELEASE in VMCOREINFO employed by crash(8) meets the
> > need -- IMO, build-id approach more foolproof, in most instances it
> > is a cryptographic hash generated using internal code/ELF bits unlike
> > kernel version string upon which linux_banner is based that is
> > external to the code.  I feel each is intended for a different purpose.
> > Also OSRELEASE is not suitable when two different kernel builds
> > from same version with different features enabled.
> >
> > Currently for most linux (and non-linux) systems build-id can be
> > extracted using standard methods for file types such as user mode crash
> > dumps, shared libraries, loadable kernel modules etc.,  This is an
> > exception for linux kernel dump.  Having build-id in VMCOREINFO brings
> > some uniformity for automation tools.
> >
> > ...
> >
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -11,6 +11,8 @@
> >   #include <asm/page.h>
> >   #include <asm/sections.h>
> >   
> > +#include <crypto/sha.h>
> > +
> >   /* vmcoreinfo stuff */
> >   unsigned char *vmcoreinfo_data;
> >   size_t vmcoreinfo_size;
> > @@ -376,6 +378,53 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
> >   }
> >   EXPORT_SYMBOL(paddr_vmcoreinfo_note);
> >   
> > +#define NOTES_SIZE (&__stop_notes - &__start_notes)
> > +#define BUILD_ID_MAX SHA1_DIGEST_SIZE
> > +#define NT_GNU_BUILD_ID 3
> > +
> > +struct elf_note_section {
> > +	struct elf_note	n_hdr;
> > +	u8 n_data[];
> > +};
> > +
> > +/*
> > + * Add build ID from .notes section as generated by the GNU ld(1)
> > + * or LLVM lld(1) --build-id option.
> > + */
> > +static void add_build_id_vmcoreinfo(void)
> > +{
> > +	char build_id[BUILD_ID_MAX * 2 + 1];
> > +	int n_remain = NOTES_SIZE;
> > +
> > +	while (n_remain >= sizeof(struct elf_note)) {
> > +		const struct elf_note_section *note_sec =
> > +			&__start_notes + NOTES_SIZE - n_remain;
> > +		const u32 n_namesz = note_sec->n_hdr.n_namesz;
> > +
> > +		if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
> > +		    n_namesz != 0 &&
> > +		    !strcmp((char *)&note_sec->n_data[0], "GNU")) {
>
> Is it guaranteed that n_data[] is null-terminated?

From ELF(5) "Notes (Nhdr)" section:

>        n_namesz  The length of the name field in bytes.  The contents will immediately follow this note in memory.  The name is null  terminated.   For
>                  example, if the name is "GNU", then n_namesz will be set to 4.

Vijay

> > +			if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
> > +				const u32 n_descsz = note_sec->n_hdr.n_descsz;
> > +				const u8 *s = &note_sec->n_data[n_namesz];
> > +
> > +				s = PTR_ALIGN(s, 4);
> > +				bin2hex(build_id, s, n_descsz);
> > +				build_id[2 * n_descsz] = '\0';
> > +				VMCOREINFO_BUILD_ID(build_id);
> > +				return;
> > +			}
> > +			pr_warn("Build ID is too large to include in vmcoreinfo: %u > %u\n",
> > +				note_sec->n_hdr.n_descsz,
> > +				BUILD_ID_MAX);
> > +			return;
> > +		}
> > +		n_remain -= sizeof(struct elf_note) +
> > +			ALIGN(note_sec->n_hdr.n_namesz, 4) +
> > +			ALIGN(note_sec->n_hdr.n_descsz, 4);
> > +	}
> > +}
> > +
> >   static int __init crash_save_vmcoreinfo_init(void)
> >   {
> >   	vmcoreinfo_data = (unsigned char *)get_zeroed_page(GFP_KERNEL);