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

[PATCHv4 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].

v4 changes:
  - use size_t instead of Elf32_Word [Andrii]
  - move elf_symtab__for_each_symbol_index and elf_sym__get
    elf_symtab.h [Andrii]
  - added ack for patch 1 [Andrii]
  - changed elf_sym__get to be simpler [Andrii]
  - changed elf_symtab__for_each_symbol_index to skip bad symbols
  - use zalloc for struct elf_symtab allocation to get zero
    initialized members

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 | 33 +++++++++++++++++----------------
 dutil.c       |  8 +++++++-
 elf_symtab.c  | 41 +++++++++++++++++++++++++++++++++++++++--
 elf_symtab.h  | 29 +++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 19 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.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
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.

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

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(-)
>

[...]

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

[Joe Lawrence]

On 1/24/21 5:15 PM, Jiri Olsa wrote:
> 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].
> 
> v4 changes:
>    - use size_t instead of Elf32_Word [Andrii]
>    - move elf_symtab__for_each_symbol_index and elf_sym__get
>      elf_symtab.h [Andrii]
>    - added ack for patch 1 [Andrii]
>    - changed elf_sym__get to be simpler [Andrii]
>    - changed elf_symtab__for_each_symbol_index to skip bad symbols
>    - use zalloc for struct elf_symtab allocation to get zero
>      initialized members
> 
> 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 | 33 +++++++++++++++++----------------
>   dutil.c       |  8 +++++++-
>   elf_symtab.c  | 41 +++++++++++++++++++++++++++++++++++++++--
>   elf_symtab.h  | 29 +++++++++++++++++++++++++++++
>   4 files changed, 92 insertions(+), 19 deletions(-)
> 

For v4 patchset:

Tested-by: Joe Lawrence <joe.lawrence@redhat.com>

Thanks Jiri!

-- Joe