[PATCHv3 0/2] libbpf: Add support to use optional extended section index table

[PATCHv3 0/2] libbpf: Add support to use optional extended section index table

[Jiri Olsa]

hi,
kpatch guys hit an issue with pahole over their vmlinux, which
contains many (over 100000) sections, pahole crashes.

With so many sections, ELF is using extended section index table,
which is used to hold values for some of the indexes and extra
code is needed to retrieve them.

This patchset adds the support for pahole to properly read string
table index and symbol's section index, which are used in btf_encoder.

This patchset also adds support for libbpf to properly parse .BTF
section on such object.

This patchset is based on previously posted fix [1].

v3 changes:
  - directly bail out for !str in elf_section_by_name [Andrii]
  - use symbol index in collect_function [Andrii] 
  - use symbol index in collect_percpu_var
  - change elf_symtab__for_each_symbol_index, move elf_sym__get
    to for's condition part
  - libbpf patch got merged

v2 changes:
  - many variables renames [Andrii]
  - use elf_getshdrstrndx() unconditionally [Andrii]
  - add elf_symtab__for_each_symbol_index macro [Andrii]
  - add more comments [Andrii]
  - verify that extended symtab section type is SHT_SYMTAB_SHNDX [Andrii]
  - fix Joe's crash in dwarves build, wrong sym.st_shndx assignment

thanks,
jirka


[1] https://lore.kernel.org/bpf/20210113102509.1338601-1-jolsa@kernel.org/
---
Jiri Olsa (2):
      elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name
      bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

 btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++++++++----------------
 dutil.c       |  8 +++++++-
 elf_symtab.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 elf_symtab.h  |  2 ++
 4 files changed, 90 insertions(+), 18 deletions(-)

[PATCH 1/2] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name

[Jiri Olsa]

In case the elf's header e_shstrndx contains SHN_XINDEX,
we need to call elf_getshdrstrndx to get the proper
string table index.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 dutil.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/dutil.c b/dutil.c
index 7b667647420f..11fb7202049c 100644
--- a/dutil.c
+++ b/dutil.c
@@ -179,12 +179,18 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 {
 	Elf_Scn *sec = NULL;
 	size_t cnt = 1;
+	size_t str_idx;
+
+	if (elf_getshdrstrndx(elf, &str_idx))
+		return NULL;
 
 	while ((sec = elf_nextscn(elf, sec)) != NULL) {
 		char *str;
 
 		gelf_getshdr(sec, shp);
-		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
+		str = elf_strptr(elf, str_idx, shp->sh_name);
+		if (!str)
+			return NULL;
 		if (!strcmp(name, str)) {
 			if (index)
 				*index = cnt;
-- 
2.26.2

[PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Jiri Olsa]

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for symbol's st_shndx.

This patch is adding code to detect the optional extended
section index table and use it to resolve symbol's section
index.

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
 elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
 elf_symtab.h  |  2 ++
 3 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 5557c9efd365..56ee55965093 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -63,13 +63,13 @@ static void delete_functions(void)
 #define max(x, y) ((x) < (y) ? (y) : (x))
 #endif
 
-static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
+static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
+			    Elf32_Word sym_sec_idx)
 {
 	struct elf_function *new;
 	static GElf_Shdr sh;
-	static int last_idx;
+	static Elf32_Word last_idx;
 	const char *name;
-	int idx;
 
 	if (elf_sym__type(sym) != STT_FUNC)
 		return 0;
@@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 		functions = new;
 	}
 
-	idx = elf_sym__section(sym);
-
-	if (idx != last_idx) {
-		if (!elf_section_by_idx(btfe->elf, &sh, idx))
+	if (sym_sec_idx != last_idx) {
+		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
 			return 0;
-		last_idx = idx;
+		last_idx = sym_sec_idx;
 	}
 
 	functions[functions_cnt].name = name;
@@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
 	return true;
 }
 
-static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
+static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
+			      Elf32_Word sym_sec_idx)
 {
 	const char *sym_name;
 	uint64_t addr;
 	uint32_t size;
 
 	/* compare a symbol's shndx to determine if it's a percpu variable */
-	if (elf_sym__section(sym) != btfe->percpu_shndx)
+	if (sym_sec_idx != btfe->percpu_shndx)
 		return 0;
 	if (elf_sym__type(sym) != STT_OBJECT)
 		return 0;
@@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
 	return 0;
 }
 
-static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
+static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
+			   Elf32_Word sym_sec_idx)
 {
 	if (!fl->mcount_start &&
 	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
 		fl->mcount_start = sym->st_value;
-		fl->mcount_sec_idx = sym->st_shndx;
+		fl->mcount_sec_idx = sym_sec_idx;
 	}
 
 	if (!fl->mcount_stop &&
@@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 		fl->mcount_stop = sym->st_value;
 }
 
+static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+			 int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+	if (!gelf_getsym(syms, id, sym))
+		return false;
+
+	*sym_sec_idx = sym->st_shndx;
+
+	if (sym->st_shndx == SHN_XINDEX) {
+		if (!syms_sec_idx_table)
+			return false;
+		if (!gelf_getsymshndx(syms, syms_sec_idx_table,
+				      id, sym, sym_sec_idx))
+			return false;
+	}
+
+	return true;
+}
+
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)		\
+	for (id = 0;								\
+	     id < symtab->nr_syms &&						\
+	     elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,		\
+			  id, &sym, &sym_sec_idx);				\
+	     id++)
+
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
 	struct funcs_layout fl = { };
+	Elf32_Word sym_sec_idx;
 	uint32_t core_id;
 	GElf_Sym sym;
 
@@ -608,12 +635,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 	percpu_var_cnt = 0;
 
 	/* search within symtab for percpu variables */
-	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
-		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
+	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
+		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
 			return -1;
-		if (collect_function(btfe, &sym))
+		if (collect_function(btfe, &sym, sym_sec_idx))
 			return -1;
-		collect_symbol(&sym, &fl);
+		collect_symbol(&sym, &fl, sym_sec_idx);
 	}
 
 	if (collect_percpu_vars) {
diff --git a/elf_symtab.c b/elf_symtab.c
index 741990ea3ed9..fad5e0c0ba3c 100644
--- a/elf_symtab.c
+++ b/elf_symtab.c
@@ -17,11 +17,13 @@
 
 struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 {
+	size_t symtab_index;
+
 	if (name == NULL)
 		name = ".symtab";
 
 	GElf_Shdr shdr;
-	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL);
+	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index);
 
 	if (sec == NULL)
 		return NULL;
