Re: [Qemu-devel] [PATCH v3 4/7] dump: add vmcoreinfo ELF note

[Laszlo Ersek]

On 07/11/17 12:30, Marc-André Lureau wrote:
> Read the vmcoreinfo ELF PT_NOTE from guest memory when vmcoreinfo
> device provides the location, and write it as an ELF note in the dump.
> 
> There are now 2 possible sources of phys_base information.
> 
> (1) arch guessed value from cpu_get_dump_info()
> (2) vmcoreinfo ELF note NUMBER(phys_base)= field
> 
>     NUMBER(phys_base) in vmcoreinfo has only been recently introduced
>     in Linux 4.10 (401721ecd1dc "kexec: export the value of phys_base
>     instead of symbol address").
> 
> Since (2) has better chances to be accurate, the guessed value is
> replaced by the value from the vmcoreinfo ELF note.
> 
> The phys_base value is stored in the same dump field locations as
> before, and may duplicate the information available in the vmcoreinfo
> ELF PT_NOTE. Crash tools should be prepared to handle this case.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/sysemu/dump.h |   2 +
>  dump.c                | 134 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 2672a15f8b..111a7dcaa4 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -192,6 +192,8 @@ typedef struct DumpState {
>                                    * this could be used to calculate
>                                    * how much work we have
>                                    * finished. */
> +    uint8_t *vmcoreinfo;         /* ELF note content */
> +    size_t vmcoreinfo_size;
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> diff --git a/dump.c b/dump.c
> index d9090a24cc..2928757584 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -26,6 +26,8 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qmp-commands.h"
>  #include "qapi-event.h"
> +#include "qemu/error-report.h"
> +#include "hw/acpi/vmcoreinfo.h"
>  
>  #include <zlib.h>
>  #ifdef CONFIG_LZO
> @@ -38,6 +40,13 @@
>  #define ELF_MACHINE_UNAME "Unknown"
>  #endif
>  
> +#define MAX_VMCOREINFO_SIZE (1 << 20) /* 1MB should be enough */
> +
> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
> +    ((DIV_ROUND_UP((hdr_size), 4) +                     \
> +      DIV_ROUND_UP((name_size), 4) +                    \
> +      DIV_ROUND_UP((desc_size), 4)) * 4)
> +
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
>  {
>      if (s->dump_info.d_endian == ELFDATA2LSB) {
> @@ -76,6 +85,8 @@ static int dump_cleanup(DumpState *s)
>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
>      close(s->fd);
> +    g_free(s->vmcoreinfo);
> +    s->vmcoreinfo = NULL;
>      if (s->resume) {
>          if (s->detached) {
>              qemu_mutex_lock_iothread();
> @@ -235,6 +246,19 @@ static inline int cpu_index(CPUState *cpu)
>      return cpu->cpu_index + 1;
>  }
>  
> +static void write_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
> +                                  Error **errp)
> +{
> +    int ret;
> +
> +    if (s->vmcoreinfo) {
> +        ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
> +        if (ret < 0) {
> +            error_setg(errp, "dump: failed to write vmcoreinfo");
> +        }
> +    }
> +}
> +
>  static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>                                Error **errp)
>  {
> @@ -258,6 +282,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>              return;
>          }
>      }
> +
> +    write_vmcoreinfo_note(f, s, errp);
>  }
>  
>  static void write_elf32_note(DumpState *s, Error **errp)
> @@ -303,6 +329,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>              return;
>          }
>      }
> +
> +    write_vmcoreinfo_note(f, s, errp);
>  }
>  
>  static void write_elf_section(DumpState *s, int type, Error **errp)
> @@ -714,6 +742,44 @@ static int buf_write_note(const void *buf, size_t size, 
> void *opaque)
>      return 0;
>  }
>  
> +/*
> + * This function retrieves various sizes from an elf header.
> + *
> + * @note has to be a valid ELF note. The return sizes are unmodified
> + * (not padded or rounded up to be multiple of 4).
> + */
> +static void get_note_sizes(DumpState *s, const void *note,
> +                           uint64_t *note_head_size,
> +                           uint64_t *name_size,
> +                           uint64_t *desc_size)
> +{
> +    uint64_t note_head_sz;
> +    uint64_t name_sz;
> +    uint64_t desc_sz;
> +
> +    if (s->dump_info.d_class == ELFCLASS64) {
> +        const Elf64_Nhdr *hdr = note;
> +        note_head_sz = sizeof(Elf64_Nhdr);
> +        name_sz = tswap64(hdr->n_namesz);
> +        desc_sz = tswap64(hdr->n_descsz);
> +    } else {
> +        const Elf32_Nhdr *hdr = note;
> +        note_head_sz = sizeof(Elf32_Nhdr);
> +        name_sz = tswap32(hdr->n_namesz);
> +        desc_sz = tswap32(hdr->n_descsz);
> +    }
> +
> +    if (note_head_size) {
> +        *note_head_size = note_head_sz;
> +    }
> +    if (name_size) {
> +        *name_size = name_sz;
> +    }
> +    if (desc_size) {
> +        *desc_size = desc_sz;
> +    }
> +}
> +
>  /* write common header, sub header and elf note to vmcore */
>  static void create_header32(DumpState *s, Error **errp)
>  {
> @@ -1488,10 +1554,40 @@ static int64_t dump_calculate_size(DumpState *s)
>      return total;
>  }
>  
> +static void vmcoreinfo_update_phys_base(DumpState *s)
> +{
> +    uint64_t size, note_head_size, name_size, phys_base;
> +    char **lines;
> +    uint8_t *vmci;
> +    size_t i;
> +
> +    get_note_sizes(s, s->vmcoreinfo, &note_head_size, &name_size, &size);
> +    note_head_size = ROUND_UP(note_head_size, 4);
> +    name_size = ROUND_UP(name_size, 4);
> +    vmci = s->vmcoreinfo + note_head_size + name_size;
> +    *(vmci + size) = '\0';
> +
> +    lines = g_strsplit((char *)vmci, "\n", -1);
> +    for (i = 0; lines[i]; i++) {
> +        if (g_str_has_prefix(lines[i], "NUMBER(phys_base)=")) {
> +            if (qemu_strtou64(lines[i] + 18, NULL, 16,
> +                              &phys_base) < 0) {
> +                error_report("warning: Failed to read NUMBER(phys_base)=");

good change, adding "warning:" :)

> +            } else {
> +                s->dump_info.phys_base = phys_base;
> +            }
> +            break;
> +        }
> +    }
> +
> +    g_strfreev(lines);
> +}
> +
>  static void dump_init(DumpState *s, int fd, bool has_format,
>                        DumpGuestMemoryFormat format, bool paging, bool 
> has_filter,
>                        int64_t begin, int64_t length, Error **errp)
>  {
> +    Object *vmcoreinfo_dev = find_vmcoreinfo_dev();
>      CPUState *cpu;
>      int nr_cpus;
>      Error *err = NULL;
> @@ -1563,6 +1659,44 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>          goto cleanup;
>      }
>  
> +    /*
> +     * the goal of this block is to (a) update the previously guessed
> +     * phys_base, (b) copy the vmcoreinfo note out of the guest. And
> +     * that failure to do so is not fatal for dumping.
> +     */

