bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table
@ 2021-01-21 20:22 Jiri Olsa
  2021-01-21 20:22 ` [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name Jiri Olsa
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-21 20:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

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

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/
---
dwarves:

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 | 36 ++++++++++++++++++++++++++++++++----
 dutil.c       |  8 ++++++--
 elf_symtab.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 elf_symtab.h  |  2 ++
 4 files changed, 78 insertions(+), 7 deletions(-)


libbpf:

Jiri Olsa (1):
      libbpf: Use string table index from index table if needed

 tools/lib/bpf/btf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name
  2021-01-21 20:22 [PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
@ 2021-01-21 20:22 ` Jiri Olsa
  2021-01-21 23:10   ` Andrii Nakryiko
  2021-01-21 20:22 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
  2021-01-21 20:22 ` [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed Jiri Olsa
  2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-21 20:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

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, 6 insertions(+), 2 deletions(-)

diff --git a/dutil.c b/dutil.c
index 7b667647420f..9e0fdca3ae04 100644
--- a/dutil.c
+++ b/dutil.c
@@ -179,13 +179,17 @@ 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);
-		if (!strcmp(name, str)) {
+		str = elf_strptr(elf, str_idx, shp->sh_name);
+		if (str && !strcmp(name, str)) {
 			if (index)
 				*index = cnt;
 			break;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-21 20:22 [PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
  2021-01-21 20:22 ` [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name Jiri Olsa
@ 2021-01-21 20:22 ` Jiri Olsa
  2021-01-21 23:32   ` Andrii Nakryiko
  2021-01-21 20:22 ` [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed Jiri Olsa
  2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-21 20:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

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_symbols function.

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

diff --git a/btf_encoder.c b/btf_encoder.c
index 5557c9efd365..6e6f22c438ce 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -585,12 +585,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 +599,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, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,	\
+				  id, &sym, &sym_sec_idx);			\
+	     id < symtab->nr_syms;						\
+	     id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,	\
+				id, &sym, &sym_sec_idx))
+
 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 +636,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) {
+	elf_symtab__for_each_symbol_index(btfe->symtab, core_id, sym, sym_sec_idx) {
 		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
 			return -1;
 		if (collect_function(btfe, &sym))
 			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.27.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed
  2021-01-21 20:22 [PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
  2021-01-21 20:22 ` [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name Jiri Olsa
  2021-01-21 20:22 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
@ 2021-01-21 20:22 ` Jiri Olsa
  2021-01-21 23:46   ` Andrii Nakryiko
  2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-21 20:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

For very large ELF objects (with many sections), we could
get special value SHN_XINDEX (65535) for elf object's string
table index - e_shstrndx.

Call elf_getshdrstrndx to get the proper string table index,
instead of reading it directly from ELF header.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/btf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 9970a288dda5..d9c10830d749 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -858,6 +858,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 	Elf_Scn *scn = NULL;
 	Elf *elf = NULL;
 	GElf_Ehdr ehdr;
+	size_t shstrndx;
 
 	if (elf_version(EV_CURRENT) == EV_NONE) {
 		pr_warn("failed to init libelf for %s\n", path);
@@ -882,7 +883,14 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 		pr_warn("failed to get EHDR from %s\n", path);
 		goto done;
 	}
-	if (!elf_rawdata(elf_getscn(elf, ehdr.e_shstrndx), NULL)) {
+
+	if (elf_getshdrstrndx(elf, &shstrndx)) {
+		pr_warn("failed to get section names section index for %s\n",
+			path);
+		goto done;
+	}
+
+	if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
 		pr_warn("failed to get e_shstrndx from %s\n", path);
 		goto done;
 	}
@@ -897,7 +905,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
 				idx, path);
 			goto done;
 		}
-		name = elf_strptr(elf, ehdr.e_shstrndx, sh.sh_name);
+		name = elf_strptr(elf, shstrndx, sh.sh_name);
 		if (!name) {
 			pr_warn("failed to get section(%d) name from %s\n",
 				idx, path);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name
  2021-01-21 20:22 ` [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name Jiri Olsa
@ 2021-01-21 23:10   ` Andrii Nakryiko
  2021-01-21 23:34     ` Jiri Olsa
  0 siblings, 1 reply; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 23:10 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, dwarves, Networking, bpf, Yonghong Song,
	Hao Luo, Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 12:24 PM 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>
> ---
>  dutil.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/dutil.c b/dutil.c
> index 7b667647420f..9e0fdca3ae04 100644
> --- a/dutil.c
> +++ b/dutil.c
> @@ -179,13 +179,17 @@ 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);
> -               if (!strcmp(name, str)) {
> +               str = elf_strptr(elf, str_idx, shp->sh_name);
> +               if (str && !strcmp(name, str)) {

if (!str) would be an error? should we bail out here?

>                         if (index)
>                                 *index = cnt;
>                         break;
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-21 20:22 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
@ 2021-01-21 23:32   ` Andrii Nakryiko
  2021-01-22  9:32     ` Jiri Olsa
                       ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 23:32 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, dwarves, Networking, bpf, Yonghong Song,
	Hao Luo, Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 12:25 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_symbols function.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

You missed fixing up collect_function() as well, which is using
elf_sym__section(), which doesn't know about extended numbering.

>  btf_encoder.c | 36 ++++++++++++++++++++++++++++++++----
>  elf_symtab.c  | 39 ++++++++++++++++++++++++++++++++++++++-
>  elf_symtab.h  |  2 ++
>  3 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5557c9efd365..6e6f22c438ce 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -585,12 +585,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 +599,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))


gelf_getsymshndx() is supposed to work even for cases that don't use
extended numbering, so this should work, right?

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;

?

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

what do we want to do if elf_sym__get() returns error (false)? We can
either stop or ignore that symbol, right? But currently you are
returning invalid symbol data.

so either

for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)

or

for (id = 0; id < symtab->nr_syms; id++)
  if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
&sym_sec_idx))


