[PATCHv4 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[PATCHv4 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[Jiri Olsa]

hi,
because of gcc bug [1] we can no longer rely on DW_AT_declaration
attribute to filter out declarations and end up with just
one copy of the function in the BTF data.

It seems this bug is not easy to fix, but regardless if the
it's coming soon, it's probably good idea not to depend so
much only on dwarf data and make some extra checks.

Thus for function encoding we are now doing following checks:
  - argument names are defined for the function
  - there's symbol and address defined for the function
  - function address belongs to ftrace locations (new in v2)
  - function is generated only once

v4 changes:
  - added acks
  - renames and change functions_valid to be local var [Andrii]
  - fixed error path (return err) of collect_symbols

v3 changes:
  - added Hao's ack for patch 1
  - fixed realloc memory leak [Andrii]
  - fixed addrs_cmp function [Andrii]
  - removed SET_SYMBOL macro [Andrii]
  - fixed the 'valid' function logic
  - added .init.bpf.preserve_type check
  - added iterator functions to new kernel section
    .init.bpf.preserve_type [Yonghong]

v2 changes:
  - add check ensuring functions belong to ftrace's mcount
    locations, this way we ensure to have in BTF only
    functions available for ftrace - patch 2 changelog
    describes all details
  - use collect* function names [Andrii]
  - use conventional size increase in realloc [Andrii]
  - drop elf_sym__is_function check
  - drop patch 3, it's not needed, because we follow ftrace
    locations

thanks,
jirka


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060

[PATCH 1/3] bpf: Move iterator functions into special init section

[Jiri Olsa]

With upcoming changes to pahole, that change the way how and
which kernel functions are stored in BTF data, we need a way
to recognize iterator functions.

Iterator functions need to be in BTF data, but have no real
body and are currently placed in .init.text section, so they
are freed after kernel init and are filtered out of BTF data
because of that.

The solution is to place these functions under new section:
  .init.bpf.preserve_type

And add 2 new symbols to mark that area:
  __init_bpf_preserve_type_begin
  __init_bpf_preserve_type_end

The code in pahole responsible for picking up the functions will
be able to recognize functions from this section and add them to
the BTF data and filter out all other .init.text functions.

Suggested-by: Yonghong Song <yhs@fb.com>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 include/asm-generic/vmlinux.lds.h | 16 +++++++++++++++-
 include/linux/bpf.h               |  8 +++++++-
 include/linux/init.h              |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index cd14444bf600..e18e1030dabf 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -685,8 +685,21 @@
 	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
 		*(.BTF_ids)						\
 	}
+
+/*
+ * .init.bpf.preserve_type
+ *
+ * This section store special BPF function and marks them
+ * with begin/end symbols pair for the sake of pahole tool.
+ */
+#define INIT_BPF_PRESERVE_TYPE						\
+	__init_bpf_preserve_type_begin = .;                             \
+	*(.init.bpf.preserve_type)                                      \
+	__init_bpf_preserve_type_end = .;				\
+	MEM_DISCARD(init.bpf.preserve_type)
 #else
 #define BTF
+#define INIT_BPF_PRESERVE_TYPE
 #endif
 
 /*
@@ -740,7 +753,8 @@
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
 	*(.text.startup)						\
-	MEM_DISCARD(init.text*)
+	MEM_DISCARD(init.text*)						\
+	INIT_BPF_PRESERVE_TYPE
 
 #define EXIT_DATA							\
 	*(.exit.data .exit.data.*)					\
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 73d5381a5d5c..894f66c7703e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1277,10 +1277,16 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
 int bpf_obj_get_user(const char __user *pathname, int flags);
 
+#ifdef CONFIG_DEBUG_INFO_BTF
+#define BPF_INIT __init_bpf_preserve_type
+#else
+#define BPF_INIT __init
+#endif
+
 #define BPF_ITER_FUNC_PREFIX "bpf_iter_"
 #define DEFINE_BPF_ITER_FUNC(target, args...)			\
 	extern int bpf_iter_ ## target(args);			\
-	int __init bpf_iter_ ## target(args) { return 0; }
+	int BPF_INIT bpf_iter_ ## target(args) { return 0; }
 
 struct bpf_iter_aux_info {
 	struct bpf_map *map;
diff --git a/include/linux/init.h b/include/linux/init.h
index 212fc9e2f691..133462863711 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -52,6 +52,7 @@
 #define __initconst	__section(.init.rodata)
 #define __exitdata	__section(.exit.data)
 #define __exit_call	__used __section(.exitcall.exit)
+#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)
 
 /*
  * modpost check for section mismatches during the kernel build.
-- 
2.26.2

Re: [PATCH 1/3] bpf: Move iterator functions into special init section

[Arnaldo Carvalho de Melo]

Em Fri, Nov 06, 2020 at 11:25:10PM +0100, Jiri Olsa escreveu:
> With upcoming changes to pahole, that change the way how and
> which kernel functions are stored in BTF data, we need a way
> to recognize iterator functions.
> 
> Iterator functions need to be in BTF data, but have no real
> body and are currently placed in .init.text section, so they
> are freed after kernel init and are filtered out of BTF data
> because of that.
> 
> The solution is to place these functions under new section:
>   .init.bpf.preserve_type
> 
> And add 2 new symbols to mark that area:
>   __init_bpf_preserve_type_begin
>   __init_bpf_preserve_type_end
> 
> The code in pahole responsible for picking up the functions will
> be able to recognize functions from this section and add them to
> the BTF data and filter out all other .init.text functions.

This isn't applying on torvalds/master:

[acme@five linux]$ patch -p1 < /wb/1.patch
patching file include/asm-generic/vmlinux.lds.h
Hunk #2 succeeded at 754 (offset 1 line).
patching file include/linux/bpf.h
Hunk #1 succeeded at 1276 (offset -1 lines).
patching file include/linux/init.h
Hunk #1 FAILED at 52.
1 out of 1 hunk FAILED -- saving rejects to file include/linux/init.h.rej
[acme@five linux]$
[acme@five linux]$ cat include/linux/init.h.rej
--- include/linux/init.h
+++ include/linux/init.h
@@ -52,6 +52,7 @@
 #define __initconst	__section(.init.rodata)
 #define __exitdata	__section(.exit.data)
 #define __exit_call	__used __section(.exitcall.exit)
+#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)

 /*
  * modpost check for section mismatches during the kernel build.
[acme@five linux]$


I'm fixing it up by hand to try together with pahole's patches.

- Arnaldo
 
> Suggested-by: Yonghong Song <yhs@fb.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  include/asm-generic/vmlinux.lds.h | 16 +++++++++++++++-
>  include/linux/bpf.h               |  8 +++++++-
>  include/linux/init.h              |  1 +
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index cd14444bf600..e18e1030dabf 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -685,8 +685,21 @@
>  	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
>  		*(.BTF_ids)						\
>  	}
> +
> +/*
> + * .init.bpf.preserve_type
> + *
> + * This section store special BPF function and marks them
> + * with begin/end symbols pair for the sake of pahole tool.
> + */
> +#define INIT_BPF_PRESERVE_TYPE						\
> +	__init_bpf_preserve_type_begin = .;                             \
> +	*(.init.bpf.preserve_type)                                      \
> +	__init_bpf_preserve_type_end = .;				\
> +	MEM_DISCARD(init.bpf.preserve_type)
>  #else
>  #define BTF
> +#define INIT_BPF_PRESERVE_TYPE
>  #endif
>  
>  /*
> @@ -740,7 +753,8 @@
>  #define INIT_TEXT							\
>  	*(.init.text .init.text.*)					\
>  	*(.text.startup)						\
> -	MEM_DISCARD(init.text*)
> +	MEM_DISCARD(init.text*)						\
> +	INIT_BPF_PRESERVE_TYPE
>  
>  #define EXIT_DATA							\
>  	*(.exit.data .exit.data.*)					\
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 73d5381a5d5c..894f66c7703e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1277,10 +1277,16 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
>  int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
>  int bpf_obj_get_user(const char __user *pathname, int flags);
>  
> +#ifdef CONFIG_DEBUG_INFO_BTF
> +#define BPF_INIT __init_bpf_preserve_type
> +#else
> +#define BPF_INIT __init
> +#endif
> +
>  #define BPF_ITER_FUNC_PREFIX "bpf_iter_"
>  #define DEFINE_BPF_ITER_FUNC(target, args...)			\
>  	extern int bpf_iter_ ## target(args);			\
> -	int __init bpf_iter_ ## target(args) { return 0; }
> +	int BPF_INIT bpf_iter_ ## target(args) { return 0; }
>  
>  struct bpf_iter_aux_info {
>  	struct bpf_map *map;
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 212fc9e2f691..133462863711 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -52,6 +52,7 @@
>  #define __initconst	__section(.init.rodata)
>  #define __exitdata	__section(.exit.data)
>  #define __exit_call	__used __section(.exitcall.exit)
> +#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)
>  
>  /*
>   * modpost check for section mismatches during the kernel build.
> -- 
> 2.26.2
> 

-- 

- Arnaldo

Re: [PATCH 1/3] bpf: Move iterator functions into special init section

[Arnaldo Carvalho de Melo]

Em Mon, Nov 09, 2020 at 03:05:00PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Nov 06, 2020 at 11:25:10PM +0100, Jiri Olsa escreveu:
> > With upcoming changes to pahole, that change the way how and
> > which kernel functions are stored in BTF data, we need a way
> > to recognize iterator functions.
> > 
> > Iterator functions need to be in BTF data, but have no real
> > body and are currently placed in .init.text section, so they
> > are freed after kernel init and are filtered out of BTF data
> > because of that.
> > 
> > The solution is to place these functions under new section:
> >   .init.bpf.preserve_type
> > 
> > And add 2 new symbols to mark that area:
> >   __init_bpf_preserve_type_begin
> >   __init_bpf_preserve_type_end
> > 
> > The code in pahole responsible for picking up the functions will
> > be able to recognize functions from this section and add them to
> > the BTF data and filter out all other .init.text functions.
> 
> This isn't applying on torvalds/master:
> 
> [acme@five linux]$ patch -p1 < /wb/1.patch
> patching file include/asm-generic/vmlinux.lds.h
> Hunk #2 succeeded at 754 (offset 1 line).
> patching file include/linux/bpf.h
> Hunk #1 succeeded at 1276 (offset -1 lines).
> patching file include/linux/init.h
> Hunk #1 FAILED at 52.
> 1 out of 1 hunk FAILED -- saving rejects to file include/linux/init.h.rej
> [acme@five linux]$
> [acme@five linux]$ cat include/linux/init.h.rej
> --- include/linux/init.h
> +++ include/linux/init.h
> @@ -52,6 +52,7 @@
>  #define __initconst	__section(.init.rodata)
>  #define __exitdata	__section(.exit.data)
>  #define __exit_call	__used __section(.exitcall.exit)
> +#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)
> 
>  /*
>   * modpost check for section mismatches during the kernel build.
> [acme@five linux]$
> 
> 
> I'm fixing it up by hand to try together with pahole's patches.

Due to:

33def8498fdde180 ("treewide: Convert macro and uses of __section(foo) to __section("foo")")

I'm using this now:

diff --git a/include/linux/init.h b/include/linux/init.h
index 7b53cb3092ee9956..a7c71e3b5f9a1d65 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -52,6 +52,7 @@
 #define __initconst	__section(".init.rodata")
 #define __exitdata	__section(".exit.data")
 #define __exit_call	__used __section(".exitcall.exit")
+#define __init_bpf_preserve_type __section(".init.bpf.preserve_type")
 
 /*
  * modpost check for section mismatches during the kernel build.

Re: [PATCH 1/3] bpf: Move iterator functions into special init section

[Arnaldo Carvalho de Melo]

Em Mon, Nov 09, 2020 at 03:06:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> > I'm fixing it up by hand to try together with pahole's patches.
 
> Due to:
 
> 33def8498fdde180 ("treewide: Convert macro and uses of __section(foo) to __section("foo")")
> 

For convenience:

 asm-generic/vmlinux.lds.h |   16 +++++++++++++++-
 linux/bpf.h               |    8 +++++++-
 linux/init.h              |    1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

---

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b2b3d81b1535a5ab..f91029b3443bf0d2 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -685,8 +685,21 @@
 	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
 		*(.BTF_ids)						\
 	}
+
+/*
+ * .init.bpf.preserve_type
+ *
+ * This section store special BPF function and marks them
+ * with begin/end symbols pair for the sake of pahole tool.
+ */
+#define INIT_BPF_PRESERVE_TYPE						\
+	__init_bpf_preserve_type_begin = .;                             \
+	*(.init.bpf.preserve_type)                                      \
+	__init_bpf_preserve_type_end = .;				\
+	MEM_DISCARD(init.bpf.preserve_type)
 #else
 #define BTF
+#define INIT_BPF_PRESERVE_TYPE
 #endif
 
 /*
@@ -741,7 +754,8 @@
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
 	*(.text.startup)						\
-	MEM_DISCARD(init.text*)
+	MEM_DISCARD(init.text*)						\
+	INIT_BPF_PRESERVE_TYPE
 
 #define EXIT_DATA							\
 	*(.exit.data .exit.data.*)					\
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2b16bf48aab61a1f..73e8ededde3e9c09 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1276,10 +1276,16 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
 int bpf_obj_get_user(const char __user *pathname, int flags);
 
+#ifdef CONFIG_DEBUG_INFO_BTF
+#define BPF_INIT __init_bpf_preserve_type
+#else
+#define BPF_INIT __init
+#endif
+
 #define BPF_ITER_FUNC_PREFIX "bpf_iter_"
 #define DEFINE_BPF_ITER_FUNC(target, args...)			\
 	extern int bpf_iter_ ## target(args);			\
-	int __init bpf_iter_ ## target(args) { return 0; }
+	int BPF_INIT bpf_iter_ ## target(args) { return 0; }
 
 struct bpf_iter_aux_info {
 	struct bpf_map *map;
diff --git a/include/linux/init.h b/include/linux/init.h
index 7b53cb3092ee9956..a7c71e3b5f9a1d65 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -52,6 +52,7 @@
 #define __initconst	__section(".init.rodata")
 #define __exitdata	__section(".exit.data")
 #define __exit_call	__used __section(".exitcall.exit")
+#define __init_bpf_preserve_type __section(".init.bpf.preserve_type")
 
 /*
  * modpost check for section mismatches during the kernel build.

Re: [PATCH 1/3] bpf: Move iterator functions into special init section

[Jiri Olsa]

On Mon, Nov 09, 2020 at 03:06:55PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 09, 2020 at 03:05:00PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Nov 06, 2020 at 11:25:10PM +0100, Jiri Olsa escreveu:
> > > With upcoming changes to pahole, that change the way how and
> > > which kernel functions are stored in BTF data, we need a way
> > > to recognize iterator functions.
> > > 
> > > Iterator functions need to be in BTF data, but have no real
> > > body and are currently placed in .init.text section, so they
> > > are freed after kernel init and are filtered out of BTF data
> > > because of that.
> > > 
> > > The solution is to place these functions under new section:
> > >   .init.bpf.preserve_type
> > > 
> > > And add 2 new symbols to mark that area:
> > >   __init_bpf_preserve_type_begin
> > >   __init_bpf_preserve_type_end
> > > 
> > > The code in pahole responsible for picking up the functions will
> > > be able to recognize functions from this section and add them to
> > > the BTF data and filter out all other .init.text functions.
> > 
> > This isn't applying on torvalds/master:
> > 
> > [acme@five linux]$ patch -p1 < /wb/1.patch
> > patching file include/asm-generic/vmlinux.lds.h
> > Hunk #2 succeeded at 754 (offset 1 line).
> > patching file include/linux/bpf.h
> > Hunk #1 succeeded at 1276 (offset -1 lines).
> > patching file include/linux/init.h
> > Hunk #1 FAILED at 52.
> > 1 out of 1 hunk FAILED -- saving rejects to file include/linux/init.h.rej
> > [acme@five linux]$
> > [acme@five linux]$ cat include/linux/init.h.rej
> > --- include/linux/init.h
> > +++ include/linux/init.h
> > @@ -52,6 +52,7 @@
> >  #define __initconst	__section(.init.rodata)
> >  #define __exitdata	__section(.exit.data)
> >  #define __exit_call	__used __section(.exitcall.exit)
> > +#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)
> > 
> >  /*
> >   * modpost check for section mismatches during the kernel build.
> > [acme@five linux]$
> > 
> > 
> > I'm fixing it up by hand to try together with pahole's patches.
> 
> Due to:
> 
> 33def8498fdde180 ("treewide: Convert macro and uses of __section(foo) to __section("foo")")

ok, I'll send new version for the kernel patch

thanks,
jirka

[PATCH 2/3] btf_encoder: Move find_all_percpu_vars in generic collect_symbols

[Jiri Olsa]

Moving find_all_percpu_vars under generic collect_symbols
function that walks over symbols and calls collect_percpu_var.

We will add another collect function that needs to go through
all the symbols, so it's better we go through them just once.

There's no functional change intended.

Acked-by: Hao Luo <haoluo@google.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 124 +++++++++++++++++++++++++++-----------------------
 1 file changed, 67 insertions(+), 57 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 4c92908beab2..1866bb16a8ba 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -250,75 +250,85 @@ static bool percpu_var_exists(uint64_t addr, uint32_t *sz, const char **name)
 	return true;
 }
 
-static int find_all_percpu_vars(struct btf_elf *btfe)
+static int collect_percpu_var(struct btf_elf *btfe, GElf_Sym *sym)
 {
-	uint32_t core_id;
-	GElf_Sym sym;
+	const char *sym_name;
+	uint64_t addr;
+	uint32_t size;
 
-	/* cache variables' addresses, preparing for searching in symtab. */
-	percpu_var_cnt = 0;
+	/* compare a symbol's shndx to determine if it's a percpu variable */
+	if (elf_sym__section(sym) != btfe->percpu_shndx)
+		return 0;
+	if (elf_sym__type(sym) != STT_OBJECT)
+		return 0;
 
-	/* search within symtab for percpu variables */
-	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
-		const char *sym_name;
-		uint64_t addr;
-		uint32_t size;
+	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;
 
-		/* compare a symbol's shndx to determine if it's a percpu variable */
-		if (elf_sym__section(&sym) != btfe->percpu_shndx)
-			continue;
-		if (elf_sym__type(&sym) != STT_OBJECT)
-			continue;
+	size = elf_sym__size(sym);
+	if (!size)
+		return 0; /* ignore zero-sized symbols */
 
-		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)
-			continue;
+	sym_name = elf_sym__name(sym, btfe->symtab);
+	if (!btf_name_valid(sym_name)) {
+		dump_invalid_symbol("Found symbol of invalid name when encoding btf",
+				    sym_name, btf_elf__verbose, btf_elf__force);
+		if (btf_elf__force)
+			return 0;
+		return -1;
+	}
 