The words "And that" are not necessary here, they were only part of my
v2 commentary. Admittedly, my commentary should have been better formulated.

No need to repost because of this, the meaning is not harmed in any way.

> +    if (vmcoreinfo_dev) {
> +        uint64_t addr, note_head_size, name_size, desc_size;
> +        uint32_t size;
> +
> +        note_head_size = s->dump_info.d_class == ELFCLASS32 ?
> +            sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
> +
> +        if (!vmcoreinfo_get(VMCOREINFO(vmcoreinfo_dev),
> +                            &addr, &size, &err)) {
> +            error_report_err(err);
> +            err = NULL;
> +        } else if (size < note_head_size || size > MAX_VMCOREINFO_SIZE) {
> +            error_report("warning: vmcoreinfo size is invalid: %" PRIu32, 
> size);
> +        } else {
> +            s->vmcoreinfo = g_malloc(size + 1); /* +1 for adding \0 */
> +            cpu_physical_memory_read(addr, s->vmcoreinfo, size);
> +
> +            get_note_sizes(s, s->vmcoreinfo, NULL, &name_size, &desc_size);
> +            s->vmcoreinfo_size = ELF_NOTE_SIZE(note_head_size, name_size,
> +                                               desc_size);
> +            if (name_size > MAX_VMCOREINFO_SIZE ||
> +                desc_size > MAX_VMCOREINFO_SIZE ||
> +                s->vmcoreinfo_size > size) {

Nice. ELF_NOTE_SIZE() is safe to use with those uint64_t variables. Even
if the result overflows (and therefore s->vmcoreinfo_size is
mathematically invalid), it is all well-defined in C, and we catch the
problem later, with the new individual checks. It's not a problem that
"s->vmcoreinfo_size" carries an invalid value temporarily.

This way you managed to reuse the same error handling block.

> +                error_report("warning: Invalid vmcoreinfo header");
> +                g_free(s->vmcoreinfo);
> +                s->vmcoreinfo = NULL;
> +            } else {
> +                vmcoreinfo_update_phys_base(s);
> +                s->note_size += s->vmcoreinfo_size;
> +            }
> +        }
> +    }
> +
>      /* get memory mapping */
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
> 

Reviewed-by: Laszlo Ersek <address@hidden>

Thanks
Laszlo