@@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->syms == NULL)
 		goto out_free_name;
 
+	/*
+	 * This returns extended section index table's
+	 * section index, if it exists.
+	 */
+	int symtab_xindex = elf_scnshndx(sec);
+
 	sec = elf_getscn(elf, shdr.sh_link);
 	if (sec == NULL)
 		goto out_free_name;
@@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->symstrs == NULL)
 		goto out_free_name;
 
+	/*
+	 * The .symtab section has optional extended section index
+	 * table, load its data so it can be used to resolve symbol's
+	 * section index.
+	 **/
+	if (symtab_xindex > 0) {
+		GElf_Shdr shdr_xindex;
+		Elf_Scn *sec_xindex;
+
+		sec_xindex = elf_getscn(elf, symtab_xindex);
+		if (sec_xindex == NULL)
+			goto out_free_name;
+
+		if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL)
+			goto out_free_name;
+
+		/* Extra check to verify it's correct type */
+		if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX)
+			goto out_free_name;
+
+		/* Extra check to verify it belongs to the .symtab */
+		if (symtab_index != shdr_xindex.sh_link)
+			goto out_free_name;
+
+		symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL);
+		if (symtab->syms_sec_idx_table == NULL)
+			goto out_free_name;
+	}
+
 	symtab->nr_syms = shdr.sh_size / shdr.sh_entsize;
 
 	return symtab;
diff --git a/elf_symtab.h b/elf_symtab.h
index 359add69c8ab..2e05ca98158b 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h
@@ -16,6 +16,8 @@ struct elf_symtab {
 	uint32_t  nr_syms;
 	Elf_Data  *syms;
 	Elf_Data  *symstrs;
+	/* Data of SHT_SYMTAB_SHNDX section. */
+	Elf_Data  *syms_sec_idx_table;
 	char	  *name;
 };
 
-- 
2.26.2

Re: [PATCH 1/2] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2021 at 05:39:19PM +0100, Jiri Olsa escreveu:
> In case the elf's header e_shstrndx contains SHN_XINDEX,
> we need to call elf_getshdrstrndx to get the proper
> string table index.

Applied, but changed the changelog comment to:

------------------------------------------------------------------
elf_symtab: Handle SHN_XINDEX index in elf_section_by_name()

Use elf_getshdrstrndx() to cover the case where the ELF header
'e_shstrndx' field contains the special value SHN_XINDEX so that we get
the proper string table index.

This is necessary to handle files with over 65536 sections, such as when
building the kernel with -f[function|data]-sections.  Other cases may
include when using FG-ASLR, LTO.

With so many sections, ELF is using extended section index table, which
is used to hold values for some of the indexes and extra code is needed
to retrieve them.
------------------------------------------------------------------

This is from the thread, so that we can have a more comprehensive idea
of what is this SHN_XINDEX and where it can show up when looking at this
code 10 years from now (or next month) :-)

Holler if I've messed up something.

