When compiling a kernel with Clang and LTO, we need to run
recordmcount on vmlinux.o with a large number of sections, which
currently fails as the program doesn't understand extended
section indexes. This change adds support for processing binaries
with >64k sections.
Signed-off-by: Sami Tolvanen <[email protected]>
---
scripts/recordmcount.h | 75 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 69 insertions(+), 6 deletions(-)
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index 74eab03e31d4..b48163864cca 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -29,6 +29,9 @@
#undef has_rel_mcount
#undef tot_relsize
#undef get_mcountsym
+#undef get_shnum
+#undef get_shstrndx
+#undef get_symindex
#undef get_sym_str_and_relp
#undef do_func
#undef Elf_Addr
@@ -58,6 +61,9 @@
# define __has_rel_mcount __has64_rel_mcount
# define has_rel_mcount has64_rel_mcount
# define tot_relsize tot64_relsize
+# define get_shnum get_shnum64
+# define get_shstrndx get_shstrndx64
+# define get_symindex get_symindex64
# define get_sym_str_and_relp get_sym_str_and_relp_64
# define do_func do64
# define get_mcountsym get_mcountsym_64
@@ -91,6 +97,9 @@
# define __has_rel_mcount __has32_rel_mcount
# define has_rel_mcount has32_rel_mcount
# define tot_relsize tot32_relsize
+# define get_shnum get_shnum32
+# define get_shstrndx get_shstrndx32
+# define get_symindex get_symindex32
# define get_sym_str_and_relp get_sym_str_and_relp_32
# define do_func do32
# define get_mcountsym get_mcountsym_32
@@ -173,6 +182,38 @@ static int MIPS_is_fake_mcount(Elf_Rel const *rp)
return is_fake;
}
+static unsigned int get_symindex(Elf_Sym const *sym, Elf32_Word const *symtab,
+ Elf32_Word const *symtab_shndx)
+{
+ unsigned long offset;
+ int index;
+
+ if (sym->st_shndx != SHN_XINDEX)
+ return w2(sym->st_shndx);
+
+ offset = (unsigned long)sym - (unsigned long)symtab;
+ index = offset / sizeof(*sym);
+
+ return w(symtab_shndx[index]);
+}
+
+static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
+{
+ if (shdr0 && !ehdr->e_shnum)
+ return w(shdr0->sh_size);
+
+ return w2(ehdr->e_shnum);
+}
+
+static int get_shstrndx(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
+{
+ if (ehdr->e_shstrndx != SHN_XINDEX)
+ return w2(ehdr->e_shstrndx);
+
+ return w(shdr0->sh_link);
+}
+
+
/* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
static int append_func(Elf_Ehdr *const ehdr,
Elf_Shdr *const shstr,
@@ -188,10 +229,12 @@ static int append_func(Elf_Ehdr *const ehdr,
char const *mc_name = (sizeof(Elf_Rela) == rel_entsize)
? ".rela__mcount_loc"
: ".rel__mcount_loc";
- unsigned const old_shnum = w2(ehdr->e_shnum);
uint_t const old_shoff = _w(ehdr->e_shoff);
uint_t const old_shstr_sh_size = _w(shstr->sh_size);
uint_t const old_shstr_sh_offset = _w(shstr->sh_offset);
+ Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
+ unsigned const old_shnum = get_shnum(ehdr, shdr0);
+ unsigned const new_shnum = 2 + old_shnum; /* {.rel,}__mcount_loc */
uint_t t = 1 + strlen(mc_name) + _w(shstr->sh_size);
uint_t new_e_shoff;
@@ -201,6 +244,12 @@ static int append_func(Elf_Ehdr *const ehdr,
t += (_align & -t); /* word-byte align */
new_e_shoff = t;
+ if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
+ ehdr->e_shnum = 0;
+ shdr0->sh_size = w(new_shnum);
+ } else
+ ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
+
/* body for new shstrtab */
if (ulseek(sb.st_size, SEEK_SET) < 0)
return -1;
@@ -255,7 +304,6 @@ static int append_func(Elf_Ehdr *const ehdr,
return -1;
ehdr->e_shoff = _w(new_e_shoff);
- ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /* {.rel,}__mcount_loc */
if (ulseek(0, SEEK_SET) < 0)
return -1;
if (uwrite(ehdr, sizeof(*ehdr)) < 0)
@@ -434,6 +482,8 @@ static int find_secsym_ndx(unsigned const txtndx,
uint_t *const recvalp,
unsigned int *sym_index,
Elf_Shdr const *const symhdr,
+ Elf32_Word const *symtab,
+ Elf32_Word const *symtab_shndx,
Elf_Ehdr const *const ehdr)
{
Elf_Sym const *const sym0 = (Elf_Sym const *)(_w(symhdr->sh_offset)
@@ -445,7 +495,7 @@ static int find_secsym_ndx(unsigned const txtndx,
for (symp = sym0, t = nsym; t; --t, ++symp) {
unsigned int const st_bind = ELF_ST_BIND(symp->st_info);
- if (txtndx == w2(symp->st_shndx)
+ if (txtndx == get_symindex(symp, symtab, symtab_shndx)
/* avoid STB_WEAK */
&& (STB_LOCAL == st_bind || STB_GLOBAL == st_bind)) {
/* function symbols on ARM have quirks, avoid them */
@@ -516,21 +566,23 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
return totrelsz;
}
-
/* Overall supervision for Elf32 ET_REL file. */
static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
unsigned const reltype)
{
Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
+ (void *)ehdr);
- unsigned const nhdr = w2(ehdr->e_shnum);
- Elf_Shdr *const shstr = &shdr0[w2(ehdr->e_shstrndx)];
+ unsigned const nhdr = get_shnum(ehdr, shdr0);
+ Elf_Shdr *const shstr = &shdr0[get_shstrndx(ehdr, shdr0)];
char const *const shstrtab = (char const *)(_w(shstr->sh_offset)
+ (void *)ehdr);
Elf_Shdr const *relhdr;
unsigned k;
+ Elf32_Word *symtab = NULL;
+ Elf32_Word *symtab_shndx = NULL;
+
/* Upper bound on space: assume all relevant relocs are for mcount. */
unsigned totrelsz;
@@ -561,6 +613,16 @@ static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
return -1;
}
+ for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
+ if (relhdr->sh_type == SHT_SYMTAB)
+ symtab = (void *)ehdr + relhdr->sh_offset;
+ else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
+ symtab_shndx = (void *)ehdr + relhdr->sh_offset;
+
+ if (symtab && symtab_shndx)
+ break;
+ }
+
for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
char const *const txtname = has_rel_mcount(relhdr, shdr0,
shstrtab, fname);
@@ -577,6 +639,7 @@ static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
result = find_secsym_ndx(w(relhdr->sh_info), txtname,
&recval, &recsym,
&shdr0[symsec_sh_link],
+ symtab, symtab_shndx,
ehdr);
if (result)
goto out;
base-commit: c578ddb39e565139897124e74e5a43e56538cb33
--
2.26.1.301.g55bc3eb7cb9-goog
On Wed, 22 Apr 2020 16:24:17 -0700
Sami Tolvanen <[email protected]> wrote:
> When compiling a kernel with Clang and LTO, we need to run
> recordmcount on vmlinux.o with a large number of sections, which
> currently fails as the program doesn't understand extended
> section indexes. This change adds support for processing binaries
> with >64k sections.
Thanks! As you have also Cc'd Matt Helsley, perhaps you have noticed we
are trying to get this code merged with objtool.
How would that affect this?
-- Steve
>
> Signed-off-by: Sami Tolvanen <[email protected]>
> ---
> scripts/recordmcount.h | 75
> ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 69
> insertions(+), 6 deletions(-)
>
> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
> index 74eab03e31d4..b48163864cca 100644
> --- a/scripts/recordmcount.h
> +++ b/scripts/recordmcount.h
> @@ -29,6 +29,9 @@
> #undef has_rel_mcount
> #undef tot_relsize
> #undef get_mcountsym
> +#undef get_shnum
> +#undef get_shstrndx
> +#undef get_symindex
> #undef get_sym_str_and_relp
> #undef do_func
> #undef Elf_Addr
> @@ -58,6 +61,9 @@
> # define __has_rel_mcount __has64_rel_mcount
> # define has_rel_mcount has64_rel_mcount
> # define tot_relsize tot64_relsize
> +# define get_shnum get_shnum64
> +# define get_shstrndx get_shstrndx64
> +# define get_symindex get_symindex64
> # define get_sym_str_and_relp get_sym_str_and_relp_64
> # define do_func do64
> # define get_mcountsym get_mcountsym_64
> @@ -91,6 +97,9 @@
> # define __has_rel_mcount __has32_rel_mcount
> # define has_rel_mcount has32_rel_mcount
> # define tot_relsize tot32_relsize
> +# define get_shnum get_shnum32
> +# define get_shstrndx get_shstrndx32
> +# define get_symindex get_symindex32
> # define get_sym_str_and_relp get_sym_str_and_relp_32
> # define do_func do32
> # define get_mcountsym get_mcountsym_32
> @@ -173,6 +182,38 @@ static int MIPS_is_fake_mcount(Elf_Rel const *rp)
> return is_fake;
> }
>
> +static unsigned int get_symindex(Elf_Sym const *sym, Elf32_Word
> const *symtab,
> + Elf32_Word const *symtab_shndx)
> +{
> + unsigned long offset;
> + int index;
> +
> + if (sym->st_shndx != SHN_XINDEX)
> + return w2(sym->st_shndx);
> +
> + offset = (unsigned long)sym - (unsigned long)symtab;
> + index = offset / sizeof(*sym);
> +
> + return w(symtab_shndx[index]);
> +}
> +
> +static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const
> *shdr0) +{
> + if (shdr0 && !ehdr->e_shnum)
> + return w(shdr0->sh_size);
> +
> + return w2(ehdr->e_shnum);
> +}
> +
> +static int get_shstrndx(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
> +{
> + if (ehdr->e_shstrndx != SHN_XINDEX)
> + return w2(ehdr->e_shstrndx);
> +
> + return w(shdr0->sh_link);
> +}
> +
> +
> /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its
> relocations. */ static int append_func(Elf_Ehdr *const ehdr,
> Elf_Shdr *const shstr,
> @@ -188,10 +229,12 @@ static int append_func(Elf_Ehdr *const ehdr,
> char const *mc_name = (sizeof(Elf_Rela) == rel_entsize)
> ? ".rela__mcount_loc"
> : ".rel__mcount_loc";
> - unsigned const old_shnum = w2(ehdr->e_shnum);
> uint_t const old_shoff = _w(ehdr->e_shoff);
> uint_t const old_shstr_sh_size = _w(shstr->sh_size);
> uint_t const old_shstr_sh_offset = _w(shstr->sh_offset);
> + Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void
> *)ehdr);
> + unsigned const old_shnum = get_shnum(ehdr, shdr0);
> + unsigned const new_shnum = 2 + old_shnum; /*
> {.rel,}__mcount_loc */ uint_t t = 1 + strlen(mc_name) +
> _w(shstr->sh_size); uint_t new_e_shoff;
>
> @@ -201,6 +244,12 @@ static int append_func(Elf_Ehdr *const ehdr,
> t += (_align & -t); /* word-byte align */
> new_e_shoff = t;
>
> + if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
> + ehdr->e_shnum = 0;
> + shdr0->sh_size = w(new_shnum);
> + } else
> + ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
> +
> /* body for new shstrtab */
> if (ulseek(sb.st_size, SEEK_SET) < 0)
> return -1;
> @@ -255,7 +304,6 @@ static int append_func(Elf_Ehdr *const ehdr,
> return -1;
>
> ehdr->e_shoff = _w(new_e_shoff);
> - ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /*
> {.rel,}__mcount_loc */ if (ulseek(0, SEEK_SET) < 0)
> return -1;
> if (uwrite(ehdr, sizeof(*ehdr)) < 0)
> @@ -434,6 +482,8 @@ static int find_secsym_ndx(unsigned const txtndx,
> uint_t *const recvalp,
> unsigned int *sym_index,
> Elf_Shdr const *const symhdr,
> + Elf32_Word const *symtab,
> + Elf32_Word const *symtab_shndx,
> Elf_Ehdr const *const ehdr)
> {
> Elf_Sym const *const sym0 = (Elf_Sym const
> *)(_w(symhdr->sh_offset) @@ -445,7 +495,7 @@ static int
> find_secsym_ndx(unsigned const txtndx, for (symp = sym0, t = nsym; t;
> --t, ++symp) { unsigned int const st_bind =
> ELF_ST_BIND(symp->st_info);
> - if (txtndx == w2(symp->st_shndx)
> + if (txtndx == get_symindex(symp, symtab,
> symtab_shndx) /* avoid STB_WEAK */
> && (STB_LOCAL == st_bind || STB_GLOBAL ==
> st_bind)) { /* function symbols on ARM have quirks, avoid them */
> @@ -516,21 +566,23 @@ static unsigned tot_relsize(Elf_Shdr const
> *const shdr0, return totrelsz;
> }
>
> -
> /* Overall supervision for Elf32 ET_REL file. */
> static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
> unsigned const reltype)
> {
> Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
> + (void *)ehdr);
> - unsigned const nhdr = w2(ehdr->e_shnum);
> - Elf_Shdr *const shstr = &shdr0[w2(ehdr->e_shstrndx)];
> + unsigned const nhdr = get_shnum(ehdr, shdr0);
> + Elf_Shdr *const shstr = &shdr0[get_shstrndx(ehdr, shdr0)];
> char const *const shstrtab = (char const
> *)(_w(shstr->sh_offset)
> + (void *)ehdr);
>
> Elf_Shdr const *relhdr;
> unsigned k;
>
> + Elf32_Word *symtab = NULL;
> + Elf32_Word *symtab_shndx = NULL;
> +
> /* Upper bound on space: assume all relevant relocs are for
> mcount. */ unsigned totrelsz;
>
> @@ -561,6 +613,16 @@ static int do_func(Elf_Ehdr *const ehdr, char
> const *const fname, return -1;
> }
>
> + for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> + if (relhdr->sh_type == SHT_SYMTAB)
> + symtab = (void *)ehdr + relhdr->sh_offset;
> + else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
> + symtab_shndx = (void *)ehdr +
> relhdr->sh_offset; +
> + if (symtab && symtab_shndx)
> + break;
> + }
> +
> for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> char const *const txtname = has_rel_mcount(relhdr,
> shdr0, shstrtab, fname);
> @@ -577,6 +639,7 @@ static int do_func(Elf_Ehdr *const ehdr, char
> const *const fname, result = find_secsym_ndx(w(relhdr->sh_info),
> txtname, &recval, &recsym,
> &shdr0[symsec_sh_link],
> + symtab, symtab_shndx,
> ehdr);
> if (result)
> goto out;
>
> base-commit: c578ddb39e565139897124e74e5a43e56538cb33
On Wed, Apr 22, 2020 at 08:05:32PM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 16:24:17 -0700
> Sami Tolvanen <[email protected]> wrote:
>
> > When compiling a kernel with Clang and LTO, we need to run
> > recordmcount on vmlinux.o with a large number of sections, which
> > currently fails as the program doesn't understand extended
> > section indexes. This change adds support for processing binaries
> > with >64k sections.
>
> Thanks! As you have also Cc'd Matt Helsley, perhaps you have noticed we
> are trying to get this code merged with objtool.
>
> How would that affect this?
Yes, I saw the patches. If you are copying over code, a similar fix
might be needed, but otherwise I don't see any issues. I also sent a
patch for objtool a couple of days ago to support >64k sections:
https://lore.kernel.org/lkml/[email protected]/
Sami
On Wed, Apr 22, 2020 at 04:24:17PM -0700, Sami Tolvanen wrote:
> When compiling a kernel with Clang and LTO, we need to run
> recordmcount on vmlinux.o with a large number of sections, which
> currently fails as the program doesn't understand extended
> section indexes. This change adds support for processing binaries
> with >64k sections.
In addition to Clang + LTO I wonder if use of -ffunction-sections
these days is more likely to push us over SHN_LORESERVE even on
individual .o files. If so then this may also be necessary to prepare
for other patches being proposed such as (off the top of my head)
some KASLR changes being discussed.
>
> Signed-off-by: Sami Tolvanen <[email protected]>
> ---
> scripts/recordmcount.h | 75 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
> index 74eab03e31d4..b48163864cca 100644
> --- a/scripts/recordmcount.h
> +++ b/scripts/recordmcount.h
> @@ -29,6 +29,9 @@
> #undef has_rel_mcount
> #undef tot_relsize
> #undef get_mcountsym
> +#undef get_shnum
> +#undef get_shstrndx
> +#undef get_symindex
> #undef get_sym_str_and_relp
> #undef do_func
> #undef Elf_Addr
> @@ -58,6 +61,9 @@
> # define __has_rel_mcount __has64_rel_mcount
> # define has_rel_mcount has64_rel_mcount
> # define tot_relsize tot64_relsize
> +# define get_shnum get_shnum64
> +# define get_shstrndx get_shstrndx64
> +# define get_symindex get_symindex64
> # define get_sym_str_and_relp get_sym_str_and_relp_64
> # define do_func do64
> # define get_mcountsym get_mcountsym_64
> @@ -91,6 +97,9 @@
> # define __has_rel_mcount __has32_rel_mcount
> # define has_rel_mcount has32_rel_mcount
> # define tot_relsize tot32_relsize
> +# define get_shnum get_shnum32
> +# define get_shstrndx get_shstrndx32
> +# define get_symindex get_symindex32
> # define get_sym_str_and_relp get_sym_str_and_relp_32
> # define do_func do32
> # define get_mcountsym get_mcountsym_32
> @@ -173,6 +182,38 @@ static int MIPS_is_fake_mcount(Elf_Rel const *rp)
> return is_fake;
> }
>
I like these extra helpers. When we convert to objtool it will be easier
to see how they change or get removed entirely in favor of objtool data
structures or libelf accessors.
> +static unsigned int get_symindex(Elf_Sym const *sym, Elf32_Word const *symtab,
> + Elf32_Word const *symtab_shndx)
> +{
> + unsigned long offset;
> + int index;
> +
> + if (sym->st_shndx != SHN_XINDEX)
> + return w2(sym->st_shndx);
> +
> + offset = (unsigned long)sym - (unsigned long)symtab;
> + index = offset / sizeof(*sym);
> +
> + return w(symtab_shndx[index]);
> +}
> +
> +static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
I noticed this returns an unsigned int ...
> +{
> + if (shdr0 && !ehdr->e_shnum)
> + return w(shdr0->sh_size);
> +
> + return w2(ehdr->e_shnum);
> +}
> +
> +static int get_shstrndx(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
> +{
> + if (ehdr->e_shstrndx != SHN_XINDEX)
> + return w2(ehdr->e_shstrndx);
> +
> + return w(shdr0->sh_link);
> +}
> +
> +
> /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
> static int append_func(Elf_Ehdr *const ehdr,
> Elf_Shdr *const shstr,
> @@ -188,10 +229,12 @@ static int append_func(Elf_Ehdr *const ehdr,
> char const *mc_name = (sizeof(Elf_Rela) == rel_entsize)
> ? ".rela__mcount_loc"
> : ".rel__mcount_loc";
> - unsigned const old_shnum = w2(ehdr->e_shnum);
> uint_t const old_shoff = _w(ehdr->e_shoff);
> uint_t const old_shstr_sh_size = _w(shstr->sh_size);
> uint_t const old_shstr_sh_offset = _w(shstr->sh_offset);
> + Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
> + unsigned const old_shnum = get_shnum(ehdr, shdr0);
While this is not explicitly called out as an unsigned int. Perhaps we
could just make this and new_shnum explicit unsigned ints and then...
> + unsigned const new_shnum = 2 + old_shnum; /* {.rel,}__mcount_loc */
> uint_t t = 1 + strlen(mc_name) + _w(shstr->sh_size);
> uint_t new_e_shoff;
>
> @@ -201,6 +244,12 @@ static int append_func(Elf_Ehdr *const ehdr,
> t += (_align & -t); /* word-byte align */
> new_e_shoff = t;
>
> + if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
> + ehdr->e_shnum = 0;
> + shdr0->sh_size = w(new_shnum);
> + } else
> + ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
If we make the unsigned int change proposed above can we reuse new_shnum
here like so:
ehdr->e_shnum = w2(new_shnum);
So this if/else is doing the inverse of get_shnum(). I think the code
could be cleaned up a little and prepare for moving to objtool by
putting it in a helper function.
> +
> /* body for new shstrtab */
> if (ulseek(sb.st_size, SEEK_SET) < 0)
> return -1;
> @@ -255,7 +304,6 @@ static int append_func(Elf_Ehdr *const ehdr,
> return -1;
>
> ehdr->e_shoff = _w(new_e_shoff);
> - ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /* {.rel,}__mcount_loc */
> if (ulseek(0, SEEK_SET) < 0)
> return -1;
> if (uwrite(ehdr, sizeof(*ehdr)) < 0)
> @@ -434,6 +482,8 @@ static int find_secsym_ndx(unsigned const txtndx,
> uint_t *const recvalp,
> unsigned int *sym_index,
> Elf_Shdr const *const symhdr,
> + Elf32_Word const *symtab,
> + Elf32_Word const *symtab_shndx,
> Elf_Ehdr const *const ehdr)
> {
> Elf_Sym const *const sym0 = (Elf_Sym const *)(_w(symhdr->sh_offset)
> @@ -445,7 +495,7 @@ static int find_secsym_ndx(unsigned const txtndx,
> for (symp = sym0, t = nsym; t; --t, ++symp) {
> unsigned int const st_bind = ELF_ST_BIND(symp->st_info);
>
> - if (txtndx == w2(symp->st_shndx)
> + if (txtndx == get_symindex(symp, symtab, symtab_shndx)
> /* avoid STB_WEAK */
> && (STB_LOCAL == st_bind || STB_GLOBAL == st_bind)) {
> /* function symbols on ARM have quirks, avoid them */
> @@ -516,21 +566,23 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
> return totrelsz;
> }
>
> -
> /* Overall supervision for Elf32 ET_REL file. */
> static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
> unsigned const reltype)
> {
> Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
> + (void *)ehdr);
> - unsigned const nhdr = w2(ehdr->e_shnum);
> - Elf_Shdr *const shstr = &shdr0[w2(ehdr->e_shstrndx)];
> + unsigned const nhdr = get_shnum(ehdr, shdr0);
> + Elf_Shdr *const shstr = &shdr0[get_shstrndx(ehdr, shdr0)];
> char const *const shstrtab = (char const *)(_w(shstr->sh_offset)
> + (void *)ehdr);
>
> Elf_Shdr const *relhdr;
> unsigned k;
>
> + Elf32_Word *symtab = NULL;
> + Elf32_Word *symtab_shndx = NULL;
> +
> /* Upper bound on space: assume all relevant relocs are for mcount. */
> unsigned totrelsz;
>
> @@ -561,6 +613,16 @@ static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
> return -1;
> }
>
> + for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> + if (relhdr->sh_type == SHT_SYMTAB)
> + symtab = (void *)ehdr + relhdr->sh_offset;
> + else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
> + symtab_shndx = (void *)ehdr + relhdr->sh_offset;
> +
> + if (symtab && symtab_shndx)
> + break;
> + }
Could you break this out into a helper function? find_symtab() maybe? Again, I think
that helper would go away with conversion to objtool.
(and even if we don't convert to objtool I think the helpers will make
the code easier to read)
Cheers,
-Matt Helsley
Hi Matt,
On Thu, Apr 23, 2020 at 02:47:34PM -0700, Matt Helsley wrote:
> > +static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
>
> I noticed this returns an unsigned int ...
>
> > + Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
> > + unsigned const old_shnum = get_shnum(ehdr, shdr0);
>
> While this is not explicitly called out as an unsigned int. Perhaps we
> could just make this and new_shnum explicit unsigned ints and then...
> > + if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
> > + ehdr->e_shnum = 0;
> > + shdr0->sh_size = w(new_shnum);
> > + } else
> > + ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
>
> If we make the unsigned int change proposed above can we reuse new_shnum
> here like so:
> ehdr->e_shnum = w2(new_shnum);
>
> So this if/else is doing the inverse of get_shnum(). I think the code
> could be cleaned up a little and prepare for moving to objtool by
> putting it in a helper function.
Sure, sounds good to me.
> > + for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> > + if (relhdr->sh_type == SHT_SYMTAB)
> > + symtab = (void *)ehdr + relhdr->sh_offset;
> > + else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
> > + symtab_shndx = (void *)ehdr + relhdr->sh_offset;
> > +
> > + if (symtab && symtab_shndx)
> > + break;
> > + }
>
> Could you break this out into a helper function? find_symtab() maybe? Again, I think
> that helper would go away with conversion to objtool.
Agreed, this wouldn't be needed with libelf. I'll send v2 shortly.
Thanks for the review!
Sami
When compiling a kernel with Clang and LTO, we need to run
recordmcount on vmlinux.o with a large number of sections, which
currently fails as the program doesn't understand extended
section indexes. This change adds support for processing binaries
with >64k sections.
Signed-off-by: Sami Tolvanen <[email protected]>
---
Changes in v2:
- Switched to unsigned int for (old|new)_shnum in append_func.
- Added set_shnum and find_symtab helper functions and moved
the new logic there.
---
scripts/recordmcount.h | 98 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 92 insertions(+), 6 deletions(-)
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index 74eab03e31d4..f9b19524da11 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -29,6 +29,11 @@
#undef has_rel_mcount
#undef tot_relsize
#undef get_mcountsym
+#undef find_symtab
+#undef get_shnum
+#undef set_shnum
+#undef get_shstrndx
+#undef get_symindex
#undef get_sym_str_and_relp
#undef do_func
#undef Elf_Addr
@@ -58,6 +63,11 @@
# define __has_rel_mcount __has64_rel_mcount
# define has_rel_mcount has64_rel_mcount
# define tot_relsize tot64_relsize
+# define find_symtab find_symtab64
+# define get_shnum get_shnum64
+# define set_shnum set_shnum64
+# define get_shstrndx get_shstrndx64
+# define get_symindex get_symindex64
# define get_sym_str_and_relp get_sym_str_and_relp_64
# define do_func do64
# define get_mcountsym get_mcountsym_64
@@ -91,6 +101,11 @@
# define __has_rel_mcount __has32_rel_mcount
# define has_rel_mcount has32_rel_mcount
# define tot_relsize tot32_relsize
+# define find_symtab find_symtab32
+# define get_shnum get_shnum32
+# define set_shnum set_shnum32
+# define get_shstrndx get_shstrndx32
+# define get_symindex get_symindex32
# define get_sym_str_and_relp get_sym_str_and_relp_32
# define do_func do32
# define get_mcountsym get_mcountsym_32
@@ -173,6 +188,67 @@ static int MIPS_is_fake_mcount(Elf_Rel const *rp)
return is_fake;
}
+static unsigned int get_symindex(Elf_Sym const *sym, Elf32_Word const *symtab,
+ Elf32_Word const *symtab_shndx)
+{
+ unsigned long offset;
+ int index;
+
+ if (sym->st_shndx != SHN_XINDEX)
+ return w2(sym->st_shndx);
+
+ offset = (unsigned long)sym - (unsigned long)symtab;
+ index = offset / sizeof(*sym);
+
+ return w(symtab_shndx[index]);
+}
+
+static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
+{
+ if (shdr0 && !ehdr->e_shnum)
+ return w(shdr0->sh_size);
+
+ return w2(ehdr->e_shnum);
+}
+
+static void set_shnum(Elf_Ehdr *ehdr, Elf_Shdr *shdr0, unsigned int new_shnum)
+{
+ if (new_shnum >= SHN_LORESERVE) {
+ ehdr->e_shnum = 0;
+ shdr0->sh_size = w(new_shnum);
+ } else
+ ehdr->e_shnum = w2(new_shnum);
+}
+
+static int get_shstrndx(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
+{
+ if (ehdr->e_shstrndx != SHN_XINDEX)
+ return w2(ehdr->e_shstrndx);
+
+ return w(shdr0->sh_link);
+}
+
+static void find_symtab(Elf_Ehdr *const ehdr, Elf_Shdr const *shdr0,
+ unsigned const nhdr, Elf32_Word **symtab,
+ Elf32_Word **symtab_shndx)
+{
+ Elf_Shdr const *relhdr;
+ unsigned k;
+
+ *symtab = NULL;
+ *symtab_shndx = NULL;
+
+ for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
+ if (relhdr->sh_type == SHT_SYMTAB)
+ *symtab = (void *)ehdr + relhdr->sh_offset;
+ else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
+ *symtab_shndx = (void *)ehdr + relhdr->sh_offset;
+
+ if (*symtab && *symtab_shndx)
+ break;
+ }
+}
+
/* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
static int append_func(Elf_Ehdr *const ehdr,
Elf_Shdr *const shstr,
@@ -188,10 +264,12 @@ static int append_func(Elf_Ehdr *const ehdr,
char const *mc_name = (sizeof(Elf_Rela) == rel_entsize)
? ".rela__mcount_loc"
: ".rel__mcount_loc";
- unsigned const old_shnum = w2(ehdr->e_shnum);
uint_t const old_shoff = _w(ehdr->e_shoff);
uint_t const old_shstr_sh_size = _w(shstr->sh_size);
uint_t const old_shstr_sh_offset = _w(shstr->sh_offset);
+ Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
+ unsigned int const old_shnum = get_shnum(ehdr, shdr0);
+ unsigned int const new_shnum = 2 + old_shnum; /* {.rel,}__mcount_loc */
uint_t t = 1 + strlen(mc_name) + _w(shstr->sh_size);
uint_t new_e_shoff;
@@ -201,6 +279,8 @@ static int append_func(Elf_Ehdr *const ehdr,
t += (_align & -t); /* word-byte align */
new_e_shoff = t;
+ set_shnum(ehdr, shdr0, new_shnum);
+
/* body for new shstrtab */
if (ulseek(sb.st_size, SEEK_SET) < 0)
return -1;
@@ -255,7 +335,6 @@ static int append_func(Elf_Ehdr *const ehdr,
return -1;
ehdr->e_shoff = _w(new_e_shoff);
- ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /* {.rel,}__mcount_loc */
if (ulseek(0, SEEK_SET) < 0)
return -1;
if (uwrite(ehdr, sizeof(*ehdr)) < 0)
@@ -434,6 +513,8 @@ static int find_secsym_ndx(unsigned const txtndx,
uint_t *const recvalp,
unsigned int *sym_index,
Elf_Shdr const *const symhdr,
+ Elf32_Word const *symtab,
+ Elf32_Word const *symtab_shndx,
Elf_Ehdr const *const ehdr)
{
Elf_Sym const *const sym0 = (Elf_Sym const *)(_w(symhdr->sh_offset)
@@ -445,7 +526,7 @@ static int find_secsym_ndx(unsigned const txtndx,
for (symp = sym0, t = nsym; t; --t, ++symp) {
unsigned int const st_bind = ELF_ST_BIND(symp->st_info);
- if (txtndx == w2(symp->st_shndx)
+ if (txtndx == get_symindex(symp, symtab, symtab_shndx)
/* avoid STB_WEAK */
&& (STB_LOCAL == st_bind || STB_GLOBAL == st_bind)) {
/* function symbols on ARM have quirks, avoid them */
@@ -516,21 +597,23 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
return totrelsz;
}
-
/* Overall supervision for Elf32 ET_REL file. */
static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
unsigned const reltype)
{
Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
+ (void *)ehdr);
- unsigned const nhdr = w2(ehdr->e_shnum);
- Elf_Shdr *const shstr = &shdr0[w2(ehdr->e_shstrndx)];
+ unsigned const nhdr = get_shnum(ehdr, shdr0);
+ Elf_Shdr *const shstr = &shdr0[get_shstrndx(ehdr, shdr0)];
char const *const shstrtab = (char const *)(_w(shstr->sh_offset)
+ (void *)ehdr);
Elf_Shdr const *relhdr;
unsigned k;
+ Elf32_Word *symtab;
+ Elf32_Word *symtab_shndx;
+
/* Upper bound on space: assume all relevant relocs are for mcount. */
unsigned totrelsz;
@@ -561,6 +644,8 @@ static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
return -1;
}
+ find_symtab(ehdr, shdr0, nhdr, &symtab, &symtab_shndx);
+
for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
char const *const txtname = has_rel_mcount(relhdr, shdr0,
shstrtab, fname);
@@ -577,6 +662,7 @@ static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
result = find_secsym_ndx(w(relhdr->sh_info), txtname,
&recval, &recsym,
&shdr0[symsec_sh_link],
+ symtab, symtab_shndx,
ehdr);
if (result)
goto out;
base-commit: b4f633221f0aeac102e463a4be46a643b2e3b819
--
2.26.2.303.gf8c07b1a785-goog
On Fri, Apr 24, 2020 at 12:30:46PM -0700, Sami Tolvanen wrote:
> When compiling a kernel with Clang and LTO, we need to run
> recordmcount on vmlinux.o with a large number of sections, which
> currently fails as the program doesn't understand extended
> section indexes. This change adds support for processing binaries
> with >64k sections.
>
> Signed-off-by: Sami Tolvanen <[email protected]>
Feel free to add
Reviewed-by: Matt Helsley <[email protected]>
> ---
> Changes in v2:
> - Switched to unsigned int for (old|new)_shnum in append_func.
> - Added set_shnum and find_symtab helper functions and moved
> the new logic there.
>
> ---
> scripts/recordmcount.h | 98 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 92 insertions(+), 6 deletions(-)
<snip>
On Fri, Apr 24, 2020 at 03:22:14PM -0700, Matt Helsley wrote:
> On Fri, Apr 24, 2020 at 12:30:46PM -0700, Sami Tolvanen wrote:
> > When compiling a kernel with Clang and LTO, we need to run
> > recordmcount on vmlinux.o with a large number of sections, which
> > currently fails as the program doesn't understand extended
> > section indexes. This change adds support for processing binaries
> > with >64k sections.
> >
> > Signed-off-by: Sami Tolvanen <[email protected]>
>
> Feel free to add
>
> Reviewed-by: Matt Helsley <[email protected]>
Hi!
Can this patch please be applied and sent before -rc2? FGKASLR, LTO, and
link time improvements[1] all depend on this fix, and I'd really like
them all to be able to sanely rebase for the development window.
Thanks!
-Kees
[1] https://lore.kernel.org/lkml/CAK7LNARbZhoaA=Nnuw0=gBrkuKbr_4Ng_Ei57uafujZf7Xazgw@mail.gmail.com/
>
> > ---
> > Changes in v2:
> > - Switched to unsigned int for (old|new)_shnum in append_func.
> > - Added set_shnum and find_symtab helper functions and moved
> > the new logic there.
> >
> > ---
> > scripts/recordmcount.h | 98 +++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 92 insertions(+), 6 deletions(-)
>
> <snip>
--
Kees Cook
On Tue, 16 Jun 2020 11:03:18 -0700
Kees Cook <[email protected]> wrote:
> > Reviewed-by: Matt Helsley <[email protected]>
>
> Hi!
>
> Can this patch please be applied and sent before -rc2? FGKASLR, LTO, and
> link time improvements[1] all depend on this fix, and I'd really like
> them all to be able to sanely rebase for the development window.
>
> Thanks!
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/CAK7LNARbZhoaA=Nnuw0=gBrkuKbr_4Ng_Ei57uafujZf7Xazgw@mail.gmail.com/
The patch seems to have fallen behind in my patch stack (unfortunately,
it's most recent first!)
Anyway, I'm putting together now a set of patches for -rc2. I'll
include this one in it as well.
-- Steve
On Tue, Jun 16, 2020 at 02:38:44PM -0400, Steven Rostedt wrote:
> On Tue, 16 Jun 2020 11:03:18 -0700
> Kees Cook <[email protected]> wrote:
>
> > > Reviewed-by: Matt Helsley <[email protected]>
> >
> > Hi!
> >
> > Can this patch please be applied and sent before -rc2? FGKASLR, LTO, and
> > link time improvements[1] all depend on this fix, and I'd really like
> > them all to be able to sanely rebase for the development window.
> >
> > Thanks!
> >
> > -Kees
> >
> > [1] https://lore.kernel.org/lkml/CAK7LNARbZhoaA=Nnuw0=gBrkuKbr_4Ng_Ei57uafujZf7Xazgw@mail.gmail.com/
>
> The patch seems to have fallen behind in my patch stack (unfortunately,
> it's most recent first!)
>
> Anyway, I'm putting together now a set of patches for -rc2. I'll
> include this one in it as well.
Awesome! Thanks very much. :)
--
Kees Cook