[PATCHv2 0/3] btf_encoder: Detect kernel modules

[PATCHv2 0/3] btf_encoder: Detect kernel modules

[Jiri Olsa]

hi,
adding support to detect kernel module and use its
mcount_loc section data as function filter.

v2 changes:
  - initialize return arguments in get_vmlinux_addrs [Andrii]
  - use elf_getscn in elf_section_by_idx [Andrii]
  - use address size from ELF's class for both kmod and vmlinux

thanks,
jirka


---
Jiri Olsa (3):
      btf_encoder: Factor filter_functions function
      btf_encoder: Use address size based on ELF's class
      btf_encoder: Detect kernel module ftrace addresses

 btf_encoder.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 dutil.c       |  10 ++++++++
 dutil.h       |   2 ++
 3 files changed, 164 insertions(+), 27 deletions(-)

[PATCH 1/3] btf_encoder: Factor filter_functions function

[Jiri Olsa]

Reorder the filter_functions function so we can add
processing of kernel modules in following patch.

There's no functional change intended.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 61 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index c40f059580da..0a33388db675 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -101,14 +101,21 @@ static int addrs_cmp(const void *_a, const void *_b)
 	return *a < *b ? -1 : 1;
 }
 
-static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
+static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
+			     unsigned long **paddrs, unsigned long *pcount)
 {
-	unsigned long *addrs, count, offset, i;
-	int functions_valid = 0;
+	unsigned long *addrs, count, offset;
 	Elf_Data *data;
 	GElf_Shdr shdr;
 	Elf_Scn *sec;
 
+	/* Initialize for the sake of all error paths below. */
+	*paddrs = NULL;
+	*pcount = 0;
+
+	if (!fl->mcount_start || !fl->mcount_stop)
+		return 0;
+
 	/*
 	 * Find mcount addressed marked by __start_mcount_loc
 	 * and __stop_mcount_loc symbols and load them into
@@ -138,7 +145,32 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	}
 
 	memcpy(addrs, data->d_buf + offset, count * sizeof(addrs[0]));
+	*paddrs = addrs;
+	*pcount = count;
+	return 0;
+}
+
+static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
+{
+	unsigned long *addrs, count, i;
+	int functions_valid = 0;
+
+	/*
+	 * Check if we are processing vmlinux image and
+	 * get mcount data if it's detected.
+	 */
+	if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
+		return -1;
+
+	if (!addrs) {
+		if (btf_elf__verbose)
+			printf("ftrace symbols not detected, falling back to DWARF data\n");
+		delete_functions();
+		return 0;
+	}
+
 	qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
+	qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
 
 	/*
 	 * Let's got through all collected functions and filter
@@ -162,6 +194,9 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 
 	functions_cnt = functions_valid;
 	free(addrs);
+
+	if (btf_elf__verbose)
+		printf("Found %d functions!\n", functions_cnt);
 	return 0;
 }
 
@@ -470,11 +505,6 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 		fl->mcount_stop = sym->st_value;
 }
 
-static int has_all_symbols(struct funcs_layout *fl)
-{
-	return fl->mcount_start && fl->mcount_stop;
-}
-
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
 	struct funcs_layout fl = { };
@@ -501,18 +531,9 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 			printf("Found %d per-CPU variables!\n", percpu_var_cnt);
 	}
 
-	if (functions_cnt && has_all_symbols(&fl)) {
-		qsort(functions, functions_cnt, sizeof(functions[0]), functions_cmp);
-		if (filter_functions(btfe, &fl)) {
-			fprintf(stderr, "Failed to filter dwarf functions\n");
-			return -1;
-		}
-		if (btf_elf__verbose)
-			printf("Found %d functions!\n", functions_cnt);
-	} else {
-		if (btf_elf__verbose)
-			printf("ftrace symbols not detected, falling back to DWARF data\n");
-		delete_functions();
+	if (functions_cnt && setup_functions(btfe, &fl)) {
+		fprintf(stderr, "Failed to filter DWARF functions\n");
+		return -1;
 	}
 
 	return 0;
-- 
2.26.2

[PATCH 2/3] btf_encoder: Use address size based on ELF's class

[Jiri Olsa]

We can't assume the address size is always size of unsigned
long, we have to use directly the ELF's address size.

Changing addrs array to __u64 and convert 32 bit address
values when copying from ELF section.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 0a33388db675..398be0fbf7c7 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -93,8 +93,8 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 
 static int addrs_cmp(const void *_a, const void *_b)
 {
-	const unsigned long *a = _a;
-	const unsigned long *b = _b;
+	const __u64 *a = _a;
+	const __u64 *b = _b;
 
 	if (*a == *b)
 		return 0;
@@ -102,9 +102,10 @@ static int addrs_cmp(const void *_a, const void *_b)
 }
 
 static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
-			     unsigned long **paddrs, unsigned long *pcount)
+			     __u64 **paddrs, __u64 *pcount)
 {
-	unsigned long *addrs, count, offset;
+	__u64 *addrs, count, offset;
+	unsigned int addr_size, i;
 	Elf_Data *data;
 	GElf_Shdr shdr;
 	Elf_Scn *sec;
@@ -128,8 +129,11 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
 		return -1;
 	}
 
+	/* Get address size from processed file's ELF class. */
+	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
+
 	offset = fl->mcount_start - shdr.sh_addr;
-	count  = (fl->mcount_stop - fl->mcount_start) / 8;
+	count  = (fl->mcount_stop - fl->mcount_start) / addr_size;
 
 	data = elf_getdata(sec, 0);
 	if (!data) {
@@ -144,7 +148,13 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
 		return -1;
 	}
 
-	memcpy(addrs, data->d_buf + offset, count * sizeof(addrs[0]));
+	if (addr_size == sizeof(__u64)) {
+		memcpy(addrs, data->d_buf + offset, count * addr_size);
+	} else {
+		for (i = 0; i < count; i++)
+			addrs[i] = (__u64) *((__u32 *) (data->d_buf + offset + i * addr_size));
+	}
+
 	*paddrs = addrs;
 	*pcount = count;
 	return 0;
@@ -152,7 +162,7 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
 
 static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
-	unsigned long *addrs, count, i;
+	__u64 *addrs, count, i;
 	int functions_valid = 0;
 
 	/*
-- 
2.26.2

[PATCH 3/3] btf_encoder: Detect kernel module ftrace addresses

[Jiri Olsa]

Add support to detect kernel module ftrace addresses and use
it as filter for detected functions.

For kernel modules the ftrace addresses are stored in __mcount_loc
section. Adding the code that detects this section and reads
its data into array, which is then processed as filter by
current code.

There's one tricky point with kernel modules wrt Elf object,
which we get from dwfl_module_getelf function. This function
performs all possible relocations, including __mcount_loc
section.

So addrs array contains relocated values, which we need take
into account when we compare them to functions values which
are relative to their sections.

With this change for example for xfs.ko module in my kernel
config, I'm getting slightly bigger number of functions:

  before: 2373, after: 2601

The ftrace's available_filter_functions still shows 2701, but
it includes functions like:

  suffix_kstrtoint.constprop.0
  xchk_btree_check_minrecs.isra.0
  xfs_ascii_ci_compname.part.0

which are not part of dwarf data, the rest matches BTF functions.

Because of the malfunction DWARF's declaration tag, the 'before'
functions contain also functions that are not part of the module.
The 'after' functions contain only functions that are traceable
and part of xfs.ko.

Despite filtering out some declarations, this change also adds
static functions, hence the total number of functions is bigger.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++--
 dutil.c       |  10 +++++
 dutil.h       |   2 +
 3 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 398be0fbf7c7..570fce46b6d9 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -36,6 +36,7 @@ struct funcs_layout {
 struct elf_function {
 	const char	*name;
 	unsigned long	 addr;
+	unsigned long	 sh_addr;
 	bool		 generated;
 };
 
@@ -65,11 +66,12 @@ static void delete_functions(void)
 static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 {
 	struct elf_function *new;
+	static GElf_Shdr sh;
+	static int last_idx;
+	int idx;
 
 	if (elf_sym__type(sym) != STT_FUNC)
 		return 0;
-	if (!elf_sym__value(sym))
-		return 0;
 
 	if (functions_cnt == functions_alloc) {
 		functions_alloc = max(1000, functions_alloc * 3 / 2);
@@ -84,8 +86,17 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
 		functions = new;
 	}
 
+	idx = elf_sym__section(sym);
+
+	if (idx != last_idx) {
+		if (!elf_section_by_idx(btfe->elf, &sh, idx))
+			return 0;
+		last_idx = idx;
+	}
+
 	functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
 	functions[functions_cnt].addr = elf_sym__value(sym);
+	functions[functions_cnt].sh_addr = sh.sh_addr;
 	functions[functions_cnt].generated = false;
 	functions_cnt++;
 	return 0;
@@ -160,10 +171,74 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
 	return 0;
 }
 
+static int
+get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
+{
+	__u64 *addrs, count;
+	unsigned int addr_size, i;
+	GElf_Shdr shdr_mcount;
+	Elf_Data *data;
+	Elf_Scn *sec;
+
+	/* Initialize for the sake of all error paths below. */
+	*paddrs = NULL;
+	*pcount = 0;
+
+	/* get __mcount_loc */
+	sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
+				  "__mcount_loc", NULL);
+	if (!sec) {
+		if (btf_elf__verbose) {
+			printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
+			       btfe->filename);
+		}
+		return 0;
+	}
+
+	data = elf_getdata(sec, NULL);
+	if (!data) {
+		fprintf(stderr, "Failed to data for __mcount_loc section.\n");
+		return -1;
+	}
+
+	/* Get address size from processed file's ELF class. */
+	addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
+
+	count = data->d_size / addr_size;
+
+	addrs = malloc(count * sizeof(addrs[0]));
+	if (!addrs) {
+		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
+		return -1;
+	}
+
+	if (addr_size == sizeof(__u64)) {
+		memcpy(addrs, data->d_buf, count * addr_size);
+	} else {
+		for (i = 0; i < count; i++)
+			addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
+	}
+
+	/*
+	 * We get Elf object from dwfl_module_getelf function,
+	 * which performs all possible relocations, including
+	 * __mcount_loc section.
+	 *
+	 * So addrs array now contains relocated values, which
+	 * we need take into account when we compare them to
+	 * functions values, see comment in setup_functions
+	 * function.
+	 */
+	*paddrs = addrs;
+	*pcount = count;
+	return 0;
+}
+
 static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
 	__u64 *addrs, count, i;
 	int functions_valid = 0;