-		size = elf_sym__size(&sym);
-		if (!size)
-			continue; /* ignore zero-sized symbols */
+	if (btf_elf__verbose)
+		printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr);
 
-		sym_name = elf_sym__name(&sym, btfe->symtab);
-		if (!btf_name_valid(sym_name)) {
-			dump_invalid_symbol("Found symbol of invalid name when encoding btf",
-					    sym_name, btf_elf__verbose, btf_elf__force);
-			if (btf_elf__force)
-				continue;
-			return -1;
-		}
+	if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) {
+		fprintf(stderr, "Reached the limit of per-CPU variables: %d\n",
+			MAX_PERCPU_VAR_CNT);
+		return -1;
+	}
+	percpu_vars[percpu_var_cnt].addr = addr;
+	percpu_vars[percpu_var_cnt].sz = size;
+	percpu_vars[percpu_var_cnt].name = sym_name;
+	percpu_var_cnt++;
 
-		if (btf_elf__verbose)
-			printf("Found per-CPU symbol '%s' at address 0x%lx\n", sym_name, addr);
+	return 0;
+}
+
+static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
+{
+	uint32_t core_id;
+	GElf_Sym sym;
 
-		if (percpu_var_cnt == MAX_PERCPU_VAR_CNT) {
-			fprintf(stderr, "Reached the limit of per-CPU variables: %d\n",
-				MAX_PERCPU_VAR_CNT);
+	/* cache variables' addresses, preparing for searching in symtab. */
+	percpu_var_cnt = 0;
+
+	/* search within symtab for percpu variables */
+	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
+		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
 			return -1;
-		}
-		percpu_vars[percpu_var_cnt].addr = addr;
-		percpu_vars[percpu_var_cnt].sz = size;
-		percpu_vars[percpu_var_cnt].name = sym_name;
-		percpu_var_cnt++;
 	}
 