But the current variant looks broken. Oh, and
elf_symtab__for_each_symbol() is similarly broken, can you please fix
that as well?

And this new macro should probably be in elf_symtab.h, along the
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;
>

[...]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name
  2021-01-21 23:10   ` Andrii Nakryiko
@ 2021-01-21 23:34     ` Jiri Olsa
  0 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-21 23:34 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 03:10:25PM -0800, Andrii Nakryiko wrote:
> On Thu, Jan 21, 2021 at 12:24 PM 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>
> > ---
> >  dutil.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/dutil.c b/dutil.c
> > index 7b667647420f..9e0fdca3ae04 100644
> > --- a/dutil.c
> > +++ b/dutil.c
> > @@ -179,13 +179,17 @@ 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);
> > -               if (!strcmp(name, str)) {
> > +               str = elf_strptr(elf, str_idx, shp->sh_name);
> > +               if (str && !strcmp(name, str)) {
> 
> if (!str) would be an error? should we bail out here?

seems like if elf_nextscn returns NULL, it will be the case for all the
sections in here.. but bailing out on (!str) is more direct and safer

I'll send an update

thanks,
jirka

> 
> >                         if (index)
> >                                 *index = cnt;
> >                         break;
> > --
> > 2.27.0
> >
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed
  2021-01-21 20:22 ` [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed Jiri Olsa
@ 2021-01-21 23:46   ` Andrii Nakryiko
  0 siblings, 0 replies; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-21 23:46 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, dwarves, Networking, bpf, Yonghong Song,
	Hao Luo, Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 12:26 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> For very large ELF objects (with many sections), we could
> get special value SHN_XINDEX (65535) for elf object's string
> table index - e_shstrndx.
>
> Call elf_getshdrstrndx to get the proper string table index,
> instead of reading it directly from ELF header.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

I've applied this patch to bpf-next, you don't need to re-send it in
the next version of this patch set.

>  tools/lib/bpf/btf.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 9970a288dda5..d9c10830d749 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -858,6 +858,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>         Elf_Scn *scn = NULL;
>         Elf *elf = NULL;
>         GElf_Ehdr ehdr;
> +       size_t shstrndx;
>
>         if (elf_version(EV_CURRENT) == EV_NONE) {
>                 pr_warn("failed to init libelf for %s\n", path);
> @@ -882,7 +883,14 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>                 pr_warn("failed to get EHDR from %s\n", path);
>                 goto done;
>         }
> -       if (!elf_rawdata(elf_getscn(elf, ehdr.e_shstrndx), NULL)) {
> +
> +       if (elf_getshdrstrndx(elf, &shstrndx)) {
> +               pr_warn("failed to get section names section index for %s\n",
> +                       path);
> +               goto done;
> +       }
> +
> +       if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
>                 pr_warn("failed to get e_shstrndx from %s\n", path);
>                 goto done;
>         }
> @@ -897,7 +905,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>                                 idx, path);
>                         goto done;
>                 }
> -               name = elf_strptr(elf, ehdr.e_shstrndx, sh.sh_name);
> +               name = elf_strptr(elf, shstrndx, sh.sh_name);
>                 if (!name) {
>                         pr_warn("failed to get section(%d) name from %s\n",
>                                 idx, path);
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-21 23:32   ` Andrii Nakryiko
@ 2021-01-22  9:32     ` Jiri Olsa
  2021-01-22 20:46     ` Jiri Olsa
  2021-01-23 21:23     ` Jiri Olsa
  2 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-22  9:32 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:
> On Thu, Jan 21, 2021 at 12:25 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_symbols function.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> You missed fixing up collect_function() as well, which is using
> elf_sym__section(), which doesn't know about extended numbering.

ah right, it's for modules, I guess it's why it did not show up

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-21 23:32   ` Andrii Nakryiko
  2021-01-22  9:32     ` Jiri Olsa
@ 2021-01-22 20:46     ` Jiri Olsa
  2021-01-22 22:55       ` Andrii Nakryiko
  2021-01-23 21:23     ` Jiri Olsa
  2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-22 20:46 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

> > @@ -598,9 +599,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))
> 
> 
> gelf_getsymshndx() is supposed to work even for cases that don't use
> extended numbering, so this should work, right?
> 
> if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
>     return false;
> 

it seems you're right, gelf_getsymshndx seem to work for
both cases, I'll check


> if (sym->st_shndx == SHN_XINDEX)
>   *sym_sec_idx = sym->st_shndx;

I don't understand this..  gelf_getsymshndx will return both
symbol and proper index, no? also sym_sec_idx is already
assigned from previou call

> 
> return true;
> 
> ?
> 
> > +                       return false;
> > +       }
> > +
> > +       return true;
> > +}
> > +
> > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> > +       for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,     \
> > +                                 id, &sym, &sym_sec_idx);                      \
> > +            id < symtab->nr_syms;                                              \
> > +            id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,       \
> > +                               id, &sym, &sym_sec_idx))
> 
> what do we want to do if elf_sym__get() returns error (false)? We can
> either stop or ignore that symbol, right? But currently you are
> returning invalid symbol data.
> 
> so either
> 
> for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
> symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)
> 
> or
> 
> for (id = 0; id < symtab->nr_syms; id++)
>   if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
> &sym_sec_idx))

if we go ahead with skipping symbols, this one seems good

> 
> 
> But the current variant looks broken. Oh, and
> elf_symtab__for_each_symbol() is similarly broken, can you please fix
> that as well?
> 
> And this new macro should probably be in elf_symtab.h, along the
> elf_symtab__for_each_symbol.

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-22 20:46     ` Jiri Olsa
@ 2021-01-22 22:55       ` Andrii Nakryiko
  2021-01-23 18:51         ` Jiri Olsa
  0 siblings, 1 reply; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-22 22:55 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > @@ -598,9 +599,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))
