[PATCH] recordmcount: support >64k sections

[PATCH] recordmcount: support >64k sections

[Sami Tolvanen]

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 <samitolvanen@google.com>
---
 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

Re: [PATCH] recordmcount: support >64k sections

[Steven Rostedt]

On Wed, 22 Apr 2020 16:24:17 -0700
Sami Tolvanen <samitolvanen@google.com> 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 <samitolvanen@google.com>
> ---
>  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

Re: [PATCH] recordmcount: support >64k sections

[Sami Tolvanen]

On Wed, Apr 22, 2020 at 08:05:32PM -0400, Steven Rostedt wrote:
> On Wed, 22 Apr 2020 16:24:17 -0700
> Sami Tolvanen <samitolvanen@google.com> 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/20200421220843.188260-2-samitolvanen@google.com/

Sami

Re: [PATCH] recordmcount: support >64k sections

[Matt Helsley]

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 <samitolvanen@google.com>
> ---
>  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 

Re: [PATCH] recordmcount: support >64k sections

[Matt Helsley]

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 <samitolvanen@google.com>
> ---
>  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 

Re: [PATCH] recordmcount: support >64k sections

[Sami Tolvanen]

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

[PATCH v2] recordmcount: support >64k sections

[Sami Tolvanen]

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 <samitolvanen@google.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(-)

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

Re: [PATCH v2] recordmcount: support >64k sections

[Matt Helsley]

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 <samitolvanen@google.com>

Feel free to add

Reviewed-by: Matt Helsley <mhelsley@vmware.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>

Re: [PATCH v2] recordmcount: support >64k sections

[Matt Helsley]

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 <samitolvanen@google.com>

Feel free to add

Reviewed-by: Matt Helsley <mhelsley@vmware.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>

Re: [PATCH v2] recordmcount: support >64k sections

[Kees Cook]

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 <samitolvanen@google.com>
> 
> Feel free to add
> 
> Reviewed-by: Matt Helsley <mhelsley@vmware.com>

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

Re: [PATCH v2] recordmcount: support >64k sections

[Steven Rostedt]

On Tue, 16 Jun 2020 11:03:18 -0700
Kees Cook <keescook@chromium.org> wrote:

> > Reviewed-by: Matt Helsley <mhelsley@vmware.com>  
> 
> 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

Re: [PATCH v2] recordmcount: support >64k sections

[Kees Cook]

On Tue, Jun 16, 2020 at 02:38:44PM -0400, Steven Rostedt wrote:
> On Tue, 16 Jun 2020 11:03:18 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > > Reviewed-by: Matt Helsley <mhelsley@vmware.com>  
> > 
> > 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