[PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table

[PATCHv2 0/3] dwarves,libbpf: Add support to use optional extended section index table

[Jiri Olsa]

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

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

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

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

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

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

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

[Jiri Olsa]

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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 dutil.c | 8 ++++++--
 1 file changed, 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

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

[Andrii Nakryiko]

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
>

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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

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

Adding elf_symtab__for_each_symbol_index macro that returns
symbol's section index and usign it in collect_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

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

[Andrii Nakryiko]

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;
>

[...]

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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

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

[Andrii Nakryiko]

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
>

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

[Jiri Olsa]

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

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

[Andrii Nakryiko]

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
>

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

[Mark Wielaard]

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

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

[Mark Wielaard]

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

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

[Jiri Olsa]

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

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

[Jiri Olsa]

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 */

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

[Andrii Nakryiko]

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 */
>

[PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Jiri Olsa]

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

Re: [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Andrii Nakryiko]

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
>

[PATCH 0/3] dwarves,libbpf: Add support to use optional extended section index table

[Jiri Olsa]

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

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

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

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

This patchset based on previously posted fix [1].

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 | 18 ++++++++++++++++++
 dutil.c       |  8 ++++++--
 elf_symtab.c  | 31 ++++++++++++++++++++++++++++++-
 elf_symtab.h  |  1 +
 4 files changed, 55 insertions(+), 3 deletions(-)


libbpf:

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

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

[PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Jiri Olsa]

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.

In such case we need to call elf_getshdrstrndx to get the
proper string table index.

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

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 3c3f2bc6c652..4fe987846bc0 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -863,6 +863,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);
@@ -887,7 +888,16 @@ 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)) {
+
+	/*
+	 * Get string table index from extended section index
+	 * table if needed.
+	 */
+	shstrndx = ehdr.e_shstrndx;
+	if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
+		goto done;
+
+	if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
 		pr_warn("failed to get e_shstrndx from %s\n", path);
 		goto done;
 	}
@@ -902,7 +912,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

Re: [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Andrii Nakryiko]

On Tue, Jan 19, 2021 at 2:15 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.
>
> In such case we need to call elf_getshdrstrndx to get the
> proper string table index.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/lib/bpf/btf.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 3c3f2bc6c652..4fe987846bc0 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -863,6 +863,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);
> @@ -887,7 +888,16 @@ 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)) {
> +
> +       /*
> +        * Get string table index from extended section index
> +        * table if needed.
> +        */
> +       shstrndx = ehdr.e_shstrndx;
> +       if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
> +               goto done;

just use elf_getshdrstrndx() unconditionally, it works for extended
and non-extended numbering (see libbpf.c).

> +
> +       if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
>                 pr_warn("failed to get e_shstrndx from %s\n", path);
>                 goto done;
>         }
> @@ -902,7 +912,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
>

Re: [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Arnaldo Carvalho de Melo]

Em Tue, Jan 19, 2021 at 05:22:08PM -0800, Andrii Nakryiko escreveu:
> On Tue, Jan 19, 2021 at 2:15 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.
> >
> > In such case we need to call elf_getshdrstrndx to get the
> > proper string table index.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/btf.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 3c3f2bc6c652..4fe987846bc0 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -863,6 +863,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);
> > @@ -887,7 +888,16 @@ 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)) {
> > +
> > +       /*
> > +        * Get string table index from extended section index
> > +        * table if needed.
> > +        */
> > +       shstrndx = ehdr.e_shstrndx;
> > +       if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
> > +               goto done;
> 
> just use elf_getshdrstrndx() unconditionally, it works for extended
> and non-extended numbering (see libbpf.c).

Yeah, we even have this in tools/perf/util/symbol-elf.c:

#ifndef HAVE_ELF_GETSHDRSTRNDX_SUPPORT
static int elf_getshdrstrndx(Elf *elf __maybe_unused, size_t *dst __maybe_unused)
{
        pr_err("%s: update your libelf to > 0.140, this one lacks elf_getshdrstrndx().\n", __func__);
        return -1;
}
#endif

And a feature detection for that:

[acme@five perf]$ cat tools/build/feature/test-libelf-getshdrstrndx.c 
// SPDX-License-Identifier: GPL-2.0
#include <libelf.h>

int main(void)
{
	size_t dst;

	return elf_getshdrstrndx(0, &dst);
}
[acme@five perf]$ 

Your implementation would provide a good fallback tho to avoid requiring
updating to 0.140 :-)

- Arnaldo

> > +
> > +       if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
> >                 pr_warn("failed to get e_shstrndx from %s\n", path);
> >                 goto done;
> >         }
> > @@ -902,7 +912,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
> >

-- 

- Arnaldo

Re: [PATCH bpf-next 3/3] libbpf: Use string table index from index table if needed

[Jiri Olsa]

On Tue, Jan 19, 2021 at 05:22:08PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 19, 2021 at 2:15 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.
> >
> > In such case we need to call elf_getshdrstrndx to get the
> > proper string table index.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/btf.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 3c3f2bc6c652..4fe987846bc0 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -863,6 +863,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);
> > @@ -887,7 +888,16 @@ 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)) {
> > +
> > +       /*
> > +        * Get string table index from extended section index
> > +        * table if needed.
> > +        */
> > +       shstrndx = ehdr.e_shstrndx;
> > +       if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
> > +               goto done;
> 
> just use elf_getshdrstrndx() unconditionally, it works for extended
> and non-extended numbering (see libbpf.c).

I did not see that, ok

thanks,
jirka

> 
> > +
> > +       if (!elf_rawdata(elf_getscn(elf, shstrndx), NULL)) {
> >                 pr_warn("failed to get e_shstrndx from %s\n", path);
> >                 goto done;
> >         }
> > @@ -902,7 +912,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
> >
>