Thanks,

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  dutil.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/dutil.c b/dutil.c
> index 7b667647420f..11fb7202049c 100644
> --- a/dutil.c
> +++ b/dutil.c
> @@ -179,12 +179,18 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>  {
>  	Elf_Scn *sec = NULL;
>  	size_t cnt = 1;
> +	size_t str_idx;
> +
> +	if (elf_getshdrstrndx(elf, &str_idx))
> +		return NULL;
>  
>  	while ((sec = elf_nextscn(elf, sec)) != NULL) {
>  		char *str;
>  
>  		gelf_getshdr(sec, shp);
> -		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
> +		str = elf_strptr(elf, str_idx, shp->sh_name);
> +		if (!str)
> +			return NULL;
>  		if (!strcmp(name, str)) {
>  			if (index)
>  				*index = cnt;
> -- 
> 2.26.2
> 

-- 

- Arnaldo

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu:
> For very large ELF objects (with many sections), we could
> get special value SHN_XINDEX (65535) for symbol's st_shndx.
> 
> This patch is adding code to detect the optional extended
> section index table and use it to resolve symbol's section
> index.
> 
> Adding elf_symtab__for_each_symbol_index macro that returns
> symbol's section index and usign it in collect functions.

From a quick look it seems you addressed Andrii's review comments,
right?

I've merged it locally, but would like to have some detailed set of
steps on how to test this, so that I can add it to a "Committer testing"
section in the cset commit log and probably add it to my local set of
regression tests.

Who originally reported this? Joe? Also can someone provide a Tested-by:
in addition to mine when I get this detailed set of steps to test?

Thanks,

- Arnaldo
 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
>  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
>  elf_symtab.h  |  2 ++
>  3 files changed, 83 insertions(+), 17 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5557c9efd365..56ee55965093 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -63,13 +63,13 @@ static void delete_functions(void)
>  #define max(x, y) ((x) < (y) ? (y) : (x))
>  #endif
>  
> -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> +			    Elf32_Word sym_sec_idx)
>  {
>  	struct elf_function *new;
>  	static GElf_Shdr sh;
> -	static int last_idx;
> +	static Elf32_Word last_idx;
>  	const char *name;
> -	int idx;
>  
>  	if (elf_sym__type(sym) != STT_FUNC)
>  		return 0;
> @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>  		functions = new;
>  	}
>  
> -	idx = elf_sym__section(sym);
> -
> -	if (idx != last_idx) {
> -		if (!elf_section_by_idx(btfe->elf, &sh, idx))
> +	if (sym_sec_idx != last_idx) {
> +		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
>  			return 0;
> -		last_idx = idx;
> +		last_idx = sym_sec_idx;
>  	}
>  
>  	functions[functions_cnt].name = name;
> @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
>  	return true;
>  }
>  
> -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> +			      Elf32_Word sym_sec_idx)
>  {
>  	const char *sym_name;
>  	uint64_t addr;
>  	uint32_t size;
>  
>  	/* compare a symbol's shndx to determine if it's a percpu variable */
> -	if (elf_sym__section(sym) != btfe->percpu_shndx)
> +	if (sym_sec_idx != btfe->percpu_shndx)
>  		return 0;
>  	if (elf_sym__type(sym) != STT_OBJECT)
>  		return 0;
> @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
>  	return 0;
>  }
>  
> -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> +			   Elf32_Word sym_sec_idx)
>  {
>  	if (!fl->mcount_start &&
>  	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
>  		fl->mcount_start = sym->st_value;
> -		fl->mcount_sec_idx = sym->st_shndx;
> +		fl->mcount_sec_idx = sym_sec_idx;
>  	}
>  
>  	if (!fl->mcount_stop &&
> @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
>  		fl->mcount_stop = sym->st_value;
>  }
>  
> +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> +			 int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
> +{
> +	if (!gelf_getsym(syms, id, sym))
> +		return false;
> +
> +	*sym_sec_idx = sym->st_shndx;
> +
> +	if (sym->st_shndx == SHN_XINDEX) {
> +		if (!syms_sec_idx_table)
> +			return false;
> +		if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> +				      id, sym, sym_sec_idx))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)		\
> +	for (id = 0;								\
> +	     id < symtab->nr_syms &&						\
> +	     elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,		\
> +			  id, &sym, &sym_sec_idx);				\
> +	     id++)
> +
>  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  {
>  	struct funcs_layout fl = { };
> +	Elf32_Word sym_sec_idx;
>  	uint32_t core_id;
>  	GElf_Sym sym;
>  
> @@ -608,12 +635,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  	percpu_var_cnt = 0;
>  
>  	/* search within symtab for percpu variables */
> -	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> -		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
> +	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
> +		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
>  			return -1;
> -		if (collect_function(btfe, &sym))
> +		if (collect_function(btfe, &sym, sym_sec_idx))
>  			return -1;
> -		collect_symbol(&sym, &fl);
> +		collect_symbol(&sym, &fl, sym_sec_idx);
>  	}
>  
>  	if (collect_percpu_vars) {
> diff --git a/elf_symtab.c b/elf_symtab.c
> index 741990ea3ed9..fad5e0c0ba3c 100644
> --- a/elf_symtab.c
> +++ b/elf_symtab.c
> @@ -17,11 +17,13 @@
>  
>  struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  {
> +	size_t symtab_index;
> +
>  	if (name == NULL)
>  		name = ".symtab";
>  
>  	GElf_Shdr shdr;
> -	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL);
> +	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index);
>  
>  	if (sec == NULL)
>  		return NULL;
> @@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  	if (symtab->syms == NULL)
>  		goto out_free_name;
>  
> +	/*
> +	 * This returns extended section index table's
> +	 * section index, if it exists.
> +	 */
> +	int symtab_xindex = elf_scnshndx(sec);
> +
>  	sec = elf_getscn(elf, shdr.sh_link);
>  	if (sec == NULL)
>  		goto out_free_name;
> @@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>  	if (symtab->symstrs == NULL)
>  		goto out_free_name;
>  
> +	/*
> +	 * The .symtab section has optional extended section index
> +	 * table, load its data so it can be used to resolve symbol's
> +	 * section index.
> +	 **/
> +	if (symtab_xindex > 0) {
> +		GElf_Shdr shdr_xindex;
> +		Elf_Scn *sec_xindex;
> +
> +		sec_xindex = elf_getscn(elf, symtab_xindex);
> +		if (sec_xindex == NULL)
> +			goto out_free_name;
> +
> +		if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL)
> +			goto out_free_name;
> +
> +		/* Extra check to verify it's correct type */
> +		if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX)
> +			goto out_free_name;
> +
> +		/* Extra check to verify it belongs to the .symtab */
> +		if (symtab_index != shdr_xindex.sh_link)
> +			goto out_free_name;
> +
> +		symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL);
> +		if (symtab->syms_sec_idx_table == NULL)
> +			goto out_free_name;
> +	}
> +
>  	symtab->nr_syms = shdr.sh_size / shdr.sh_entsize;
>  
>  	return symtab;
> diff --git a/elf_symtab.h b/elf_symtab.h
> index 359add69c8ab..2e05ca98158b 100644
> --- a/elf_symtab.h
> +++ b/elf_symtab.h
> @@ -16,6 +16,8 @@ struct elf_symtab {
>  	uint32_t  nr_syms;
>  	Elf_Data  *syms;
>  	Elf_Data  *symstrs;
> +	/* Data of SHT_SYMTAB_SHNDX section. */
> +	Elf_Data  *syms_sec_idx_table;
>  	char	  *name;
>  };
>  
> -- 
> 2.26.2
> 

-- 

- Arnaldo

Re: [PATCH 1/2] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name

[Andrii Nakryiko]

On Fri, Jan 22, 2021 at 9:22 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> In case the elf's header e_shstrndx contains SHN_XINDEX,
> we need to call elf_getshdrstrndx to get the proper
> string table index.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  dutil.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/dutil.c b/dutil.c
> index 7b667647420f..11fb7202049c 100644
> --- a/dutil.c
> +++ b/dutil.c
> @@ -179,12 +179,18 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>  {
>         Elf_Scn *sec = NULL;
>         size_t cnt = 1;
> +       size_t str_idx;
> +
> +       if (elf_getshdrstrndx(elf, &str_idx))
> +               return NULL;
>
>         while ((sec = elf_nextscn(elf, sec)) != NULL) {
>                 char *str;
>
>                 gelf_getshdr(sec, shp);
> -               str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
> +               str = elf_strptr(elf, str_idx, shp->sh_name);
> +               if (!str)
> +                       return NULL;
>                 if (!strcmp(name, str)) {
>                         if (index)
>                                 *index = cnt;
> --
> 2.26.2
>

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Andrii Nakryiko]

On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> For very large ELF objects (with many sections), we could
> get special value SHN_XINDEX (65535) for symbol's st_shndx.
>
> This patch is adding code to detect the optional extended
> section index table and use it to resolve symbol's section
> index.
>
> Adding elf_symtab__for_each_symbol_index macro that returns
> symbol's section index and usign it in collect functions.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
>  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
>  elf_symtab.h  |  2 ++
>  3 files changed, 83 insertions(+), 17 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5557c9efd365..56ee55965093 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -63,13 +63,13 @@ static void delete_functions(void)
>  #define max(x, y) ((x) < (y) ? (y) : (x))
>  #endif
>
> -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> +                           Elf32_Word sym_sec_idx)

nit: we use size_t or int for this, no need for libelf types here, imo



>  {
>         struct elf_function *new;
>         static GElf_Shdr sh;
> -       static int last_idx;
> +       static Elf32_Word last_idx;
>         const char *name;
> -       int idx;
>
>         if (elf_sym__type(sym) != STT_FUNC)
>                 return 0;
> @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>                 functions = new;
>         }
>
> -       idx = elf_sym__section(sym);
> -
> -       if (idx != last_idx) {
> -               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> +       if (sym_sec_idx != last_idx) {
> +               if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
>                         return 0;
> -               last_idx = idx;
> +               last_idx = sym_sec_idx;
>         }
>
>         functions[functions_cnt].name = name;
> @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
>         return true;
>  }
>
> -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> +                             Elf32_Word sym_sec_idx)

