Re: [PATCH v5 19/28] build_id: Provide ld-embedded build-ids

["Jan Beulich" <JBeulich@suse.com>]

>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote:
> The version of ld that first implemented --build-id is v2.18.
> Hence we check for that or later version - if older version
> found we do not build the hypervisor with the build-id
> (and the return code is -ENODATA for xen_build_id() call).

This appears to be stale.

> The EFI binary is more complicated. Having any non-recognizable
> sections (.note, .data.note, etc) causes the boot to hang.
> Moving the .note in the .data section makes it work. It is also
> worth noting that the PE/COFF does not have any "comment"
> sections to the author.

I'm afraid this is too vague: What exactly is happening there? And
is this due to binutils doing something wrong, or an issue with the
firmware on the box you've tried? While the placement of .note is
not really a big deal, any unusual placement needs a source
comment attached explaining the whys, to prevent people down
the road moving the section back into the "expected" place. But
see also below.

> Lastly, we MUST call --binary-id=sha1 on all linker invocation so that
> symbol offsets don't changes (which means we have multiple binary
> ids - except that the last one is the final one). Without this change,
> the symbol table embedded in Xen are incorrect - some of the values it
> contains are offset by the size of the included build id.
> This obviously causes problems when resolving symbols.

Would this also be a problem if you place the notes segment/section
last in the binary?

> --- a/Config.mk
> +++ b/Config.mk
> @@ -126,6 +126,17 @@ endef
>  check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1")
>  $(eval $(check-y))
>  
> +ld-ver-build-id = $(shell $(1) --build-id 2>&1 | \
> +					grep -q unrecognized && echo n || echo y)
> +
> +# binutils 2.18 implement build-id.

I don't think this comment is really relevant anymore.

> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -22,6 +22,9 @@ OUTPUT_ARCH(FORMAT)
>  PHDRS
>  {
>    text PT_LOAD /* XXX should be AT ( XEN_PHYS_START ) */ ;
> +#if defined(BUILD_ID)
> +  note PT_NOTE ;
> +#endif

Is this in line with ...

> @@ -57,9 +60,18 @@ SECTIONS
>         *(.lockprofile.data)
>         __lock_profile_end = .;
>  #endif
> +  } :text
>  
> -        _erodata = .;          /* End of read-only data */
> +#if defined(BUILD_ID)
> +  .note : {
> +       __note_gnu_build_id_start = .;
> +       *(.note.gnu.build-id)
> +       __note_gnu_build_id_end = .;
> +       *(.note)
> +       *(.note.*)
>    } :text
> +#endif

... there not being any :note here? And if so, why the earlier
"} :text"?

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -72,9 +72,16 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h 
> -o \
>                        -O $(BASEDIR)/include/xen/compile.h ]; then \
>                           echo '$(TARGET).efi'; fi)
>  
> +ifdef build_id_linker

According to Config.mk this is always true. DYM
"ifneq ($(build_id_linker),)"?