> >
> >
> > gelf_getsymshndx() is supposed to work even for cases that don't use
> > extended numbering, so this should work, right?
> >
> > if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
> >     return false;
> >
>
> it seems you're right, gelf_getsymshndx seem to work for
> both cases, I'll check
>
>
> > if (sym->st_shndx == SHN_XINDEX)
> >   *sym_sec_idx = sym->st_shndx;
>
> I don't understand this..  gelf_getsymshndx will return both
> symbol and proper index, no? also sym_sec_idx is already
> assigned from previou call

Reading (some) implementation of gelf_getsymshndx() that I found
online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
numbering. But it will still return symbol data. So to return the
section index in all cases, we need to check again *after* we got
symbol, and if it's not extended, then set index manually.

>
> >
> > return true;
> >
> > ?
> >
> > > +                       return false;
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +#define elf_symtab__for_each_symbol_index(symtab, id, sym, sym_sec_idx)                \
> > > +       for (id = 0, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,     \
> > > +                                 id, &sym, &sym_sec_idx);                      \
> > > +            id < symtab->nr_syms;                                              \
> > > +            id++, elf_sym__get(symtab->syms, symtab->syms_sec_idx_table,       \
> > > +                               id, &sym, &sym_sec_idx))
> >
> > what do we want to do if elf_sym__get() returns error (false)? We can
> > either stop or ignore that symbol, right? But currently you are
> > returning invalid symbol data.
> >
> > so either
> >
> > for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
> > symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)
> >
> > or
> >
> > for (id = 0; id < symtab->nr_syms; id++)
> >   if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
> > &sym_sec_idx))
>
> if we go ahead with skipping symbols, this one seems good

I think skipping symbols is nicer. If ELF is totally broken, then all
symbols are going to be ignored anyway. If it's some one-off issue for
a specific symbol, we'll just ignore it (unfortunately, silently).

>
> >
> >
> > But the current variant looks broken. Oh, and
> > elf_symtab__for_each_symbol() is similarly broken, can you please fix
> > that as well?
> >
> > And this new macro should probably be in elf_symtab.h, along the
> > elf_symtab__for_each_symbol.
>
> thanks,
> jirka
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-22 22:55       ` Andrii Nakryiko
@ 2021-01-23 18:51         ` Jiri Olsa
  2021-01-23 20:07           ` Andrii Nakryiko
  2021-01-23 20:08           ` Mark Wielaard
  0 siblings, 2 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-23 18:51 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:
> On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > @@ -598,9 +599,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))
> > >
> > >
> > > gelf_getsymshndx() is supposed to work even for cases that don't use
> > > extended numbering, so this should work, right?
> > >
> > > if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
> > >     return false;
> > >
> >
> > it seems you're right, gelf_getsymshndx seem to work for
> > both cases, I'll check
> >
> >
> > > if (sym->st_shndx == SHN_XINDEX)
> > >   *sym_sec_idx = sym->st_shndx;
> >
> > I don't understand this..  gelf_getsymshndx will return both
> > symbol and proper index, no? also sym_sec_idx is already
> > assigned from previou call
> 
> Reading (some) implementation of gelf_getsymshndx() that I found
> online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
> numbering. But it will still return symbol data. So to return the

the latest upstream code seems to set it always,
but I agree we should be careful

Mark, any insight in here? thanks

> section index in all cases, we need to check again *after* we got
> symbol, and if it's not extended, then set index manually.

hum, then we should use '!=', right?

  if (sym->st_shndx != SHN_XINDEX)
    *sym_sec_idx = sym->st_shndx;

SNIP

> > > so either
> > >
> > > for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
> > > symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)
> > >
> > > or
> > >
> > > for (id = 0; id < symtab->nr_syms; id++)
> > >   if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
> > > &sym_sec_idx))
> >
> > if we go ahead with skipping symbols, this one seems good
> 
> I think skipping symbols is nicer. If ELF is totally broken, then all
> symbols are going to be ignored anyway. If it's some one-off issue for
> a specific symbol, we'll just ignore it (unfortunately, silently).