-	if (percpu_var_cnt)
-		qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp);
+	if (collect_percpu_vars) {
+		if (percpu_var_cnt)
+			qsort(percpu_vars, percpu_var_cnt, sizeof(percpu_vars[0]), percpu_var_cmp);
 
-	if (btf_elf__verbose)
-		printf("Found %d per-CPU variables!\n", percpu_var_cnt);
+		if (btf_elf__verbose)
+			printf("Found %d per-CPU variables!\n", percpu_var_cnt);
+	}
 	return 0;
 }
 
@@ -347,7 +357,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		if (!btfe)
 			return -1;
 
-		if (!skip_encoding_vars && find_all_percpu_vars(btfe))
+		if (collect_symbols(btfe, !skip_encoding_vars))
 			goto out;
 
 		has_index_type = false;
-- 
2.26.2

[PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa]

We need to generate just single BTF instance for the
function, while DWARF data contains multiple instances
of DW_TAG_subprogram tag.

Unfortunately we can no longer rely on DW_AT_declaration
tag (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060)

Instead we apply following checks:
  - argument names are defined for the function
  - there's symbol and address defined for the function
  - function is generated only once

Also because we want to follow kernel's ftrace traceable
functions, this patchset is adding extra check that the
function is one of the ftrace's functions.

All ftrace functions addresses are stored in vmlinux
binary within symbols:
  __start_mcount_loc
  __stop_mcount_loc

During object preparation code we read those addresses,
sort them and use them as filter for all detected dwarf
functions.

We also filter out functions within .init section, ftrace
is doing that in runtime. At the same time we keep functions
from .init.bpf.preserve_type, because they are needed in BTF.

I can still see several differences to ftrace functions in
/sys/kernel/debug/tracing/available_filter_functions file:

  - available_filter_functions includes modules
  - available_filter_functions includes functions like:
      __acpi_match_device.part.0.constprop.0
      acpi_ns_check_sorted_list.constprop.0
      acpi_os_unmap_generic_address.part.0
      acpiphp_check_bridge.part.0

    which are not part of dwarf data
  - BTF includes multiple functions like:
      __clk_register_clkdev
      clk_register_clkdev

    which share same code so they appear just as single function
    in available_filter_functions, but dwarf keeps track of both
    of them

  - BTF includes iterator functions, which do not make it to
    available_filter_functions

With this change I'm getting 38384 BTF functions, which
when added above functions to consideration gives same
amount of functions in available_filter_functions.

The patch still keeps the original function filter condition
(that uses current fn->declaration check) in case the object
does not contain *_mcount_loc symbol -> object is not vmlinux.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 267 insertions(+), 3 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 1866bb16a8ba..9b93e9963727 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -26,6 +26,179 @@
  */
 #define KSYM_NAME_LEN 128
 