nit: same, size_t or just int would be fine

>  {
>         const char *sym_name;
>         uint64_t addr;
>         uint32_t size;
>
>         /* compare a symbol's shndx to determine if it's a percpu variable */
> -       if (elf_sym__section(sym) != btfe->percpu_shndx)
> +       if (sym_sec_idx != btfe->percpu_shndx)
>                 return 0;
>         if (elf_sym__type(sym) != STT_OBJECT)
>                 return 0;
> @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
>         return 0;
>  }
>
> -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> +                          Elf32_Word sym_sec_idx)
>  {
>         if (!fl->mcount_start &&
>             !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
>                 fl->mcount_start = sym->st_value;
> -               fl->mcount_sec_idx = sym->st_shndx;
> +               fl->mcount_sec_idx = sym_sec_idx;
>         }
>
>         if (!fl->mcount_stop &&
> @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
>                 fl->mcount_stop = sym->st_value;
>  }
>
> +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> +                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)

This is a generic function, why don't you want to move it into elf_symtab.h?

> +{
> +       if (!gelf_getsym(syms, id, sym))
> +               return false;
> +
> +       *sym_sec_idx = sym->st_shndx;
> +
> +       if (sym->st_shndx == SHN_XINDEX) {
> +               if (!syms_sec_idx_table)
> +                       return false;
> +               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> +                                     id, sym, sym_sec_idx))
> +                       return false;

You also ignored my feedback about not fetching symbol twice. Why?

> +       }
> +
> +       return true;
> +}
> +
> +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> +       for (id = 0;                                                            \
> +            id < symtab->nr_syms &&                                            \
> +            elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,             \
> +                         id, &sym, &sym_sec_idx);                              \
> +            id++)

This should be in elf_symtab.h next to elf_symtab__for_each_symbol.

And thinking a bit more, the variant with just ignoring symbols that
we failed to get is probably a safer alternative. I.e., currently
there is no way to communicate that we terminated iteration with
error, so it's probably better to skip failed symbols and still get
the rest, no? I was hoping to discuss stuff like this on the previous
version of the patch...

And please do fix elf_symtab__for_each_symbol().

> +
>  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  {
>         struct funcs_layout fl = { };
> +       Elf32_Word sym_sec_idx;
>         uint32_t core_id;
>         GElf_Sym sym;
>

[...]

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Jiri Olsa]

On Fri, Jan 22, 2021 at 04:52:28PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu:
> > For very large ELF objects (with many sections), we could
> > get special value SHN_XINDEX (65535) for symbol's st_shndx.
> > 
> > This patch is adding code to detect the optional extended
> > section index table and use it to resolve symbol's section
> > index.
> > 
> > Adding elf_symtab__for_each_symbol_index macro that returns
> > symbol's section index and usign it in collect functions.
> 
> From a quick look it seems you addressed Andrii's review comments,
> right?

yep, it's described in the cover email

> 
> I've merged it locally, but would like to have some detailed set of
> steps on how to test this, so that I can add it to a "Committer testing"
> section in the cset commit log and probably add it to my local set of
> regression tests.

sorry I forgot to mention that:

The test was to run pahole on kernel compiled with:
  make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux

and ensure FUNC records are generated and match normal
build (without above KCFLAGS)

Also bpf selftest passed.


> 
> Who originally reported this? Joe? Also can someone provide a Tested-by:
> in addition to mine when I get this detailed set of steps to test?

oops, it was reported by Yulia Kopkova (just cc-ed)

Joe tested the v2 of the patchset, I'll make a dwarves scratch
build with v3 and let them test it

jirka

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2021 at 09:24:03PM +0100, Jiri Olsa escreveu:
> On Fri, Jan 22, 2021 at 04:52:28PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu:
> > > For very large ELF objects (with many sections), we could
> > > get special value SHN_XINDEX (65535) for symbol's st_shndx.
> > > 
> > > This patch is adding code to detect the optional extended
> > > section index table and use it to resolve symbol's section
> > > index.
> > > 
> > > Adding elf_symtab__for_each_symbol_index macro that returns
> > > symbol's section index and usign it in collect functions.
> > 
> > From a quick look it seems you addressed Andrii's review comments,
> > right?
> 
> yep, it's described in the cover email
> 
> > 
> > I've merged it locally, but would like to have some detailed set of
> > steps on how to test this, so that I can add it to a "Committer testing"
> > section in the cset commit log and probably add it to my local set of
> > regression tests.
> 
> sorry I forgot to mention that:
> 
> The test was to run pahole on kernel compiled with:
>   make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux
> 
> and ensure FUNC records are generated and match normal
> build (without above KCFLAGS)
> 
> Also bpf selftest passed.

Thanks, I'll come up with some shell script to test that.
 
> 
> > 
> > Who originally reported this? Joe? Also can someone provide a Tested-by:
> > in addition to mine when I get this detailed set of steps to test?
> 
> oops, it was reported by Yulia Kopkova (just cc-ed)
> 
> Joe tested the v2 of the patchset, I'll make a dwarves scratch
> build with v3 and let them test it

Thanks, and there is a new comment by Andrii that I've found relevant
about using size_t instead of Elf_something.

- Arnaldo

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Jiri Olsa]

On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote:
> On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > For very large ELF objects (with many sections), we could
> > get special value SHN_XINDEX (65535) for symbol's st_shndx.
> >
> > This patch is adding code to detect the optional extended
> > section index table and use it to resolve symbol's section
> > index.
> >
> > Adding elf_symtab__for_each_symbol_index macro that returns
> > symbol's section index and usign it in collect functions.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
> >  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
> >  elf_symtab.h  |  2 ++
> >  3 files changed, 83 insertions(+), 17 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 5557c9efd365..56ee55965093 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -63,13 +63,13 @@ static void delete_functions(void)
> >  #define max(x, y) ((x) < (y) ? (y) : (x))
> >  #endif
> >
> > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> > +                           Elf32_Word sym_sec_idx)
> 
> nit: we use size_t or int for this, no need for libelf types here, imo

ok

> 
> 
> 
> >  {
> >         struct elf_function *new;
> >         static GElf_Shdr sh;
> > -       static int last_idx;
> > +       static Elf32_Word last_idx;
> >         const char *name;
> > -       int idx;
> >
> >         if (elf_sym__type(sym) != STT_FUNC)
> >                 return 0;
> > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >                 functions = new;
> >         }
> >
> > -       idx = elf_sym__section(sym);
> > -
> > -       if (idx != last_idx) {
> > -               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> > +       if (sym_sec_idx != last_idx) {
> > +               if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
> >                         return 0;
> > -               last_idx = idx;
> > +               last_idx = sym_sec_idx;
> >         }
> >
> >         functions[functions_cnt].name = name;
> > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
> >         return true;
> >  }
> >
> > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> > +                             Elf32_Word sym_sec_idx)
> 
> nit: same, size_t or just int would be fine
> 
> >  {
> >         const char *sym_name;
> >         uint64_t addr;
> >         uint32_t size;
> >
> >         /* compare a symbol's shndx to determine if it's a percpu variable */
> > -       if (elf_sym__section(sym) != btfe->percpu_shndx)
> > +       if (sym_sec_idx != btfe->percpu_shndx)
> >                 return 0;
> >         if (elf_sym__type(sym) != STT_OBJECT)
> >                 return 0;
> > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> >         return 0;
> >  }
> >
> > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> > +                          Elf32_Word sym_sec_idx)
> >  {
> >         if (!fl->mcount_start &&
> >             !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> >                 fl->mcount_start = sym->st_value;
> > -               fl->mcount_sec_idx = sym->st_shndx;
> > +               fl->mcount_sec_idx = sym_sec_idx;
> >         }
> >
> >         if (!fl->mcount_stop &&
> > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> >                 fl->mcount_stop = sym->st_value;
> >  }
> >
> > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> > +                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
> 
> This is a generic function, why don't you want to move it into elf_symtab.h?
> 
> > +{
> > +       if (!gelf_getsym(syms, id, sym))
> > +               return false;
> > +
> > +       *sym_sec_idx = sym->st_shndx;
> > +
> > +       if (sym->st_shndx == SHN_XINDEX) {
> > +               if (!syms_sec_idx_table)
> > +                       return false;
> > +               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> > +                                     id, sym, sym_sec_idx))
> > +                       return false;
> 
> You also ignored my feedback about not fetching symbol twice. Why?

ugh, I did not read your last 2 comments.. let me check that, sry

> 
> > +       }
> > +
> > +       return true;
> > +}
> > +
> > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> > +       for (id = 0;                                                            \
> > +            id < symtab->nr_syms &&                                            \
> > +            elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,             \
> > +                         id, &sym, &sym_sec_idx);                              \
> > +            id++)
> 
> This should be in elf_symtab.h next to elf_symtab__for_each_symbol.
> 
> And thinking a bit more, the variant with just ignoring symbols that
> we failed to get is probably a safer alternative. I.e., currently
> there is no way to communicate that we terminated iteration with
> error, so it's probably better to skip failed symbols and still get
> the rest, no? I was hoping to discuss stuff like this on the previous
> version of the patch...

I was thinking of that, but then I thought it's better to fail,
than have possibly wrong data in BTF, because the ELF data is
possibly damaged? no idea.. so I took the safer way

jirka

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Joe Lawrence]

On 1/22/21 2:52 PM, Arnaldo Carvalho de Melo wrote:
> 
> Who originally reported this? Joe? Also can someone provide a Tested-by:
> in addition to mine when I get this detailed set of steps to test?
> 

As Jiri noted, we tested v2 I think, so if there is a v4 build we could 
give a spin, just let us know.

In the meantime, for kpatch, we figured that we could just temporarily 
disable CONFIG_DEBUG_INFO_BTF in the scripts/link-vmlinux.sh file during 
kpatch builds ... that would leave kernel code intact, but skip the BTF 
generation step (for which kpatch doesn't need anyway).

Thanks,

-- Joe

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Arnaldo Carvalho de Melo]

Em Fri, Jan 22, 2021 at 09:37:48PM +0100, Jiri Olsa escreveu:
> On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote:
> > On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > For very large ELF objects (with many sections), we could
> > > get special value SHN_XINDEX (65535) for symbol's st_shndx.
> > >
> > > This patch is adding code to detect the optional extended
> > > section index table and use it to resolve symbol's section
> > > index.
> > >
> > > Adding elf_symtab__for_each_symbol_index macro that returns
> > > symbol's section index and usign it in collect functions.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
> > >  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
> > >  elf_symtab.h  |  2 ++
> > >  3 files changed, 83 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index 5557c9efd365..56ee55965093 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -63,13 +63,13 @@ static void delete_functions(void)
> > >  #define max(x, y) ((x) < (y) ? (y) : (x))
> > >  #endif
> > >
> > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> > > +                           Elf32_Word sym_sec_idx)
> > 
> > nit: we use size_t or int for this, no need for libelf types here, imo
> 
> ok

I think it is because this patch ends up calling 

extern GElf_Sym *gelf_getsymshndx (Elf_Data *__symdata, Elf_Data *__shndxdata,
                                   int __ndx, GElf_Sym *__sym,
                                   Elf32_Word *__xshndx);

Which expects a pointer to a Elf32_Word, right Jiri?

Jiri, are you going to submit a new version of this patch? I processed
the first one, collecting Andrii's Acked-by, if you're busy I can try to
address the other concerns from Andrii, please let me know.

- Arnaldo
 
> > 
> > 
> > 
> > >  {
> > >         struct elf_function *new;
> > >         static GElf_Shdr sh;
> > > -       static int last_idx;
> > > +       static Elf32_Word last_idx;
> > >         const char *name;
> > > -       int idx;
> > >
> > >         if (elf_sym__type(sym) != STT_FUNC)
> > >                 return 0;
> > > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > >                 functions = new;
> > >         }
> > >
> > > -       idx = elf_sym__section(sym);
> > > -
> > > -       if (idx != last_idx) {
> > > -               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> > > +       if (sym_sec_idx != last_idx) {
> > > +               if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
> > >                         return 0;
> > > -               last_idx = idx;
> > > +               last_idx = sym_sec_idx;
> > >         }
> > >
> > >         functions[functions_cnt].name = name;
> > > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
> > >         return true;
> > >  }
> > >
> > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> > > +                             Elf32_Word sym_sec_idx)
> > 
> > nit: same, size_t or just int would be fine
> > 
> > >  {
> > >         const char *sym_name;
> > >         uint64_t addr;
> > >         uint32_t size;
> > >
> > >         /* compare a symbol's shndx to determine if it's a percpu variable */
> > > -       if (elf_sym__section(sym) != btfe->percpu_shndx)
> > > +       if (sym_sec_idx != btfe->percpu_shndx)
> > >                 return 0;
> > >         if (elf_sym__type(sym) != STT_OBJECT)
> > >                 return 0;
> > > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> > >         return 0;
> > >  }
> > >
> > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> > > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> > > +                          Elf32_Word sym_sec_idx)
> > >  {
> > >         if (!fl->mcount_start &&
> > >             !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> > >                 fl->mcount_start = sym->st_value;
> > > -               fl->mcount_sec_idx = sym->st_shndx;
> > > +               fl->mcount_sec_idx = sym_sec_idx;
> > >         }
> > >
> > >         if (!fl->mcount_stop &&
> > > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> > >                 fl->mcount_stop = sym->st_value;
> > >  }
> > >
> > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> > > +                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
> > 
> > This is a generic function, why don't you want to move it into elf_symtab.h?
> > 
> > > +{
> > > +       if (!gelf_getsym(syms, id, sym))
> > > +               return false;
> > > +
> > > +       *sym_sec_idx = sym->st_shndx;
> > > +
> > > +       if (sym->st_shndx == SHN_XINDEX) {
> > > +               if (!syms_sec_idx_table)
> > > +                       return false;
> > > +               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> > > +                                     id, sym, sym_sec_idx))
> > > +                       return false;
> > 
> > You also ignored my feedback about not fetching symbol twice. Why?
> 
> ugh, I did not read your last 2 comments.. let me check that, sry
> 
> > 
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> > > +       for (id = 0;                                                            \
> > > +            id < symtab->nr_syms &&                                            \
> > > +            elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,             \
> > > +                         id, &sym, &sym_sec_idx);                              \
> > > +            id++)
> > 
> > This should be in elf_symtab.h next to elf_symtab__for_each_symbol.
> > 
> > And thinking a bit more, the variant with just ignoring symbols that
> > we failed to get is probably a safer alternative. I.e., currently
> > there is no way to communicate that we terminated iteration with
> > error, so it's probably better to skip failed symbols and still get
> > the rest, no? I was hoping to discuss stuff like this on the previous
> > version of the patch...
> 
> I was thinking of that, but then I thought it's better to fail,
> than have possibly wrong data in BTF, because the ELF data is
> possibly damaged? no idea.. so I took the safer way
> 
> jirka
> 

-- 

- Arnaldo

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Arnaldo Carvalho de Melo]

Em Mon, Jan 25, 2021 at 02:37:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jan 22, 2021 at 09:37:48PM +0100, Jiri Olsa escreveu:
> > On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote:
> > > On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > For very large ELF objects (with many sections), we could
> > > > get special value SHN_XINDEX (65535) for symbol's st_shndx.
> > > >
> > > > This patch is adding code to detect the optional extended
> > > > section index table and use it to resolve symbol's section
> > > > index.
> > > >
> > > > Adding elf_symtab__for_each_symbol_index macro that returns
> > > > symbol's section index and usign it in collect functions.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  btf_encoder.c | 59 +++++++++++++++++++++++++++++++++++++--------------
> > > >  elf_symtab.c  | 39 +++++++++++++++++++++++++++++++++-
> > > >  elf_symtab.h  |  2 ++
> > > >  3 files changed, 83 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > index 5557c9efd365..56ee55965093 100644
> > > > --- a/btf_encoder.c
> > > > +++ b/btf_encoder.c
> > > > @@ -63,13 +63,13 @@ static void delete_functions(void)
> > > >  #define max(x, y) ((x) < (y) ? (y) : (x))
> > > >  #endif
> > > >
> > > > -static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > > +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> > > > +                           Elf32_Word sym_sec_idx)
> > > 
> > > nit: we use size_t or int for this, no need for libelf types here, imo
> > 
> > ok
> 
> I think it is because this patch ends up calling 
> 
> extern GElf_Sym *gelf_getsymshndx (Elf_Data *__symdata, Elf_Data *__shndxdata,
>                                    int __ndx, GElf_Sym *__sym,
>                                    Elf32_Word *__xshndx);
> 
> Which expects a pointer to a Elf32_Word, right Jiri?
> 
> Jiri, are you going to submit a new version of this patch? I processed

Sorry, saw v4, processing...