agreed, I'll use this

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-23 18:51         ` Jiri Olsa
@ 2021-01-23 20:07           ` Andrii Nakryiko
  2021-01-23 20:21             ` Mark Wielaard
  2021-01-23 20:08           ` Mark Wielaard
  1 sibling, 1 reply; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-23 20:07 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Sat, Jan 23, 2021 at 10:51 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:
> > On Fri, Jan 22, 2021 at 12:47 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > @@ -598,9 +599,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))
> > > >
> > > >
> > > > gelf_getsymshndx() is supposed to work even for cases that don't use
> > > > extended numbering, so this should work, right?
> > > >
> > > > if (!gelf_getsymshndx(syms, syms_sec_idx_table, id, sym, sym_sec_idx))
> > > >     return false;
> > > >
> > >
> > > it seems you're right, gelf_getsymshndx seem to work for
> > > both cases, I'll check
> > >
> > >
> > > > if (sym->st_shndx == SHN_XINDEX)
> > > >   *sym_sec_idx = sym->st_shndx;
> > >
> > > I don't understand this..  gelf_getsymshndx will return both
> > > symbol and proper index, no? also sym_sec_idx is already
> > > assigned from previou call
> >
> > Reading (some) implementation of gelf_getsymshndx() that I found
> > online, it won't set sym_sec_idx, if the symbol *doesn't* use extended
> > numbering. But it will still return symbol data. So to return the
>
> the latest upstream code seems to set it always,
> but I agree we should be careful

oh, then maybe it's not necessary. I honestly don't even know where
the authoritative source code of libelf is, so I just found some
random source code with Google.

>
> Mark, any insight in here? thanks
>
> > section index in all cases, we need to check again *after* we got
> > symbol, and if it's not extended, then set index manually.
>
> hum, then we should use '!=', right?
>
>   if (sym->st_shndx != SHN_XINDEX)
>     *sym_sec_idx = sym->st_shndx;


yeah, sorry, that was a typo

>
> SNIP
>
> > > > so either
> > > >
> > > > for (id = 0; id < symtab->nr_syms && elf_sym__get(symtab->syms,
> > > > symtab->syms_sec_idx_table, d, &sym, &sym_sec_idx); id++)
> > > >
> > > > or
> > > >
> > > > for (id = 0; id < symtab->nr_syms; id++)
> > > >   if (elf_sym__get(symtab->syms, symtab->syms_sec_idx_table, d, &sym,
> > > > &sym_sec_idx))
> > >
> > > if we go ahead with skipping symbols, this one seems good
> >
> > I think skipping symbols is nicer. If ELF is totally broken, then all
> > symbols are going to be ignored anyway. If it's some one-off issue for
> > a specific symbol, we'll just ignore it (unfortunately, silently).
>
> agreed, I'll use this

sounds good

>
> thanks,
> jirka
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-23 18:51         ` Jiri Olsa
  2021-01-23 20:07           ` Andrii Nakryiko
@ 2021-01-23 20:08           ` Mark Wielaard
  2021-01-23 20:15             ` Jiri Olsa
  1 sibling, 1 reply; 21+ messages in thread
From: Mark Wielaard @ 2021-01-23 20:08 UTC (permalink / raw)
  To: Jiri Olsa, Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence

Hi Jiri,

On Sat, 2021-01-23 at 19:51 +0100, Jiri Olsa wrote:
> On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:
> > 
> > > I don't understand this..  gelf_getsymshndx will return both
> > > symbol and proper index, no? also sym_sec_idx is already
> > > assigned from previou call
> > 
> > Reading (some) implementation of gelf_getsymshndx() that I found
> > online, it won't set sym_sec_idx, if the symbol *doesn't* use
> > extended
> > numbering. But it will still return symbol data. So to return the
> 
> the latest upstream code seems to set it always,
> but I agree we should be careful
> 
> Mark, any insight in here? thanks

GElf_Sym *
gelf_getsymshndx (Elf_Data *symdata, Elf_Data *shndxdata, int ndx,
                  GElf_Sym *dst, Elf32_Word *dstshndx)

Will always set *dst, but only set *dstshndx if both it and shndxdata
are not NULL and no error occurred (the function returns NULL and set
libelf_error in case of error).

So as long as shndxdata != NULL you can rely on *dstshndx being set.
Otherwise you get the section index from dst->st_shndx.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-23 20:08           ` Mark Wielaard
@ 2021-01-23 20:15             ` Jiri Olsa
  0 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-23 20:15 UTC (permalink / raw)
  To: Mark Wielaard
  Cc: Andrii Nakryiko, Jiri Olsa, Arnaldo Carvalho de Melo,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, dwarves,
	Networking, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence

On Sat, Jan 23, 2021 at 09:08:15PM +0100, Mark Wielaard wrote:
> Hi Jiri,
> 
> On Sat, 2021-01-23 at 19:51 +0100, Jiri Olsa wrote:
> > On Fri, Jan 22, 2021 at 02:55:51PM -0800, Andrii Nakryiko wrote:
> > > 
> > > > I don't understand this..  gelf_getsymshndx will return both
> > > > symbol and proper index, no? also sym_sec_idx is already
> > > > assigned from previou call
> > > 
> > > Reading (some) implementation of gelf_getsymshndx() that I found
> > > online, it won't set sym_sec_idx, if the symbol *doesn't* use
> > > extended
> > > numbering. But it will still return symbol data. So to return the
> > 
> > the latest upstream code seems to set it always,
> > but I agree we should be careful
> > 
> > Mark, any insight in here? thanks
> 
> GElf_Sym *
> gelf_getsymshndx (Elf_Data *symdata, Elf_Data *shndxdata, int ndx,
>                   GElf_Sym *dst, Elf32_Word *dstshndx)
> 
> Will always set *dst, but only set *dstshndx if both it and shndxdata
> are not NULL and no error occurred (the function returns NULL and set
> libelf_error in case of error).
> 
> So as long as shndxdata != NULL you can rely on *dstshndx being set.
> Otherwise you get the section index from dst->st_shndx.

ok, so it's as Andrii said, I'll make the extra check then

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-23 20:07           ` Andrii Nakryiko
@ 2021-01-23 20:21             ` Mark Wielaard
  0 siblings, 0 replies; 21+ messages in thread
From: Mark Wielaard @ 2021-01-23 20:21 UTC (permalink / raw)
  To: Andrii Nakryiko, Jiri Olsa
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence

Hi,

On Sat, 2021-01-23 at 12:07 -0800, Andrii Nakryiko wrote:
> > the latest upstream code seems to set it always,
> > but I agree we should be careful
> 
> oh, then maybe it's not necessary. I honestly don't even know where
> the authoritative source code of libelf is, so I just found some
> random source code with Google.

The elfutils.org libelf implementation can be found here:
https://sourceware.org/git/?p=elfutils.git;a=tree;f=libelf;hb=HEAD