+struct funcs_layout {
+	unsigned long mcount_start;
+	unsigned long mcount_stop;
+	unsigned long init_begin;
+	unsigned long init_end;
+	unsigned long init_bpf_begin;
+	unsigned long init_bpf_end;
+	unsigned long mcount_sec_idx;
+};
+
+struct elf_function {
+	const char	*name;
+	unsigned long	 addr;
+	bool		 generated;
+};
+
+static struct elf_function *functions;
+static int functions_alloc;
+static int functions_cnt;
+
+static int functions_cmp(const void *_a, const void *_b)
+{
+	const struct elf_function *a = _a;
+	const struct elf_function *b = _b;
+
+	return strcmp(a->name, b->name);
+}
+
+static void delete_functions(void)
+{
+	free(functions);
+	functions_alloc = functions_cnt = 0;
+	functions = NULL;
+}
+
+#ifndef max
+#define max(x, y) ((x) < (y) ? (y) : (x))
+#endif
+
+static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
+{
+	struct elf_function *new;
+
+	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);
+		new = realloc(functions, functions_alloc * sizeof(*functions));
+		if (!new) {
+			/*
+			 * The cleanup - delete_functions is called
+			 * in cu__encode_btf error path.
+			 */
+			return -1;
+		}
+		functions = new;
+	}
+
+	functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
+	functions[functions_cnt].addr = elf_sym__value(sym);
+	functions[functions_cnt].generated = false;
+	functions_cnt++;
+	return 0;
+}
+
+static int addrs_cmp(const void *_a, const void *_b)
+{
+	const unsigned long *a = _a;
+	const unsigned long *b = _b;
+
+	if (*a == *b)
+		return 0;
+	return *a < *b ? -1 : 1;
+}
+
+static bool is_init(struct funcs_layout *fl, unsigned long addr)
+{
+	return addr >= fl->init_begin && addr < fl->init_end;
+}
+
+static bool is_bpf_init(struct funcs_layout *fl, unsigned long addr)
+{
+	return addr >= fl->init_bpf_begin && addr < fl->init_bpf_end;
+}
+
+static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
+{
+	unsigned long *addrs, count, offset, i;
+	int functions_valid = 0;
+	Elf_Data *data;
+	GElf_Shdr shdr;
+	Elf_Scn *sec;
+
+	/*
+	 * Find mcount addressed marked by __start_mcount_loc
+	 * and __stop_mcount_loc symbols and load them into
+	 * sorted array.
+	 */
+	sec = elf_getscn(btfe->elf, fl->mcount_sec_idx);
+	if (!sec || !gelf_getshdr(sec, &shdr)) {
+		fprintf(stderr, "Failed to get section(%lu) header.\n",
+			fl->mcount_sec_idx);
+		return -1;
+	}
+
+	offset = fl->mcount_start - shdr.sh_addr;
+	count  = (fl->mcount_stop - fl->mcount_start) / 8;
+
+	data = elf_getdata(sec, 0);
+	if (!data) {
+		fprintf(stderr, "Failed to get section(%lu) data.\n",
+			fl->mcount_sec_idx);
+		return -1;
+	}
+
+	addrs = malloc(count * sizeof(addrs[0]));
+	if (!addrs) {
+		fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
+		return -1;
+	}
+
+	memcpy(addrs, data->d_buf + offset, count * sizeof(addrs[0]));
+	qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
+
+	/*
+	 * Let's got through all collected functions and filter
+	 * out those that are not in ftrace and init code.
+	 */
+	for (i = 0; i < functions_cnt; i++) {
+		struct elf_function *func = &functions[i];
+
+		/*
+		 * Do not enable .init section functions,
+		 * but keep .init.bpf.preserve_type functions.
+		 */
+		if (is_init(fl, func->addr) && !is_bpf_init(fl, func->addr))
+			continue;
+
+		/* Make sure function is within ftrace addresses. */
+		if (bsearch(&func->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
+			 * its place, and still keep the 'new' array sorted.
+			 */
+			if (i != functions_valid)
+				functions[functions_valid] = functions[i];
+			functions_valid++;
+		}
+	}
+
+	functions_cnt = functions_valid;
+	free(addrs);
+	return 0;
+}
+
+static bool should_generate_function(const struct btf_elf *btfe, const char *name)
+{
+	struct elf_function *p;
+	struct elf_function key = { .name = name };
+
+	p = bsearch(&key, functions, functions_cnt,
+		    sizeof(functions[0]), functions_cmp);
+	if (!p || p->generated)
+		return false;
+
+	p->generated = true;
+	return true;
+}
+
 static bool btf_name_char_ok(char c, bool first)
 {
 	if (c == '_' || c == '.')
@@ -207,6 +380,7 @@ int btf_encoder__encode()
 		btf_elf__add_datasec_type(btfe, PERCPU_SECTION, &btfe->percpu_secinfo);
 
 	err = btf_elf__encode(btfe, 0);
+	delete_functions();
 	btf_elf__delete(btfe);
 	btfe = NULL;
 
@@ -308,8 +482,45 @@ 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)
+{
+	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;
+	}
+
+	if (!fl->mcount_stop &&
+	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
+		fl->mcount_stop = sym->st_value;
+
+	if (!fl->init_begin &&
+	    !strcmp("__init_begin", elf_sym__name(sym, btfe->symtab)))
+		fl->init_begin = sym->st_value;
+
+	if (!fl->init_end &&
+	    !strcmp("__init_end", elf_sym__name(sym, btfe->symtab)))
+		fl->init_end = sym->st_value;
+
+	if (!fl->init_bpf_begin &&
+	    !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
+		fl->init_bpf_begin = sym->st_value;
+
+	if (!fl->init_bpf_end &&
+	    !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
+		fl->init_bpf_end = sym->st_value;
+}
+
+static int has_all_symbols(struct funcs_layout *fl)
+{
+	return fl->mcount_start && fl->mcount_stop &&
+	       fl->init_begin && fl->init_end &&
+	       fl->init_bpf_begin && fl->init_bpf_end;
+}
+
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 {
+	struct funcs_layout fl = { };
 	uint32_t core_id;
 	GElf_Sym sym;
 
@@ -320,6 +531,9 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 	elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
 		if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
 			return -1;
+		if (collect_function(btfe, &sym))
+			return -1;
+		collect_symbol(&sym, &fl);
 	}
 
 	if (collect_percpu_vars) {
@@ -329,9 +543,37 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
 		if (btf_elf__verbose)
 			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("vmlinux not detected, falling back to dwarf data\n");
+		delete_functions();
+	}
+
 	return 0;
 }
 
+static bool has_arg_names(struct cu *cu, struct ftype *ftype)
+{
+	struct parameter *param;
+	const char *name;
+
+	ftype__for_each_parameter(ftype, param) {
+		name = dwarves__active_loader->strings__ptr(cu, param->name);
+		if (name == NULL)
+			return false;
+	}
+	return true;
+}
+
 int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		   bool skip_encoding_vars)
 {
@@ -357,7 +599,8 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		if (!btfe)
 			return -1;
 
-		if (collect_symbols(btfe, !skip_encoding_vars))
+		err = collect_symbols(btfe, !skip_encoding_vars);
+		if (err)
 			goto out;
 
 		has_index_type = false;
@@ -407,8 +650,28 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		int btf_fnproto_id, btf_fn_id;
 		const char *name;
 
-		if (fn->declaration || !fn->external)
-			continue;
+		/*
+		 * The functions_cnt != 0 means we parsed all necessary
+		 * kernel symbols and we are using ftrace location filter
+		 * for functions. If it's not available keep the current
+		 * dwarf declaration check.
+		 */
+		if (functions_cnt) {
+			/*
+			 * We check following conditions:
+			 *   - argument names are defined
+			 *   - there's symbol and address defined for the function
+			 *   - function address belongs to ftrace locations
+			 *   - function is generated only once
+			 */
+			if (!has_arg_names(cu, &fn->proto))
+				continue;
+			if (!should_generate_function(btfe, function__name(fn, cu)))
+				continue;
+		} else {
+			if (fn->declaration || !fn->external)
+				continue;
+		}
 
 		btf_fnproto_id = btf_elf__add_func_proto(btfe, cu, &fn->proto, type_id_off);
 		name = dwarves__active_loader->strings__ptr(cu, fn->name);
@@ -492,6 +755,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 
 out:
 	if (err) {
+		delete_functions();
 		btf_elf__delete(btfe);
 		btfe = NULL;
 	}
-- 
2.26.2

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Andrii Nakryiko]

On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We need to generate just single BTF instance for the
> function, while DWARF data contains multiple instances
> of DW_TAG_subprogram tag.
>
> Unfortunately we can no longer rely on DW_AT_declaration
> tag (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060)
>
> Instead we apply following checks:
>   - argument names are defined for the function
>   - there's symbol and address defined for the function
>   - function is generated only once
>
> Also because we want to follow kernel's ftrace traceable
> functions, this patchset is adding extra check that the
> function is one of the ftrace's functions.
>
> All ftrace functions addresses are stored in vmlinux
> binary within symbols:
>   __start_mcount_loc
>   __stop_mcount_loc
>
> During object preparation code we read those addresses,
> sort them and use them as filter for all detected dwarf
> functions.
>
> We also filter out functions within .init section, ftrace
> is doing that in runtime. At the same time we keep functions
> from .init.bpf.preserve_type, because they are needed in BTF.
>
> I can still see several differences to ftrace functions in
> /sys/kernel/debug/tracing/available_filter_functions file:
>
>   - available_filter_functions includes modules
>   - available_filter_functions includes functions like:
>       __acpi_match_device.part.0.constprop.0
>       acpi_ns_check_sorted_list.constprop.0
>       acpi_os_unmap_generic_address.part.0
>       acpiphp_check_bridge.part.0
>
>     which are not part of dwarf data
>   - BTF includes multiple functions like:
>       __clk_register_clkdev
>       clk_register_clkdev
>
>     which share same code so they appear just as single function
>     in available_filter_functions, but dwarf keeps track of both
>     of them
>
>   - BTF includes iterator functions, which do not make it to
>     available_filter_functions
>
> With this change I'm getting 38384 BTF functions, which
> when added above functions to consideration gives same
> amount of functions in available_filter_functions.
>
> The patch still keeps the original function filter condition
> (that uses current fn->declaration check) in case the object
> does not contain *_mcount_loc symbol -> object is not vmlinux.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Jiri,

This patch breaks the bpf tree pretty badly right now by generating
bad BTF for (at least some) FUNCs. Please investigate ASAP. And please
also make sure that you run test_progs for the kernel with BTF
generated by pahole.

bpf-next is not broken only because pahole falls back to old logic if
it doesn't find __init_bpf_preserve_type_begin and
__init_bpf_preserve_type_end symbols.

>  btf_encoder.c | 270 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 267 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 1866bb16a8ba..9b93e9963727 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -26,6 +26,179 @@
>   */
>  #define KSYM_NAME_LEN 128
>
> +struct funcs_layout {
> +       unsigned long mcount_start;
> +       unsigned long mcount_stop;
> +       unsigned long init_begin;
> +       unsigned long init_end;
> +       unsigned long init_bpf_begin;
> +       unsigned long init_bpf_end;
> +       unsigned long mcount_sec_idx;
> +};
> +
> +struct elf_function {
> +       const char      *name;
> +       unsigned long    addr;
> +       bool             generated;
> +};
> +
> +static struct elf_function *functions;
> +static int functions_alloc;
> +static int functions_cnt;
> +
> +static int functions_cmp(const void *_a, const void *_b)
> +{
> +       const struct elf_function *a = _a;
> +       const struct elf_function *b = _b;
> +
> +       return strcmp(a->name, b->name);
> +}
> +
> +static void delete_functions(void)
> +{
> +       free(functions);
> +       functions_alloc = functions_cnt = 0;
> +       functions = NULL;
> +}
> +
> +#ifndef max
> +#define max(x, y) ((x) < (y) ? (y) : (x))
> +#endif
> +
> +static int collect_function(struct btf_elf *btfe, GElf_Sym *sym)
> +{
> +       struct elf_function *new;
> +
> +       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);
> +               new = realloc(functions, functions_alloc * sizeof(*functions));
> +               if (!new) {
> +                       /*
> +                        * The cleanup - delete_functions is called
> +                        * in cu__encode_btf error path.
> +                        */
> +                       return -1;
> +               }
> +               functions = new;
> +       }
> +
> +       functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> +       functions[functions_cnt].addr = elf_sym__value(sym);
> +       functions[functions_cnt].generated = false;
> +       functions_cnt++;
> +       return 0;
> +}
> +
> +static int addrs_cmp(const void *_a, const void *_b)
> +{
> +       const unsigned long *a = _a;
> +       const unsigned long *b = _b;
> +
> +       if (*a == *b)
> +               return 0;
> +       return *a < *b ? -1 : 1;
> +}
> +
> +static bool is_init(struct funcs_layout *fl, unsigned long addr)
> +{
> +       return addr >= fl->init_begin && addr < fl->init_end;
> +}
> +
> +static bool is_bpf_init(struct funcs_layout *fl, unsigned long addr)
> +{
> +       return addr >= fl->init_bpf_begin && addr < fl->init_bpf_end;
> +}
> +
> +static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> +{
> +       unsigned long *addrs, count, offset, i;
> +       int functions_valid = 0;
> +       Elf_Data *data;
> +       GElf_Shdr shdr;
> +       Elf_Scn *sec;
> +
> +       /*
> +        * Find mcount addressed marked by __start_mcount_loc
> +        * and __stop_mcount_loc symbols and load them into
> +        * sorted array.
> +        */
> +       sec = elf_getscn(btfe->elf, fl->mcount_sec_idx);
> +       if (!sec || !gelf_getshdr(sec, &shdr)) {
> +               fprintf(stderr, "Failed to get section(%lu) header.\n",
> +                       fl->mcount_sec_idx);
> +               return -1;
> +       }
> +
> +       offset = fl->mcount_start - shdr.sh_addr;
> +       count  = (fl->mcount_stop - fl->mcount_start) / 8;
> +
> +       data = elf_getdata(sec, 0);
> +       if (!data) {
> +               fprintf(stderr, "Failed to get section(%lu) data.\n",
> +                       fl->mcount_sec_idx);
> +               return -1;
> +       }
> +
> +       addrs = malloc(count * sizeof(addrs[0]));
> +       if (!addrs) {
> +               fprintf(stderr, "Failed to allocate memory for ftrace addresses.\n");
> +               return -1;
> +       }
> +
> +       memcpy(addrs, data->d_buf + offset, count * sizeof(addrs[0]));
> +       qsort(addrs, count, sizeof(addrs[0]), addrs_cmp);
> +
> +       /*
> +        * Let's got through all collected functions and filter
> +        * out those that are not in ftrace and init code.
> +        */
> +       for (i = 0; i < functions_cnt; i++) {
> +               struct elf_function *func = &functions[i];
> +
> +               /*
> +                * Do not enable .init section functions,
> +                * but keep .init.bpf.preserve_type functions.
> +                */
> +               if (is_init(fl, func->addr) && !is_bpf_init(fl, func->addr))
> +                       continue;
> +
> +               /* Make sure function is within ftrace addresses. */
> +               if (bsearch(&func->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
> +                        * its place, and still keep the 'new' array sorted.
> +                        */
> +                       if (i != functions_valid)
> +                               functions[functions_valid] = functions[i];
> +                       functions_valid++;
> +               }
> +       }
> +
> +       functions_cnt = functions_valid;
> +       free(addrs);
> +       return 0;
> +}
> +
> +static bool should_generate_function(const struct btf_elf *btfe, const char *name)
> +{
> +       struct elf_function *p;
> +       struct elf_function key = { .name = name };
> +
> +       p = bsearch(&key, functions, functions_cnt,
> +                   sizeof(functions[0]), functions_cmp);
> +       if (!p || p->generated)
> +               return false;
> +
> +       p->generated = true;
> +       return true;
> +}
> +
>  static bool btf_name_char_ok(char c, bool first)
>  {
>         if (c == '_' || c == '.')
> @@ -207,6 +380,7 @@ int btf_encoder__encode()
>                 btf_elf__add_datasec_type(btfe, PERCPU_SECTION, &btfe->percpu_secinfo);
>
>         err = btf_elf__encode(btfe, 0);
> +       delete_functions();
>         btf_elf__delete(btfe);
>         btfe = NULL;
>
> @@ -308,8 +482,45 @@ 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)
> +{
> +       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;
> +       }
> +
> +       if (!fl->mcount_stop &&
> +           !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
> +               fl->mcount_stop = sym->st_value;
> +
> +       if (!fl->init_begin &&
> +           !strcmp("__init_begin", elf_sym__name(sym, btfe->symtab)))
> +               fl->init_begin = sym->st_value;
> +
> +       if (!fl->init_end &&
> +           !strcmp("__init_end", elf_sym__name(sym, btfe->symtab)))
> +               fl->init_end = sym->st_value;
> +
> +       if (!fl->init_bpf_begin &&
> +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> +               fl->init_bpf_begin = sym->st_value;
> +
> +       if (!fl->init_bpf_end &&
> +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> +               fl->init_bpf_end = sym->st_value;
> +}
> +
> +static int has_all_symbols(struct funcs_layout *fl)
> +{
> +       return fl->mcount_start && fl->mcount_stop &&
> +              fl->init_begin && fl->init_end &&
> +              fl->init_bpf_begin && fl->init_bpf_end;

See below for what seems to be the root cause for the immediate problem.

But me, Alexei and Daniel had a discussion offline, and we concluded
that this special bpf_preserve_init section is probably not the right
approach overall. We should roll back the bpf patch and instead adjust
pahole's approach. I think we should just drop the __init check and
include all the __init functions into BTF. There could be cases where
we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
security cases), so having BTFs for those FUNCs are necessary as well.
Ftrace currently disallows that, but it's only because no user-space
application has a way to attach probes early enough. This might change
in the future, so there is no need to invent special mechanisms now
for bpf_iter function preservation. Let's just include all __init
functions in BTF. Can you please do that change and check how much
more functions we get in BTF? Thanks!

> +}
> +
>  static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>  {
> +       struct funcs_layout fl = { };
>         uint32_t core_id;
>         GElf_Sym sym;
>
> @@ -320,6 +531,9 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>         elf_symtab__for_each_symbol(btfe->symtab, core_id, sym) {
>                 if (collect_percpu_vars && collect_percpu_var(btfe, &sym))
>                         return -1;
> +               if (collect_function(btfe, &sym))
> +                       return -1;
> +               collect_symbol(&sym, &fl);
>         }
>
>         if (collect_percpu_vars) {
> @@ -329,9 +543,37 @@ static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
>                 if (btf_elf__verbose)
>                         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("vmlinux not detected, falling back to dwarf data\n");
> +               delete_functions();
> +       }
> +
>         return 0;
>  }
>
> +static bool has_arg_names(struct cu *cu, struct ftype *ftype)
> +{
> +       struct parameter *param;
> +       const char *name;
> +
> +       ftype__for_each_parameter(ftype, param) {
> +               name = dwarves__active_loader->strings__ptr(cu, param->name);
> +               if (name == NULL)
> +                       return false;
> +       }
> +       return true;
> +}
> +

I suspect (but haven't verified) that the problem is in this function.
If it happens that DWARF for a function has no arguments, then we'll
conclude it has all arg names. Don't know what's the best solution
here, but please double-check this.

Specifically, two selftests are failing now. One of them:

libbpf: load bpf program failed: Permission denied
libbpf: -- BEGIN DUMP LOG ---
libbpf:
arg#0 type is not a struct
Unrecognized arg#0 type PTR
; int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
0: (79) r6 = *(u64 *)(r1 +0)
func 'security_inode_getattr' doesn't have 1-th argument
invalid bpf_context access off=0 size=8
processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0
libbpf: -- END LOG --
libbpf: failed to load program 'prog_stat'
libbpf: failed to load object 'test_d_path'
libbpf: failed to load BPF skeleton 'test_d_path': -4007
test_d_path:FAIL:setup d_path skeleton failed
#27 d_path:FAIL

This is because in generated BTF security_inode_getattr has a
prototype void security_inode_getattr(void); And once we emit this
prototype, due to logic in should_generate_function() we won't attempt
to do it again, even for the prototype with the right arguments.


>  int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                    bool skip_encoding_vars)
>  {
> @@ -357,7 +599,8 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                 if (!btfe)
>                         return -1;
>
> -               if (collect_symbols(btfe, !skip_encoding_vars))
> +               err = collect_symbols(btfe, !skip_encoding_vars);
> +               if (err)
>                         goto out;
>
>                 has_index_type = false;
> @@ -407,8 +650,28 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                 int btf_fnproto_id, btf_fn_id;
>                 const char *name;
>
> -               if (fn->declaration || !fn->external)
> -                       continue;
> +               /*
> +                * The functions_cnt != 0 means we parsed all necessary
> +                * kernel symbols and we are using ftrace location filter
> +                * for functions. If it's not available keep the current
> +                * dwarf declaration check.
> +                */
> +               if (functions_cnt) {
> +                       /*
> +                        * We check following conditions:
> +                        *   - argument names are defined
> +                        *   - there's symbol and address defined for the function
> +                        *   - function address belongs to ftrace locations
> +                        *   - function is generated only once
> +                        */
> +                       if (!has_arg_names(cu, &fn->proto))
> +                               continue;
> +                       if (!should_generate_function(btfe, function__name(fn, cu)))
> +                               continue;
> +               } else {
> +                       if (fn->declaration || !fn->external)
> +                               continue;
> +               }
>
>                 btf_fnproto_id = btf_elf__add_func_proto(btfe, cu, &fn->proto, type_id_off);
>                 name = dwarves__active_loader->strings__ptr(cu, fn->name);
> @@ -492,6 +755,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>
>  out:
>         if (err) {
> +               delete_functions();
>                 btf_elf__delete(btfe);
>                 btfe = NULL;
>         }
> --
> 2.26.2
>

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa]

On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:

SNIP

> > +       if (!fl->init_bpf_begin &&
> > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > +               fl->init_bpf_begin = sym->st_value;
> > +
> > +       if (!fl->init_bpf_end &&
> > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > +               fl->init_bpf_end = sym->st_value;
> > +}
> > +
> > +static int has_all_symbols(struct funcs_layout *fl)
> > +{
> > +       return fl->mcount_start && fl->mcount_stop &&
> > +              fl->init_begin && fl->init_end &&
> > +              fl->init_bpf_begin && fl->init_bpf_end;
> 
> See below for what seems to be the root cause for the immediate problem.
> 
> But me, Alexei and Daniel had a discussion offline, and we concluded
> that this special bpf_preserve_init section is probably not the right
> approach overall. We should roll back the bpf patch and instead adjust
> pahole's approach. I think we should just drop the __init check and
> include all the __init functions into BTF. There could be cases where
> we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> security cases), so having BTFs for those FUNCs are necessary as well.
> Ftrace currently disallows that, but it's only because no user-space
> application has a way to attach probes early enough. This might change
> in the future, so there is no need to invent special mechanisms now
> for bpf_iter function preservation. Let's just include all __init
> functions in BTF. Can you please do that change and check how much
> more functions we get in BTF? Thanks!

sure, not problem to keep all init functions, will give you the count

SNIP

> >
> > +static bool has_arg_names(struct cu *cu, struct ftype *ftype)
> > +{
> > +       struct parameter *param;
> > +       const char *name;
> > +
> > +       ftype__for_each_parameter(ftype, param) {
> > +               name = dwarves__active_loader->strings__ptr(cu, param->name);
> > +               if (name == NULL)
> > +                       return false;
> > +       }
> > +       return true;
> > +}
> > +
> 
> I suspect (but haven't verified) that the problem is in this function.
> If it happens that DWARF for a function has no arguments, then we'll
> conclude it has all arg names. Don't know what's the best solution
> here, but please double-check this.
> 
> Specifically, two selftests are failing now. One of them:
> 
> libbpf: load bpf program failed: Permission denied
> libbpf: -- BEGIN DUMP LOG ---
> libbpf:
> arg#0 type is not a struct
> Unrecognized arg#0 type PTR
> ; int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
> 0: (79) r6 = *(u64 *)(r1 +0)
> func 'security_inode_getattr' doesn't have 1-th argument
> invalid bpf_context access off=0 size=8
> processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> libbpf: -- END LOG --
> libbpf: failed to load program 'prog_stat'
> libbpf: failed to load object 'test_d_path'
> libbpf: failed to load BPF skeleton 'test_d_path': -4007
> test_d_path:FAIL:setup d_path skeleton failed
> #27 d_path:FAIL
> 
> This is because in generated BTF security_inode_getattr has a
> prototype void security_inode_getattr(void); And once we emit this
> prototype, due to logic in should_generate_function() we won't attempt
> to do it again, even for the prototype with the right arguments.

hum it works for me :-\

	#27 d_path:OK

with:

	[25962] FUNC_PROTO '(anon)' ret_type_id=17 vlen=1
		'path' type_id=729
	[31327] FUNC 'security_inode_getattr' type_id=25962 linkage=static


perhaps your gcc generates DWARF that breaks the way you described
above, but I'd expect to see function with argument without name,
not function without arguments at all

what gcc version are you on?

when you dump debug information, do you see security_inode_getattr
record with no arguments?

thanks,
jirka

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Andrii Nakryiko]

On Wed, Nov 11, 2020 at 12:20 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > +       if (!fl->init_bpf_begin &&
> > > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_begin = sym->st_value;
> > > +
> > > +       if (!fl->init_bpf_end &&
> > > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_end = sym->st_value;
> > > +}
> > > +
> > > +static int has_all_symbols(struct funcs_layout *fl)
> > > +{
> > > +       return fl->mcount_start && fl->mcount_stop &&
> > > +              fl->init_begin && fl->init_end &&
> > > +              fl->init_bpf_begin && fl->init_bpf_end;
> >
> > See below for what seems to be the root cause for the immediate problem.
> >
> > But me, Alexei and Daniel had a discussion offline, and we concluded
> > that this special bpf_preserve_init section is probably not the right
> > approach overall. We should roll back the bpf patch and instead adjust
> > pahole's approach. I think we should just drop the __init check and
> > include all the __init functions into BTF. There could be cases where
> > we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> > security cases), so having BTFs for those FUNCs are necessary as well.
> > Ftrace currently disallows that, but it's only because no user-space
> > application has a way to attach probes early enough. This might change
> > in the future, so there is no need to invent special mechanisms now
> > for bpf_iter function preservation. Let's just include all __init
> > functions in BTF. Can you please do that change and check how much
> > more functions we get in BTF? Thanks!
>
> sure, not problem to keep all init functions, will give you the count
>
> SNIP
>
> > >
> > > +static bool has_arg_names(struct cu *cu, struct ftype *ftype)
> > > +{
> > > +       struct parameter *param;
> > > +       const char *name;
> > > +
> > > +       ftype__for_each_parameter(ftype, param) {
> > > +               name = dwarves__active_loader->strings__ptr(cu, param->name);
> > > +               if (name == NULL)
> > > +                       return false;
> > > +       }
> > > +       return true;
> > > +}
> > > +
> >
> > I suspect (but haven't verified) that the problem is in this function.
> > If it happens that DWARF for a function has no arguments, then we'll
> > conclude it has all arg names. Don't know what's the best solution
> > here, but please double-check this.
> >
> > Specifically, two selftests are failing now. One of them:
> >
> > libbpf: load bpf program failed: Permission denied
> > libbpf: -- BEGIN DUMP LOG ---
> > libbpf:
> > arg#0 type is not a struct
> > Unrecognized arg#0 type PTR
> > ; int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
> > 0: (79) r6 = *(u64 *)(r1 +0)
> > func 'security_inode_getattr' doesn't have 1-th argument
> > invalid bpf_context access off=0 size=8
> > processed 1 insns (limit 1000000) max_states_per_insn 0 total_states 0
> > peak_states 0 mark_read 0
> > libbpf: -- END LOG --
> > libbpf: failed to load program 'prog_stat'
> > libbpf: failed to load object 'test_d_path'
> > libbpf: failed to load BPF skeleton 'test_d_path': -4007
> > test_d_path:FAIL:setup d_path skeleton failed
> > #27 d_path:FAIL
> >
> > This is because in generated BTF security_inode_getattr has a
> > prototype void security_inode_getattr(void); And once we emit this
> > prototype, due to logic in should_generate_function() we won't attempt
> > to do it again, even for the prototype with the right arguments.
>
> hum it works for me :-\
>
>         #27 d_path:OK
>
> with:
>
>         [25962] FUNC_PROTO '(anon)' ret_type_id=17 vlen=1
>                 'path' type_id=729
>         [31327] FUNC 'security_inode_getattr' type_id=25962 linkage=static
>
>
> perhaps your gcc generates DWARF that breaks the way you described
> above, but I'd expect to see function with argument without name,
> not function without arguments at all
>
> what gcc version are you on?

10.2.0, built from sources

>
> when you dump debug information, do you see security_inode_getattr
> record with no arguments?

Yeah, I think so:

21158467- <1><2b7e168>: Abbrev Number: 93 (DW_TAG_subprogram)
21158468-    <2b7e169>   DW_AT_external    : 1
21158469-    <2b7e169>   DW_AT_declaration : 1

  ..  BTW, we should probably still ignore DW_AT_declaration: 1, if it's set.

21158470:    <2b7e169>   DW_AT_linkage_name: (indirect string, offset:
0x120a0a): security_inode_getattr
21158471:    <2b7e16d>   DW_AT_name        : (indirect string, offset:
0x120a0a): security_inode_getattr
21158472-    <2b7e171>   DW_AT_decl_file   : 141
21158473-    <2b7e172>   DW_AT_decl_line   : 346
21158474-    <2b7e174>   DW_AT_decl_column : 5

...

36920783- <1><4c3bc3c>: Abbrev Number: 26 (DW_TAG_subprogram)
36920784-    <4c3bc3d>   DW_AT_external    : 1
36920785:    <4c3bc3d>   DW_AT_name        : (indirect string, offset:
0x120a0a): security_inode_getattr
36920786-    <4c3bc41>   DW_AT_decl_file   : 1
36920787-    <4c3bc42>   DW_AT_decl_line   : 1275
36920788-    <4c3bc44>   DW_AT_decl_column : 5
36920789-    <4c3bc45>   DW_AT_prototyped  : 1
36920790-    <4c3bc45>   DW_AT_type        : <0x4c17ffc>
36920791-    <4c3bc49>   DW_AT_low_pc      : 0xffffffff817d9d70
36920792-    <4c3bc51>   DW_AT_high_pc     : 0x67
36920793-    <4c3bc59>   DW_AT_frame_base  : 1 byte block: 9c
(DW_OP_call_frame_cfa)
36920794-    <4c3bc5b>   DW_AT_GNU_all_call_sites: 1
36920795-    <4c3bc5b>   DW_AT_sibling     : <0x4c3be10>
36920796- <2><4c3bc5f>: Abbrev Number: 17 (DW_TAG_formal_parameter)
36920797-    <4c3bc60>   DW_AT_name        : (indirect string, offset:
0x137dc3): path
36920798-    <4c3bc64>   DW_AT_decl_file   : 1
36920799-    <4c3bc65>   DW_AT_decl_line   : 1275
36920800-    <4c3bc67>   DW_AT_decl_column : 47
36920801-    <4c3bc68>   DW_AT_type        : <0x4c22144>
36920802-    <4c3bc6c>   DW_AT_location    : 0x1b2122c (location list)
36920803-    <4c3bc70>   DW_AT_GNU_locviews: 0x1b21226


>
> thanks,
> jirka
>

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa]

On Wed, Nov 11, 2020 at 12:26:23PM -0800, Andrii Nakryiko wrote:

SNIP

> > perhaps your gcc generates DWARF that breaks the way you described
> > above, but I'd expect to see function with argument without name,
> > not function without arguments at all
> >
> > what gcc version are you on?
> 
> 10.2.0, built from sources
> 
> >
> > when you dump debug information, do you see security_inode_getattr
> > record with no arguments?
> 
> Yeah, I think so:
> 
> 21158467- <1><2b7e168>: Abbrev Number: 93 (DW_TAG_subprogram)
> 21158468-    <2b7e169>   DW_AT_external    : 1
> 21158469-    <2b7e169>   DW_AT_declaration : 1
> 
>   ..  BTW, we should probably still ignore DW_AT_declaration: 1, if it's set.
> 
> 21158470:    <2b7e169>   DW_AT_linkage_name: (indirect string, offset:
> 0x120a0a): security_inode_getattr
> 21158471:    <2b7e16d>   DW_AT_name        : (indirect string, offset:
> 0x120a0a): security_inode_getattr
> 21158472-    <2b7e171>   DW_AT_decl_file   : 141
> 21158473-    <2b7e172>   DW_AT_decl_line   : 346
> 21158474-    <2b7e174>   DW_AT_decl_column : 5

nice.. so how about making extra loop through cu functions
and collect all distinct functions and for each collect
most detailed arguments and use this set for final func
generation

will try to think of some way without extra loop, but can't
think of any now

jirka

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa]

On Wed, Nov 11, 2020 at 09:19:29PM +0100, Jiri Olsa wrote:
> On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:
> 
> SNIP
> 
> > > +       if (!fl->init_bpf_begin &&
> > > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_begin = sym->st_value;
> > > +
> > > +       if (!fl->init_bpf_end &&
> > > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > > +               fl->init_bpf_end = sym->st_value;
> > > +}
> > > +
> > > +static int has_all_symbols(struct funcs_layout *fl)
> > > +{
> > > +       return fl->mcount_start && fl->mcount_stop &&
> > > +              fl->init_begin && fl->init_end &&
> > > +              fl->init_bpf_begin && fl->init_bpf_end;
> > 
> > See below for what seems to be the root cause for the immediate problem.
> > 
> > But me, Alexei and Daniel had a discussion offline, and we concluded
> > that this special bpf_preserve_init section is probably not the right
> > approach overall. We should roll back the bpf patch and instead adjust
> > pahole's approach. I think we should just drop the __init check and
> > include all the __init functions into BTF. There could be cases where
> > we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> > security cases), so having BTFs for those FUNCs are necessary as well.
> > Ftrace currently disallows that, but it's only because no user-space
> > application has a way to attach probes early enough. This might change
> > in the future, so there is no need to invent special mechanisms now
> > for bpf_iter function preservation. Let's just include all __init
> > functions in BTF. Can you please do that change and check how much
> > more functions we get in BTF? Thanks!
> 
> sure, not problem to keep all init functions, will give you the count

with pahole change below (on top of current master) and kernel
without the special init section, I'm getting over ~2000 functions
more on my .config:

  $ bpftool btf dump file ./vmlinux | grep 'FUNC ' | wc -l
  41505
  $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep 'FUNC ' | wc -l
  39256

jirka


---
diff --git a/btf_encoder.c b/btf_encoder.c
index 9b93e9963727..d531651b1e9e 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -29,10 +29,6 @@
 struct funcs_layout {
 	unsigned long mcount_start;
 	unsigned long mcount_stop;
-	unsigned long init_begin;
-	unsigned long init_end;
-	unsigned long init_bpf_begin;
-	unsigned long init_bpf_end;
 	unsigned long mcount_sec_idx;
 };
 
@@ -104,16 +100,6 @@ static int addrs_cmp(const void *_a, const void *_b)
 	return *a < *b ? -1 : 1;
 }
 
-static bool is_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_begin && addr < fl->init_end;
-}
-
-static bool is_bpf_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_bpf_begin && addr < fl->init_bpf_end;
-}
-
 static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
 	unsigned long *addrs, count, offset, i;
@@ -155,18 +141,11 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 
 	/*
 	 * Let's got through all collected functions and filter
-	 * out those that are not in ftrace and init code.
+	 * out those that are not in ftrace.
 	 */
 	for (i = 0; i < functions_cnt; i++) {
 		struct elf_function *func = &functions[i];
 
-		/*
-		 * Do not enable .init section functions,
-		 * but keep .init.bpf.preserve_type functions.
-		 */
-		if (is_init(fl, func->addr) && !is_bpf_init(fl, func->addr))
-			continue;
-
 		/* Make sure function is within ftrace addresses. */
 		if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
 			/*
@@ -493,29 +472,11 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 	if (!fl->mcount_stop &&
 	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
 		fl->mcount_stop = sym->st_value;
-
-	if (!fl->init_begin &&
-	    !strcmp("__init_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_begin = sym->st_value;
-
-	if (!fl->init_end &&
-	    !strcmp("__init_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_end = sym->st_value;
-
-	if (!fl->init_bpf_begin &&
-	    !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_begin = sym->st_value;
-
-	if (!fl->init_bpf_end &&
-	    !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_end = sym->st_value;
 }
 
 static int has_all_symbols(struct funcs_layout *fl)
 {
-	return fl->mcount_start && fl->mcount_stop &&
-	       fl->init_begin && fl->init_end &&
-	       fl->init_bpf_begin && fl->init_bpf_end;
+	return fl->mcount_start && fl->mcount_stop;
 }
 
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
diff --git a/lib/bpf b/lib/bpf
--- a/lib/bpf
+++ b/lib/bpf
@@ -1 +1 @@
-Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f
+Subproject commit ff797cc905d9c5fe9acab92d2da127342b20f80f-dirty

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Andrii Nakryiko]

On Wed, Nov 11, 2020 at 12:31 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Nov 11, 2020 at 09:19:29PM +0100, Jiri Olsa wrote:
> > On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > +       if (!fl->init_bpf_begin &&
> > > > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > > > +               fl->init_bpf_begin = sym->st_value;
> > > > +
> > > > +       if (!fl->init_bpf_end &&
> > > > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > > > +               fl->init_bpf_end = sym->st_value;
> > > > +}
> > > > +
> > > > +static int has_all_symbols(struct funcs_layout *fl)
> > > > +{
> > > > +       return fl->mcount_start && fl->mcount_stop &&
> > > > +              fl->init_begin && fl->init_end &&
> > > > +              fl->init_bpf_begin && fl->init_bpf_end;
> > >
> > > See below for what seems to be the root cause for the immediate problem.
> > >
> > > But me, Alexei and Daniel had a discussion offline, and we concluded
> > > that this special bpf_preserve_init section is probably not the right
> > > approach overall. We should roll back the bpf patch and instead adjust
> > > pahole's approach. I think we should just drop the __init check and
> > > include all the __init functions into BTF. There could be cases where
> > > we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> > > security cases), so having BTFs for those FUNCs are necessary as well.
> > > Ftrace currently disallows that, but it's only because no user-space
> > > application has a way to attach probes early enough. This might change
> > > in the future, so there is no need to invent special mechanisms now
> > > for bpf_iter function preservation. Let's just include all __init
> > > functions in BTF. Can you please do that change and check how much
> > > more functions we get in BTF? Thanks!
> >
> > sure, not problem to keep all init functions, will give you the count
>
> with pahole change below (on top of current master) and kernel
> without the special init section, I'm getting over ~2000 functions
> more on my .config:
>
>   $ bpftool btf dump file ./vmlinux | grep 'FUNC ' | wc -l
>   41505
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep 'FUNC ' | wc -l
>   39256

That's a very small percentage increase, let's just do this.

>
> jirka
>
>

[...]

Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

[Arnaldo Carvalho de Melo]

Em Wed, Nov 11, 2020 at 12:36:55PM -0800, Andrii Nakryiko escreveu:
> On Wed, Nov 11, 2020 at 12:31 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > On Wed, Nov 11, 2020 at 09:19:29PM +0100, Jiri Olsa wrote:
> > > On Wed, Nov 11, 2020 at 11:59:20AM -0800, Andrii Nakryiko wrote:
> > > SNIP

> > > > > +       if (!fl->init_bpf_begin &&
> > > > > +           !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
> > > > > +               fl->init_bpf_begin = sym->st_value;
> > > > > +
> > > > > +       if (!fl->init_bpf_end &&
> > > > > +           !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
> > > > > +               fl->init_bpf_end = sym->st_value;
> > > > > +}
> > > > > +
> > > > > +static int has_all_symbols(struct funcs_layout *fl)
> > > > > +{
> > > > > +       return fl->mcount_start && fl->mcount_stop &&
> > > > > +              fl->init_begin && fl->init_end &&
> > > > > +              fl->init_bpf_begin && fl->init_bpf_end;

> > > > See below for what seems to be the root cause for the immediate problem.

> > > > But me, Alexei and Daniel had a discussion offline, and we concluded
> > > > that this special bpf_preserve_init section is probably not the right
> > > > approach overall. We should roll back the bpf patch and instead adjust
> > > > pahole's approach. I think we should just drop the __init check and
> > > > include all the __init functions into BTF. There could be cases where
> > > > we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> > > > security cases), so having BTFs for those FUNCs are necessary as well.
> > > > Ftrace currently disallows that, but it's only because no user-space
> > > > application has a way to attach probes early enough. This might change
> > > > in the future, so there is no need to invent special mechanisms now
> > > > for bpf_iter function preservation. Let's just include all __init
> > > > functions in BTF. Can you please do that change and check how much
> > > > more functions we get in BTF? Thanks!

> > > sure, not problem to keep all init functions, will give you the count

> > with pahole change below (on top of current master) and kernel
> > without the special init section, I'm getting over ~2000 functions
> > more on my .config:

> >   $ bpftool btf dump file ./vmlinux | grep 'FUNC ' | wc -l
> >   41505
> >   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep 'FUNC ' | wc -l
> >   39256

> That's a very small percentage increase, let's just do this.

Agreed, if that is the only difference, no point in complicating things
as before, we end up with unforeseen trouble as we noticed.

- Arnaldo

Re: [PATCHv4 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[Andrii Nakryiko]

On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> because of gcc bug [1] we can no longer rely on DW_AT_declaration
> attribute to filter out declarations and end up with just
> one copy of the function in the BTF data.
>
> It seems this bug is not easy to fix, but regardless if the
> it's coming soon, it's probably good idea not to depend so
> much only on dwarf data and make some extra checks.
>
> Thus for function encoding we are now doing following checks:
>   - argument names are defined for the function
>   - there's symbol and address defined for the function
>   - function address belongs to ftrace locations (new in v2)
>   - function is generated only once
>
> v4 changes:
>   - added acks
>   - renames and change functions_valid to be local var [Andrii]
>   - fixed error path (return err) of collect_symbols
>
> v3 changes:
>   - added Hao's ack for patch 1
>   - fixed realloc memory leak [Andrii]
>   - fixed addrs_cmp function [Andrii]
>   - removed SET_SYMBOL macro [Andrii]
>   - fixed the 'valid' function logic
>   - added .init.bpf.preserve_type check
>   - added iterator functions to new kernel section
>     .init.bpf.preserve_type [Yonghong]
>
> v2 changes:
>   - add check ensuring functions belong to ftrace's mcount
>     locations, this way we ensure to have in BTF only
>     functions available for ftrace - patch 2 changelog
>     describes all details
>   - use collect* function names [Andrii]
>   - use conventional size increase in realloc [Andrii]
>   - drop elf_sym__is_function check
>   - drop patch 3, it's not needed, because we follow ftrace
>     locations
>
> thanks,
> jirka
>
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060
>

For the series:

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

Re: [PATCHv4 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[Arnaldo Carvalho de Melo]

Em Fri, Nov 06, 2020 at 02:56:45PM -0800, Andrii Nakryiko escreveu:
> On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
 
> For the series:
 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks, applied, testing now.

- Arnaldo

Re: [PATCHv4 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[Arnaldo Carvalho de Melo]

Em Mon, Nov 09, 2020 at 02:29:11PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Nov 06, 2020 at 02:56:45PM -0800, Andrii Nakryiko escreveu:
> > On Fri, Nov 6, 2020 at 2:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>  
> > For the series:
>  
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> Thanks, applied, testing now.

Now we have:

$ pfunct -F btf /sys/kernel/btf/vmlinux  | wc -l
38816
$ pfunct -F btf /sys/kernel/btf/vmlinux  | head
get_e820_md5
relocate_restore_code
resume_play_dead
bsp_pm_callback
msr_initialize_bdw
msr_save_cpuid_features
pm_check_save_msr
amd_bus_cpu_online
update_res
pci_read
$

$ pfunct -F btf /sys/kernel/btf/vmlinux -f msr_save_cpuid_features
int msr_save_cpuid_features(const struct x86_cpu_id  * c);
$
$ pfunct -F btf /sys/kernel/btf/vmlinux -f tcp_v4_rcv
int tcp_v4_rcv(struct sk_buff * skb);
$

[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux --class=sk_buff | head
pskb_expand_head
skb_put
audit_list_rules_send
netlink_ack
consume_skb
skb_queue_head
skb_queue_tail
netlink_broadcast
__nlmsg_put
kfree_skb
[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux -f audit_list_rules_send
int audit_list_rules_send(struct sk_buff * request_skb, int seq);
[acme@five ~]$ pfunct -F btf /sys/kernel/btf/vmlinux -f netlink_broadcast
int netlink_broadcast(struct sock * ssk, struct sk_buff * skb, __u32 portid, __u32 group, gfp_t allocation);
[acme@five ~]$

Seems to work :-)

In a future version I'll make it work with btf and
/sys/kernel/btf/vmlinux by default if only function names are provided,
like pahole with types:

[acme@five ~]$ pahole sk_buff_head
struct sk_buff_head {
	struct sk_buff *           next;                 /*     0     8 */
	struct sk_buff *           prev;                 /*     8     8 */
	__u32                      qlen;                 /*    16     4 */
	spinlock_t                 lock;                 /*    20     4 */

	/* size: 24, cachelines: 1, members: 4 */
	/* last cacheline: 24 bytes */
};
[acme@five ~]$ pahole list_head
struct list_head {
	struct list_head *         next;                 /*     0     8 */
	struct list_head *         prev;                 /*     8     8 */

	/* size: 16, cachelines: 1, members: 2 */
	/* last cacheline: 16 bytes */
};
[acme@five ~]$

Pushed out.

- Arnaldo

[PATCHv3 0/3] pahole/kernel: Workaround dwarf bug for function encoding

[Jiri Olsa]

hi,
because of gcc bug [1] we can no longer rely on DW_AT_declaration
attribute to filter out declarations and end up with just
one copy of the function in the BTF data.

It seems this bug is not easy to fix, but regardless if the
it's coming soon, it's probably good idea not to depend so
much only on dwarf data and make some extra checks.

Thus for function encoding we are now doing following checks:
  - argument names are defined for the function
  - there's symbol and address defined for the function
  - function address belongs to ftrace locations (new in v2)
  - function is generated only once

v3 changes:
  - added Hao's ack for patch 1
  - fixed realloc memory leak [Andrii]
  - fixed addrs_cmp function [Andrii]
  - removed SET_SYMBOL macro [Andrii]
  - fixed the 'valid' function logic
  - added .init.bpf.preserve_type check
  - added iterator functions to new kernel section
    .init.bpf.preserve_type [Yonghong]

v2 changes:
  - add check ensuring functions belong to ftrace's mcount
    locations, this way we ensure to have in BTF only
    functions available for ftrace - patch 2 changelog
    describes all details
  - use collect* function names [Andrii]
  - use conventional size increase in realloc [Andrii]
  - drop elf_sym__is_function check
  - drop patch 3, it's not needed, because we follow ftrace
    locations

thanks,
jirka


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060

[PATCH 1/3] bpf: Move iterator functions into special init section

[Jiri Olsa]

With upcoming changes to pahole, that change the way how and
which kernel functions are stored in BTF data, we need a way
to recognize iterator functions.

Iterator functions need to be in BTF data, but have no real
body and are currently placed in .init.text section, so they
are freed after kernel init and are filtered out of BTF data
because of that.

The solution is to place these functions under new section:
  .init.bpf.preserve_type

And add 2 new symbols to mark that area:
  __init_bpf_preserve_type_begin
  __init_bpf_preserve_type_end

The code in pahole responsible for picking up the functions will
be able to recognize functions from this section and add them to
the BTF data and filter out all other .init.text functions.

Suggested-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 include/asm-generic/vmlinux.lds.h | 16 +++++++++++++++-
 include/linux/bpf.h               |  8 +++++++-
 include/linux/init.h              |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index cd14444bf600..e18e1030dabf 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -685,8 +685,21 @@
 	.BTF_ids : AT(ADDR(.BTF_ids) - LOAD_OFFSET) {			\
 		*(.BTF_ids)						\
 	}
+
+/*
+ * .init.bpf.preserve_type
+ *
+ * This section store special BPF function and marks them
+ * with begin/end symbols pair for the sake of pahole tool.
+ */
+#define INIT_BPF_PRESERVE_TYPE						\
+	__init_bpf_preserve_type_begin = .;                             \
+	*(.init.bpf.preserve_type)                                      \
+	__init_bpf_preserve_type_end = .;				\
+	MEM_DISCARD(init.bpf.preserve_type)
 #else
 #define BTF
+#define INIT_BPF_PRESERVE_TYPE
 #endif
 
 /*
@@ -740,7 +753,8 @@
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
 	*(.text.startup)						\
-	MEM_DISCARD(init.text*)
+	MEM_DISCARD(init.text*)						\
+	INIT_BPF_PRESERVE_TYPE
 
 #define EXIT_DATA							\
 	*(.exit.data .exit.data.*)					\
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 2fffd30e13ac..12ab39a034a3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1276,10 +1276,16 @@ struct bpf_link *bpf_link_get_from_fd(u32 ufd);
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
 int bpf_obj_get_user(const char __user *pathname, int flags);
 
+#ifdef CONFIG_DEBUG_INFO_BTF
+#define BPF_INIT __init_bpf_preserve_type
+#else
+#define BPF_INIT __init
+#endif
+
 #define BPF_ITER_FUNC_PREFIX "bpf_iter_"
 #define DEFINE_BPF_ITER_FUNC(target, args...)			\
 	extern int bpf_iter_ ## target(args);			\
-	int __init bpf_iter_ ## target(args) { return 0; }
+	int BPF_INIT bpf_iter_ ## target(args) { return 0; }
 
 struct bpf_iter_aux_info {
 	struct bpf_map *map;
diff --git a/include/linux/init.h b/include/linux/init.h
index 212fc9e2f691..133462863711 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -52,6 +52,7 @@
 #define __initconst	__section(.init.rodata)
 #define __exitdata	__section(.exit.data)
 #define __exit_call	__used __section(.exitcall.exit)
+#define __init_bpf_preserve_type __section(.init.bpf.preserve_type)
 
 /*
  * modpost check for section mismatches during the kernel build.
-- 
2.26.2

Re: [PATCH 1/3] bpf: Move iterator functions into special init section

[Song Liu]

On Wed, Nov 4, 2020 at 2:02 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> With upcoming changes to pahole, that change the way how and
> which kernel functions are stored in BTF data, we need a way
> to recognize iterator functions.
>
> Iterator functions need to be in BTF data, but have no real
> body and are currently placed in .init.text section, so they
> are freed after kernel init and are filtered out of BTF data
> because of that.
>
> The solution is to place these functions under new section:
>   .init.bpf.preserve_type
>
> And add 2 new symbols to mark that area:
>   __init_bpf_preserve_type_begin
>   __init_bpf_preserve_type_end
>
> The code in pahole responsible for picking up the functions will
> be able to recognize functions from this section and add them to
> the BTF data and filter out all other .init.text functions.
>
> Suggested-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Acked-by: Song Liu <songliubraving@fb.com>

[...]