> the first one, collecting Andrii's Acked-by, if you're busy I can try to
> address the other concerns from Andrii, please let me know.
> 
> - Arnaldo
>  
> > > 
> > > 
> > > 
> > > >  {
> > > >         struct elf_function *new;
> > > >         static GElf_Shdr sh;
> > > > -       static int last_idx;
> > > > +       static Elf32_Word last_idx;
> > > >         const char *name;
> > > > -       int idx;
> > > >
> > > >         if (elf_sym__type(sym) != STT_FUNC)
> > > >                 return 0;
> > > > @@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> > > >                 functions = new;
> > > >         }
> > > >
> > > > -       idx = elf_sym__section(sym);
> > > > -
> > > > -       if (idx != last_idx) {
> > > > -               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> > > > +       if (sym_sec_idx != last_idx) {
> > > > +               if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
> > > >                         return 0;
> > > > -               last_idx = idx;
> > > > +               last_idx = sym_sec_idx;
> > > >         }
> > > >
> > > >         functions[functions_cnt].name = name;
> > > > @@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
> > > >         return true;
> > > >  }
> > > >
> > > > -static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> > > > +static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
> > > > +                             Elf32_Word sym_sec_idx)
> > > 
> > > nit: same, size_t or just int would be fine
> > > 
> > > >  {
> > > >         const char *sym_name;
> > > >         uint64_t addr;
> > > >         uint32_t size;
> > > >
> > > >         /* compare a symbol's shndx to determine if it's a percpu variable */
> > > > -       if (elf_sym__section(sym) != btfe->percpu_shndx)
> > > > +       if (sym_sec_idx != btfe->percpu_shndx)
> > > >                 return 0;
> > > >         if (elf_sym__type(sym) != STT_OBJECT)
> > > >                 return 0;
> > > > @@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
> > > >         return 0;
> > > >  }
> > > >
> > > > -static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> > > > +static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
> > > > +                          Elf32_Word sym_sec_idx)
> > > >  {
> > > >         if (!fl->mcount_start &&
> > > >             !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
> > > >                 fl->mcount_start = sym->st_value;
> > > > -               fl->mcount_sec_idx = sym->st_shndx;
> > > > +               fl->mcount_sec_idx = sym_sec_idx;
> > > >         }
> > > >
> > > >         if (!fl->mcount_stop &&
> > > > @@ -598,9 +598,36 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
> > > >                 fl->mcount_stop = sym->st_value;
> > > >  }
> > > >
> > > > +static bool elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
> > > > +                        int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
> > > 
> > > This is a generic function, why don't you want to move it into elf_symtab.h?
> > > 
> > > > +{
> > > > +       if (!gelf_getsym(syms, id, sym))
> > > > +               return false;
> > > > +
> > > > +       *sym_sec_idx = sym->st_shndx;
> > > > +
> > > > +       if (sym->st_shndx == SHN_XINDEX) {
> > > > +               if (!syms_sec_idx_table)
> > > > +                       return false;
> > > > +               if (!gelf_getsymshndx(syms, syms_sec_idx_table,
> > > > +                                     id, sym, sym_sec_idx))
> > > > +                       return false;
> > > 
> > > You also ignored my feedback about not fetching symbol twice. Why?
> > 
> > ugh, I did not read your last 2 comments.. let me check that, sry
> > 
> > > 
> > > > +       }
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> > > > +       for (id = 0;                                                            \
> > > > +            id < symtab->nr_syms &&                                            \
> > > > +            elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,             \
> > > > +                         id, &sym, &sym_sec_idx);                              \
> > > > +            id++)
> > > 
> > > This should be in elf_symtab.h next to elf_symtab__for_each_symbol.
> > > 
> > > And thinking a bit more, the variant with just ignoring symbols that
> > > we failed to get is probably a safer alternative. I.e., currently
> > > there is no way to communicate that we terminated iteration with
> > > error, so it's probably better to skip failed symbols and still get
> > > the rest, no? I was hoping to discuss stuff like this on the previous
> > > version of the patch...
> > 
> > I was thinking of that, but then I thought it's better to fail,
> > than have possibly wrong data in BTF, because the ELF data is
> > possibly damaged? no idea.. so I took the safer way
> > 
> > jirka
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Andrii Nakryiko]

On Sun, Jan 24, 2021 at 2:18 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> For very large ELF objects (with many sections), we could
> get special value SHN_XINDEX (65535) for symbol's st_shndx.
>
> This patch is adding code to detect the optional extended
> section index table and use it to resolve symbol's section
> index.
>
> Adding elf_symtab__for_each_symbol_index macro that returns
> symbol's section index and usign it in collect functions.
>
> Tested by running pahole on kernel compiled with:
>   make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux
>
> and ensure FUNC records are generated and match normal
> build (without above KCFLAGS).
>
> Also bpf selftest passed and generated kernel BTF,
> is same as without the patch.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>


>  btf_encoder.c | 33 +++++++++++++++++----------------
>  elf_symtab.c  | 41 +++++++++++++++++++++++++++++++++++++++--
>  elf_symtab.h  | 29 +++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 18 deletions(-)
>

[...]

[PATCH 2/2] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values

[Jiri Olsa]

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for symbol's st_shndx.

This patch is adding code to detect the optional extended
section index table and use it to resolve symbol's section
index.

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect functions.

Tested by running pahole on kernel compiled with:
  make KCFLAGS="-ffunction-sections -fdata-sections" -j$(nproc) vmlinux

and ensure FUNC records are generated and match normal
build (without above KCFLAGS).

Also bpf selftest passed and generated kernel BTF,
is same as without the patch.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 33 +++++++++++++++++----------------
 elf_symtab.c  | 41 +++++++++++++++++++++++++++++++++++++++--
 elf_symtab.h  | 29 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 18 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 5557c9efd365..b124ec20a689 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -63,13 +63,13 @@ static void delete_functions(void)
 #define max(x, y) ((x) < (y) ? (y) : (x))
 #endif
 
-static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
+static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
+			    size_t sym_sec_idx)
 {
 	struct elf_function *new;
 	static GElf_Shdr sh;
-	static int last_idx;
+	static size_t last_idx;
 	const char *name;
-	int idx;
 
 	if (elf_sym__type(sym) != STT_FUNC)
 		return 0;
@@ -90,12 +90,10 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 		functions = new;
 	}
 
-	idx = elf_sym__section(sym);
-
-	if (idx != last_idx) {
-		if (!elf_section_by_idx(btfe->elf, &sh, idx))
+	if (sym_sec_idx != last_idx) {
+		if (!elf_section_by_idx(btfe->elf, &sh, sym_sec_idx))
 			return 0;
-		last_idx = idx;
+		last_idx = sym_sec_idx;
 	}
 
 	functions[functions_cnt].name = name;
@@ -542,14 +540,15 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
 	return true;
 }
 
-static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
+static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym,
+			      size_t sym_sec_idx)
 {
 	const char *sym_name;
 	uint64_t addr;
 	uint32_t size;
 
 	/* compare a symbol's shndx to determine if it's a percpu variable */
-	if (elf_sym__section(sym) != btfe->percpu_shndx)
+	if (sym_sec_idx != btfe->percpu_shndx)
 		return 0;
 	if (elf_sym__type(sym) != STT_OBJECT)
 		return 0;
@@ -585,12 +584,13 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
 	return 0;
 }
 
-static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
+static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl,
+			   size_t sym_sec_idx)
 {
 	if (!fl->mcount_start &&
 	    !strcmp("__start_mcount_loc", elf_sym__name(sym, btfe->symtab))) {
 		fl->mcount_start = sym->st_value;
-		fl->mcount_sec_idx = sym->st_shndx;
+		fl->mcount_sec_idx = sym_sec_idx;
 	}
 
 	if (!fl->mcount_stop &&
@@ -601,6 +601,7 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
 	struct funcs_layout fl = { };
+	Elf32_Word sym_sec_idx;
 	uint32_t core_id;
 	GElf_Sym sym;
 
@@ -608,12 +609,12 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 	percpu_var_cnt = 0;
 
 	/* search within symtab for percpu variables */
-	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
-		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
+	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
+		if (collect_percpu_vars && collect_percpu_var(btfe, &sym, sym_sec_idx))
 			return -1;
-		if (collect_function(btfe, &sym))
+		if (collect_function(btfe, &sym, sym_sec_idx))
 			return -1;
-		collect_symbol(&sym, &fl);
+		collect_symbol(&sym, &fl, sym_sec_idx);
 	}
 
 	if (collect_percpu_vars) {
diff --git a/elf_symtab.c b/elf_symtab.c
index 741990ea3ed9..77c5dc423c56 100644
--- a/elf_symtab.c
+++ b/elf_symtab.c
@@ -17,11 +17,13 @@
 
 struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 {
+	size_t symtab_index;
+
 	if (name == NULL)
 		name = ".symtab";
 
 	GElf_Shdr shdr;
-	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, NULL);
+	Elf_Scn *sec = elf_section_by_name(elf, ehdr, &shdr, name, &symtab_index);
 
 	if (sec == NULL)
 		return NULL;
@@ -29,7 +31,7 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (gelf_getshdr(sec, &shdr) == NULL)
 		return NULL;
 
-	struct elf_symtab *symtab = malloc(sizeof(*symtab));
+	struct elf_symtab *symtab = zalloc(sizeof(*symtab));
 	if (symtab == NULL)
 		return NULL;
 
@@ -41,6 +43,12 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->syms == NULL)
 		goto out_free_name;
 
+	/*
+	 * This returns extended section index table's
+	 * section index, if it exists.
+	 */
+	int symtab_xindex = elf_scnshndx(sec);
+
 	sec = elf_getscn(elf, shdr.sh_link);
 	if (sec == NULL)
 		goto out_free_name;
@@ -49,6 +57,35 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (symtab->symstrs == NULL)
 		goto out_free_name;
 
+	/*
+	 * The .symtab section has optional extended section index
+	 * table, load its data so it can be used to resolve symbol's
+	 * section index.
+	 **/
+	if (symtab_xindex > 0) {
+		GElf_Shdr shdr_xindex;
+		Elf_Scn *sec_xindex;
+
+		sec_xindex = elf_getscn(elf, symtab_xindex);
+		if (sec_xindex == NULL)
+			goto out_free_name;
+
+		if (gelf_getshdr(sec_xindex, &shdr_xindex) == NULL)
+			goto out_free_name;
+
+		/* Extra check to verify it's correct type */
+		if (shdr_xindex.sh_type != SHT_SYMTAB_SHNDX)
+			goto out_free_name;
+
+		/* Extra check to verify it belongs to the .symtab */
+		if (symtab_index != shdr_xindex.sh_link)
+			goto out_free_name;
+
+		symtab->syms_sec_idx_table = elf_getdata(elf_getscn(elf, symtab_xindex), NULL);
+		if (symtab->syms_sec_idx_table == NULL)
+			goto out_free_name;
+	}
+
 	symtab->nr_syms = shdr.sh_size / shdr.sh_entsize;
 
 	return symtab;
diff --git a/elf_symtab.h b/elf_symtab.h
index 359add69c8ab..489e2b1a3505 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h
@@ -16,6 +16,8 @@ struct elf_symtab {
 	uint32_t  nr_syms;
 	Elf_Data  *syms;
 	Elf_Data  *symstrs;
+	/* Data of SHT_SYMTAB_SHNDX section. */
+	Elf_Data  *syms_sec_idx_table;
 	char	  *name;
 };
 
@@ -77,6 +79,19 @@ static inline bool elf_sym__is_local_object(const GElf_Sym *sym)
 	       sym->st_shndx != SHN_UNDEF;
 }
 
+static inline bool
+elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
+	     int id, GElf_Sym *sym, Elf32_Word *sym_sec_idx)
+{
+	if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
+		return false;
+
+	if (sym->st_shndx != SHN_XINDEX)
+		*sym_sec_idx = sym->st_shndx;
+
+	return true;
+}
+
 /**
  * elf_symtab__for_each_symbol - iterate thru all the symbols
  *
@@ -89,4 +104,18 @@ static inline bool elf_sym__is_local_object(const GElf_Sym *sym)
 	     index < symtab->nr_syms; \
 	     index++, gelf_getsym(symtab->syms, index, &sym))
 
+/**
+ * elf_symtab__for_each_symbol_index - iterate through all the symbols,
+ * that takes extended symbols indexes into account
+ *
+ * @symtab: struct elf_symtab instance to iterate
+ * @index: uint32_t index
+ * @sym: GElf_Sym iterator
+ * @sym_sec_idx: symbol's index
+ */
+#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)		\
+	for (id = 0; id < symtab->nr_syms; id++)				\
+		if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,	\
+				 id, &sym, &sym_sec_idx))
+
 #endif /* _ELF_SYMTAB_H_ */
-- 
2.26.2