There are some other implementations, but some aren't maintained and
others aren't packaged for any distro (anymore). libelf is a semi-
standard "SVR4 Unix" library, so you might also find it for some none
GNU/Linux OSes like Solaris. The ELF specification itself is contained
in the System V Application Binary Interface (gABI). The libelf library
itself isn't actually officially part of the specification. But we
still do try to keep the implementations (source) compatible through
the generic-abi mailinglist.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-21 23:32   ` Andrii Nakryiko
  2021-01-22  9:32     ` Jiri Olsa
  2021-01-22 20:46     ` Jiri Olsa
@ 2021-01-23 21:23     ` Jiri Olsa
  2021-01-24  6:08       ` Andrii Nakryiko
  2 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-23 21:23 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:

SNIP

> But the current variant looks broken. Oh, and
> elf_symtab__for_each_symbol() is similarly broken, can you please fix
> that as well?

we'll have to change its callers a bit, because of hanging 'else'
I'll send this separately if that's ok, when I figure out how to
test ctf code

jirka


---
diff --git a/elf_symtab.h b/elf_symtab.h
index 489e2b1a3505..6823a8c37ecf 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h
@@ -99,10 +99,9 @@ elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
  * @index: uint32_t index
  * @sym: GElf_Sym iterator
  */
-#define elf_symtab__for_each_symbol(symtab, index, sym) \
-	for (index = 0, gelf_getsym(symtab->syms, index, &sym);\
-	     index < symtab->nr_syms; \
-	     index++, gelf_getsym(symtab->syms, index, &sym))
+#define elf_symtab__for_each_symbol(symtab, index, sym)		\
+	for (index = 0; index < symtab->nr_syms; index++)	\
+		if (gelf_getsym(symtab->syms, index, &sym))
 
 /**
  * elf_symtab__for_each_symbol_index - iterate through all the symbols,
diff --git a/libctf.h b/libctf.h
index 749be8955c52..ee5412bec77e 100644
--- a/libctf.h
+++ b/libctf.h
@@ -90,11 +90,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
  */
 #define ctf__for_each_symtab_function(ctf, index, sym)			      \
 	elf_symtab__for_each_symbol(ctf->symtab, index, sym)		      \
-		if (ctf__ignore_symtab_function(&sym,			      \
+		if (!ctf__ignore_symtab_function(&sym,			      \
 						elf_sym__name(&sym,	      \
 							      ctf->symtab)))  \
-			continue;					      \
-		else
 
 /**
  * ctf__for_each_symtab_object - iterate thru all the symtab objects
@@ -105,11 +103,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
  */
 #define ctf__for_each_symtab_object(ctf, index, sym)			      \
 	elf_symtab__for_each_symbol(ctf->symtab, index, sym)		      \
-		if (ctf__ignore_symtab_object(&sym,			      \
+		if (!ctf__ignore_symtab_object(&sym,			      \
 					      elf_sym__name(&sym,	      \
 							    ctf->symtab)))    \
-			continue;					      \
-		else
 
 
 #endif /* _LIBCTF_H */


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-23 21:23     ` Jiri Olsa
@ 2021-01-24  6:08       ` Andrii Nakryiko
  0 siblings, 0 replies; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-24  6:08 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Sat, Jan 23, 2021 at 1:23 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Jan 21, 2021 at 03:32:40PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > But the current variant looks broken. Oh, and
> > elf_symtab__for_each_symbol() is similarly broken, can you please fix
> > that as well?
>
> we'll have to change its callers a bit, because of hanging 'else'
> I'll send this separately if that's ok, when I figure out how to
> test ctf code
>

oh, else sucks. Sure, no problem doing it separately.

> jirka
>
>
> ---
> diff --git a/elf_symtab.h b/elf_symtab.h
> index 489e2b1a3505..6823a8c37ecf 100644
> --- a/elf_symtab.h
> +++ b/elf_symtab.h
> @@ -99,10 +99,9 @@ elf_sym__get(Elf_Data *syms, Elf_Data *syms_sec_idx_table,
>   * @index: uint32_t index
>   * @sym: GElf_Sym iterator
>   */
> -#define elf_symtab__for_each_symbol(symtab, index, sym) \
> -       for (index = 0, gelf_getsym(symtab->syms, index, &sym);\
> -            index < symtab->nr_syms; \
> -            index++, gelf_getsym(symtab->syms, index, &sym))
> +#define elf_symtab__for_each_symbol(symtab, index, sym)                \
> +       for (index = 0; index < symtab->nr_syms; index++)       \
> +               if (gelf_getsym(symtab->syms, index, &sym))
>
>  /**
>   * elf_symtab__for_each_symbol_index - iterate through all the symbols,
> diff --git a/libctf.h b/libctf.h
> index 749be8955c52..ee5412bec77e 100644
> --- a/libctf.h
> +++ b/libctf.h
> @@ -90,11 +90,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
>   */
>  #define ctf__for_each_symtab_function(ctf, index, sym)                       \
>         elf_symtab__for_each_symbol(ctf->symtab, index, sym)                  \
> -               if (ctf__ignore_symtab_function(&sym,                         \
> +               if (!ctf__ignore_symtab_function(&sym,                        \
>                                                 elf_sym__name(&sym,           \
>                                                               ctf->symtab)))  \
> -                       continue;                                             \
> -               else
>
>  /**
>   * ctf__for_each_symtab_object - iterate thru all the symtab objects
> @@ -105,11 +103,9 @@ char *ctf__string(struct ctf *ctf, uint32_t ref);
>   */
>  #define ctf__for_each_symtab_object(ctf, index, sym)                         \
>         elf_symtab__for_each_symbol(ctf->symtab, index, sym)                  \
> -               if (ctf__ignore_symtab_object(&sym,                           \
> +               if (!ctf__ignore_symtab_object(&sym,                          \
>                                               elf_sym__name(&sym,             \
>                                                             ctf->symtab)))    \
> -                       continue;                                             \
> -               else
>
>
>  #endif /* _LIBCTF_H */
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-20  2:03   ` Andrii Nakryiko
@ 2021-01-20 12:25     ` Jiri Olsa
  0 siblings, 0 replies; 21+ messages in thread
From: Jiri Olsa @ 2021-01-20 12:25 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

On Tue, Jan 19, 2021 at 06:03:28PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 19, 2021 at 2:16 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 id needed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 18 ++++++++++++++++++
> >  elf_symtab.c  | 31 ++++++++++++++++++++++++++++++-
> >  elf_symtab.h  |  1 +
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 5557c9efd365..2ab6815dc2b3 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -609,6 +609,24 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
> >
> >         /* search within symtab for percpu variables */
> >         elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
> 
> How about we introduce elf_symtab__for_each_symbol variant that uses
> gelf_getsymshndx undercover and returns a real section index as the
> 4th macro argument?

ok, that's good idea

> 
> > +               struct elf_symtab *symtab = btfe->symtab;
> > +
> > +               /*
> > +                * Symbol's st_shndx needs to be translated with the
> > +                * extended section index table.
> > +                */
> > +               if (sym.st_shndx == SHN_XINDEX) {
> > +                       Elf32_Word xndx;
> > +
> > +                       if (!symtab->syms_shndx) {
> > +                               fprintf(stderr, "SHN_XINDEX found, but no symtab shndx section.\n");
> > +                               return -1;
> > +                       }
> > +
> > +                       if (gelf_getsymshndx(symtab->syms, symtab->syms_shndx, core_id, &sym, &xndx))
> > +                               sym.st_shndx = xndx;
> 
> This bit makes me really nervous and it looks very unclean, which is
> why I think providing correct section index as part of iterator macro
> is better approach. And all this code would just go away.

ok

> 
> > +               }
> > +
> >                 if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
> >                         return -1;
> >                 if (collect_function(btfe, &sym))
> > diff --git a/elf_symtab.c b/elf_symtab.c
> > index 741990ea3ed9..c1def0189652 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 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, &index);
> >
> >         if (sec == NULL)
> >                 return NULL;
> > @@ -29,6 +31,8 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
> >         if (gelf_getshdr(sec, &shdr) == NULL)
> >                 return NULL;
> >
> > +       int xindex = elf_scnshndx(sec);
> 
> move this closer to where you check (xindex > 0)? Can you please leave
> a small comment that this returns extended section index table's
> section index (I know, this is horrible), if it exists. It's
> notoriously hard to find anything about libelf's API, especially for
> obscure APIs like this.

it's here because 'sec' gets overwitten shortly on with string table
I'll add some comment

> 
> > +
> >         struct elf_symtab *symtab = malloc(sizeof(*symtab));
> >         if (symtab == NULL)
> >                 return NULL;
> > @@ -49,6 +53,31 @@ 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 (xindex > 0) {
> > +               GElf_Shdr shdr_shndx;
> > +               Elf_Scn *sec_shndx;
> > +
> > +               sec_shndx = elf_getscn(elf, xindex);
> > +               if (sec_shndx == NULL)
> > +                       goto out_free_name;
> > +
> > +               if (gelf_getshdr(sec_shndx, &shdr_shndx) == NULL)
> > +                       goto out_free_name;
> > +
> > +               /* Extra check to verify it belongs to the .symtab */
> > +               if (index != shdr_shndx.sh_link)
> > +                       goto out_free_name;
> 
> you can also verify that section type is SHT_SYMTAB_SHNDX

ok

> 
> > +
> > +               symtab->syms_shndx = elf_getdata(elf_getscn(elf, xindex), NULL);
> 
> my mind breaks looking at all those shndxs, especially in this case

you better not look at elfutils code then ;-)

> where it's not a section number, but rather data. How about we call it
> something like symtab->syms_sec_idx_table or something similar but
> human-comprehensible?

sure, will change

> 
> > +               if (symtab->syms_shndx == 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..f9277a48ed4c 100644
> > --- a/elf_symtab.h
> > +++ b/elf_symtab.h
> > @@ -16,6 +16,7 @@ struct elf_symtab {
> >         uint32_t  nr_syms;
> >         Elf_Data  *syms;
> >         Elf_Data  *symstrs;
> > +       Elf_Data  *syms_shndx;
> 
> please add comment mentioning that it's data of SHT_SYMTAB_SHNDX
> section, it will make it a bit easier to Google what that is

ok

thanks,
jirka


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-19 22:12 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
@ 2021-01-20  2:03   ` Andrii Nakryiko
  2021-01-20 12:25     ` Jiri Olsa
  0 siblings, 1 reply; 21+ messages in thread
From: Andrii Nakryiko @ 2021-01-20  2:03 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, dwarves, Networking, bpf, Yonghong Song,
	Hao Luo, Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Joe Lawrence, Mark Wielaard

On Tue, Jan 19, 2021 at 2:16 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 id needed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  btf_encoder.c | 18 ++++++++++++++++++
>  elf_symtab.c  | 31 ++++++++++++++++++++++++++++++-
>  elf_symtab.h  |  1 +
>  3 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 5557c9efd365..2ab6815dc2b3 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -609,6 +609,24 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>
>         /* search within symtab for percpu variables */
>         elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {

How about we introduce elf_symtab__for_each_symbol variant that uses
gelf_getsymshndx undercover and returns a real section index as the
4th macro argument?

> +               struct elf_symtab *symtab = btfe->symtab;
> +
> +               /*
> +                * Symbol's st_shndx needs to be translated with the
> +                * extended section index table.
> +                */
> +               if (sym.st_shndx == SHN_XINDEX) {
> +                       Elf32_Word xndx;
> +
> +                       if (!symtab->syms_shndx) {
> +                               fprintf(stderr, "SHN_XINDEX found, but no symtab shndx section.\n");
> +                               return -1;
> +                       }
> +
> +                       if (gelf_getsymshndx(symtab->syms, symtab->syms_shndx, core_id, &sym, &xndx))
> +                               sym.st_shndx = xndx;

This bit makes me really nervous and it looks very unclean, which is
why I think providing correct section index as part of iterator macro
is better approach. And all this code would just go away.

> +               }
> +
>                 if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
>                         return -1;
>                 if (collect_function(btfe, &sym))
> diff --git a/elf_symtab.c b/elf_symtab.c
> index 741990ea3ed9..c1def0189652 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 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, &index);
>
>         if (sec == NULL)
>                 return NULL;
> @@ -29,6 +31,8 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
>         if (gelf_getshdr(sec, &shdr) == NULL)
>                 return NULL;
>
> +       int xindex = elf_scnshndx(sec);

move this closer to where you check (xindex > 0)? Can you please leave
a small comment that this returns extended section index table's
section index (I know, this is horrible), if it exists. It's
notoriously hard to find anything about libelf's API, especially for
obscure APIs like this.

> +
>         struct elf_symtab *symtab = malloc(sizeof(*symtab));
>         if (symtab == NULL)
>                 return NULL;
> @@ -49,6 +53,31 @@ 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 (xindex > 0) {
> +               GElf_Shdr shdr_shndx;
> +               Elf_Scn *sec_shndx;
> +
> +               sec_shndx = elf_getscn(elf, xindex);
> +               if (sec_shndx == NULL)
> +                       goto out_free_name;
> +
> +               if (gelf_getshdr(sec_shndx, &shdr_shndx) == NULL)
> +                       goto out_free_name;
> +
> +               /* Extra check to verify it belongs to the .symtab */
> +               if (index != shdr_shndx.sh_link)
> +                       goto out_free_name;

you can also verify that section type is SHT_SYMTAB_SHNDX

> +
> +               symtab->syms_shndx = elf_getdata(elf_getscn(elf, xindex), NULL);

my mind breaks looking at all those shndxs, especially in this case
where it's not a section number, but rather data. How about we call it
something like symtab->syms_sec_idx_table or something similar but
human-comprehensible?

> +               if (symtab->syms_shndx == 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..f9277a48ed4c 100644
> --- a/elf_symtab.h
> +++ b/elf_symtab.h
> @@ -16,6 +16,7 @@ struct elf_symtab {
>         uint32_t  nr_syms;
>         Elf_Data  *syms;
>         Elf_Data  *symstrs;
> +       Elf_Data  *syms_shndx;

please add comment mentioning that it's data of SHT_SYMTAB_SHNDX
section, it will make it a bit easier to Google what that is

>         char      *name;
>  };
>
> --
> 2.27.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values
  2021-01-19 22:12 [PATCH 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
@ 2021-01-19 22:12 ` Jiri Olsa
  2021-01-20  2:03   ` Andrii Nakryiko
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2021-01-19 22:12 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Joe Lawrence, Mark Wielaard

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 id needed.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 18 ++++++++++++++++++
 elf_symtab.c  | 31 ++++++++++++++++++++++++++++++-
 elf_symtab.h  |  1 +
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 5557c9efd365..2ab6815dc2b3 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -609,6 +609,24 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 
 	/* search within symtab for percpu variables */
 	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
+		struct elf_symtab *symtab = btfe->symtab;
+
+		/*
+		 * Symbol's st_shndx needs to be translated with the
+		 * extended section index table.
+		 */
+		if (sym.st_shndx == SHN_XINDEX) {
+			Elf32_Word xndx;
+
+			if (!symtab->syms_shndx) {
+				fprintf(stderr, "SHN_XINDEX found, but no symtab shndx section.\n");
+				return -1;
+			}
+
+			if (gelf_getsymshndx(symtab->syms, symtab->syms_shndx, core_id, &sym, &xndx))
+				sym.st_shndx = xndx;
+		}
+
 		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
 			return -1;
 		if (collect_function(btfe, &sym))
diff --git a/elf_symtab.c b/elf_symtab.c
index 741990ea3ed9..c1def0189652 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 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, &index);
 
 	if (sec == NULL)
 		return NULL;
@@ -29,6 +31,8 @@ struct elf_symtab *elf_symtab__new(const char *name, Elf *elf, GElf_Ehdr *ehdr)
 	if (gelf_getshdr(sec, &shdr) == NULL)
 		return NULL;
 
+	int xindex = elf_scnshndx(sec);
+
 	struct elf_symtab *symtab = malloc(sizeof(*symtab));
 	if (symtab == NULL)
 		return NULL;
@@ -49,6 +53,31 @@ 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 (xindex > 0) {
+		GElf_Shdr shdr_shndx;
+		Elf_Scn *sec_shndx;
+
+		sec_shndx = elf_getscn(elf, xindex);
+		if (sec_shndx == NULL)
+			goto out_free_name;
+
+		if (gelf_getshdr(sec_shndx, &shdr_shndx) == NULL)
+			goto out_free_name;
+
+		/* Extra check to verify it belongs to the .symtab */
+		if (index != shdr_shndx.sh_link)
+			goto out_free_name;
+
+		symtab->syms_shndx = elf_getdata(elf_getscn(elf, xindex), NULL);
+		if (symtab->syms_shndx == 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..f9277a48ed4c 100644
--- a/elf_symtab.h
+++ b/elf_symtab.h
@@ -16,6 +16,7 @@ struct elf_symtab {
 	uint32_t  nr_syms;
 	Elf_Data  *syms;
 	Elf_Data  *symstrs;
+	Elf_Data  *syms_shndx;
 	char	  *name;
 };
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2021-01-24  6:09 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 20:22 [PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
2021-01-21 20:22 ` [PATCH 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name Jiri Olsa
2021-01-21 23:10   ` Andrii Nakryiko
2021-01-21 23:34     ` Jiri Olsa
2021-01-21 20:22 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
2021-01-21 23:32   ` Andrii Nakryiko
2021-01-22  9:32     ` Jiri Olsa
2021-01-22 20:46     ` Jiri Olsa
2021-01-22 22:55       ` Andrii Nakryiko
2021-01-23 18:51         ` Jiri Olsa
2021-01-23 20:07           ` Andrii Nakryiko
2021-01-23 20:21             ` Mark Wielaard
2021-01-23 20:08           ` Mark Wielaard
2021-01-23 20:15             ` Jiri Olsa
2021-01-23 21:23     ` Jiri Olsa
2021-01-24  6:08       ` Andrii Nakryiko
2021-01-21 20:22 ` [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed Jiri Olsa
2021-01-21 23:46   ` Andrii Nakryiko
  -- strict thread matches above, loose matches on Subject: below --
2021-01-19 22:12 [PATCH 0/3] dwarves,libbpf: Add support to use optional extended section index table Jiri Olsa
2021-01-19 22:12 ` [PATCH 2/3] bpf_encoder: Translate SHN_XINDEX in symbol's st_shndx values Jiri Olsa
2021-01-20  2:03   ` Andrii Nakryiko
2021-01-20 12:25     ` Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).