Re: [PATCH v5 09/18] dump: Use a buffer for ELF section data and headers - Janis Schoetterl-Glausch

From: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
To: Janosch Frank <frankja@linux.ibm.com>, qemu-devel@nongnu.org
Cc: marcandre.lureau@redhat.com, pbonzini@redhat.com,
	mhartmay@linux.ibm.com,  borntraeger@linux.ibm.com,
	imbrenda@linux.ibm.com, pasic@linux.ibm.com, cohuck@redhat.com,
	thuth@redhat.com, qemu-s390x@nongnu.org, seiden@linux.ibm.com
Subject: Re: [PATCH v5 09/18] dump: Use a buffer for ELF section data and headers
Date: Mon, 29 Aug 2022 22:43:13 +0200	[thread overview]
Message-ID: <dc56340d45adbdbdc5cc2793bab222f8a4e8bab7.camel@linux.ibm.com> (raw)
In-Reply-To: <20220811121111.9878-10-frankja@linux.ibm.com>

On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote:
> Currently we're writing the NULL section header if we overflow the
> physical header number in the ELF header. But in the future we'll add
> custom section headers AND section data.
> 
> To facilitate this we need to rearange section handling a bit. As with
> the other ELF headers we split the code into a prepare and a write
> step.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 83 +++++++++++++++++++++++++++++--------------
>  include/sysemu/dump.h |  2 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index a905316fe5..0051c71d08 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error **errp)
>      }
>  }
>  
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static void prepare_elf_section_hdr_zero(DumpState *s)
>  {
> -    Elf32_Shdr shdr32;
> -    Elf64_Shdr shdr64;
> -    int shdr_size;
> -    void *shdr;
> +    if (dump_is_64bit(s)) {
> +        Elf64_Shdr *shdr64 = s->elf_section_hdrs;
> +
> +        shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    } else {
> +        Elf32_Shdr *shdr32 = s->elf_section_hdrs;
> +
> +        shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
> +    }
> +}
> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> +    size_t len, sizeof_shdr;
> +
> +    /*
> +     * Section ordering:
> +     * - HDR zero
> +     */
> +    sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +    len = sizeof_shdr * s->shdr_num;
> +    s->elf_section_hdrs = g_malloc0(len);

I'm not seeing this being freed.
> +
> +    /*
> +     * The first section header is ALWAYS a special initial section
> +     * header.
> +     *
> +     * The header should be 0 with one exception being that if
> +     * phdr_num is PN_XNUM then the sh_info field contains the real
> +     * number of segment entries.
> +     *
> +     * As we zero allocate the buffer we will only need to modify
> +     * sh_info for the PN_XNUM case.
> +     */
> +    if (s->phdr_num >= PN_XNUM) {
> +        prepare_elf_section_hdr_zero(s);
> +    }
> +}
> +
> +static void write_elf_section_headers(DumpState *s, Error **errp)

[...]

> @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>  
> +    /* write section headers to vmcore */
> +    write_elf_section_headers(s, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
>      /* write PT_NOTE to vmcore */
>      write_elf_phdr_note(s, errp);
>      if (*errp) {
> @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
>          return;
>      }
>  
> -    /* write section to vmcore */
> -    if (s->shdr_num) {
> -        write_elf_section(s, 1, errp);
> -        if (*errp) {
> -            return;
> -        }
> -    }
> -

Here you change the order of the headers, but the elf header is only
fixed in patch 11.
I agree that this should be a separate patch, with an explanation on
why it's necessary. 
So basically squashed into patch 11, except I think the comment change
in that one should go into another patch.

>      /* write notes to vmcore */
>      write_elf_notes(s, errp);
>  }
> @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
>          return;
>      }
>  
> +    /* Iterate over memory and dump it to file */
>      dump_iterate(s, errp);
> +    if (*errp) {
> +        return;
> +    }
>  }
>  
>  static int write_start_flat_header(int fd)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b62513d87d..9995f65dc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -177,6 +177,8 @@ typedef struct DumpState {
>      int64_t filter_area_begin;  /* Start address of partial guest memory area */
>      int64_t filter_area_length; /* Length of partial guest memory area */
>  
> +    void *elf_section_hdrs;     /* Pointer to section header buffer */
> +
>      uint8_t *note_buf;          /* buffer for notes */
>      size_t note_buf_offset;     /* the writing place in note_buf */
>      uint32_t nr_cpus;           /* number of guest's cpu */