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

[Laszlo Ersek]

On 07/06/17 12:16, 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_dump_info_get()

I recommend using the clipboard; the function is still called
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                | 125 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 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..f699198204 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,11 @@
>  #define ELF_MACHINE_UNAME "Unknown"
>  #endif
>  
> +#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 +83,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 +244,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 +280,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 +327,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 +740,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 +1552,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("Failed to read NUMBER(phys_base)=");
> +            } 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 +1657,37 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>          goto cleanup;
>      }
>  
> +#define MAX_VMCOREINFO_SIZE (1 << 20) /* 1MB should be enough */

I think we generally place macros like this near the top of the file.
You already #define ELF_NOTE_SIZE up there.

> +    if (vmcoreinfo_dev) {

I'm sorry, I should have asked for this in the previous version -- can
you place a comment here, above the "if", saying that 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.

> +        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);

Apologies, I should have noticed this too in the previous version. After
you print and free the error with error_report_err(), you should also
null the "err" variable. The dump_init() function goes on to receive
further errors into "err", and if "err" is not NULL at that point, two
things can happen:

- if a function uses error_propagate() into "err", then "err" will
appear to have been set earlier, and error_propagate() will drop the new
error.

- if error_setg() or similar is used instead, then that will trip an assert.

> +        } else if (size < note_head_size || size > MAX_VMCOREINFO_SIZE) {
> +            error_report("vmcoreinfo size is invalid: %u", size);

Consider using "%"PRIu32 for stylistic reasons, although in practice %u
is perfectly fine for uint32_t (since uint32_t *is* "unsigned int").

> +        } 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);

So, to answer your earlier question, please check both "name_size" and
"desc_size" individually against MAX_VMCOREINFO_SIZE. (While preserving
the cumulative size check below, of course.) This is simple enough and
ensures neither the roundings nor the additions can overflow later.

If any of these checks fails, you'll have to free and null s->vmcoreinfo
here too.

The rest looks good!

Thanks!
Laszlo

> +            s->vmcoreinfo_size = ELF_NOTE_SIZE(note_head_size, name_size,
> +                                               desc_size);
> +            if (s->vmcoreinfo_size > size) {
> +                error_report("Invalid vmcoreinfo header, size mismatch");
> +                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);
>