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

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

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

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

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:

quoted

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

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:

quoted

Em Fri, Jan 22, 2021 at 05:39:20PM +0100, Jiri Olsa escreveu:

quoted

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

quoted

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.
 

quoted

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

On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote:

On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa [off-list ref] wrote:

quoted

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

quoted

 {
        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

quoted

 {
        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?

quoted

+{
+       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

quoted

+       }
+
+       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

On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa [off-list ref] wrote:

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

nit: same, size_t or just int would be fine

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;

[...]

On Fri, Jan 22, 2021 at 9:22 AM Jiri Olsa [off-list ref] 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>

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
 

-- 

- Arnaldo

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
 

-- 

- Arnaldo

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

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:

quoted

On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa [off-list ref] wrote:

quoted

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
 

quoted

quoted

 {
        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

quoted

 {
        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?

quoted

+{
+       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

quoted

quoted

+       }
+
+       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

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:

quoted

On Fri, Jan 22, 2021 at 12:16:42PM -0800, Andrii Nakryiko wrote:

quoted

On Fri, Jan 22, 2021 at 8:46 AM Jiri Olsa [off-list ref] wrote:

quoted

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
 

quoted

quoted

quoted

 {
        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

quoted

 {
        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?

quoted

+{
+       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

quoted

quoted

+       }
+
+       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