[PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[Andrii Nakryiko]

Two bug fixes to make pahole emit correct kernel module BTF variable
information.

Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@redhat.com>

Andrii Nakryiko (2):
  btf_encoder: fix BTF variable generation for kernel modules
  btf_encoder: fix skipping per-CPU variables at offset 0

 btf_encoder.c | 61 +++++++++++++++++++++++++++++++++------------------
 libbtf.c      |  1 +
 libbtf.h      |  1 +
 3 files changed, 42 insertions(+), 21 deletions(-)

-- 
2.24.1

[PATCH dwarves 1/2] btf_encoder: fix BTF variable generation for kernel modules

[Andrii Nakryiko]

Fix pahole's logic for determining per-CPU variables. For vmlinux,
btfe->percpu_base_addr is always 0, so it didn't matter at which point to
subtract it to get offset that later was matched against corresponding ELF
symbol.

For kernel module, though, the situation is different. Kernel module's per-CPU
data section has non-zero offset, which is taken into account in all DWARF
variable addresses calculation. For such cases, it's important to subtract
section offset (btfe->percpu_base_addr) before ELF symbol look up is
performed.

This patch also records per-CPU data section size and uses it for early
filtering of non-per-CPU variables by their address.

Fixes: 2e719cca6672 ("btf_encoder: revamp how per-CPU variables are encoded")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 btf_encoder.c | 21 ++++++++++++++++-----
 libbtf.c      |  1 +
 libbtf.h      |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index c40f059580da..a7d484765ce2 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -651,7 +651,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		printf("search cu '%s' for percpu global variables.\n", cu->name);
 
 	cu__for_each_variable(cu, core_id, pos) {
-		uint32_t size, type, linkage, offset;
+		uint32_t size, type, linkage;
 		const char *name;
 		uint64_t addr;
 		int id;
@@ -665,12 +665,24 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 
 		/* addr has to be recorded before we follow spec */
 		addr = var->ip.addr;
-		if (var->spec)
-			var = var->spec;
+
+		/* DWARF takes into account .data..percpu section offset
+		 * within its segment, which for vmlinux is 0, but for kernel
+		 * modules is >0. ELF symbols, on the other hand, don't take
+		 * into account these offsets (as they are relative to the
+		 * section start), so to match DWARF and ELF symbols we need
+		 * to negate the section base address here.
+		 */
+		if (addr < btfe->percpu_base_addr || addr >= btfe->percpu_base_addr + btfe->percpu_sec_sz)
+			continue;
+		addr -= btfe->percpu_base_addr;
 
 		if (!percpu_var_exists(addr, &size, &name))
 			continue; /* not a per-CPU variable */
 
+		if (var->spec)
+			var = var->spec;
+
 		if (var->ip.tag.type == 0) {
 			fprintf(stderr, "error: found variable '%s' in CU '%s' that has void type\n",
 				name, cu->name);
@@ -701,8 +713,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		 * add a BTF_VAR_SECINFO in btfe->percpu_secinfo, which will be added into
 		 * btfe->types later when we add BTF_VAR_DATASEC.
 		 */
-		offset = addr - btfe->percpu_base_addr;
-		id = btf_elf__add_var_secinfo(&btfe->percpu_secinfo, id, offset, size);
+		id = btf_elf__add_var_secinfo(&btfe->percpu_secinfo, id, addr, size);
 		if (id < 0) {
 			err = -1;
 			fprintf(stderr, "error: failed to encode section info for variable '%s' at addr 0x%lx\n",
diff --git a/libbtf.c b/libbtf.c
index 246762c4b4e1..16e1d451e433 100644
--- a/libbtf.c
+++ b/libbtf.c
@@ -170,6 +170,7 @@ try_as_raw_btf:
 	}
 	btfe->percpu_shndx = elf_ndxscn(sec);
 	btfe->percpu_base_addr = shdr.sh_addr;
+	btfe->percpu_sec_sz = shdr.sh_size;
 
 	return btfe;
 
diff --git a/libbtf.h b/libbtf.h
index 71f6cecbea93..191f5862a695 100644
--- a/libbtf.h
+++ b/libbtf.h
@@ -26,6 +26,7 @@ struct btf_elf {
 	bool		  raw_btf; // "/sys/kernel/btf/vmlinux"
 	uint32_t	  percpu_shndx;
 	uint64_t	  percpu_base_addr;
+	uint64_t	  percpu_sec_sz;
 	struct btf	  *btf;
 	struct btf	  *base_btf;
 };
-- 
2.24.1

[PATCH dwarves 2/2] btf_encoder: fix skipping per-CPU variables at offset 0

[Andrii Nakryiko]

Adjust pahole logic of skipping any per-CPU symbol with offset 0, which is
especially bad for kernel modules, because it most certainly skips the very
first per-CPU variable.

Instead, do collect per-CPU ELF symbol with 0 offset, but do extra check for
non-kernel module case by verifying that ELF symbol name and DWARF variable
name match. Due to the bug of DWARF name of variable sometimes being NULL,
this is necessarily too pessimistic check (e.g., on my vmlinux image,
fixed_percpu_data variable is still not emitted due to missing DWARF variable
name), it allows to emit data for all module per-CPU variables.

Fixes: f3d9054ba8ff ("btf_encoder: Teach pahole to store percpu variables in vmlinux BTF.")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 btf_encoder.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index a7d484765ce2..1d7817078f89 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -412,21 +412,6 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
 		return 0;
 
 	addr = elf_sym__value(sym);
-	/*
-	 * Store only those symbols that have allocated space in the percpu section.
-	 * This excludes the following three types of symbols:
-	 *
-	 *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
-	 *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
-	 *  3. __exitcall(fn), functions which are labeled as exit calls.
-	 *
-	 * In addition, the variables defined using DEFINE_PERCPU_FIRST are
-	 * also not included, which currently includes:
-	 *
-	 *  1. fixed_percpu_data
-	 */
-	if (!addr)
-		return 0;
 
 	size = elf_sym__size(sym);
 	if (!size)
@@ -652,7 +637,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 
 	cu__for_each_variable(cu, core_id, pos) {
 		uint32_t size, type, linkage;
-		const char *name;
+		const char *name, *dwarf_name;
 		uint64_t addr;
 		int id;
 
@@ -680,6 +665,29 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		if (!percpu_var_exists(addr, &size, &name))
 			continue; /* not a per-CPU variable */
 
+		/* A lot of "special" DWARF variables (e.g, __UNIQUE_ID___xxx)
+		 * have addr == 0, which is the same as, say, valid
+		 * fixed_percpu_data per-CPU variable. To distinguish between
+		 * them, additionally compare DWARF and ELF symbol names. If
+		 * DWARF doesn't provide proper name, pessimistically assume
+		 * bad variable.
+		 *
+		 * Examples of such special variables are:
+		 *
+		 *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
+		 *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
+		 *  3. __exitcall(fn), functions which are labeled as exit calls.
+		 *
+		 *  This is relevant only for vmlinux image, as for kernel
+		 *  modules per-CPU data section has non-zero offset so all
+		 *  per-CPU symbols have non-zero values.
+		 */
+		if (var->ip.addr == 0) {
+			dwarf_name = variable__name(var, cu);
+			if (!dwarf_name || strcmp(dwarf_name, name))
+				continue;
+		}
+
 		if (var->spec)
 			var = var->spec;
 
-- 
2.24.1

Re: [PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[Jiri Olsa]

On Thu, Dec 10, 2020 at 08:11:36PM -0800, Andrii Nakryiko wrote:
> Two bug fixes to make pahole emit correct kernel module BTF variable
> information.
> 
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> 
> Andrii Nakryiko (2):
>   btf_encoder: fix BTF variable generation for kernel modules
>   btf_encoder: fix skipping per-CPU variables at offset 0

Acked-by: Jiri Olsa <jolsa@redhat.com>

jirka

> 
>  btf_encoder.c | 61 +++++++++++++++++++++++++++++++++------------------
>  libbtf.c      |  1 +
>  libbtf.h      |  1 +
>  3 files changed, 42 insertions(+), 21 deletions(-)
> 
> -- 
> 2.24.1
> 

Re: [PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[Arnaldo Carvalho de Melo]

Em Sun, Dec 13, 2020 at 09:27:57PM +0100, Jiri Olsa escreveu:
> On Thu, Dec 10, 2020 at 08:11:36PM -0800, Andrii Nakryiko wrote:
> > Two bug fixes to make pahole emit correct kernel module BTF variable
> > information.
> > 
> > Cc: Hao Luo <haoluo@google.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > 
> > Andrii Nakryiko (2):
> >   btf_encoder: fix BTF variable generation for kernel modules
> >   btf_encoder: fix skipping per-CPU variables at offset 0
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo

Re: [PATCH dwarves 2/2] btf_encoder: fix skipping per-CPU variables at offset 0

[Hao Luo]

Andrii, sorry for the late reply. This change looks good. It makes
much more sense than before.

Thanks,
Hao

On Thu, Dec 10, 2020 at 8:11 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Adjust pahole logic of skipping any per-CPU symbol with offset 0, which is
> especially bad for kernel modules, because it most certainly skips the very
> first per-CPU variable.
>
> Instead, do collect per-CPU ELF symbol with 0 offset, but do extra check for
> non-kernel module case by verifying that ELF symbol name and DWARF variable
> name match. Due to the bug of DWARF name of variable sometimes being NULL,
> this is necessarily too pessimistic check (e.g., on my vmlinux image,
> fixed_percpu_data variable is still not emitted due to missing DWARF variable
> name), it allows to emit data for all module per-CPU variables.
>
> Fixes: f3d9054ba8ff ("btf_encoder: Teach pahole to store percpu variables in vmlinux BTF.")
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  btf_encoder.c | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index a7d484765ce2..1d7817078f89 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -412,21 +412,6 @@ static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
>                 return 0;
>
>         addr = elf_sym__value(sym);
> -       /*
> -        * Store only those symbols that have allocated space in the percpu section.
> -        * This excludes the following three types of symbols:
> -        *
> -        *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
> -        *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
> -        *  3. __exitcall(fn), functions which are labeled as exit calls.
> -        *
> -        * In addition, the variables defined using DEFINE_PERCPU_FIRST are
> -        * also not included, which currently includes:
> -        *
> -        *  1. fixed_percpu_data
> -        */
> -       if (!addr)
> -               return 0;
>
>         size = elf_sym__size(sym);
>         if (!size)
> @@ -652,7 +637,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>
>         cu__for_each_variable(cu, core_id, pos) {
>                 uint32_t size, type, linkage;
> -               const char *name;
> +               const char *name, *dwarf_name;
>                 uint64_t addr;
>                 int id;
>
> @@ -680,6 +665,29 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                 if (!percpu_var_exists(addr, &size, &name))
>                         continue; /* not a per-CPU variable */
>
> +               /* A lot of "special" DWARF variables (e.g, __UNIQUE_ID___xxx)
> +                * have addr == 0, which is the same as, say, valid
> +                * fixed_percpu_data per-CPU variable. To distinguish between
> +                * them, additionally compare DWARF and ELF symbol names. If
> +                * DWARF doesn't provide proper name, pessimistically assume
> +                * bad variable.
> +                *
> +                * Examples of such special variables are:
> +                *
> +                *  1. __ADDRESSABLE(sym), which are forcely emitted as symbols.
> +                *  2. __UNIQUE_ID(prefix), which are introduced to generate unique ids.
> +                *  3. __exitcall(fn), functions which are labeled as exit calls.
> +                *
> +                *  This is relevant only for vmlinux image, as for kernel
> +                *  modules per-CPU data section has non-zero offset so all
> +                *  per-CPU symbols have non-zero values.
> +                */
> +               if (var->ip.addr == 0) {
> +                       dwarf_name = variable__name(var, cu);
> +                       if (!dwarf_name || strcmp(dwarf_name, name))
> +                               continue;
> +               }
> +
>                 if (var->spec)
>                         var = var->spec;
>
> --
> 2.24.1
>

Re: [PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[Andrii Nakryiko]

On Mon, Dec 14, 2020 at 5:43 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Sun, Dec 13, 2020 at 09:27:57PM +0100, Jiri Olsa escreveu:
> > On Thu, Dec 10, 2020 at 08:11:36PM -0800, Andrii Nakryiko wrote:
> > > Two bug fixes to make pahole emit correct kernel module BTF variable
> > > information.
> > >
> > > Cc: Hao Luo <haoluo@google.com>
> > > Cc: Jiri Olsa <jolsa@redhat.com>
> > >
> > > Andrii Nakryiko (2):
> > >   btf_encoder: fix BTF variable generation for kernel modules
> > >   btf_encoder: fix skipping per-CPU variables at offset 0
> >
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> Thanks, applied.
>

Thanks, Arnaldo. I'm not seeing them on
gitolite.kernel.org/pub/scm/devel/pahole/pahole.git, did you push them
somewhere else?

> - Arnaldo
>

Re: [PATCH dwarves 0/2] Fix pahole to emit kernel module BTF variables

[Arnaldo Carvalho de Melo]

Em Mon, Dec 14, 2020 at 05:28:22PM -0800, Andrii Nakryiko escreveu:
> On Mon, Dec 14, 2020 at 5:43 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Sun, Dec 13, 2020 at 09:27:57PM +0100, Jiri Olsa escreveu:
> > > On Thu, Dec 10, 2020 at 08:11:36PM -0800, Andrii Nakryiko wrote:
> > > > Two bug fixes to make pahole emit correct kernel module BTF variable
> > > > information.
> > > >
> > > > Cc: Hao Luo <haoluo@google.com>
> > > > Cc: Jiri Olsa <jolsa@redhat.com>
> > > >
> > > > Andrii Nakryiko (2):
> > > >   btf_encoder: fix BTF variable generation for kernel modules
> > > >   btf_encoder: fix skipping per-CPU variables at offset 0
> > >
> > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> >
> > Thanks, applied.
> >
> 
> Thanks, Arnaldo. I'm not seeing them on
> gitolite.kernel.org/pub/scm/devel/pahole/pahole.git, did you push them
> somewhere else?

I need to push it out, I'm doing kernel builds and tests and will
incorporate Hao's Acked-by.

- Arnaldo