Re: [PATCH v4 11/17] dump/dump: Add section string table support

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On 7/26/22 11:22, Janosch Frank wrote:
> As sections don't have a type like the notes do we need another way to

Having a string table seems like a good idea to me, as we don't know
the requirements any architecture might have, but sections do have sh_type.
Could we use one of those, e.g. one of the processor specific ones?
Is
	SHT_PROGBITS
    		The section holds information defined by the program,
		whose format and meaning are determined solely by the program.
appropriate for us? It seems to me that our program is the guest and
it doesn't determine the meta information on how to decrypt the dump.

> determine their contents. The string table allows us to assign each
> section an identification string which architectures can then use to
> tag their sections with.
> 
> There will be no string table if the architecture doesn't add custom
> sections which are introduced in a following patch.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c           | 81 ++++++++++++++++++++++++++++++++++++++++++-
>  include/sysemu/dump.h |  1 +
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 3cf846d0a0..298a1e923f 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c

[...]

>  static void prepare_elf_section_hdrs(DumpState *s)
>  {
>      size_t len, sizeof_shdr;
> +    Elf64_Ehdr *hdr64 = s->elf_header;
> +    Elf32_Ehdr *hdr32 = s->elf_header;
> +    void *buff_hdr;
>  
>      /*
>       * Section ordering:
>       * - HDR zero (if needed)
> +     * - String table hdr
>       */
>      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);
> +    buff_hdr = s->elf_section_hdrs;
>  
>      /* Write special section first */
>      if (s->phdr_num == PN_XNUM) {
>          prepare_elf_section_hdr_zero(s);
> +        buff_hdr += sizeof_shdr;
> +    }
> +
> +    if (s->shdr_num < 2) {
> +        return;
> +    }
> +
> +    /*
> +     * String table needs to be last section since strings are added
> +     * via arch_sections_write_hdr().
> +     */
> +    write_elf_section_hdr_string(s, buff_hdr);
> +    if (dump_is_64bit(s)) {
> +        hdr64->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
> +    } else {
> +        hdr32->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1);
>      }

Might want to assert e_shstrndx < SHN_LORESERVE (0xff00) or handle that case correctly.

>  }
>  
> @@ -401,11 +453,18 @@ static void write_elf_sections(DumpState *s, Error **errp)
>  {
>      int ret;
>  
> -    /* Write section zero */
> +    /* Write section zero and arch sections */

Since that doesn't concern the string section it seems wrong to change this in this patch.

>      ret = fd_write_vmcore(s->elf_section_data, s->elf_section_data_size, s);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "dump: failed to write section data");
>      }
> +
> +    /* Write string table data */
> +    ret = fd_write_vmcore(s->string_table_buf->data,
> +                          s->string_table_buf->len, s);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "dump: failed to write string table data");
> +    }
>  }
>  
>  static void write_data(DumpState *s, void *buf, int length, Error **errp)
> @@ -713,6 +772,7 @@ static void create_vmcore(DumpState *s, Error **errp)
>          return;
>      }
>  
> +    /* Iterate over memory and dump it to file */

This should be going into the patch introducing dump_end.

>      dump_iterate(s, errp);
>      if (*errp) {
>          return;
> @@ -1695,6 +1755,13 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      s->has_filter = has_filter;
>      s->begin = begin;
>      s->length = length;
> +    /* First index is 0, it's the special null name */
> +    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> +    /*
> +     * Allocate the null name, due to the clearing option set to true
> +     * it will be 0.
> +     */
> +    g_array_set_size(s->string_table_buf, 1);
>  
>      memory_mapping_list_init(&s->list);
>  
> @@ -1855,6 +1922,18 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          }
>      }
>  
> +    /*
> +     * calculate shdr_num and elf_section_data_size so we know the offsets and
> +     * sizes of all parts.
> +     *
> +     * If phdr_num overflowed we have at least one section header
> +     * More sections/hdrs can be added by the architectures
> +     */

This needs to be adjusted like we talked about, i.e. adding the special 0 section unless
it already exists.

> +    if (s->shdr_num > 1) {
> +        /* Reserve the string table */
> +        s->shdr_num += 1;
> +    }
> +
>      tmp = (s->phdr_num == PN_XNUM) ? s->sh_info : s->phdr_num;
>      if (dump_is_64bit(s)) {
>          s->shdr_offset = sizeof(Elf64_Ehdr);
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 696e6c67d6..299b611180 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -178,6 +178,7 @@ typedef struct DumpState {
>      void *elf_section_hdrs;
>      uint64_t elf_section_data_size;
>      void *elf_section_data;
> +    GArray *string_table_buf;  /* String table section */
>  
>      uint8_t *note_buf;          /* buffer for notes */
>      size_t note_buf_offset;     /* the writing place in note_buf */