+	bool kmod = false;
 
 	/*
 	 * Check if we are processing vmlinux image and
@@ -172,6 +247,16 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
 		return -1;
 
+	/*
+	 * Check if we are processing kernel module and
+	 * get mcount data if it's detected.
+	 */
+	if (!addrs) {
+		if (get_kmod_addrs(btfe, &addrs, &count))
+			return -1;
+		kmod = true;
+	}
+
 	if (!addrs) {
 		if (btf_elf__verbose)
 			printf("ftrace symbols not detected, falling back to DWARF data\n");
@@ -188,9 +273,18 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	 */
 	for (i = 0; i < functions_cnt; i++) {
 		struct elf_function *func = &functions[i];
+		/*
+		 * For vmlinux image both addrs[x] and functions[x]::addr
+		 * values are final address and are comparable.
+		 *
+		 * For kernel module addrs[x] is final address, but
+		 * functions[x]::addr is relative address within section
+		 * and needs to be relocated by adding sh_addr.
+		 */
+		__u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
 
 		/* Make sure function is within ftrace addresses. */
-		if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
+		if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
 			/*
 			 * We iterate over sorted array, so we can easily skip
 			 * not valid item and move following valid field into
diff --git a/dutil.c b/dutil.c
index f7b853f0660d..7b667647420f 100644
--- a/dutil.c
+++ b/dutil.c
@@ -196,6 +196,16 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 	return sec;
 }
 
+Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
+{
+	Elf_Scn *sec;
+
+	sec = elf_getscn(elf, idx);
+	if (sec)
+		gelf_getshdr(sec, shp);
+	return sec;
+}
+
 char *strlwr(char *s)
 {
 	int len = strlen(s), i;
diff --git a/dutil.h b/dutil.h
index 676770d5d5c9..0838dff2d679 100644
--- a/dutil.h
+++ b/dutil.h
@@ -324,6 +324,8 @@ void *zalloc(const size_t size);
 Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 			     GElf_Shdr *shp, const char *name, size_t *index);
 
+Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx);
+
 #ifndef SHT_GNU_ATTRIBUTES
 /* Just a way to check if we're using an old elfutils version */
 static inline int elf_getshdrstrndx(Elf *elf, size_t *dst)
-- 
2.26.2

Re: [PATCH 1/3] btf_encoder: Factor filter_functions function

[Andrii Nakryiko]

On Thu, Dec 3, 2020 at 2:07 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Reorder the filter_functions function so we can add
> processing of kernel modules in following patch.
>
> There's no functional change intended.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  btf_encoder.c | 61 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 20 deletions(-)
>

[...]

Re: [PATCH 2/3] btf_encoder: Use address size based on ELF's class

[Andrii Nakryiko]

On Thu, Dec 3, 2020 at 2:08 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We can't assume the address size is always size of unsigned
> long, we have to use directly the ELF's address size.
>
> Changing addrs array to __u64 and convert 32 bit address
> values when copying from ELF section.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

It looks ok to me, but I didn't expect that changes would be so
numerous... Makes me wonder if pahole ever supported working with ELF
files of different bitness. I'll defer to Arnaldo to make a call on
whether this is necessary.

>  btf_encoder.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>

[...]

Re: [PATCH 3/3] btf_encoder: Detect kernel module ftrace addresses

[Andrii Nakryiko]

On Thu, Dec 3, 2020 at 2:08 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Add support to detect kernel module ftrace addresses and use
> it as filter for detected functions.
>
> For kernel modules the ftrace addresses are stored in __mcount_loc
> section. Adding the code that detects this section and reads
> its data into array, which is then processed as filter by
> current code.
>
> There's one tricky point with kernel modules wrt Elf object,
> which we get from dwfl_module_getelf function. This function
> performs all possible relocations, including __mcount_loc
> section.
>
> So addrs array contains relocated values, which we need take
> into account when we compare them to functions values which
> are relative to their sections.
>
> With this change for example for xfs.ko module in my kernel
> config, I'm getting slightly bigger number of functions:
>
>   before: 2373, after: 2601
>
> The ftrace's available_filter_functions still shows 2701, but
> it includes functions like:
>
>   suffix_kstrtoint.constprop.0
>   xchk_btree_check_minrecs.isra.0
>   xfs_ascii_ci_compname.part.0
>
> which are not part of dwarf data, the rest matches BTF functions.
>
> Because of the malfunction DWARF's declaration tag, the 'before'
> functions contain also functions that are not part of the module.
> The 'after' functions contain only functions that are traceable
> and part of xfs.ko.
>
> Despite filtering out some declarations, this change also adds
> static functions, hence the total number of functions is bigger.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

I've tested locally on bpf_testmod that I'm adding to selftests in
[0]. All worked well. I changed the test function from global to
non-inlined static, and BTF had it. And the tests passed. So LGTM.

Tested-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>

  [0] https://patchwork.kernel.org/user/todo/netdevbpf/?series=395715&delegate=121173&state=*

>  btf_encoder.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  dutil.c       |  10 +++++
>  dutil.h       |   2 +
>  3 files changed, 109 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 398be0fbf7c7..570fce46b6d9 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
>  struct elf_function {
>         const char      *name;
>         unsigned long    addr;
> +       unsigned long    sh_addr;
>         bool             generated;
>  };
>
> @@ -65,11 +66,12 @@ static void delete_functions(void)
>  static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>  {
>         struct elf_function *new;
> +       static GElf_Shdr sh;
> +       static int last_idx;
> +       int idx;
>
>         if (elf_sym__type(sym) != STT_FUNC)
>                 return 0;
> -       if (!elf_sym__value(sym))
> -               return 0;
>
>         if (functions_cnt == functions_alloc) {
>                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> @@ -84,8 +86,17 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
>                 functions = new;
>         }
>
> +       idx = elf_sym__section(sym);
> +
> +       if (idx != last_idx) {
> +               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> +                       return 0;
> +               last_idx = idx;
> +       }
> +
>         functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
>         functions[functions_cnt].addr = elf_sym__value(sym);
> +       functions[functions_cnt].sh_addr = sh.sh_addr;
>         functions[functions_cnt].generated = false;
>         functions_cnt++;
>         return 0;
> @@ -160,10 +171,74 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
>         return 0;
>  }
>
> +static int
> +get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> +{
> +       __u64 *addrs, count;
> +       unsigned int addr_size, i;
> +       GElf_Shdr shdr_mcount;
> +       Elf_Data *data;
> +       Elf_Scn *sec;
> +
> +       /* Initialize for the sake of all error paths below. */
> +       *paddrs = NULL;
> +       *pcount = 0;
> +
> +       /* get __mcount_loc */
> +       sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> +                                 "__mcount_loc", NULL);
> +       if (!sec) {
> +               if (btf_elf__verbose) {
> +                       printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> +                              btfe->filename);
> +               }
> +               return 0;
> +       }
> +
> +       data = elf_getdata(sec, NULL);
> +       if (!data) {
> +               fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> +               return -1;
> +       }
> +
> +       /* Get address size from processed file's ELF class. */
> +       addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> +
> +       count = data->d_size / addr_size;
> +
> +       addrs = malloc(count * sizeof(addrs[0]));
> +       if (!addrs) {
> +               fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> +               return -1;
> +       }
> +
> +       if (addr_size == sizeof(__u64)) {
> +               memcpy(addrs, data->d_buf, count * addr_size);
> +       } else {
> +               for (i = 0; i < count; i++)
> +                       addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
> +       }
> +
> +       /*
> +        * We get Elf object from dwfl_module_getelf function,
> +        * which performs all possible relocations, including
> +        * __mcount_loc section.
> +        *
> +        * So addrs array now contains relocated values, which
> +        * we need take into account when we compare them to
> +        * functions values, see comment in setup_functions
> +        * function.
> +        */
> +       *paddrs = addrs;
> +       *pcount = count;
> +       return 0;
> +}
> +
>  static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
>  {
>         __u64 *addrs, count, i;
>         int functions_valid = 0;
> +       bool kmod = false;
>
>         /*
>          * Check if we are processing vmlinux image and
> @@ -172,6 +247,16 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
>         if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
>                 return -1;
>
> +       /*
> +        * Check if we are processing kernel module and
> +        * get mcount data if it's detected.
> +        */
> +       if (!addrs) {
> +               if (get_kmod_addrs(btfe, &addrs, &count))
> +                       return -1;
> +               kmod = true;
> +       }
> +
>         if (!addrs) {
>                 if (btf_elf__verbose)
>                         printf("ftrace symbols not detected, falling back to DWARF data\n");
> @@ -188,9 +273,18 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
>          */
>         for (i = 0; i < functions_cnt; i++) {
>                 struct elf_function *func = &functions[i];
> +               /*
> +                * For vmlinux image both addrs[x] and functions[x]::addr
> +                * values are final address and are comparable.
> +                *
> +                * For kernel module addrs[x] is final address, but
> +                * functions[x]::addr is relative address within section
> +                * and needs to be relocated by adding sh_addr.
> +                */
> +               __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
>
>                 /* Make sure function is within ftrace addresses. */
> -               if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> +               if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
>                         /*
>                          * We iterate over sorted array, so we can easily skip
>                          * not valid item and move following valid field into
> diff --git a/dutil.c b/dutil.c
> index f7b853f0660d..7b667647420f 100644
> --- a/dutil.c
> +++ b/dutil.c
> @@ -196,6 +196,16 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>         return sec;
>  }
>
> +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
> +{
> +       Elf_Scn *sec;
> +
> +       sec = elf_getscn(elf, idx);
> +       if (sec)
> +               gelf_getshdr(sec, shp);
> +       return sec;
> +}
> +
>  char *strlwr(char *s)
>  {
>         int len = strlen(s), i;
> diff --git a/dutil.h b/dutil.h
> index 676770d5d5c9..0838dff2d679 100644
> --- a/dutil.h
> +++ b/dutil.h
> @@ -324,6 +324,8 @@ void *zalloc(const size_t size);
>  Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
>                              GElf_Shdr *shp, const char *name, size_t *index);
>
> +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx);
> +
>  #ifndef SHT_GNU_ATTRIBUTES
>  /* Just a way to check if we're using an old elfutils version */
>  static inline int elf_getshdrstrndx(Elf *elf, size_t *dst)
> --
> 2.26.2
>

Re: [PATCH 2/3] btf_encoder: Use address size based on ELF's class

[Jiri Olsa]

On Thu, Dec 03, 2020 at 03:22:18PM -0800, Andrii Nakryiko wrote:
> On Thu, Dec 3, 2020 at 2:08 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We can't assume the address size is always size of unsigned
> > long, we have to use directly the ELF's address size.
> >
> > Changing addrs array to __u64 and convert 32 bit address
> > values when copying from ELF section.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> It looks ok to me, but I didn't expect that changes would be so
> numerous... Makes me wonder if pahole ever supported working with ELF
> files of different bitness. I'll defer to Arnaldo to make a call on
> whether this is necessary.

so to test this I built 32bit vmlinux and used 64bit pahole
to generate BTF data on both vmlinux and modules, which I
thought was valid use case

jirka

> 
> >  btf_encoder.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> 
> [...]
> 

Re: [PATCH 2/3] btf_encoder: Use address size based on ELF's class

[Arnaldo Carvalho de Melo]

Em Fri, Dec 04, 2020 at 12:37:50AM +0100, Jiri Olsa escreveu:
> On Thu, Dec 03, 2020 at 03:22:18PM -0800, Andrii Nakryiko wrote:
> > On Thu, Dec 3, 2020 at 2:08 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > We can't assume the address size is always size of unsigned
> > > long, we have to use directly the ELF's address size.
> > >
> > > Changing addrs array to __u64 and convert 32 bit address
> > > values when copying from ELF section.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > 
> > It looks ok to me, but I didn't expect that changes would be so
> > numerous... Makes me wonder if pahole ever supported working with ELF
> > files of different bitness. I'll defer to Arnaldo to make a call on
> > whether this is necessary.
> 
> so to test this I built 32bit vmlinux and used 64bit pahole
> to generate BTF data on both vmlinux and modules, which I
> thought was valid use case

It is valid, yeah.

- Arnaldo
 
> jirka
> 
> > 
> > >  btf_encoder.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > >
> > 
> > [...]
> > 
> 

-- 

- Arnaldo

Re: [PATCH 3/3] btf_encoder: Detect kernel module ftrace addresses

[Arnaldo Carvalho de Melo]

Em Thu, Dec 03, 2020 at 03:23:57PM -0800, Andrii Nakryiko escreveu:
> On Thu, Dec 3, 2020 at 2:08 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Add support to detect kernel module ftrace addresses and use
> > it as filter for detected functions.
> >
> > For kernel modules the ftrace addresses are stored in __mcount_loc
> > section. Adding the code that detects this section and reads
> > its data into array, which is then processed as filter by
> > current code.
> >
> > There's one tricky point with kernel modules wrt Elf object,
> > which we get from dwfl_module_getelf function. This function
> > performs all possible relocations, including __mcount_loc
> > section.
> >
> > So addrs array contains relocated values, which we need take
> > into account when we compare them to functions values which
> > are relative to their sections.
> >
> > With this change for example for xfs.ko module in my kernel
> > config, I'm getting slightly bigger number of functions:
> >
> >   before: 2373, after: 2601
> >
> > The ftrace's available_filter_functions still shows 2701, but
> > it includes functions like:
> >
> >   suffix_kstrtoint.constprop.0
> >   xchk_btree_check_minrecs.isra.0
> >   xfs_ascii_ci_compname.part.0
> >
> > which are not part of dwarf data, the rest matches BTF functions.
> >
> > Because of the malfunction DWARF's declaration tag, the 'before'
> > functions contain also functions that are not part of the module.
> > The 'after' functions contain only functions that are traceable
> > and part of xfs.ko.
> >
> > Despite filtering out some declarations, this change also adds
> > static functions, hence the total number of functions is bigger.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> I've tested locally on bpf_testmod that I'm adding to selftests in
> [0]. All worked well. I changed the test function from global to
> non-inlined static, and BTF had it. And the tests passed. So LGTM.
> 
> Tested-by: Andrii Nakryiko <andrii@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks, applied the three patches in this series,

- Arnaldo
 
>   [0] https://patchwork.kernel.org/user/todo/netdevbpf/?series=395715&delegate=121173&state=*
> 
> >  btf_encoder.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  dutil.c       |  10 +++++
> >  dutil.h       |   2 +
> >  3 files changed, 109 insertions(+), 3 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 398be0fbf7c7..570fce46b6d9 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -36,6 +36,7 @@ struct funcs_layout {
> >  struct elf_function {
> >         const char      *name;
> >         unsigned long    addr;
> > +       unsigned long    sh_addr;
> >         bool             generated;
> >  };
> >
> > @@ -65,11 +66,12 @@ static void delete_functions(void)
> >  static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >  {
> >         struct elf_function *new;
> > +       static GElf_Shdr sh;
> > +       static int last_idx;
> > +       int idx;
> >
> >         if (elf_sym__type(sym) != STT_FUNC)
> >                 return 0;
> > -       if (!elf_sym__value(sym))
> > -               return 0;
> >
> >         if (functions_cnt == functions_alloc) {
> >                 functions_alloc = max(1000, functions_alloc * 3 / 2);
> > @@ -84,8 +86,17 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> >                 functions = new;
> >         }
> >
> > +       idx = elf_sym__section(sym);
> > +
> > +       if (idx != last_idx) {
> > +               if (!elf_section_by_idx(btfe->elf, &sh, idx))
> > +                       return 0;
> > +               last_idx = idx;
> > +       }
> > +
> >         functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> >         functions[functions_cnt].addr = elf_sym__value(sym);
> > +       functions[functions_cnt].sh_addr = sh.sh_addr;
> >         functions[functions_cnt].generated = false;
> >         functions_cnt++;
> >         return 0;
> > @@ -160,10 +171,74 @@ static int get_vmlinux_addrs(struct btf_elf *btfe, struct funcs_layout *fl,
> >         return 0;
> >  }
> >
> > +static int
> > +get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> > +{
> > +       __u64 *addrs, count;
> > +       unsigned int addr_size, i;
> > +       GElf_Shdr shdr_mcount;
> > +       Elf_Data *data;
> > +       Elf_Scn *sec;
> > +
> > +       /* Initialize for the sake of all error paths below. */
> > +       *paddrs = NULL;
> > +       *pcount = 0;
> > +
> > +       /* get __mcount_loc */
> > +       sec = elf_section_by_name(btfe->elf, &btfe->ehdr, &shdr_mcount,
> > +                                 "__mcount_loc", NULL);
> > +       if (!sec) {
> > +               if (btf_elf__verbose) {
> > +                       printf("%s: '%s' doesn't have __mcount_loc section\n", __func__,
> > +                              btfe->filename);
> > +               }
> > +               return 0;
> > +       }
> > +
> > +       data = elf_getdata(sec, NULL);
> > +       if (!data) {
> > +               fprintf(stderr, "Failed to data for __mcount_loc section.\n");
> > +               return -1;
> > +       }
> > +
> > +       /* Get address size from processed file's ELF class. */
> > +       addr_size = gelf_getclass(btfe->elf) == ELFCLASS32 ? 4 : 8;
> > +
> > +       count = data->d_size / addr_size;
> > +
> > +       addrs = malloc(count * sizeof(addrs[0]));
> > +       if (!addrs) {
> > +               fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> > +               return -1;
> > +       }
> > +
> > +       if (addr_size == sizeof(__u64)) {
> > +               memcpy(addrs, data->d_buf, count * addr_size);
> > +       } else {
> > +               for (i = 0; i < count; i++)
> > +                       addrs[i] = (__u64) *((__u32 *) (data->d_buf + i * addr_size));
> > +       }
> > +
> > +       /*
> > +        * We get Elf object from dwfl_module_getelf function,
> > +        * which performs all possible relocations, including
> > +        * __mcount_loc section.
> > +        *
> > +        * So addrs array now contains relocated values, which
> > +        * we need take into account when we compare them to
> > +        * functions values, see comment in setup_functions
> > +        * function.
> > +        */
> > +       *paddrs = addrs;
> > +       *pcount = count;
> > +       return 0;
> > +}
> > +
> >  static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >  {
> >         __u64 *addrs, count, i;
> >         int functions_valid = 0;
> > +       bool kmod = false;
> >
> >         /*
> >          * Check if we are processing vmlinux image and
> > @@ -172,6 +247,16 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >         if (get_vmlinux_addrs(btfe, fl, &addrs, &count))
> >                 return -1;
> >
> > +       /*
> > +        * Check if we are processing kernel module and
> > +        * get mcount data if it's detected.
> > +        */
> > +       if (!addrs) {
> > +               if (get_kmod_addrs(btfe, &addrs, &count))
> > +                       return -1;
> > +               kmod = true;
> > +       }
> > +
> >         if (!addrs) {
> >                 if (btf_elf__verbose)
> >                         printf("ftrace symbols not detected, falling back to DWARF data\n");
> > @@ -188,9 +273,18 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >          */
> >         for (i = 0; i < functions_cnt; i++) {
> >                 struct elf_function *func = &functions[i];
> > +               /*
> > +                * For vmlinux image both addrs[x] and functions[x]::addr
> > +                * values are final address and are comparable.
> > +                *
> > +                * For kernel module addrs[x] is final address, but
> > +                * functions[x]::addr is relative address within section
> > +                * and needs to be relocated by adding sh_addr.
> > +                */
> > +               __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> >
> >                 /* Make sure function is within ftrace addresses. */
> > -               if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > +               if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> >                         /*
> >                          * We iterate over sorted array, so we can easily skip
> >                          * not valid item and move following valid field into
> > diff --git a/dutil.c b/dutil.c
> > index f7b853f0660d..7b667647420f 100644
> > --- a/dutil.c
> > +++ b/dutil.c
> > @@ -196,6 +196,16 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> >         return sec;
> >  }
> >
> > +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx)
> > +{
> > +       Elf_Scn *sec;
> > +
> > +       sec = elf_getscn(elf, idx);
> > +       if (sec)
> > +               gelf_getshdr(sec, shp);
> > +       return sec;
> > +}
> > +
> >  char *strlwr(char *s)
> >  {
> >         int len = strlen(s), i;
> > diff --git a/dutil.h b/dutil.h
> > index 676770d5d5c9..0838dff2d679 100644
> > --- a/dutil.h
> > +++ b/dutil.h
> > @@ -324,6 +324,8 @@ void *zalloc(const size_t size);
> >  Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
> >                              GElf_Shdr *shp, const char *name, size_t *index);
> >
> > +Elf_Scn *elf_section_by_idx(Elf *elf, GElf_Shdr *shp, int idx);
> > +
> >  #ifndef SHT_GNU_ATTRIBUTES
> >  /* Just a way to check if we're using an old elfutils version */
> >  static inline int elf_getshdrstrndx(Elf *elf, size_t *dst)
> > --
> > 2.26.2
> >

-- 

- Arnaldo