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

[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 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..321f4be6669e 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 shstrndx = ep->e_shstrndx;
+
+	if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
+		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, shstrndx, shp->sh_name);
+		if (str && !strcmp(name, str)) {
 			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 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

[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 0/3] dwarves,libbpf: Add support to use optional extended section index table

[Joe Lawrence]

On Tue, Jan 19, 2021 at 11:12:17PM +0100, Jiri Olsa wrote:
> hi,
> kpatch guys hit an issue with pahole over their vmlinux, which
> contains many (over 100000) sections, pahole crashes.
> 

FWIW this is probably only going to be problem when building the kernel
with -f[function|data]-sections and tipping over 65536 sections.  We
only use these option to determine code deltas, but other users
(FG-ASLR, LTO?) may need to actually build runtime code.

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

Hi Jiri,

Thanks for posting a potential fix, here's what I saw when running it:

1-Installed your scratch build:

% rpm -q --whatprovides $(which pahole)
dwarves-1.19-2.el8.x86_64


2-From the kernel build:

  LD      vmlinux.o
  MODPOST vmlinux.o
  BTF     .btf.vmlinux.bin.o
scripts/link-vmlinux.sh: line 127: 1851330 Segmentation fault      (core dumped) LLVM_OBJCOPY=${OBJCOPY} ${PAHOLE} -J ${1}
objcopy: --change-section-vma .BTF=0x0000000000000000 never used
objcopy: --change-section-lma .BTF=0x0000000000000000 never used
objcopy: error: the input file '.btf.vmlinux.bin' is empty
Failed to generate BTF for vmlinux
Try to disable CONFIG_DEBUG_INFO_BTF
make: *** [Makefile:1050: vmlinux] Error 1


3-coredump backtrace:
...
Core was generated by `pahole -J .tmp_vmlinux.btf'.
...
(gdb) bt
#0  0x00007fc0e81e31c0 in __memcpy_ssse3 () from /lib64/libc.so.6
#1  0x00007fc0e8b6700a in memcpy (__len=306248, __src=<optimized out>, __dest=0x7fc0e91a0010) at /usr/include/bits/string_fortified.h:34
#2  get_vmlinux_addrs (btfe=<optimized out>, pcount=<synthetic pointer>, paddrs=<synthetic pointer>, fl=<synthetic pointer>)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/btf_encoder.c:167
#3  setup_functions (fl=<synthetic pointer>, btfe=<optimized out>) at /usr/src/debug/dwarves-1.19-2.el8.x86_64/btf_encoder.c:251
#4  collect_symbols (collect_percpu_vars=<optimized out>, btfe=<optimized out>)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/btf_encoder.c:645
#5  cu__encode_btf (cu=<optimized out>, verbose=0, force=false, skip_encoding_vars=<optimized out>)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/btf_encoder.c:694
#6  0x000055eb2e4b6cc5 in pahole_stealer (cu=0x55eb2ecbb920, conf_load=<optimized out>)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/pahole.c:2402
#7  0x00007fc0e8b6d2db in finalize_cu_immediately (conf=0x55eb2e6bc0e0 <conf_load>, dcu=0x7ffd88fda9d0, cu=0x55eb2ecbb920, 
    cus=0x55eb2ecbb5d0) at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarf_loader.c:2477
#8  cus__load_module (cus=cus@entry=0x55eb2ecbb5d0, conf=0x55eb2e6bc0e0 <conf_load>, mod=mod@entry=0x55eb2ecbb5f0, dw=0x55eb2ecbe6a0, 
    elf=elf@entry=0x7fc0ad941010, filename=0x7ffd8905b98d ".tmp_vmlinux.btf")
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarf_loader.c:2477
#9  0x00007fc0e8b6d5c5 in cus__process_dwflmod (dwflmod=0x55eb2ecbb5f0, userdata=<optimized out>, name=<optimized out>, 
    base=<optimized out>, arg=0x7ffd8905ab00) at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarf_loader.c:2522
#10 0x00007fc0e88f9c71 in dwfl_getmodules () from /lib64/libdw.so.1
#11 0x00007fc0e8b69cdc in cus__process_file (filename=<optimized out>, fd=5, conf=0x55eb2e6bc0e0 <conf_load>, cus=0x55eb2ecbb5d0)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarf_loader.c:2575
#12 dwarf__load_file (cus=0x55eb2ecbb5d0, conf=0x55eb2e6bc0e0 <conf_load>, filename=<optimized out>)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarf_loader.c:2592
#13 0x00007fc0e8b5cb82 in cus__load_file (cus=cus@entry=0x55eb2ecbb5d0, conf=conf@entry=0x55eb2e6bc0e0 <conf_load>, 
    filename=0x7ffd8905b98d ".tmp_vmlinux.btf") at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarves.c:1963
#14 0x00007fc0e8b5ce29 in cus__load_files (cus=0x55eb2ecbb5d0, conf=0x55eb2e6bc0e0 <conf_load>, filenames=0x7ffd8905aea8)
    at /usr/src/debug/dwarves-1.19-2.el8.x86_64/dwarves.c:2324
#15 0x000055eb2e4b371e in main (argc=3, argv=0x7ffd8905ae98) at /usr/src/debug/dwarves-1.19-2.el8.x86_64/pahole.c:2760


I uploaded a gzipped core file here:
http://people.redhat.com/~jolawren/coredump.gz

If it's easier to get you setup on a repro system, let me know and I can
do that.

Thanks,

-- Joe

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 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name

[Andrii Nakryiko]

On Tue, Jan 19, 2021 at 2:16 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..321f4be6669e 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 shstrndx = ep->e_shstrndx;
> +
> +       if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
> +               return NULL;
>

see comment for patch #3, no need for SHN_XINDEX checks,
elf_getshdrstrndx() handles this transparently

>         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, shstrndx, shp->sh_name);
> +               if (str && !strcmp(name, str)) {
>                         if (index)
>                                 *index = cnt;
>                         break;
> --
> 2.27.0
>

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

[Andrii Nakryiko]

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
>

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 1/3] elf_symtab: Add support for SHN_XINDEX index to elf_section_by_name

[Jiri Olsa]

On Tue, Jan 19, 2021 at 05:23:00PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 19, 2021 at 2:16 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..321f4be6669e 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 shstrndx = ep->e_shstrndx;
> > +
> > +       if (shstrndx == SHN_XINDEX && elf_getshdrstrndx(elf, &shstrndx))
> > +               return NULL;
> >
> 
> see comment for patch #3, no need for SHN_XINDEX checks,
> elf_getshdrstrndx() handles this transparently

ok, will change

jirka

> 
> >         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, shstrndx, shp->sh_name);
> > +               if (str && !strcmp(name, str)) {
> >                         if (index)
> >                                 *index = cnt;
> >                         break;
> > --
> > 2.27.0
> >
> 

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

[Jiri Olsa]

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

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

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

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
>

[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