> +build_id.o: $(TARGET)-syms
> +	$(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin

Considering your xen.lds.S changes, won't this potentially copy quite
a bit more than just the build ID (i.e. all notes)? This may be okay, and
may be even intended, but then the generate file's name should
reflect this to not misguide people.

> +	$(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
> +		--rename-section=.data=.note.gnu.build-id -S $@.bin $@

Since you put the notes into .rodata anyway, why name the
section .note.*? Just name it .rodata.*, avoiding to mislead
others who also think that a section's name has much of a
meaning.

> @@ -138,6 +151,13 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIR
>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
>  # Don't use $(wildcard ...) here - at least make 3.80 expands this too early!
>  $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
> +ifdef build_id_linker

See above.

> @@ -228,21 +257,22 @@ static void do_read(int fd, void *data, int len)
>  int main(int argc, char **argv)
>  {
>      u64        final_exec_addr;
> -    u32        loadbase, dat_siz, mem_siz;
> +    u32        loadbase, dat_siz, mem_siz, note_base, note_sz, offset;
>      char      *inimage, *outimage;
>      int        infd, outfd;
>      char       buffer[1024];
>      int        bytes, todo, i;
> +    int        num_phdrs;
>  
>      Elf32_Ehdr in32_ehdr;
>  
>      Elf64_Ehdr in64_ehdr;
>      Elf64_Phdr in64_phdr;
>  
> -    if ( argc != 5 )
> +    if ( argc != 6 )
>      {
>          fprintf(stderr, "Usage: mkelf32 <in-image> <out-image> "
> -                "<load-base> <final-exec-addr>\n");
> +                "<load-base> <final-exec-addr> <number of program headers>\n");

Even if only an internally used tool, I think this is a bad interface:
You want notes copied, and this only happens to result in a second
PHDR. Hence I think there should be an option "--notes" or some
such.

> @@ -299,11 +334,36 @@ int main(int argc, char **argv)
>  
>      (void)lseek(infd, in64_phdr.p_offset, SEEK_SET);
>      dat_siz = (u32)in64_phdr.p_filesz;
> -
>      /* Do not use p_memsz: it does not include BSS alignment padding. */

Stray deletion of a blank line.

> @@ -322,6 +382,31 @@ int main(int argc, char **argv)
>      out_shdr[1].sh_size   = dat_siz;
>      out_shdr[2].sh_offset = RAW_OFFSET + dat_siz + sizeof(out_shdr);
>  
> +    if ( num_phdrs > 1 )
> +    {
> +        /* We have two of them! */
> +        out_ehdr.e_phnum = num_phdrs;
> +        /* Extra .note section. */
> +        out_ehdr.e_shnum++;
> +
> +        /* Fill out the PT_NOTE program header. */
> +        note_phdr.p_vaddr   = note_base;
> +        note_phdr.p_paddr   = note_base;
> +        note_phdr.p_filesz  = note_sz;
> +        note_phdr.p_memsz   = note_sz;
> +        note_phdr.p_offset  = offset;
> +
> +        /* Tack on the .note\0 */
> +        out_shdr[2].sh_size += sizeof(out_shstrtab_extra);
> +        /* And move it past the .note section. */
> +        out_shdr[2].sh_offset += sizeof(out_shdr_extra);

Why "extra" and not "note" or "notes"?

> @@ -335,8 +420,15 @@ int main(int argc, char **argv)
>  
>      endianadjust_phdr32(&out_phdr);
>      do_write(outfd, &out_phdr, sizeof(out_phdr));
> -    
> -    if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr)) < 0 )
> +
> +    if ( num_phdrs > 1 )
> +    {
> +        endianadjust_phdr32(&note_phdr);
> +        do_write(outfd, &note_phdr, sizeof(note_phdr));
> +    }
> +
> +    if ( (bytes = RAW_OFFSET - sizeof(out_ehdr) - sizeof(out_phdr) -
> +          ( num_phdrs > 1 ? sizeof(note_phdr) : 0 ) ) < 0 )

Stray blanks. Also with each PHDR being the same size, why not
just num_phdrs * sizeof()?

> @@ -99,6 +107,21 @@ SECTIONS
>         _erodata = .;
>    } :text
>  
> +#if defined(BUILD_ID) && !defined(EFI)
> +/*
> + * No mechanism to put an PT_NOTE in the EFI file - so put
> + * it in .data section.
> + */

I think this comment really belongs into the earlier addition.

> +  . = ALIGN(4);

I think this would better be added in the EFI case too.

> +  .note : {
> +       __note_gnu_build_id_start = .;
> +       *(.note.gnu.build-id)
> +       __note_gnu_build_id_end = .;
> +       *(.note)
> +       *(.note.*)

The last two don't get dealt with in the EFI case - why? And if you
include all notes here and copy them in their entirety into xen.efi,
how is __note_gnu_build_id_end expected to be correct then?

> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -1,5 +1,9 @@
>  #include <xen/compile.h>
> +#include <xen/errno.h>
> +#include <xen/string.h>
> +#include <xen/types.h>
>  #include <xen/version.h>
> +#include <xen/elf.h>

Please put this last one into its designated slot as well, considering
that you arrange for all the rest to be nicely sorted.

> +int xen_build_id(const void **p, unsigned int *len)
> +{
> +    const Elf_Note *n = __note_gnu_build_id_start;
> +    static bool_t checked = 0;
> +
> +    if ( checked )
> +    {
> +        *len = n->descsz;
> +        *p = ELFNOTE_DESC(n);
> +        return 0;
> +    }
> +    /* --build-id invoked with wrong parameters. */
> +    if ( __note_gnu_build_id_end <= __note_gnu_build_id_start )

Please use the local variable here, just like you do ...

> +        return -ENODATA;
> +
> +    /* Check for full Note header. */
> +    if ( &n[1] > __note_gnu_build_id_end )

... here.

> +        return -ENODATA;
> +
> +    /* Check if we really have a build-id. */
> +    if ( NT_GNU_BUILD_ID != n->type )
> +        return -ENODATA;
> +
> +    /* Sanity check, name should be "GNU" for ld-generated build-id. */
> +    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
> +        return -ENODATA;
> +
> +    *len = n->descsz;
> +    *p = ELFNOTE_DESC(n);
> +
> +    checked = 1;

All this checking would perhaps better be done in an initcall, at once
avoiding it to be done over and over if it fails for some reason. 

> +    return 0;
> +}
> +
> +#else
> +
> +int xen_build_id(const void **p, unsigned int *len)
> +{
> +    return -ENODATA;
> +}
> +#endif

Is this #else part really needed? If you omitted the BUILD_ID
dependency in xen.lds.S, wouldn't you just find an empty section
here when the linker is incapable of --build-id?

> --- a/xen/include/xen/version.h
> +++ b/xen/include/xen/version.h
> @@ -14,4 +14,7 @@ const char *xen_changeset(void);
>  const char *xen_banner(void);
>  const char *xen_deny(void);
>  
> +#include <xen/types.h>

Why? And if really needed, it doesn't belong in the middle of the file.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel