| Message ID | 20220811121111.9878-13-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
Hi Janosch, On 8/11/22 14:11, Janosch Frank wrote: > As sections don't have a type like the notes do we need another way to > 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 | 71 +++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 4 +++ > 2 files changed, 75 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 31eb20108c..0d6dbf453a 100644 > --- a/dump/dump.c > +++ b/dump/dump.c [ snip ] > } > > +static void prepare_elf_section_hdr_string(DumpState *s, void *buff) > +{ > + Elf32_Shdr shdr32; > + Elf64_Shdr shdr64; > + int shdr_size; > + void *shdr; > + > + if (dump_is_64bit(s)) { > + shdr_size = sizeof(Elf64_Shdr); > + memset(&shdr64, 0, shdr_size); > + shdr64.sh_type = SHT_STRTAB; > + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr64.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); I think you mixed up .strtab and .shstrtab here. '.shstrtab' should be used here. The ELF specs define bots as follows (from man 5 elf) : .shstrtab This section holds section names. This section is of type SHT_STRTAB. No attribute types are used. .strtab This section holds strings, most commonly the strings that represent the names associated with symbol table entries. If the file has a loadable segment that includes the symbol string table, the section's attributes will include the SHF_ALLOC bit. Otherwise, the bit will be off. This section is of type SHT_STRTAB. However, the name lookup works, as you correctly specified that this section holds the section header names via the 'e_shstrndx' field in the elf header. > + shdr64.sh_size = s->string_table_buf->len; > + shdr = &shdr64; > + } else { > + shdr_size = sizeof(Elf32_Shdr); > + memset(&shdr32, 0, shdr_size); > + shdr32.sh_type = SHT_STRTAB; > + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr32.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); > + shdr32.sh_size = s->string_table_buf->len; > + shdr = &shdr32; > + } > + > + memcpy(buff, shdr, shdr_size); > + [snip] Also, with your patches the dump output places the headers in this ordering: [elf hdr] [section hdrs] [program hdrs] **normally** program hdrs are placed before section hdrs, but this is just a convention IIRC. Steffen
On 8/30/22 13:35, Steffen Eiden wrote: > Hi Janosch, > > On 8/11/22 14:11, Janosch Frank wrote: >> As sections don't have a type like the notes do we need another way to >> 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 | 71 +++++++++++++++++++++++++++++++++++++++++++ >> include/sysemu/dump.h | 4 +++ >> 2 files changed, 75 insertions(+) >> >> diff --git a/dump/dump.c b/dump/dump.c >> index 31eb20108c..0d6dbf453a 100644 >> --- a/dump/dump.c >> +++ b/dump/dump.c > [ snip ] >> } >> >> +static void prepare_elf_section_hdr_string(DumpState *s, void *buff) >> +{ >> + Elf32_Shdr shdr32; >> + Elf64_Shdr shdr64; >> + int shdr_size; >> + void *shdr; >> + >> + if (dump_is_64bit(s)) { >> + shdr_size = sizeof(Elf64_Shdr); >> + memset(&shdr64, 0, shdr_size); >> + shdr64.sh_type = SHT_STRTAB; >> + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; >> + shdr64.sh_name = s->string_table_buf->len; >> + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); > I think you mixed up .strtab and .shstrtab here. > '.shstrtab' should be used here. > > The ELF specs define bots as follows (from man 5 elf) : > > .shstrtab > This section holds section names. This section is of type > SHT_STRTAB. No attribute types are used. > > .strtab > This section holds strings, most commonly the strings that > represent the names associated with symbol table entries. > If the file has a loadable segment that includes the > symbol string table, the section's attributes will include > the SHF_ALLOC bit. Otherwise, the bit will be off. This > section is of type SHT_STRTAB. > > However, the name lookup works, as you correctly specified that this > section holds the section header names via the 'e_shstrndx' field in the > elf header. Sigh We can make this a shstrtab only strtab since that's effectively what it does right now. It annoys me that we'll need a second strtab if we ever want to name other structures. Or at least we'll need special handling. > >> + shdr64.sh_size = s->string_table_buf->len; >> + shdr = &shdr64; >> + } else { >> + shdr_size = sizeof(Elf32_Shdr); >> + memset(&shdr32, 0, shdr_size); >> + shdr32.sh_type = SHT_STRTAB; >> + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; >> + shdr32.sh_name = s->string_table_buf->len; >> + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); >> + shdr32.sh_size = s->string_table_buf->len; >> + shdr = &shdr32; >> + } >> + >> + memcpy(buff, shdr, shdr_size); >> + > [snip] > Also, with your patches the dump output places the headers in this ordering: > [elf hdr] > [section hdrs] > [program hdrs] > > **normally** program hdrs are placed before section hdrs, > but this is just a convention IIRC. I don't see why this should be a problem, that's what the offsets are for. > > > Steffen >
On Thu, 2022-08-11 at 12:11 +0000, Janosch Frank wrote: > As sections don't have a type like the notes do we need another way to > 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. Why? Is there any harm in always having a (possibly empty) string table? Can't put garbage into sh_name then but that's not relevant. Seems like it would make the code a bit simpler. I would also expect the string data to be written in this patch, instead you do that in the next. With that and Steffen's .shstrtab addressed: Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> Some minor suggestions below. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > dump/dump.c | 71 +++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/dump.h | 4 +++ > 2 files changed, 75 insertions(+) > > diff --git a/dump/dump.c b/dump/dump.c > index 31eb20108c..0d6dbf453a 100644 > --- a/dump/dump.c > +++ b/dump/dump.c [...] > @@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s) > } > } > > +static void prepare_elf_section_hdr_string(DumpState *s, void *buff) > +{ > + Elf32_Shdr shdr32; > + Elf64_Shdr shdr64; Could just = {} those and drop the memset below. > + int shdr_size; > + void *shdr; > + > + if (dump_is_64bit(s)) { > + shdr_size = sizeof(Elf64_Shdr); > + memset(&shdr64, 0, shdr_size); > + shdr64.sh_type = SHT_STRTAB; > + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr64.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); Could put the ".shstrtab" in a char[] variable. > + shdr64.sh_size = s->string_table_buf->len; > + shdr = &shdr64; > + } else { > + shdr_size = sizeof(Elf32_Shdr); > + memset(&shdr32, 0, shdr_size); > + shdr32.sh_type = SHT_STRTAB; > + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; > + shdr32.sh_name = s->string_table_buf->len; > + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); > + shdr32.sh_size = s->string_table_buf->len; > + shdr = &shdr32; > + } > + > + memcpy(buff, shdr, shdr_size); > +} [...] > @@ -1844,6 +1903,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 What is the elf_section_data_size thing about? > + * sizes of all parts. > + * > + * If phdr_num overflowed we have at least one section header > + * More sections/hdrs can be added by the architectures > + */ > + if (s->shdr_num > 1) { > + /* Reserve the string table */ > + s->shdr_num += 1; > + } > + > if (dump_is_64bit(s)) { > s->shdr_offset = sizeof(Elf64_Ehdr); > s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; [...]
diff --git a/dump/dump.c b/dump/dump.c index 31eb20108c..0d6dbf453a 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -103,6 +103,7 @@ static int dump_cleanup(DumpState *s) memory_mapping_list_free(&s->list); close(s->fd); g_free(s->guest_note); + g_array_unref(s->string_table_buf); s->guest_note = NULL; if (s->resume) { if (s->detached) { @@ -156,6 +157,9 @@ static void prepare_elf64_header(DumpState *s, Elf64_Ehdr *elf_header) elf_header->e_shoff = cpu_to_dump64(s, s->shdr_offset); elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf64_Shdr)); elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num); + if (s->string_table_usage) { + elf_header->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); + } } } @@ -184,6 +188,9 @@ static void prepare_elf32_header(DumpState *s, Elf32_Ehdr *elf_header) elf_header->e_shoff = cpu_to_dump32(s, s->shdr_offset); elf_header->e_shentsize = cpu_to_dump16(s, sizeof(Elf32_Shdr)); elf_header->e_shnum = cpu_to_dump16(s, s->shdr_num); + if (s->string_table_usage) { + elf_header->e_shstrndx = cpu_to_dump16(s, s->shdr_num - 1); + } } } @@ -393,17 +400,50 @@ static void prepare_elf_section_hdr_zero(DumpState *s) } } +static void prepare_elf_section_hdr_string(DumpState *s, void *buff) +{ + Elf32_Shdr shdr32; + Elf64_Shdr shdr64; + int shdr_size; + void *shdr; + + if (dump_is_64bit(s)) { + shdr_size = sizeof(Elf64_Shdr); + memset(&shdr64, 0, shdr_size); + shdr64.sh_type = SHT_STRTAB; + shdr64.sh_offset = s->section_offset + s->elf_section_data_size; + shdr64.sh_name = s->string_table_buf->len; + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); + shdr64.sh_size = s->string_table_buf->len; + shdr = &shdr64; + } else { + shdr_size = sizeof(Elf32_Shdr); + memset(&shdr32, 0, shdr_size); + shdr32.sh_type = SHT_STRTAB; + shdr32.sh_offset = s->section_offset + s->elf_section_data_size; + shdr32.sh_name = s->string_table_buf->len; + g_array_append_vals(s->string_table_buf, ".strtab", sizeof(".strtab")); + shdr32.sh_size = s->string_table_buf->len; + shdr = &shdr32; + } + + memcpy(buff, shdr, shdr_size); +} + static void prepare_elf_section_hdrs(DumpState *s) { size_t len, sizeof_shdr; + void *buff_hdr; /* * Section ordering: * - HDR zero + * - 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; /* * The first section header is ALWAYS a special initial section @@ -419,6 +459,17 @@ static void prepare_elf_section_hdrs(DumpState *s) if (s->phdr_num >= PN_XNUM) { prepare_elf_section_hdr_zero(s); } + buff_hdr += sizeof_shdr; + + if (s->shdr_num < 2) { + return; + } + + /* + * String table is the last section since strings are added via + * arch_sections_write_hdr(). + */ + prepare_elf_section_hdr_string(s, buff_hdr); } static void write_elf_section_headers(DumpState *s, Error **errp) @@ -1688,6 +1739,14 @@ static void dump_init(DumpState *s, int fd, bool has_format, s->filter_area_begin = begin; s->filter_area_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); guest_phys_blocks_init(&s->guest_phys_blocks); @@ -1844,6 +1903,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 + */ + if (s->shdr_num > 1) { + /* Reserve the string table */ + s->shdr_num += 1; + } + if (dump_is_64bit(s)) { s->shdr_offset = sizeof(Elf64_Ehdr); s->phdr_offset = s->shdr_offset + sizeof(Elf64_Shdr) * s->shdr_num; diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index 9ed811b313..358b038d47 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -180,6 +180,10 @@ typedef struct DumpState { hwaddr note_offset; void *elf_section_hdrs; /* Pointer to section header buffer */ + void *elf_section_data; /* Pointer to section data buffer */ + uint64_t elf_section_data_size; /* Size of section data */ + GArray *string_table_buf; /* String table data buffer */ + bool string_table_usage; /* Indicates if string table is used */ uint8_t *note_buf; /* buffer for notes */ size_t note_buf_offset; /* the writing place in note_buf */
As sections don't have a type like the notes do we need another way to 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 | 71 +++++++++++++++++++++++++++++++++++++++++++ include/sysemu/dump.h | 4 +++ 2 files changed, 75 insertions(+)