[PATCH bpf-next v5 0/9] libbpf: Implement BTFGen

[PATCH bpf-next v5 0/9] libbpf: Implement BTFGen

[Mauricio Vásquez]

CO-RE requires to have BTF information describing the kernel types in
order to perform the relocations. This is usually provided by the kernel
itself when it's configured with CONFIG_DEBUG_INFO_BTF. However, this
configuration is not enabled in all the distributions and it's not
available on kernels before 5.12.

It's possible to use CO-RE in kernels without CONFIG_DEBUG_INFO_BTF
support by providing the BTF information from an external source.
BTFHub[0] contains BTF files to each released kernel not supporting BTF,
for the most popular distributions.

Providing this BTF file for a given kernel has some challenges:
1. Each BTF file is a few MBs big, then it's not possible to ship the
eBPF program with all the BTF files needed to run in different kernels.
(The BTF files will be in the order of GBs if you want to support a high
number of kernels)
2. Downloading the BTF file for the current kernel at runtime delays the
start of the program and it's not always possible to reach an external
host to download such a file.

Providing the BTF file with the information about all the data types of
the kernel for running an eBPF program is an overkill in many of the
cases. Usually the eBPF programs access only some kernel fields.

This series implements BTFGen support in bpftool. This idea was
discussed during the "Towards truly portable eBPF"[1] presentation at
Linux Plumbers 2021.

There is a good example[2] on how to use BTFGen and BTFHub together
to generate multiple BTF files, to each existing/supported kernel,
tailored to one application. For example: a complex bpf object might
support nearly 400 kernels by having BTF files summing only 1.5 MB.

[0]: https://github.com/aquasecurity/btfhub/
[1]: https://www.youtube.com/watch?v=igJLKyP1lFk&t=2418s
[2]: https://github.com/aquasecurity/btfhub/tree/main/tools

Changelog:
v4 > v5:
- move some checks before invoking prog->obj->gen_loader
- use p_info() instead of printf()
- improve command output
- fix issue with record_relo_core()
- implement bash completion
- write man page
- implement some tests

v3 > v4:
- parse BTF and BTF.ext sections in bpftool and use
  bpf_core_calc_relo_insn() directly
- expose less internal details from libbpf to bpftool
- implement support for enum-based relocations
- split commits in a more granular way

v2 > v3:
- expose internal libbpf APIs to bpftool instead
- implement btfgen in bpftool
- drop btf__raw_data() from libbpf

v1 > v2:
- introduce bpf_object__prepare() and ‘record_core_relos’ to expose
  CO-RE relocations instead of bpf_object__reloc_info_gen()
- rename btf__save_to_file() to btf__raw_data()

v1: https://lore.kernel.org/bpf/20211027203727.208847-1-mauricio@kinvolk.io/
v2: https://lore.kernel.org/bpf/20211116164208.164245-1-mauricio@kinvolk.io/
v3: https://lore.kernel.org/bpf/20211217185654.311609-1-mauricio@kinvolk.io/
v4: https://lore.kernel.org/bpf/20220112142709.102423-1-mauricio@kinvolk.io/

Mauricio Vásquez (8):
  libbpf: Implement changes needed for BTFGen in bpftool
  bpftool: Add gen min_core_btf command
  bpftool: Implement btf_save_raw()
  bpftool: Add struct definitions and helpers for BTFGen
  bpftool: Implement btfgen()
  bpftool: Implement relocations recording for BTFGen
  bpftool: Implement btfgen_get_btf()
  selftest/bpf: Implement tests for bpftool gen min_core_btf

Rafael David Tinoco (1):
  bpftool: gen min_core_btf explanation and examples

 .../bpf/bpftool/Documentation/bpftool-gen.rst |  85 ++
 tools/bpf/bpftool/Makefile                    |   8 +-
 tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
 tools/bpf/bpftool/gen.c                       | 836 +++++++++++++++++-
 tools/lib/bpf/libbpf.c                        |  44 +-
 tools/lib/bpf/libbpf_internal.h               |  12 +
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   4 +-
 .../selftests/bpf/progs/btfgen_btf_source.c   |  12 +
 .../bpf/progs/btfgen_primitives_array.c       |  39 +
 .../bpf/progs/btfgen_primitives_struct.c      |  40 +
 .../bpf/progs/btfgen_primitives_struct2.c     |  44 +
 .../bpf/progs/btfgen_primitives_union.c       |  32 +
 tools/testing/selftests/bpf/test_bpftool.c    | 228 +++++
 14 files changed, 1367 insertions(+), 24 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_btf_source.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_array.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_union.c
 create mode 100644 tools/testing/selftests/bpf/test_bpftool.c

-- 
2.25.1

[PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Mauricio Vásquez]

This commit extends libbpf with the features that are needed to
implement BTFGen:

- Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
to handle candidates cache.
- Expose bpf_core_add_cands() and bpf_core_free_cands to handle
candidates list.
- Expose bpf_core_calc_relo_insn() to bpftool.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
 tools/lib/bpf/libbpf_internal.h | 12 +++++++++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 12771f71a6e7..61384d219e28 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
 	return n;
 }
 
-static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
+void bpf_core_free_cands(struct bpf_core_cand_list *cands)
 {
 	free(cands->cands);
 	free(cands);
 }
 
-static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
-			      size_t local_essent_len,
-			      const struct btf *targ_btf,
-			      const char *targ_btf_name,
-			      int targ_start_id,
-			      struct bpf_core_cand_list *cands)
+int bpf_core_add_cands(struct bpf_core_cand *local_cand,
+		       size_t local_essent_len,
+		       const struct btf *targ_btf,
+		       const char *targ_btf_name,
+		       int targ_start_id,
+		       struct bpf_core_cand_list *cands)
 {
 	struct bpf_core_cand *new_cands, *cand;
 	const struct btf_type *t, *local_t;
@@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
 				       targ_res);
 }
 
+struct hashmap *bpf_core_create_cand_cache(void)
+{
+	return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
+}
+
+void bpf_core_free_cand_cache(struct hashmap *cand_cache)
+{
+	struct hashmap_entry *entry;
+	int i;
+
+	if (IS_ERR_OR_NULL(cand_cache))
+		return;
+
+	hashmap__for_each_entry(cand_cache, entry, i) {
+		bpf_core_free_cands(entry->value);
+	}
+	hashmap__free(cand_cache);
+}
+
 static int
 bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 {
@@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 	struct bpf_core_relo_res targ_res;
 	const struct bpf_core_relo *rec;
 	const struct btf_ext_info *seg;
-	struct hashmap_entry *entry;
 	struct hashmap *cand_cache = NULL;
 	struct bpf_program *prog;
 	struct bpf_insn *insn;
@@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 		}
 	}
 
-	cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
+	cand_cache = bpf_core_create_cand_cache();
 	if (IS_ERR(cand_cache)) {
 		err = PTR_ERR(cand_cache);
 		goto out;
@@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
 	btf__free(obj->btf_vmlinux_override);
 	obj->btf_vmlinux_override = NULL;
 
-	if (!IS_ERR_OR_NULL(cand_cache)) {
-		hashmap__for_each_entry(cand_cache, entry, i) {
-			bpf_core_free_cands(entry->value);
-		}
-		hashmap__free(cand_cache);
-	}
+	bpf_core_free_cand_cache(cand_cache);
+
 	return err;
 }
 
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index bc86b82e90d1..686a5654262b 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
 	return fd;
 }
 
+struct hashmap;
+
+struct hashmap *bpf_core_create_cand_cache(void);
+void bpf_core_free_cand_cache(struct hashmap *cand_cache);
+int bpf_core_add_cands(struct bpf_core_cand *local_cand,
+		       size_t local_essent_len,
+		       const struct btf *targ_btf,
+		       const char *targ_btf_name,
+		       int targ_start_id,
+		       struct bpf_core_cand_list *cands);
+void bpf_core_free_cands(struct bpf_core_cand_list *cands);
+
 #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
-- 
2.25.1

Re: [PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Quentin Monnet]

2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> This commit extends libbpf with the features that are needed to
> implement BTFGen:
> 
> - Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
> to handle candidates cache.
> - Expose bpf_core_add_cands() and bpf_core_free_cands to handle
> candidates list.
> - Expose bpf_core_calc_relo_insn() to bpftool.
> 
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>

Hi, note that the patchset (or at least, this patch) does not apply
cleanly. Can you please double-check that it is based on bpf-next?

> ---
>  tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf_internal.h | 12 +++++++++
>  2 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 12771f71a6e7..61384d219e28 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
>  	return n;
>  }
>  
> -static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> +void bpf_core_free_cands(struct bpf_core_cand_list *cands)
>  {
>  	free(cands->cands);
>  	free(cands);
>  }
>  
> -static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> -			      size_t local_essent_len,
> -			      const struct btf *targ_btf,
> -			      const char *targ_btf_name,
> -			      int targ_start_id,
> -			      struct bpf_core_cand_list *cands)
> +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> +		       size_t local_essent_len,
> +		       const struct btf *targ_btf,
> +		       const char *targ_btf_name,
> +		       int targ_start_id,
> +		       struct bpf_core_cand_list *cands)
>  {
>  	struct bpf_core_cand *new_cands, *cand;
>  	const struct btf_type *t, *local_t;
> @@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
>  				       targ_res);
>  }
>  
> +struct hashmap *bpf_core_create_cand_cache(void)
> +{
> +	return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> +}
> +
> +void bpf_core_free_cand_cache(struct hashmap *cand_cache)
> +{
> +	struct hashmap_entry *entry;
> +	int i;
> +
> +	if (IS_ERR_OR_NULL(cand_cache))
> +		return;
> +
> +	hashmap__for_each_entry(cand_cache, entry, i) {
> +		bpf_core_free_cands(entry->value);
> +	}
> +	hashmap__free(cand_cache);
> +}
> +
>  static int
>  bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>  {
> @@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>  	struct bpf_core_relo_res targ_res;
>  	const struct bpf_core_relo *rec;
>  	const struct btf_ext_info *seg;
> -	struct hashmap_entry *entry;
>  	struct hashmap *cand_cache = NULL;
>  	struct bpf_program *prog;
>  	struct bpf_insn *insn;
> @@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>  		}
>  	}
>  
> -	cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> +	cand_cache = bpf_core_create_cand_cache();
>  	if (IS_ERR(cand_cache)) {
>  		err = PTR_ERR(cand_cache);
>  		goto out;
> @@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>  	btf__free(obj->btf_vmlinux_override);
>  	obj->btf_vmlinux_override = NULL;
>  
> -	if (!IS_ERR_OR_NULL(cand_cache)) {
> -		hashmap__for_each_entry(cand_cache, entry, i) {
> -			bpf_core_free_cands(entry->value);
> -		}
> -		hashmap__free(cand_cache);
> -	}
> +	bpf_core_free_cand_cache(cand_cache);
> +
>  	return err;
>  }
>  
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index bc86b82e90d1..686a5654262b 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
>  	return fd;
>  }
>  
> +struct hashmap;
> +
> +struct hashmap *bpf_core_create_cand_cache(void);
> +void bpf_core_free_cand_cache(struct hashmap *cand_cache);
> +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> +		       size_t local_essent_len,
> +		       const struct btf *targ_btf,
> +		       const char *targ_btf_name,
> +		       int targ_start_id,
> +		       struct bpf_core_cand_list *cands);
> +void bpf_core_free_cands(struct bpf_core_cand_list *cands);

I wonder if these might deserve a comment to mention that they are
exposed for bpftool? I fear someone might attempt to clean it up and
remove the unused exports otherwise.

Re: [PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Mauricio Vásquez Bernal]

On Tue, Feb 1, 2022 at 3:57 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> > This commit extends libbpf with the features that are needed to
> > implement BTFGen:
> >
> > - Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
> > to handle candidates cache.
> > - Expose bpf_core_add_cands() and bpf_core_free_cands to handle
> > candidates list.
> > - Expose bpf_core_calc_relo_insn() to bpftool.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
>
> Hi, note that the patchset (or at least, this patch) does not apply
> cleanly. Can you please double-check that it is based on bpf-next?

I missed one commit on this submission...

> > ---
> >  tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
> >  tools/lib/bpf/libbpf_internal.h | 12 +++++++++
> >  2 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 12771f71a6e7..61384d219e28 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
> >       return n;
> >  }
> >
> > -static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> > +void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> >  {
> >       free(cands->cands);
> >       free(cands);
> >  }
> >
> > -static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > -                           size_t local_essent_len,
> > -                           const struct btf *targ_btf,
> > -                           const char *targ_btf_name,
> > -                           int targ_start_id,
> > -                           struct bpf_core_cand_list *cands)
> > +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > +                    size_t local_essent_len,
> > +                    const struct btf *targ_btf,
> > +                    const char *targ_btf_name,
> > +                    int targ_start_id,
> > +                    struct bpf_core_cand_list *cands)
> >  {
> >       struct bpf_core_cand *new_cands, *cand;
> >       const struct btf_type *t, *local_t;
> > @@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
> >                                      targ_res);
> >  }
> >
> > +struct hashmap *bpf_core_create_cand_cache(void)
> > +{
> > +     return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +}
> > +
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache)
> > +{
> > +     struct hashmap_entry *entry;
> > +     int i;
> > +
> > +     if (IS_ERR_OR_NULL(cand_cache))
> > +             return;
> > +
> > +     hashmap__for_each_entry(cand_cache, entry, i) {
> > +             bpf_core_free_cands(entry->value);
> > +     }
> > +     hashmap__free(cand_cache);
> > +}
> > +
> >  static int
> >  bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >  {
> > @@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >       struct bpf_core_relo_res targ_res;
> >       const struct bpf_core_relo *rec;
> >       const struct btf_ext_info *seg;
> > -     struct hashmap_entry *entry;
> >       struct hashmap *cand_cache = NULL;
> >       struct bpf_program *prog;
> >       struct bpf_insn *insn;
> > @@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >               }
> >       }
> >
> > -     cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +     cand_cache = bpf_core_create_cand_cache();
> >       if (IS_ERR(cand_cache)) {
> >               err = PTR_ERR(cand_cache);
> >               goto out;
> > @@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >       btf__free(obj->btf_vmlinux_override);
> >       obj->btf_vmlinux_override = NULL;
> >
> > -     if (!IS_ERR_OR_NULL(cand_cache)) {
> > -             hashmap__for_each_entry(cand_cache, entry, i) {
> > -                     bpf_core_free_cands(entry->value);
> > -             }
> > -             hashmap__free(cand_cache);
> > -     }
> > +     bpf_core_free_cand_cache(cand_cache);
> > +
> >       return err;
> >  }
> >
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index bc86b82e90d1..686a5654262b 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
> >       return fd;
> >  }
> >
> > +struct hashmap;
> > +
> > +struct hashmap *bpf_core_create_cand_cache(void);
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache);
> > +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > +                    size_t local_essent_len,
> > +                    const struct btf *targ_btf,
> > +                    const char *targ_btf_name,
> > +                    int targ_start_id,
> > +                    struct bpf_core_cand_list *cands);
> > +void bpf_core_free_cands(struct bpf_core_cand_list *cands);
>
> I wonder if these might deserve a comment to mention that they are
> exposed for bpftool? I fear someone might attempt to clean it up and
> remove the unused exports otherwise.

I added a comment there.

Re: [PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> This commit extends libbpf with the features that are needed to
> implement BTFGen:
>
> - Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
> to handle candidates cache.
> - Expose bpf_core_add_cands() and bpf_core_free_cands to handle
> candidates list.
> - Expose bpf_core_calc_relo_insn() to bpftool.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf_internal.h | 12 +++++++++
>  2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 12771f71a6e7..61384d219e28 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
>         return n;
>  }
>
> -static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> +void bpf_core_free_cands(struct bpf_core_cand_list *cands)
>  {
>         free(cands->cands);
>         free(cands);
>  }
>
> -static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> -                             size_t local_essent_len,
> -                             const struct btf *targ_btf,
> -                             const char *targ_btf_name,
> -                             int targ_start_id,
> -                             struct bpf_core_cand_list *cands)
> +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> +                      size_t local_essent_len,
> +                      const struct btf *targ_btf,
> +                      const char *targ_btf_name,
> +                      int targ_start_id,
> +                      struct bpf_core_cand_list *cands)
>  {
>         struct bpf_core_cand *new_cands, *cand;
>         const struct btf_type *t, *local_t;
> @@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
>                                        targ_res);
>  }
>
> +struct hashmap *bpf_core_create_cand_cache(void)
> +{
> +       return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> +}
> +
> +void bpf_core_free_cand_cache(struct hashmap *cand_cache)
> +{
> +       struct hashmap_entry *entry;
> +       int i;
> +
> +       if (IS_ERR_OR_NULL(cand_cache))
> +               return;
> +
> +       hashmap__for_each_entry(cand_cache, entry, i) {
> +               bpf_core_free_cands(entry->value);
> +       }
> +       hashmap__free(cand_cache);
> +}
> +
>  static int
>  bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>  {
> @@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>         struct bpf_core_relo_res targ_res;
>         const struct bpf_core_relo *rec;
>         const struct btf_ext_info *seg;
> -       struct hashmap_entry *entry;
>         struct hashmap *cand_cache = NULL;
>         struct bpf_program *prog;
>         struct bpf_insn *insn;
> @@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>                 }
>         }
>
> -       cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> +       cand_cache = bpf_core_create_cand_cache();
>         if (IS_ERR(cand_cache)) {
>                 err = PTR_ERR(cand_cache);
>                 goto out;
> @@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
>         btf__free(obj->btf_vmlinux_override);
>         obj->btf_vmlinux_override = NULL;
>
> -       if (!IS_ERR_OR_NULL(cand_cache)) {
> -               hashmap__for_each_entry(cand_cache, entry, i) {
> -                       bpf_core_free_cands(entry->value);
> -               }
> -               hashmap__free(cand_cache);
> -       }
> +       bpf_core_free_cand_cache(cand_cache);
> +
>         return err;
>  }
>
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index bc86b82e90d1..686a5654262b 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
>         return fd;
>  }
>
> +struct hashmap;
> +
> +struct hashmap *bpf_core_create_cand_cache(void);
> +void bpf_core_free_cand_cache(struct hashmap *cand_cache);

looking at patch #5, there is nothing special about this cand_cache,
it's just a hashmap from u32 to some pointer. There is no need for
libbpf to expose it to bpftool, you already have hashmap itself and
also btfgen_hash_fn and equality callback, just do the same thing as
you do with btfgen_info->types hashmap.


> +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> +                      size_t local_essent_len,
> +                      const struct btf *targ_btf,
> +                      const char *targ_btf_name,
> +                      int targ_start_id,
> +                      struct bpf_core_cand_list *cands);
> +void bpf_core_free_cands(struct bpf_core_cand_list *cands);
> +
>  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Andrii Nakryiko]

On Wed, Feb 2, 2022 at 10:54 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This commit extends libbpf with the features that are needed to
> > implement BTFGen:
> >
> > - Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
> > to handle candidates cache.
> > - Expose bpf_core_add_cands() and bpf_core_free_cands to handle
> > candidates list.
> > - Expose bpf_core_calc_relo_insn() to bpftool.

I don't see this in this patch. I also don't see
bpf_core_calc_relo_insn() in current bpf-next either.

> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
> >  tools/lib/bpf/libbpf_internal.h | 12 +++++++++
> >  2 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 12771f71a6e7..61384d219e28 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
> >         return n;
> >  }
> >
> > -static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> > +void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> >  {
> >         free(cands->cands);
> >         free(cands);
> >  }
> >
> > -static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > -                             size_t local_essent_len,
> > -                             const struct btf *targ_btf,
> > -                             const char *targ_btf_name,
> > -                             int targ_start_id,
> > -                             struct bpf_core_cand_list *cands)
> > +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > +                      size_t local_essent_len,
> > +                      const struct btf *targ_btf,
> > +                      const char *targ_btf_name,
> > +                      int targ_start_id,
> > +                      struct bpf_core_cand_list *cands)
> >  {
> >         struct bpf_core_cand *new_cands, *cand;
> >         const struct btf_type *t, *local_t;
> > @@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
> >                                        targ_res);
> >  }
> >
> > +struct hashmap *bpf_core_create_cand_cache(void)
> > +{
> > +       return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +}
> > +
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache)
> > +{
> > +       struct hashmap_entry *entry;
> > +       int i;
> > +
> > +       if (IS_ERR_OR_NULL(cand_cache))
> > +               return;
> > +
> > +       hashmap__for_each_entry(cand_cache, entry, i) {
> > +               bpf_core_free_cands(entry->value);
> > +       }
> > +       hashmap__free(cand_cache);
> > +}
> > +
> >  static int
> >  bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >  {
> > @@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >         struct bpf_core_relo_res targ_res;
> >         const struct bpf_core_relo *rec;
> >         const struct btf_ext_info *seg;
> > -       struct hashmap_entry *entry;
> >         struct hashmap *cand_cache = NULL;
> >         struct bpf_program *prog;
> >         struct bpf_insn *insn;
> > @@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >                 }
> >         }
> >
> > -       cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +       cand_cache = bpf_core_create_cand_cache();
> >         if (IS_ERR(cand_cache)) {
> >                 err = PTR_ERR(cand_cache);
> >                 goto out;
> > @@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >         btf__free(obj->btf_vmlinux_override);
> >         obj->btf_vmlinux_override = NULL;
> >
> > -       if (!IS_ERR_OR_NULL(cand_cache)) {
> > -               hashmap__for_each_entry(cand_cache, entry, i) {
> > -                       bpf_core_free_cands(entry->value);
> > -               }
> > -               hashmap__free(cand_cache);
> > -       }
> > +       bpf_core_free_cand_cache(cand_cache);
> > +
> >         return err;
> >  }
> >
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index bc86b82e90d1..686a5654262b 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
> >         return fd;
> >  }
> >
> > +struct hashmap;
> > +
> > +struct hashmap *bpf_core_create_cand_cache(void);
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache);
>
> looking at patch #5, there is nothing special about this cand_cache,
> it's just a hashmap from u32 to some pointer. There is no need for
> libbpf to expose it to bpftool, you already have hashmap itself and
> also btfgen_hash_fn and equality callback, just do the same thing as
> you do with btfgen_info->types hashmap.
>
>
> > +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > +                      size_t local_essent_len,
> > +                      const struct btf *targ_btf,
> > +                      const char *targ_btf_name,
> > +                      int targ_start_id,
> > +                      struct bpf_core_cand_list *cands);
> > +void bpf_core_free_cands(struct bpf_core_cand_list *cands);
> > +
> >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > --
> > 2.25.1
> >

Re: [PATCH bpf-next v5 1/9] libbpf: Implement changes needed for BTFGen in bpftool

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 1:54 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This commit extends libbpf with the features that are needed to
> > implement BTFGen:
> >
> > - Implement bpf_core_create_cand_cache() and bpf_core_free_cand_cache()
> > to handle candidates cache.
> > - Expose bpf_core_add_cands() and bpf_core_free_cands to handle
> > candidates list.
> > - Expose bpf_core_calc_relo_insn() to bpftool.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/lib/bpf/libbpf.c          | 44 ++++++++++++++++++++++-----------
> >  tools/lib/bpf/libbpf_internal.h | 12 +++++++++
> >  2 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 12771f71a6e7..61384d219e28 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -5195,18 +5195,18 @@ size_t bpf_core_essential_name_len(const char *name)
> >         return n;
> >  }
> >
> > -static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> > +void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> >  {
> >         free(cands->cands);
> >         free(cands);
> >  }
> >
> > -static int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > -                             size_t local_essent_len,
> > -                             const struct btf *targ_btf,
> > -                             const char *targ_btf_name,
> > -                             int targ_start_id,
> > -                             struct bpf_core_cand_list *cands)
> > +int bpf_core_add_cands(struct bpf_core_cand *local_cand,
> > +                      size_t local_essent_len,
> > +                      const struct btf *targ_btf,
> > +                      const char *targ_btf_name,
> > +                      int targ_start_id,
> > +                      struct bpf_core_cand_list *cands)
> >  {
> >         struct bpf_core_cand *new_cands, *cand;
> >         const struct btf_type *t, *local_t;
> > @@ -5577,6 +5577,25 @@ static int bpf_core_resolve_relo(struct bpf_program *prog,
> >                                        targ_res);
> >  }
> >
> > +struct hashmap *bpf_core_create_cand_cache(void)
> > +{
> > +       return hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +}
> > +
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache)
> > +{
> > +       struct hashmap_entry *entry;
> > +       int i;
> > +
> > +       if (IS_ERR_OR_NULL(cand_cache))
> > +               return;
> > +
> > +       hashmap__for_each_entry(cand_cache, entry, i) {
> > +               bpf_core_free_cands(entry->value);
> > +       }
> > +       hashmap__free(cand_cache);
> > +}
> > +
> >  static int
> >  bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >  {
> > @@ -5584,7 +5603,6 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >         struct bpf_core_relo_res targ_res;
> >         const struct bpf_core_relo *rec;
> >         const struct btf_ext_info *seg;
> > -       struct hashmap_entry *entry;
> >         struct hashmap *cand_cache = NULL;
> >         struct bpf_program *prog;
> >         struct bpf_insn *insn;
> > @@ -5603,7 +5621,7 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >                 }
> >         }
> >
> > -       cand_cache = hashmap__new(bpf_core_hash_fn, bpf_core_equal_fn, NULL);
> > +       cand_cache = bpf_core_create_cand_cache();
> >         if (IS_ERR(cand_cache)) {
> >                 err = PTR_ERR(cand_cache);
> >                 goto out;
> > @@ -5694,12 +5712,8 @@ bpf_object__relocate_core(struct bpf_object *obj, const char *targ_btf_path)
> >         btf__free(obj->btf_vmlinux_override);
> >         obj->btf_vmlinux_override = NULL;
> >
> > -       if (!IS_ERR_OR_NULL(cand_cache)) {
> > -               hashmap__for_each_entry(cand_cache, entry, i) {
> > -                       bpf_core_free_cands(entry->value);
> > -               }
> > -               hashmap__free(cand_cache);
> > -       }
> > +       bpf_core_free_cand_cache(cand_cache);
> > +
> >         return err;
> >  }
> >
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index bc86b82e90d1..686a5654262b 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -529,4 +529,16 @@ static inline int ensure_good_fd(int fd)
> >         return fd;
> >  }
> >
> > +struct hashmap;
> > +
> > +struct hashmap *bpf_core_create_cand_cache(void);
> > +void bpf_core_free_cand_cache(struct hashmap *cand_cache);
>
> looking at patch #5, there is nothing special about this cand_cache,
> it's just a hashmap from u32 to some pointer. There is no need for
> libbpf to expose it to bpftool, you already have hashmap itself and
> also btfgen_hash_fn and equality callback, just do the same thing as
> you do with btfgen_info->types hashmap.
>

I'll drop them and handle the hashmap directly from bpftool.

[PATCH bpf-next v5 2/9] bpftool: Add gen min_core_btf command

[Mauricio Vásquez]

This command is implemented under the "gen" command in bpftool and the
syntax is the following:

$ bpftool gen min_core_btf INPUT OUTPUT OBJECT(S)

INPUT can be either a single BTF file or a folder containing BTF files,
when it's a folder, a BTF file is generated for each BTF file contained
in this folder. OUTPUT is the file (or folder) where generated files are
stored and OBJECT(S) is the list of bpf objects we want to generate the
BTF file(s) for (each generated BTF file contains all the types needed
by all the objects).

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/bash-completion/bpftool |   6 +-
 tools/bpf/bpftool/gen.c                   | 112 +++++++++++++++++++++-
 2 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 493753a4962e..958e1fd71b5c 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -1003,9 +1003,13 @@ _bpftool()
                             ;;
                     esac
                     ;;
+                min_core_btf)
+                    _filedir
+                    return 0
+                    ;;
                 *)
                     [[ $prev == $object ]] && \
-                        COMPREPLY=( $( compgen -W 'object skeleton help' -- "$cur" ) )
+                        COMPREPLY=( $( compgen -W 'object skeleton help min_core_btf' -- "$cur" ) )
                     ;;
             esac
             ;;
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 8f78c27d41f0..7db31b0f265f 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -5,6 +5,7 @@
 #define _GNU_SOURCE
 #endif
 #include <ctype.h>
+#include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/err.h>
@@ -1084,6 +1085,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
 		"       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
+		"       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT(S)\n"
 		"       %1$s %2$s help\n"
 		"\n"
 		"       " HELP_SPEC_OPTIONS " |\n"
@@ -1094,10 +1096,114 @@ static int do_help(int argc, char **argv)
 	return 0;
 }
 
+/* Create BTF file for a set of BPF objects */
+static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
+{
+	return -EOPNOTSUPP;
+}
+
+static int do_min_core_btf(int argc, char **argv)
+{
+	char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
+	bool input_is_file, output_is_file = true;
+	const char *input, *output;
+	const char **objs = NULL;
+	struct dirent *dir;
+	struct stat st;
+	DIR *d = NULL;
+	int i, err;
+
+	if (!REQ_ARGS(3)) {
+		usage();
+		return -1;
+	}
+
+	input = GET_ARG();
+	if (stat(input, &st) < 0) {
+		p_err("failed to stat %s: %s", input, strerror(errno));
+		return -errno;
+	}
+
+	if ((st.st_mode & S_IFMT) != S_IFDIR && (st.st_mode & S_IFMT) != S_IFREG) {
+		p_err("file type not valid: %s", input);
+		return -EINVAL;
+	}
+
+	input_is_file = (st.st_mode & S_IFMT) == S_IFREG;
+
+	output = GET_ARG();
+	if (stat(output, &st) == 0 && (st.st_mode & S_IFMT) == S_IFDIR)
+		output_is_file = false;
+
+	objs = (const char **) malloc((argc + 1) * sizeof(*objs));
+	if (!objs) {
+		p_err("failed to allocate array for object names");
+		return -ENOMEM;
+	}
+
+	i = 0;
+	while (argc > 0)
+		objs[i++] = GET_ARG();
+
+	objs[i] = NULL;
+
+	/* single BTF file */
+	if (input_is_file) {
+		p_info("Processing source BTF file: %s", input);
+
+		if (output_is_file) {
+			err = btfgen(input, output, objs);
+			goto out;
+		}
+		snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output,
+			 basename(input));
+		err = btfgen(input, dst_btf_path, objs);
+		goto out;
+	}
+
+	if (output_is_file) {
+		p_err("can't have just one file as output");
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* directory with BTF files */
+	d = opendir(input);
+	if (!d) {
+		p_err("error opening input dir: %s", strerror(errno));
+		err = -errno;
+		goto out;
+	}
+
+	while ((dir = readdir(d)) != NULL) {
+		if (dir->d_type != DT_REG)
+			continue;
+
+		if (strncmp(dir->d_name + strlen(dir->d_name) - 4, ".btf", 4))
+			continue;
+
+		snprintf(src_btf_path, sizeof(src_btf_path), "%s%s", input, dir->d_name);
+		snprintf(dst_btf_path, sizeof(dst_btf_path), "%s%s", output, dir->d_name);
+
+		p_info("Processing source BTF file: %s", src_btf_path);
+
+		err = btfgen(src_btf_path, dst_btf_path, objs);
+		if (err)
+			goto out;
+	}
+
+out:
+	free(objs);
+	if (d)
+		closedir(d);
+	return err;
+}
+
 static const struct cmd cmds[] = {
-	{ "object",	do_object },
-	{ "skeleton",	do_skeleton },
-	{ "help",	do_help },
+	{ "object",		do_object },
+	{ "skeleton",		do_skeleton },
+	{ "min_core_btf",	do_min_core_btf},
+	{ "help",		do_help },
 	{ 0 }
 };
 
-- 
2.25.1

Re: [PATCH bpf-next v5 2/9] bpftool: Add gen min_core_btf command

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> This command is implemented under the "gen" command in bpftool and the
> syntax is the following:
>
> $ bpftool gen min_core_btf INPUT OUTPUT OBJECT(S)
>
> INPUT can be either a single BTF file or a folder containing BTF files,
> when it's a folder, a BTF file is generated for each BTF file contained
> in this folder. OUTPUT is the file (or folder) where generated files are
> stored and OBJECT(S) is the list of bpf objects we want to generate the
> BTF file(s) for (each generated BTF file contains all the types needed
> by all the objects).
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/bash-completion/bpftool |   6 +-
>  tools/bpf/bpftool/gen.c                   | 112 +++++++++++++++++++++-
>  2 files changed, 114 insertions(+), 4 deletions(-)
>
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 493753a4962e..958e1fd71b5c 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -1003,9 +1003,13 @@ _bpftool()
>                              ;;
>                      esac
>                      ;;
> +                min_core_btf)
> +                    _filedir
> +                    return 0
> +                    ;;
>                  *)
>                      [[ $prev == $object ]] && \
> -                        COMPREPLY=( $( compgen -W 'object skeleton help' -- "$cur" ) )
> +                        COMPREPLY=( $( compgen -W 'object skeleton help min_core_btf' -- "$cur" ) )
>                      ;;
>              esac
>              ;;
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 8f78c27d41f0..7db31b0f265f 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -5,6 +5,7 @@
>  #define _GNU_SOURCE
>  #endif
>  #include <ctype.h>
> +#include <dirent.h>
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <linux/err.h>
> @@ -1084,6 +1085,7 @@ static int do_help(int argc, char **argv)
>         fprintf(stderr,
>                 "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
>                 "       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
> +               "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT(S)\n"

OBJECTS(S) should be OBJECT... for this "CLI notation", no?

>                 "       %1$s %2$s help\n"
>                 "\n"
>                 "       " HELP_SPEC_OPTIONS " |\n"
> @@ -1094,10 +1096,114 @@ static int do_help(int argc, char **argv)
>         return 0;
>  }
>
> +/* Create BTF file for a set of BPF objects */
> +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static int do_min_core_btf(int argc, char **argv)
> +{
> +       char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> +       bool input_is_file, output_is_file = true;
> +       const char *input, *output;
> +       const char **objs = NULL;
> +       struct dirent *dir;
> +       struct stat st;
> +       DIR *d = NULL;
> +       int i, err;
> +
> +       if (!REQ_ARGS(3)) {
> +               usage();
> +               return -1;
> +       }
> +
> +       input = GET_ARG();
> +       if (stat(input, &st) < 0) {
> +               p_err("failed to stat %s: %s", input, strerror(errno));
> +               return -errno;
> +       }
> +
> +       if ((st.st_mode & S_IFMT) != S_IFDIR && (st.st_mode & S_IFMT) != S_IFREG) {
> +               p_err("file type not valid: %s", input);
> +               return -EINVAL;
> +       }
> +
> +       input_is_file = (st.st_mode & S_IFMT) == S_IFREG;

move before if and use input_is_file in the if itself instead of
duplicating all the S_IFREG flags?

> +
> +       output = GET_ARG();
> +       if (stat(output, &st) == 0 && (st.st_mode & S_IFMT) == S_IFDIR)
> +               output_is_file = false;

if stat() succeeds but it's neither directory or file, should be an
error, right?

> +
> +       objs = (const char **) malloc((argc + 1) * sizeof(*objs));

calloc() seems to be better suited for this (and zero-intialization is
nice for safety and to avoid objs[argc] = NULL after the loop below)

> +       if (!objs) {
> +               p_err("failed to allocate array for object names");
> +               return -ENOMEM;
> +       }
> +
> +       i = 0;
> +       while (argc > 0)
> +               objs[i++] = GET_ARG();

for (i = 0; i < argc; i++) ?

> +
> +       objs[i] = NULL;
> +
> +       /* single BTF file */
> +       if (input_is_file) {
> +               p_info("Processing source BTF file: %s", input);
> +
> +               if (output_is_file) {
> +                       err = btfgen(input, output, objs);
> +                       goto out;
> +               }
> +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output,
> +                        basename(input));
> +               err = btfgen(input, dst_btf_path, objs);
> +               goto out;
> +       }
> +
> +       if (output_is_file) {
> +               p_err("can't have just one file as output");
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* directory with BTF files */
> +       d = opendir(input);
> +       if (!d) {
> +               p_err("error opening input dir: %s", strerror(errno));
> +               err = -errno;
> +               goto out;
> +       }
> +
> +       while ((dir = readdir(d)) != NULL) {
> +               if (dir->d_type != DT_REG)
> +                       continue;
> +
> +               if (strncmp(dir->d_name + strlen(dir->d_name) - 4, ".btf", 4))
> +                       continue;

this whole handling of input directory feels a bit icky, tbh... maybe
we should require explicit listing of input files always. In CLI
invocation those could be separated by "keywords", something like
this:

bpftool gen min_core_btf <output> inputs <file1> <file2> .... objects
<obj1> <obj2> ...

a bit of a downside is that you can't have a file named "inputs" or
"objects", but that seems extremely unlikely? Quentin, any opinion as
well?

I'm mainly off put by a bit random ".btf" naming convention, the
DT_REG skipping, etc.

Another cleaner alternative from POV of bpftool (but might be less
convenient for users) is to use @file convention to specify a file
that contains a list of files. So

bpftool gen min_core_btf <output> @btf_filelist.txt @obj_filelist.txt

would take lists of inputs and outputs from respective files?


But actually, let's take a step back again. Why should there be
multiple inputs and outputs? I can see why multiple objects are
mandatory (you have an application that has multiple BPF objects used
internally). But processing single vmlinux BTF at a time seems
absolutely fine. I don't buy that CO-RE relo processing is that slow
to require optimized batch processing.

I might have asked this before, sorry, but the duration between each
iteration of btfgen is pretty long and I'm losing the context.

> +
> +               snprintf(src_btf_path, sizeof(src_btf_path), "%s%s", input, dir->d_name);
> +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s%s", output, dir->d_name);
> +
> +               p_info("Processing source BTF file: %s", src_btf_path);
> +
> +               err = btfgen(src_btf_path, dst_btf_path, objs);
> +               if (err)
> +                       goto out;
> +       }
> +
> +out:
> +       free(objs);
> +       if (d)
> +               closedir(d);
> +       return err;
> +}
> +
>  static const struct cmd cmds[] = {
> -       { "object",     do_object },
> -       { "skeleton",   do_skeleton },
> -       { "help",       do_help },
> +       { "object",             do_object },
> +       { "skeleton",           do_skeleton },
> +       { "min_core_btf",       do_min_core_btf},
> +       { "help",               do_help },
>         { 0 }
>  };
>
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 2/9] bpftool: Add gen min_core_btf command

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 12:58 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This command is implemented under the "gen" command in bpftool and the
> > syntax is the following:
> >
> > $ bpftool gen min_core_btf INPUT OUTPUT OBJECT(S)
> >
> > INPUT can be either a single BTF file or a folder containing BTF files,
> > when it's a folder, a BTF file is generated for each BTF file contained
> > in this folder. OUTPUT is the file (or folder) where generated files are
> > stored and OBJECT(S) is the list of bpf objects we want to generate the
> > BTF file(s) for (each generated BTF file contains all the types needed
> > by all the objects).
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/bpf/bpftool/bash-completion/bpftool |   6 +-
> >  tools/bpf/bpftool/gen.c                   | 112 +++++++++++++++++++++-
> >  2 files changed, 114 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index 493753a4962e..958e1fd71b5c 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -1003,9 +1003,13 @@ _bpftool()
> >                              ;;
> >                      esac
> >                      ;;
> > +                min_core_btf)
> > +                    _filedir
> > +                    return 0
> > +                    ;;
> >                  *)
> >                      [[ $prev == $object ]] && \
> > -                        COMPREPLY=( $( compgen -W 'object skeleton help' -- "$cur" ) )
> > +                        COMPREPLY=( $( compgen -W 'object skeleton help min_core_btf' -- "$cur" ) )
> >                      ;;
> >              esac
> >              ;;
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 8f78c27d41f0..7db31b0f265f 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -5,6 +5,7 @@
> >  #define _GNU_SOURCE
> >  #endif
> >  #include <ctype.h>
> > +#include <dirent.h>
> >  #include <errno.h>
> >  #include <fcntl.h>
> >  #include <linux/err.h>
> > @@ -1084,6 +1085,7 @@ static int do_help(int argc, char **argv)
> >         fprintf(stderr,
> >                 "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
> >                 "       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
> > +               "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT(S)\n"
>
> OBJECTS(S) should be OBJECT... for this "CLI notation", no?

Updated it to be "min_core_btf INPUT OUTPUT OBJECT [OBJECT...]" like
the "bpftool object" command.

>
> >                 "       %1$s %2$s help\n"
> >                 "\n"
> >                 "       " HELP_SPEC_OPTIONS " |\n"
> > @@ -1094,10 +1096,114 @@ static int do_help(int argc, char **argv)
> >         return 0;
> >  }
> >
> > +/* Create BTF file for a set of BPF objects */
> > +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int do_min_core_btf(int argc, char **argv)
> > +{
> > +       char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> > +       bool input_is_file, output_is_file = true;
> > +       const char *input, *output;
> > +       const char **objs = NULL;
> > +       struct dirent *dir;
> > +       struct stat st;
> > +       DIR *d = NULL;
> > +       int i, err;
> > +
> > +       if (!REQ_ARGS(3)) {
> > +               usage();
> > +               return -1;
> > +       }
> > +
> > +       input = GET_ARG();
> > +       if (stat(input, &st) < 0) {
> > +               p_err("failed to stat %s: %s", input, strerror(errno));
> > +               return -errno;
> > +       }
> > +
> > +       if ((st.st_mode & S_IFMT) != S_IFDIR && (st.st_mode & S_IFMT) != S_IFREG) {
> > +               p_err("file type not valid: %s", input);
> > +               return -EINVAL;
> > +       }
> > +
> > +       input_is_file = (st.st_mode & S_IFMT) == S_IFREG;
>
> move before if and use input_is_file in the if itself instead of
> duplicating all the S_IFREG flags?
>
> > +
> > +       output = GET_ARG();
> > +       if (stat(output, &st) == 0 && (st.st_mode & S_IFMT) == S_IFDIR)
> > +               output_is_file = false;
>
> if stat() succeeds but it's neither directory or file, should be an
> error, right?
>
> > +
> > +       objs = (const char **) malloc((argc + 1) * sizeof(*objs));
>
> calloc() seems to be better suited for this (and zero-intialization is
> nice for safety and to avoid objs[argc] = NULL after the loop below)
>

You're right!

> > +       if (!objs) {
> > +               p_err("failed to allocate array for object names");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       i = 0;
> > +       while (argc > 0)
> > +               objs[i++] = GET_ARG();
>
> for (i = 0; i < argc; i++) ?

GET_ARG() does argc--. I see this loop is usually written as while
(argc) in bpftool.

>
> > +
> > +       objs[i] = NULL;
> > +
> > +       /* single BTF file */
> > +       if (input_is_file) {
> > +               p_info("Processing source BTF file: %s", input);
> > +
> > +               if (output_is_file) {
> > +                       err = btfgen(input, output, objs);
> > +                       goto out;
> > +               }
> > +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output,
> > +                        basename(input));
> > +               err = btfgen(input, dst_btf_path, objs);
> > +               goto out;
> > +       }
> > +
> > +       if (output_is_file) {
> > +               p_err("can't have just one file as output");
> > +               err = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       /* directory with BTF files */
> > +       d = opendir(input);
> > +       if (!d) {
> > +               p_err("error opening input dir: %s", strerror(errno));
> > +               err = -errno;
> > +               goto out;
> > +       }
> > +
> > +       while ((dir = readdir(d)) != NULL) {
> > +               if (dir->d_type != DT_REG)
> > +                       continue;
> > +
> > +               if (strncmp(dir->d_name + strlen(dir->d_name) - 4, ".btf", 4))
> > +                       continue;
>
> this whole handling of input directory feels a bit icky, tbh... maybe
> we should require explicit listing of input files always. In CLI
> invocation those could be separated by "keywords", something like
> this:
>
> bpftool gen min_core_btf <output> inputs <file1> <file2> .... objects
> <obj1> <obj2> ...
>
> a bit of a downside is that you can't have a file named "inputs" or
> "objects", but that seems extremely unlikely? Quentin, any opinion as
> well?
>
> I'm mainly off put by a bit random ".btf" naming convention, the
> DT_REG skipping, etc.
>
> Another cleaner alternative from POV of bpftool (but might be less
> convenient for users) is to use @file convention to specify a file
> that contains a list of files. So
>
> bpftool gen min_core_btf <output> @btf_filelist.txt @obj_filelist.txt
>
> would take lists of inputs and outputs from respective files?
>
>
> But actually, let's take a step back again. Why should there be
> multiple inputs and outputs?

We're thinking about the use case when there are multiple source BTF
files in a folder and we want to generate a BTF for each one of them,
by supporting input and output folders we're able to avoid executing
bpftool multiple times. I agree that it complicates the implementation
and that the same can be done by using a script to run bpftool
multiple times, hence let's remove it. If we find out later on that
this is really important we can implement it.


> I can see why multiple objects are
> mandatory (you have an application that has multiple BPF objects used
> internally). But processing single vmlinux BTF at a time seems
> absolutely fine. I don't buy that CO-RE relo processing is that slow
> to require optimized batch processing.
>
> I might have asked this before, sorry, but the duration between each
> iteration of btfgen is pretty long and I'm losing the context.
>
> > +
> > +               snprintf(src_btf_path, sizeof(src_btf_path), "%s%s", input, dir->d_name);
> > +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s%s", output, dir->d_name);
> > +
> > +               p_info("Processing source BTF file: %s", src_btf_path);
> > +
> > +               err = btfgen(src_btf_path, dst_btf_path, objs);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +out:
> > +       free(objs);
> > +       if (d)
> > +               closedir(d);
> > +       return err;
> > +}
> > +
> >  static const struct cmd cmds[] = {
> > -       { "object",     do_object },
> > -       { "skeleton",   do_skeleton },
> > -       { "help",       do_help },
> > +       { "object",             do_object },
> > +       { "skeleton",           do_skeleton },
> > +       { "min_core_btf",       do_min_core_btf},
> > +       { "help",               do_help },
> >         { 0 }
> >  };
> >
> > --
> > 2.25.1
> >

Re: [PATCH bpf-next v5 2/9] bpftool: Add gen min_core_btf command

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 8:07 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 12:58 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > This command is implemented under the "gen" command in bpftool and the
> > > syntax is the following:
> > >
> > > $ bpftool gen min_core_btf INPUT OUTPUT OBJECT(S)
> > >
> > > INPUT can be either a single BTF file or a folder containing BTF files,
> > > when it's a folder, a BTF file is generated for each BTF file contained
> > > in this folder. OUTPUT is the file (or folder) where generated files are
> > > stored and OBJECT(S) is the list of bpf objects we want to generate the
> > > BTF file(s) for (each generated BTF file contains all the types needed
> > > by all the objects).
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > > ---
> > >  tools/bpf/bpftool/bash-completion/bpftool |   6 +-
> > >  tools/bpf/bpftool/gen.c                   | 112 +++++++++++++++++++++-
> > >  2 files changed, 114 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > > index 493753a4962e..958e1fd71b5c 100644
> > > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > > @@ -1003,9 +1003,13 @@ _bpftool()
> > >                              ;;
> > >                      esac
> > >                      ;;
> > > +                min_core_btf)
> > > +                    _filedir
> > > +                    return 0
> > > +                    ;;
> > >                  *)
> > >                      [[ $prev == $object ]] && \
> > > -                        COMPREPLY=( $( compgen -W 'object skeleton help' -- "$cur" ) )
> > > +                        COMPREPLY=( $( compgen -W 'object skeleton help min_core_btf' -- "$cur" ) )
> > >                      ;;
> > >              esac
> > >              ;;
> > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > > index 8f78c27d41f0..7db31b0f265f 100644
> > > --- a/tools/bpf/bpftool/gen.c
> > > +++ b/tools/bpf/bpftool/gen.c
> > > @@ -5,6 +5,7 @@
> > >  #define _GNU_SOURCE
> > >  #endif
> > >  #include <ctype.h>
> > > +#include <dirent.h>
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > >  #include <linux/err.h>
> > > @@ -1084,6 +1085,7 @@ static int do_help(int argc, char **argv)
> > >         fprintf(stderr,
> > >                 "Usage: %1$s %2$s object OUTPUT_FILE INPUT_FILE [INPUT_FILE...]\n"
> > >                 "       %1$s %2$s skeleton FILE [name OBJECT_NAME]\n"
> > > +               "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT(S)\n"
> >
> > OBJECTS(S) should be OBJECT... for this "CLI notation", no?
>
> Updated it to be "min_core_btf INPUT OUTPUT OBJECT [OBJECT...]" like
> the "bpftool object" command.
>
> >
> > >                 "       %1$s %2$s help\n"
> > >                 "\n"
> > >                 "       " HELP_SPEC_OPTIONS " |\n"
> > > @@ -1094,10 +1096,114 @@ static int do_help(int argc, char **argv)
> > >         return 0;
> > >  }
> > >
> > > +/* Create BTF file for a set of BPF objects */
> > > +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > > +{
> > > +       return -EOPNOTSUPP;
> > > +}
> > > +
> > > +static int do_min_core_btf(int argc, char **argv)
> > > +{
> > > +       char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> > > +       bool input_is_file, output_is_file = true;
> > > +       const char *input, *output;
> > > +       const char **objs = NULL;
> > > +       struct dirent *dir;
> > > +       struct stat st;
> > > +       DIR *d = NULL;
> > > +       int i, err;
> > > +
> > > +       if (!REQ_ARGS(3)) {
> > > +               usage();
> > > +               return -1;
> > > +       }
> > > +
> > > +       input = GET_ARG();
> > > +       if (stat(input, &st) < 0) {
> > > +               p_err("failed to stat %s: %s", input, strerror(errno));
> > > +               return -errno;
> > > +       }
> > > +
> > > +       if ((st.st_mode & S_IFMT) != S_IFDIR && (st.st_mode & S_IFMT) != S_IFREG) {
> > > +               p_err("file type not valid: %s", input);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       input_is_file = (st.st_mode & S_IFMT) == S_IFREG;
> >
> > move before if and use input_is_file in the if itself instead of
> > duplicating all the S_IFREG flags?
> >
> > > +
> > > +       output = GET_ARG();
> > > +       if (stat(output, &st) == 0 && (st.st_mode & S_IFMT) == S_IFDIR)
> > > +               output_is_file = false;
> >
> > if stat() succeeds but it's neither directory or file, should be an
> > error, right?
> >
> > > +
> > > +       objs = (const char **) malloc((argc + 1) * sizeof(*objs));
> >
> > calloc() seems to be better suited for this (and zero-intialization is
> > nice for safety and to avoid objs[argc] = NULL after the loop below)
> >
>
> You're right!
>
> > > +       if (!objs) {
> > > +               p_err("failed to allocate array for object names");
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       i = 0;
> > > +       while (argc > 0)
> > > +               objs[i++] = GET_ARG();
> >
> > for (i = 0; i < argc; i++) ?
>
> GET_ARG() does argc--. I see this loop is usually written as while
> (argc) in bpftool.

Ah, I missed GET_ARG()'s side effect.

>
> >
> > > +
> > > +       objs[i] = NULL;
> > > +
> > > +       /* single BTF file */
> > > +       if (input_is_file) {
> > > +               p_info("Processing source BTF file: %s", input);
> > > +
> > > +               if (output_is_file) {
> > > +                       err = btfgen(input, output, objs);
> > > +                       goto out;
> > > +               }
> > > +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s/%s", output,
> > > +                        basename(input));
> > > +               err = btfgen(input, dst_btf_path, objs);
> > > +               goto out;
> > > +       }
> > > +
> > > +       if (output_is_file) {
> > > +               p_err("can't have just one file as output");
> > > +               err = -EINVAL;
> > > +               goto out;
> > > +       }
> > > +
> > > +       /* directory with BTF files */
> > > +       d = opendir(input);
> > > +       if (!d) {
> > > +               p_err("error opening input dir: %s", strerror(errno));
> > > +               err = -errno;
> > > +               goto out;
> > > +       }
> > > +
> > > +       while ((dir = readdir(d)) != NULL) {
> > > +               if (dir->d_type != DT_REG)
> > > +                       continue;
> > > +
> > > +               if (strncmp(dir->d_name + strlen(dir->d_name) - 4, ".btf", 4))
> > > +                       continue;
> >
> > this whole handling of input directory feels a bit icky, tbh... maybe
> > we should require explicit listing of input files always. In CLI
> > invocation those could be separated by "keywords", something like
> > this:
> >
> > bpftool gen min_core_btf <output> inputs <file1> <file2> .... objects
> > <obj1> <obj2> ...
> >
> > a bit of a downside is that you can't have a file named "inputs" or
> > "objects", but that seems extremely unlikely? Quentin, any opinion as
> > well?
> >
> > I'm mainly off put by a bit random ".btf" naming convention, the
> > DT_REG skipping, etc.
> >
> > Another cleaner alternative from POV of bpftool (but might be less
> > convenient for users) is to use @file convention to specify a file
> > that contains a list of files. So
> >
> > bpftool gen min_core_btf <output> @btf_filelist.txt @obj_filelist.txt
> >
> > would take lists of inputs and outputs from respective files?
> >
> >
> > But actually, let's take a step back again. Why should there be
> > multiple inputs and outputs?
>
> We're thinking about the use case when there are multiple source BTF
> files in a folder and we want to generate a BTF for each one of them,
> by supporting input and output folders we're able to avoid executing
> bpftool multiple times. I agree that it complicates the implementation
> and that the same can be done by using a script to run bpftool
> multiple times, hence let's remove it. If we find out later on that
> this is really important we can implement it.

Yeah, I figured you have this use case, but I think it's better to
keep the interface simple. If that becomes a big performance problem,
we can always extend it later.

>
>
> > I can see why multiple objects are
> > mandatory (you have an application that has multiple BPF objects used
> > internally). But processing single vmlinux BTF at a time seems
> > absolutely fine. I don't buy that CO-RE relo processing is that slow
> > to require optimized batch processing.
> >
> > I might have asked this before, sorry, but the duration between each
> > iteration of btfgen is pretty long and I'm losing the context.
> >
> > > +
> > > +               snprintf(src_btf_path, sizeof(src_btf_path), "%s%s", input, dir->d_name);
> > > +               snprintf(dst_btf_path, sizeof(dst_btf_path), "%s%s", output, dir->d_name);
> > > +
> > > +               p_info("Processing source BTF file: %s", src_btf_path);
> > > +
> > > +               err = btfgen(src_btf_path, dst_btf_path, objs);
> > > +               if (err)
> > > +                       goto out;
> > > +       }
> > > +
> > > +out:
> > > +       free(objs);
> > > +       if (d)
> > > +               closedir(d);
> > > +       return err;
> > > +}
> > > +
> > >  static const struct cmd cmds[] = {
> > > -       { "object",     do_object },
> > > -       { "skeleton",   do_skeleton },
> > > -       { "help",       do_help },
> > > +       { "object",             do_object },
> > > +       { "skeleton",           do_skeleton },
> > > +       { "min_core_btf",       do_min_core_btf},
> > > +       { "help",               do_help },
> > >         { 0 }
> > >  };
> > >
> > > --
> > > 2.25.1
> > >

[PATCH bpf-next v5 3/9] bpftool: Implement btf_save_raw()

[Mauricio Vásquez]

Helper function to save a BTF object to a file.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/gen.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 7db31b0f265f..64371f466fa6 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1096,6 +1096,28 @@ static int do_help(int argc, char **argv)
 	return 0;
 }
 
+static int btf_save_raw(const struct btf *btf, const char *path)
+{
+	const void *data;
+	FILE *f = NULL;
+	__u32 data_sz;
+	int err = 0;
+
+	data = btf__raw_data(btf, &data_sz);
+	if (!data)
+		return -ENOMEM;
+
+	f = fopen(path, "wb");
+	if (!f)
+		return -errno;
+
+	if (fwrite(data, 1, data_sz, f) != data_sz)
+		err = -errno;
+
+	fclose(f);
+	return err;
+}
+
 /* Create BTF file for a set of BPF objects */
 static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
 {
-- 
2.25.1

Re: [PATCH bpf-next v5 3/9] bpftool: Implement btf_save_raw()

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> Helper function to save a BTF object to a file.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---

The logic looks good, but you need to merge adding this static
function with the patch that's using that static function. Otherwise
you will break bisectability because compiler will warn about unused
static function.

>  tools/bpf/bpftool/gen.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 7db31b0f265f..64371f466fa6 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1096,6 +1096,28 @@ static int do_help(int argc, char **argv)
>         return 0;
>  }
>
> +static int btf_save_raw(const struct btf *btf, const char *path)
> +{
> +       const void *data;
> +       FILE *f = NULL;
> +       __u32 data_sz;
> +       int err = 0;
> +
> +       data = btf__raw_data(btf, &data_sz);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       f = fopen(path, "wb");
> +       if (!f)
> +               return -errno;
> +
> +       if (fwrite(data, 1, data_sz, f) != data_sz)
> +               err = -errno;
> +
> +       fclose(f);
> +       return err;
> +}
> +
>  /* Create BTF file for a set of BPF objects */
>  static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
>  {
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 3/9] bpftool: Implement btf_save_raw()

[Mauricio Vásquez Bernal]

> The logic looks good, but you need to merge adding this static
> function with the patch that's using that static function. Otherwise
> you will break bisectability because compiler will warn about unused
> static function.
>

It only emits a warning but it compiles fine. Is that still an issue?

Re: [PATCH bpf-next v5 3/9] bpftool: Implement btf_save_raw()

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 8:07 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> > The logic looks good, but you need to merge adding this static
> > function with the patch that's using that static function. Otherwise
> > you will break bisectability because compiler will warn about unused
> > static function.
> >
>
> It only emits a warning but it compiles fine. Is that still an issue?

Yes, it breaks selftests build (warnings are treated as errors for
bpftool, it seems).

[PATCH bpf-next v5 4/9] bpftool: Add struct definitions and helpers for BTFGen

[Mauricio Vásquez]

Add some structs and helpers that will be used by BTFGen in the next
commits.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/gen.c | 75 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 64371f466fa6..68bb88e86b27 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1118,6 +1118,81 @@ static int btf_save_raw(const struct btf *btf, const char *path)
 	return err;
 }
 
+struct btfgen_type {
+	struct btf_type *type;
+	unsigned int id;
+};
+
+struct btfgen_info {
+	struct hashmap *types;
+	struct btf *src_btf;
+};
+
+static size_t btfgen_hash_fn(const void *key, void *ctx)
+{
+	return (size_t)key;
+}
+
+static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
+{
+	return k1 == k2;
+}
+
+static void *uint_as_hash_key(int x)
+{
+	return (void *)(uintptr_t)x;
+}
+
+static void btfgen_free_type(struct btfgen_type *type)
+{
+	free(type);
+}
+
+static void btfgen_free_info(struct btfgen_info *info)
+{
+	struct hashmap_entry *entry;
+	size_t bkt;
+
+	if (!info)
+		return;
+
+	if (!IS_ERR_OR_NULL(info->types)) {
+		hashmap__for_each_entry(info->types, entry, bkt) {
+			btfgen_free_type(entry->value);
+		}
+		hashmap__free(info->types);
+	}
+
+	btf__free(info->src_btf);
+
+	free(info);
+}
+
+static struct btfgen_info *
+btfgen_new_info(const char *targ_btf_path)
+{
+	struct btfgen_info *info;
+
+	info = calloc(1, sizeof(*info));
+	if (!info)
+		return NULL;
+
+	info->src_btf = btf__parse(targ_btf_path, NULL);
+	if (libbpf_get_error(info->src_btf)) {
+		btfgen_free_info(info);
+		return NULL;
+	}
+
+	info->types = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
+	if (IS_ERR(info->types)) {
+		errno = -PTR_ERR(info->types);
+		btfgen_free_info(info);
+		return NULL;
+	}
+
+	return info;
+}
+
 /* Create BTF file for a set of BPF objects */
 static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
 {
-- 
2.25.1

Re: [PATCH bpf-next v5 4/9] bpftool: Add struct definitions and helpers for BTFGen

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> Add some structs and helpers that will be used by BTFGen in the next
> commits.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---

Similar considerations with unused static functions. It's also harder
to review when I don't see how these types are actually used, so
probably better to put it in relevant patches that are using this?

>  tools/bpf/bpftool/gen.c | 75 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 64371f466fa6..68bb88e86b27 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1118,6 +1118,81 @@ static int btf_save_raw(const struct btf *btf, const char *path)
>         return err;
>  }
>
> +struct btfgen_type {
> +       struct btf_type *type;
> +       unsigned int id;
> +};
> +
> +struct btfgen_info {
> +       struct hashmap *types;
> +       struct btf *src_btf;
> +};
> +
> +static size_t btfgen_hash_fn(const void *key, void *ctx)
> +{
> +       return (size_t)key;
> +}
> +
> +static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
> +{
> +       return k1 == k2;
> +}
> +
> +static void *uint_as_hash_key(int x)
> +{
> +       return (void *)(uintptr_t)x;
> +}
> +
> +static void btfgen_free_type(struct btfgen_type *type)
> +{
> +       free(type);
> +}
> +
> +static void btfgen_free_info(struct btfgen_info *info)
> +{
> +       struct hashmap_entry *entry;
> +       size_t bkt;
> +
> +       if (!info)
> +               return;
> +
> +       if (!IS_ERR_OR_NULL(info->types)) {
> +               hashmap__for_each_entry(info->types, entry, bkt) {
> +                       btfgen_free_type(entry->value);
> +               }
> +               hashmap__free(info->types);
> +       }
> +
> +       btf__free(info->src_btf);
> +
> +       free(info);
> +}
> +
> +static struct btfgen_info *
> +btfgen_new_info(const char *targ_btf_path)
> +{
> +       struct btfgen_info *info;
> +
> +       info = calloc(1, sizeof(*info));
> +       if (!info)
> +               return NULL;
> +
> +       info->src_btf = btf__parse(targ_btf_path, NULL);
> +       if (libbpf_get_error(info->src_btf)) {

bpftool is using libbpf 1.0 mode, so don't use libbpf_get_error()
anymore, just check for NULL

also, if you are using errno for propagating error, you need to store
it locally before btfgen_free_info() call, otherwise it can be
clobbered

> +               btfgen_free_info(info);
> +               return NULL;
> +       }
> +
> +       info->types = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> +       if (IS_ERR(info->types)) {
> +               errno = -PTR_ERR(info->types);
> +               btfgen_free_info(info);
> +               return NULL;
> +       }
> +
> +       return info;
> +}
> +
>  /* Create BTF file for a set of BPF objects */
>  static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
>  {
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 4/9] bpftool: Add struct definitions and helpers for BTFGen

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 1:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > Add some structs and helpers that will be used by BTFGen in the next
> > commits.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
>
> Similar considerations with unused static functions. It's also harder
> to review when I don't see how these types are actually used, so
> probably better to put it in relevant patches that are using this?
>

The next iteration splits the patches in a way that types are
introduced in the same commit they're used.

> >  tools/bpf/bpftool/gen.c | 75 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 64371f466fa6..68bb88e86b27 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1118,6 +1118,81 @@ static int btf_save_raw(const struct btf *btf, const char *path)
> >         return err;
> >  }
> >
> > +struct btfgen_type {
> > +       struct btf_type *type;
> > +       unsigned int id;
> > +};
> > +
> > +struct btfgen_info {
> > +       struct hashmap *types;
> > +       struct btf *src_btf;
> > +};
> > +
> > +static size_t btfgen_hash_fn(const void *key, void *ctx)
> > +{
> > +       return (size_t)key;
> > +}
> > +
> > +static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
> > +{
> > +       return k1 == k2;
> > +}
> > +
> > +static void *uint_as_hash_key(int x)
> > +{
> > +       return (void *)(uintptr_t)x;
> > +}
> > +
> > +static void btfgen_free_type(struct btfgen_type *type)
> > +{
> > +       free(type);
> > +}
> > +
> > +static void btfgen_free_info(struct btfgen_info *info)
> > +{
> > +       struct hashmap_entry *entry;
> > +       size_t bkt;
> > +
> > +       if (!info)
> > +               return;
> > +
> > +       if (!IS_ERR_OR_NULL(info->types)) {
> > +               hashmap__for_each_entry(info->types, entry, bkt) {
> > +                       btfgen_free_type(entry->value);
> > +               }
> > +               hashmap__free(info->types);
> > +       }
> > +
> > +       btf__free(info->src_btf);
> > +
> > +       free(info);
> > +}
> > +
> > +static struct btfgen_info *
> > +btfgen_new_info(const char *targ_btf_path)
> > +{
> > +       struct btfgen_info *info;
> > +
> > +       info = calloc(1, sizeof(*info));
> > +       if (!info)
> > +               return NULL;
> > +
> > +       info->src_btf = btf__parse(targ_btf_path, NULL);
> > +       if (libbpf_get_error(info->src_btf)) {
>
> bpftool is using libbpf 1.0 mode, so don't use libbpf_get_error()
> anymore, just check for NULL
>

hmm, I got confused because libbpf_get_error() is still used in many
places in bpftool. I suppose those need to be updated.

> also, if you are using errno for propagating error, you need to store
> it locally before btfgen_free_info() call, otherwise it can be
> clobbered
>

Fixed.



> > +               btfgen_free_info(info);
> > +               return NULL;
> > +       }
> > +
> > +       info->types = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > +       if (IS_ERR(info->types)) {
> > +               errno = -PTR_ERR(info->types);
> > +               btfgen_free_info(info);
> > +               return NULL;
> > +       }
> > +
> > +       return info;
> > +}
> > +
> >  /* Create BTF file for a set of BPF objects */
> >  static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> >  {
> > --
> > 2.25.1
> >

Re: [PATCH bpf-next v5 4/9] bpftool: Add struct definitions and helpers for BTFGen

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 8:08 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 1:55 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > Add some structs and helpers that will be used by BTFGen in the next
> > > commits.
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > > ---
> >
> > Similar considerations with unused static functions. It's also harder
> > to review when I don't see how these types are actually used, so
> > probably better to put it in relevant patches that are using this?
> >
>
> The next iteration splits the patches in a way that types are
> introduced in the same commit they're used.
>
> > >  tools/bpf/bpftool/gen.c | 75 +++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 75 insertions(+)
> > >
> > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > > index 64371f466fa6..68bb88e86b27 100644
> > > --- a/tools/bpf/bpftool/gen.c
> > > +++ b/tools/bpf/bpftool/gen.c
> > > @@ -1118,6 +1118,81 @@ static int btf_save_raw(const struct btf *btf, const char *path)
> > >         return err;
> > >  }
> > >
> > > +struct btfgen_type {
> > > +       struct btf_type *type;
> > > +       unsigned int id;
> > > +};
> > > +
> > > +struct btfgen_info {
> > > +       struct hashmap *types;
> > > +       struct btf *src_btf;
> > > +};
> > > +
> > > +static size_t btfgen_hash_fn(const void *key, void *ctx)
> > > +{
> > > +       return (size_t)key;
> > > +}
> > > +
> > > +static bool btfgen_equal_fn(const void *k1, const void *k2, void *ctx)
> > > +{
> > > +       return k1 == k2;
> > > +}
> > > +
> > > +static void *uint_as_hash_key(int x)
> > > +{
> > > +       return (void *)(uintptr_t)x;
> > > +}
> > > +
> > > +static void btfgen_free_type(struct btfgen_type *type)
> > > +{
> > > +       free(type);
> > > +}
> > > +
> > > +static void btfgen_free_info(struct btfgen_info *info)
> > > +{
> > > +       struct hashmap_entry *entry;
> > > +       size_t bkt;
> > > +
> > > +       if (!info)
> > > +               return;
> > > +
> > > +       if (!IS_ERR_OR_NULL(info->types)) {
> > > +               hashmap__for_each_entry(info->types, entry, bkt) {
> > > +                       btfgen_free_type(entry->value);
> > > +               }
> > > +               hashmap__free(info->types);
> > > +       }
> > > +
> > > +       btf__free(info->src_btf);
> > > +
> > > +       free(info);
> > > +}
> > > +
> > > +static struct btfgen_info *
> > > +btfgen_new_info(const char *targ_btf_path)
> > > +{
> > > +       struct btfgen_info *info;
> > > +
> > > +       info = calloc(1, sizeof(*info));
> > > +       if (!info)
> > > +               return NULL;
> > > +
> > > +       info->src_btf = btf__parse(targ_btf_path, NULL);
> > > +       if (libbpf_get_error(info->src_btf)) {
> >
> > bpftool is using libbpf 1.0 mode, so don't use libbpf_get_error()
> > anymore, just check for NULL
> >
>
> hmm, I got confused because libbpf_get_error() is still used in many
> places in bpftool. I suppose those need to be updated.

It's ok to use, but it's not necessary. Eventually I'd like to
deprecate libbpf_get_error() is it won't be necessary. So for new code
let's not add new uses of libbpf_get_error(). We can phase out
existing uses gradually (just like we do with CHECK() in selftests).


>
> > also, if you are using errno for propagating error, you need to store
> > it locally before btfgen_free_info() call, otherwise it can be
> > clobbered
> >
>
> Fixed.
>
>
>
> > > +               btfgen_free_info(info);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       info->types = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > > +       if (IS_ERR(info->types)) {
> > > +               errno = -PTR_ERR(info->types);
> > > +               btfgen_free_info(info);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       return info;
> > > +}
> > > +
> > >  /* Create BTF file for a set of BPF objects */
> > >  static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > >  {
> > > --
> > > 2.25.1
> > >

[PATCH bpf-next v5 5/9] bpftool: Implement btfgen()

[Mauricio Vásquez]

btfgen() receives the path of a source and destination BTF files and a
list of BPF objects. This function records the relocations for all
objects and then generates the BTF file by calling btfgen_get_btf()
(implemented in the following commits).

btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
objects and loops through all CO-RE relocations. It uses
bpf_core_calc_relo_insn() from libbpf and passes the target spec to
btfgen_record_reloc() that saves the types involved in such relocation.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/Makefile |   8 +-
 tools/bpf/bpftool/gen.c    | 221 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 223 insertions(+), 6 deletions(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 83369f55df61..97d447135536 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -34,10 +34,10 @@ LIBBPF_BOOTSTRAP_INCLUDE := $(LIBBPF_BOOTSTRAP_DESTDIR)/include
 LIBBPF_BOOTSTRAP_HDRS_DIR := $(LIBBPF_BOOTSTRAP_INCLUDE)/bpf
 LIBBPF_BOOTSTRAP := $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
 
-# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
-# libbpf, but still required by bpftool.
-LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
-LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h)
+# We need to copy hashmap.h, nlattr.h, relo_core.h and libbpf_internal.h
+# which are not otherwise exported by libbpf, but still required by bpftool.
+LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h relo_core.h libbpf_internal.h)
+LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h relo_core.h libbpf_internal.h)
 
 ifeq ($(BPFTOOL_VERSION),)
 BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 68bb88e86b27..bb9c56401ee5 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -15,6 +15,7 @@
 #include <unistd.h>
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
+#include <bpf/libbpf_internal.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/mman.h>
@@ -1143,6 +1144,11 @@ static void *uint_as_hash_key(int x)
 	return (void *)(uintptr_t)x;
 }
 
+static void *u32_as_hash_key(__u32 x)
+{
+	return (void *)(uintptr_t)x;
+}
+
 static void btfgen_free_type(struct btfgen_type *type)
 {
 	free(type);
@@ -1193,12 +1199,223 @@ btfgen_new_info(const char *targ_btf_path)
 	return info;
 }
 
-/* Create BTF file for a set of BPF objects */
-static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
+static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
+{
+	return -EOPNOTSUPP;
+}
+
+static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
+{
+	return -EOPNOTSUPP;
+}
+
+static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
 {
 	return -EOPNOTSUPP;
 }
 
+static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
+{
+	switch (res->relo_kind) {
+	case BPF_CORE_FIELD_BYTE_OFFSET:
+	case BPF_CORE_FIELD_BYTE_SIZE:
+	case BPF_CORE_FIELD_EXISTS:
+	case BPF_CORE_FIELD_SIGNED:
+	case BPF_CORE_FIELD_LSHIFT_U64:
+	case BPF_CORE_FIELD_RSHIFT_U64:
+		return btfgen_record_field_relo(info, res);
+	case BPF_CORE_TYPE_ID_LOCAL:
+	case BPF_CORE_TYPE_ID_TARGET:
+	case BPF_CORE_TYPE_EXISTS:
+	case BPF_CORE_TYPE_SIZE:
+		return btfgen_record_type_relo(info, res);
+	case BPF_CORE_ENUMVAL_EXISTS:
+	case BPF_CORE_ENUMVAL_VALUE:
+		return btfgen_record_enumval_relo(info, res);
+	default:
+		return -EINVAL;
+	}
+}
+
+static struct bpf_core_cand_list *
+btfgen_find_cands(const struct btf *local_btf, const struct btf *targ_btf, __u32 local_id)
+{
+	const struct btf_type *local_type;
+	struct bpf_core_cand_list *cands = NULL;
+	struct bpf_core_cand local_cand = {};
+	size_t local_essent_len;
+	const char *local_name;
+	int err;
+
+	local_cand.btf = local_btf;
+	local_cand.id = local_id;
+
+	local_type = btf__type_by_id(local_btf, local_id);
+	if (!local_type) {
+		err = -EINVAL;
+		goto err_out;
+	}
+
+	local_name = btf__name_by_offset(local_btf, local_type->name_off);
+	if (!local_name) {
+		err = -EINVAL;
+		goto err_out;
+	}
+	local_essent_len = bpf_core_essential_name_len(local_name);
+
+	cands = calloc(1, sizeof(*cands));
+	if (!cands)
+		return NULL;
+
+	err = bpf_core_add_cands(&local_cand, local_essent_len, targ_btf, "vmlinux", 1, cands);
+	if (err)
+		goto err_out;
+
+	return cands;
+
+err_out:
+	if (cands)
+		bpf_core_free_cands(cands);
+	errno = -err;
+	return NULL;
+}
+
+/* Record relocation information for a single BPF object*/
+static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
+{
+	const struct btf_ext_info_sec *sec;
+	const struct bpf_core_relo *relo;
+	const struct btf_ext_info *seg;
+	struct hashmap *cand_cache;
+	struct btf_ext *btf_ext;
+	unsigned int relo_idx;
+	struct btf *btf;
+	int err;
+
+	btf = btf__parse(obj_path, &btf_ext);
+	err = libbpf_get_error(btf);
+	if (err) {
+		p_err("failed to parse bpf object '%s': %s", obj_path, strerror(errno));
+		return err;
+	}
+
+	if (btf_ext->core_relo_info.len == 0)
+		return 0;
+
+	cand_cache = bpf_core_create_cand_cache();
+	if (IS_ERR(cand_cache))
+		return PTR_ERR(cand_cache);
+
+	seg = &btf_ext->core_relo_info;
+	for_each_btf_ext_sec(seg, sec) {
+		for_each_btf_ext_rec(seg, sec, relo_idx, relo) {
+			struct bpf_core_spec specs_scratch[3] = {};
+			struct bpf_core_relo_res targ_res = {};
+			struct bpf_core_cand_list *cands = NULL;
+			const void *type_key = u32_as_hash_key(relo->type_id);
+			const char *sec_name = btf__name_by_offset(btf, sec->sec_name_off);
+
+			if (relo->kind != BPF_CORE_TYPE_ID_LOCAL &&
+			    !hashmap__find(cand_cache, type_key, (void **)&cands)) {
+				cands = btfgen_find_cands(btf, info->src_btf, relo->type_id);
+				if (!cands) {
+					err = -errno;
+					goto out;
+				}
+
+				err = hashmap__set(cand_cache, type_key, cands, NULL, NULL);
+				if (err)
+					goto out;
+			}
+
+			err = bpf_core_calc_relo_insn(sec_name, relo, relo_idx, btf, cands,
+						      specs_scratch, &targ_res);
+			if (err)
+				goto out;
+
+			err = btfgen_record_reloc(info, &specs_scratch[2]);
+			if (err)
+				goto out;
+		}
+	}
+
+out:
+	bpf_core_free_cand_cache(cand_cache);
+
+	return err;
+}
+
+/* Generate BTF from relocation information previously recorded */
+static struct btf *btfgen_get_btf(struct btfgen_info *info)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+/* Create BTF file for a set of BPF objects.
+ *
+ * The BTFGen algorithm is divided in two main parts: (1) collect the
+ * BTF types that are involved in relocations and (2) generate the BTF
+ * object using the collected types.
+ *
+ * In order to collect the types involved in the relocations, we parse
+ * the BTF and BTF.ext sections of the BPF objects and use
+ * bpf_core_calc_relo_insn() to get the target specification, this
+ * indicates how the types and fields are used in a relocation.
+ *
+ * Types are recorded in different ways according to the kind of the
+ * relocation. For field-based relocations only the members that are
+ * actually used are saved in order to reduce the size of the generated
+ * BTF file. For type-based and enum-based relocations the whole type is
+ * saved.
+ *
+ * The second part of the algorithm generates the BTF object. It creates
+ * an empty BTF object and fills it with the types recorded in the
+ * previous step. This function takes care of only adding the structure
+ * and union members that were marked as used and it also fixes up the
+ * type IDs on the generated BTF object.
+ */
+static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
+{
+	struct btfgen_info *info;
+	struct btf *btf_new = NULL;
+	int err;
+
+	info = btfgen_new_info(src_btf);
+	if (!info) {
+		p_err("failed to allocate info structure: %s", strerror(errno));
+		err = -errno;
+		goto out;
+	}
+
+	for (int i = 0; objspaths[i] != NULL; i++) {
+		p_info("Processing BPF object: %s", objspaths[i]);
+
+		err = btfgen_record_obj(info, objspaths[i]);
+		if (err)
+			goto out;
+	}
+
+	btf_new = btfgen_get_btf(info);
+	if (!btf_new) {
+		err = -errno;
+		p_err("error generating btf: %s", strerror(errno));
+		goto out;
+	}
+
+	p_info("Creating BTF file: %s", dst_btf);
+	err = btf_save_raw(btf_new, dst_btf);
+	if (err) {
+		p_err("error saving btf file: %s", strerror(errno));
+		goto out;
+	}
+
+out:
+	btf__free(btf_new);
+	btfgen_free_info(info);
+
+	return err;
+}
+
 static int do_min_core_btf(int argc, char **argv)
 {
 	char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
-- 
2.25.1

Re: [PATCH bpf-next v5 5/9] bpftool: Implement btfgen()

[Quentin Monnet]

2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> btfgen() receives the path of a source and destination BTF files and a
> list of BPF objects. This function records the relocations for all
> objects and then generates the BTF file by calling btfgen_get_btf()
> (implemented in the following commits).
> 
> btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
> objects and loops through all CO-RE relocations. It uses
> bpf_core_calc_relo_insn() from libbpf and passes the target spec to
> btfgen_record_reloc() that saves the types involved in such relocation.
> 
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/Makefile |   8 +-
>  tools/bpf/bpftool/gen.c    | 221 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 83369f55df61..97d447135536 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -34,10 +34,10 @@ LIBBPF_BOOTSTRAP_INCLUDE := $(LIBBPF_BOOTSTRAP_DESTDIR)/include
>  LIBBPF_BOOTSTRAP_HDRS_DIR := $(LIBBPF_BOOTSTRAP_INCLUDE)/bpf
>  LIBBPF_BOOTSTRAP := $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
>  
> -# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
> -# libbpf, but still required by bpftool.
> -LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
> -LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h)
> +# We need to copy hashmap.h, nlattr.h, relo_core.h and libbpf_internal.h
> +# which are not otherwise exported by libbpf, but still required by bpftool.
> +LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h relo_core.h libbpf_internal.h)
> +LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h relo_core.h libbpf_internal.h)

Do you directly call functions from relo_core.h, or is it only required
to compile libbpf_internal.h? (Asking because I'm wondering if there
would be a way to have one fewer header copied).

Re: [PATCH bpf-next v5 5/9] bpftool: Implement btfgen()

[Mauricio Vásquez Bernal]

On Tue, Feb 1, 2022 at 3:57 PM Quentin Monnet <quentin@isovalent.com> wrote:
>
> 2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> > btfgen() receives the path of a source and destination BTF files and a
> > list of BPF objects. This function records the relocations for all
> > objects and then generates the BTF file by calling btfgen_get_btf()
> > (implemented in the following commits).
> >
> > btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
> > objects and loops through all CO-RE relocations. It uses
> > bpf_core_calc_relo_insn() from libbpf and passes the target spec to
> > btfgen_record_reloc() that saves the types involved in such relocation.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/bpf/bpftool/Makefile |   8 +-
> >  tools/bpf/bpftool/gen.c    | 221 ++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 223 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index 83369f55df61..97d447135536 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -34,10 +34,10 @@ LIBBPF_BOOTSTRAP_INCLUDE := $(LIBBPF_BOOTSTRAP_DESTDIR)/include
> >  LIBBPF_BOOTSTRAP_HDRS_DIR := $(LIBBPF_BOOTSTRAP_INCLUDE)/bpf
> >  LIBBPF_BOOTSTRAP := $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
> >
> > -# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
> > -# libbpf, but still required by bpftool.
> > -LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
> > -LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h)
> > +# We need to copy hashmap.h, nlattr.h, relo_core.h and libbpf_internal.h
> > +# which are not otherwise exported by libbpf, but still required by bpftool.
> > +LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h relo_core.h libbpf_internal.h)
> > +LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h relo_core.h libbpf_internal.h)
>
> Do you directly call functions from relo_core.h, or is it only required
> to compile libbpf_internal.h? (Asking because I'm wondering if there
> would be a way to have one fewer header copied).

bpf_core_calc_relo_insn() and bpf_core_calc_relo_insn() are used.

Re: [PATCH bpf-next v5 5/9] bpftool: Implement btfgen()

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> btfgen() receives the path of a source and destination BTF files and a
> list of BPF objects. This function records the relocations for all
> objects and then generates the BTF file by calling btfgen_get_btf()
> (implemented in the following commits).
>
> btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
> objects and loops through all CO-RE relocations. It uses
> bpf_core_calc_relo_insn() from libbpf and passes the target spec to
> btfgen_record_reloc() that saves the types involved in such relocation.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/Makefile |   8 +-
>  tools/bpf/bpftool/gen.c    | 221 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 223 insertions(+), 6 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 83369f55df61..97d447135536 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -34,10 +34,10 @@ LIBBPF_BOOTSTRAP_INCLUDE := $(LIBBPF_BOOTSTRAP_DESTDIR)/include
>  LIBBPF_BOOTSTRAP_HDRS_DIR := $(LIBBPF_BOOTSTRAP_INCLUDE)/bpf
>  LIBBPF_BOOTSTRAP := $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
>
> -# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
> -# libbpf, but still required by bpftool.
> -LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
> -LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h)
> +# We need to copy hashmap.h, nlattr.h, relo_core.h and libbpf_internal.h
> +# which are not otherwise exported by libbpf, but still required by bpftool.
> +LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h relo_core.h libbpf_internal.h)
> +LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h relo_core.h libbpf_internal.h)
>
>  ifeq ($(BPFTOOL_VERSION),)
>  BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 68bb88e86b27..bb9c56401ee5 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -15,6 +15,7 @@
>  #include <unistd.h>
>  #include <bpf/bpf.h>
>  #include <bpf/libbpf.h>
> +#include <bpf/libbpf_internal.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/mman.h>
> @@ -1143,6 +1144,11 @@ static void *uint_as_hash_key(int x)
>         return (void *)(uintptr_t)x;
>  }
>
> +static void *u32_as_hash_key(__u32 x)
> +{
> +       return (void *)(uintptr_t)x;
> +}
> +
>  static void btfgen_free_type(struct btfgen_type *type)
>  {
>         free(type);
> @@ -1193,12 +1199,223 @@ btfgen_new_info(const char *targ_btf_path)
>         return info;
>  }
>
> -/* Create BTF file for a set of BPF objects */
> -static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> +static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
>  {
>         return -EOPNOTSUPP;
>  }
>
> +static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
> +{
> +       switch (res->relo_kind) {
> +       case BPF_CORE_FIELD_BYTE_OFFSET:
> +       case BPF_CORE_FIELD_BYTE_SIZE:
> +       case BPF_CORE_FIELD_EXISTS:
> +       case BPF_CORE_FIELD_SIGNED:
> +       case BPF_CORE_FIELD_LSHIFT_U64:
> +       case BPF_CORE_FIELD_RSHIFT_U64:
> +               return btfgen_record_field_relo(info, res);
> +       case BPF_CORE_TYPE_ID_LOCAL:
> +       case BPF_CORE_TYPE_ID_TARGET:
> +       case BPF_CORE_TYPE_EXISTS:
> +       case BPF_CORE_TYPE_SIZE:
> +               return btfgen_record_type_relo(info, res);
> +       case BPF_CORE_ENUMVAL_EXISTS:
> +       case BPF_CORE_ENUMVAL_VALUE:
> +               return btfgen_record_enumval_relo(info, res);
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static struct bpf_core_cand_list *
> +btfgen_find_cands(const struct btf *local_btf, const struct btf *targ_btf, __u32 local_id)
> +{
> +       const struct btf_type *local_type;
> +       struct bpf_core_cand_list *cands = NULL;
> +       struct bpf_core_cand local_cand = {};
> +       size_t local_essent_len;
> +       const char *local_name;
> +       int err;
> +
> +       local_cand.btf = local_btf;
> +       local_cand.id = local_id;
> +
> +       local_type = btf__type_by_id(local_btf, local_id);
> +       if (!local_type) {
> +               err = -EINVAL;
> +               goto err_out;
> +       }
> +
> +       local_name = btf__name_by_offset(local_btf, local_type->name_off);
> +       if (!local_name) {
> +               err = -EINVAL;
> +               goto err_out;
> +       }
> +       local_essent_len = bpf_core_essential_name_len(local_name);
> +
> +       cands = calloc(1, sizeof(*cands));
> +       if (!cands)
> +               return NULL;
> +
> +       err = bpf_core_add_cands(&local_cand, local_essent_len, targ_btf, "vmlinux", 1, cands);
> +       if (err)
> +               goto err_out;
> +
> +       return cands;
> +
> +err_out:
> +       if (cands)
> +               bpf_core_free_cands(cands);

we can also add if (!cands) return into bpf_core_free_cands(), like
all other public destructor APIs in libbpf do

> +       errno = -err;
> +       return NULL;
> +}
> +
> +/* Record relocation information for a single BPF object*/
> +static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
> +{
> +       const struct btf_ext_info_sec *sec;
> +       const struct bpf_core_relo *relo;
> +       const struct btf_ext_info *seg;
> +       struct hashmap *cand_cache;
> +       struct btf_ext *btf_ext;
> +       unsigned int relo_idx;
> +       struct btf *btf;
> +       int err;
> +
> +       btf = btf__parse(obj_path, &btf_ext);
> +       err = libbpf_get_error(btf);

same, libbpf 1.0 mode, no need for libbpf_get_error()

> +       if (err) {
> +               p_err("failed to parse bpf object '%s': %s", obj_path, strerror(errno));

nit: "BPF object"? I'm not sure we do this consistently, but BPF
should be spelled with capitals in logs



> +               return err;
> +       }
> +
> +       if (btf_ext->core_relo_info.len == 0)
> +               return 0;
> +
> +       cand_cache = bpf_core_create_cand_cache();
> +       if (IS_ERR(cand_cache))
> +               return PTR_ERR(cand_cache);
> +
> +       seg = &btf_ext->core_relo_info;
> +       for_each_btf_ext_sec(seg, sec) {
> +               for_each_btf_ext_rec(seg, sec, relo_idx, relo) {
> +                       struct bpf_core_spec specs_scratch[3] = {};
> +                       struct bpf_core_relo_res targ_res = {};
> +                       struct bpf_core_cand_list *cands = NULL;
> +                       const void *type_key = u32_as_hash_key(relo->type_id);
> +                       const char *sec_name = btf__name_by_offset(btf, sec->sec_name_off);
> +
> +                       if (relo->kind != BPF_CORE_TYPE_ID_LOCAL &&
> +                           !hashmap__find(cand_cache, type_key, (void **)&cands)) {
> +                               cands = btfgen_find_cands(btf, info->src_btf, relo->type_id);
> +                               if (!cands) {
> +                                       err = -errno;
> +                                       goto out;
> +                               }
> +
> +                               err = hashmap__set(cand_cache, type_key, cands, NULL, NULL);
> +                               if (err)
> +                                       goto out;
> +                       }
> +
> +                       err = bpf_core_calc_relo_insn(sec_name, relo, relo_idx, btf, cands,
> +                                                     specs_scratch, &targ_res);

it feels like you forgot to submit some important patch, as I said, I
can't find bpf_core_calc_relo_insn() anywhere


> +                       if (err)
> +                               goto out;
> +
> +                       err = btfgen_record_reloc(info, &specs_scratch[2]);

at least let's leave a comment that specs_scratch[2] is target spec
(but that's an implementation detail, ugh...)



> +                       if (err)
> +                               goto out;
> +               }
> +       }
> +
> +out:
> +       bpf_core_free_cand_cache(cand_cache);
> +
> +       return err;
> +}
> +
> +/* Generate BTF from relocation information previously recorded */
> +static struct btf *btfgen_get_btf(struct btfgen_info *info)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +/* Create BTF file for a set of BPF objects.
> + *
> + * The BTFGen algorithm is divided in two main parts: (1) collect the
> + * BTF types that are involved in relocations and (2) generate the BTF
> + * object using the collected types.
> + *
> + * In order to collect the types involved in the relocations, we parse
> + * the BTF and BTF.ext sections of the BPF objects and use
> + * bpf_core_calc_relo_insn() to get the target specification, this
> + * indicates how the types and fields are used in a relocation.
> + *
> + * Types are recorded in different ways according to the kind of the
> + * relocation. For field-based relocations only the members that are
> + * actually used are saved in order to reduce the size of the generated
> + * BTF file. For type-based and enum-based relocations the whole type is
> + * saved.
> + *
> + * The second part of the algorithm generates the BTF object. It creates
> + * an empty BTF object and fills it with the types recorded in the
> + * previous step. This function takes care of only adding the structure
> + * and union members that were marked as used and it also fixes up the
> + * type IDs on the generated BTF object.
> + */
> +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])

naming nit: btfgen is actually misleading. pahole is "btfgen", but
this is actually some sort of "BTF minimizer". So something like
"minimize_btf" would be a bit more descriptive

> +{
> +       struct btfgen_info *info;
> +       struct btf *btf_new = NULL;
> +       int err;
> +
> +       info = btfgen_new_info(src_btf);
> +       if (!info) {
> +               p_err("failed to allocate info structure: %s", strerror(errno));
> +               err = -errno;
> +               goto out;
> +       }
> +
> +       for (int i = 0; objspaths[i] != NULL; i++) {
> +               p_info("Processing BPF object: %s", objspaths[i]);
> +
> +               err = btfgen_record_obj(info, objspaths[i]);
> +               if (err)
> +                       goto out;
> +       }
> +
> +       btf_new = btfgen_get_btf(info);
> +       if (!btf_new) {
> +               err = -errno;
> +               p_err("error generating btf: %s", strerror(errno));
> +               goto out;
> +       }
> +
> +       p_info("Creating BTF file: %s", dst_btf);

normally tools don't advertise each action through logs, unless it's
some verbose mode. Let's dop one BTF generated per one bpftool
invocation and drop all this descriptive logging. User will know input
and output BTFs exactly, because they will specify it as input
arguments (so no need to parse any output just to know what went
where, etc).

> +       err = btf_save_raw(btf_new, dst_btf);
> +       if (err) {
> +               p_err("error saving btf file: %s", strerror(errno));
> +               goto out;
> +       }
> +
> +out:
> +       btf__free(btf_new);
> +       btfgen_free_info(info);
> +
> +       return err;
> +}
> +
>  static int do_min_core_btf(int argc, char **argv)
>  {
>         char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 5/9] bpftool: Implement btfgen()

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 2:15 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > btfgen() receives the path of a source and destination BTF files and a
> > list of BPF objects. This function records the relocations for all
> > objects and then generates the BTF file by calling btfgen_get_btf()
> > (implemented in the following commits).
> >
> > btfgen_record_obj() loads the BTF and BTF.ext sections of the BPF
> > objects and loops through all CO-RE relocations. It uses
> > bpf_core_calc_relo_insn() from libbpf and passes the target spec to
> > btfgen_record_reloc() that saves the types involved in such relocation.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/bpf/bpftool/Makefile |   8 +-
> >  tools/bpf/bpftool/gen.c    | 221 ++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 223 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index 83369f55df61..97d447135536 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -34,10 +34,10 @@ LIBBPF_BOOTSTRAP_INCLUDE := $(LIBBPF_BOOTSTRAP_DESTDIR)/include
> >  LIBBPF_BOOTSTRAP_HDRS_DIR := $(LIBBPF_BOOTSTRAP_INCLUDE)/bpf
> >  LIBBPF_BOOTSTRAP := $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
> >
> > -# We need to copy hashmap.h and nlattr.h which is not otherwise exported by
> > -# libbpf, but still required by bpftool.
> > -LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h)
> > -LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h)
> > +# We need to copy hashmap.h, nlattr.h, relo_core.h and libbpf_internal.h
> > +# which are not otherwise exported by libbpf, but still required by bpftool.
> > +LIBBPF_INTERNAL_HDRS := $(addprefix $(LIBBPF_HDRS_DIR)/,hashmap.h nlattr.h relo_core.h libbpf_internal.h)
> > +LIBBPF_BOOTSTRAP_INTERNAL_HDRS := $(addprefix $(LIBBPF_BOOTSTRAP_HDRS_DIR)/,hashmap.h relo_core.h libbpf_internal.h)
> >
> >  ifeq ($(BPFTOOL_VERSION),)
> >  BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 68bb88e86b27..bb9c56401ee5 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -15,6 +15,7 @@
> >  #include <unistd.h>
> >  #include <bpf/bpf.h>
> >  #include <bpf/libbpf.h>
> > +#include <bpf/libbpf_internal.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <sys/mman.h>
> > @@ -1143,6 +1144,11 @@ static void *uint_as_hash_key(int x)
> >         return (void *)(uintptr_t)x;
> >  }
> >
> > +static void *u32_as_hash_key(__u32 x)
> > +{
> > +       return (void *)(uintptr_t)x;
> > +}
> > +
> >  static void btfgen_free_type(struct btfgen_type *type)
> >  {
> >         free(type);
> > @@ -1193,12 +1199,223 @@ btfgen_new_info(const char *targ_btf_path)
> >         return info;
> >  }
> >
> > -/* Create BTF file for a set of BPF objects */
> > -static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
> > +static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> >  {
> >         return -EOPNOTSUPP;
> >  }
> >
> > +static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
> > +{
> > +       switch (res->relo_kind) {
> > +       case BPF_CORE_FIELD_BYTE_OFFSET:
> > +       case BPF_CORE_FIELD_BYTE_SIZE:
> > +       case BPF_CORE_FIELD_EXISTS:
> > +       case BPF_CORE_FIELD_SIGNED:
> > +       case BPF_CORE_FIELD_LSHIFT_U64:
> > +       case BPF_CORE_FIELD_RSHIFT_U64:
> > +               return btfgen_record_field_relo(info, res);
> > +       case BPF_CORE_TYPE_ID_LOCAL:
> > +       case BPF_CORE_TYPE_ID_TARGET:
> > +       case BPF_CORE_TYPE_EXISTS:
> > +       case BPF_CORE_TYPE_SIZE:
> > +               return btfgen_record_type_relo(info, res);
> > +       case BPF_CORE_ENUMVAL_EXISTS:
> > +       case BPF_CORE_ENUMVAL_VALUE:
> > +               return btfgen_record_enumval_relo(info, res);
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> > +static struct bpf_core_cand_list *
> > +btfgen_find_cands(const struct btf *local_btf, const struct btf *targ_btf, __u32 local_id)
> > +{
> > +       const struct btf_type *local_type;
> > +       struct bpf_core_cand_list *cands = NULL;
> > +       struct bpf_core_cand local_cand = {};
> > +       size_t local_essent_len;
> > +       const char *local_name;
> > +       int err;
> > +
> > +       local_cand.btf = local_btf;
> > +       local_cand.id = local_id;
> > +
> > +       local_type = btf__type_by_id(local_btf, local_id);
> > +       if (!local_type) {
> > +               err = -EINVAL;
> > +               goto err_out;
> > +       }
> > +
> > +       local_name = btf__name_by_offset(local_btf, local_type->name_off);
> > +       if (!local_name) {
> > +               err = -EINVAL;
> > +               goto err_out;
> > +       }
> > +       local_essent_len = bpf_core_essential_name_len(local_name);
> > +
> > +       cands = calloc(1, sizeof(*cands));
> > +       if (!cands)
> > +               return NULL;
> > +
> > +       err = bpf_core_add_cands(&local_cand, local_essent_len, targ_btf, "vmlinux", 1, cands);
> > +       if (err)
> > +               goto err_out;
> > +
> > +       return cands;
> > +
> > +err_out:
> > +       if (cands)
> > +               bpf_core_free_cands(cands);
>
> we can also add if (!cands) return into bpf_core_free_cands(), like
> all other public destructor APIs in libbpf do
>

Makes sense.

> > +       errno = -err;
> > +       return NULL;
> > +}
> > +
> > +/* Record relocation information for a single BPF object*/
> > +static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
> > +{
> > +       const struct btf_ext_info_sec *sec;
> > +       const struct bpf_core_relo *relo;
> > +       const struct btf_ext_info *seg;
> > +       struct hashmap *cand_cache;
> > +       struct btf_ext *btf_ext;
> > +       unsigned int relo_idx;
> > +       struct btf *btf;
> > +       int err;
> > +
> > +       btf = btf__parse(obj_path, &btf_ext);
> > +       err = libbpf_get_error(btf);
>
> same, libbpf 1.0 mode, no need for libbpf_get_error()
>
> > +       if (err) {
> > +               p_err("failed to parse bpf object '%s': %s", obj_path, strerror(errno));
>
> nit: "BPF object"? I'm not sure we do this consistently, but BPF
> should be spelled with capitals in logs
>
>
>
> > +               return err;
> > +       }
> > +
> > +       if (btf_ext->core_relo_info.len == 0)
> > +               return 0;
> > +
> > +       cand_cache = bpf_core_create_cand_cache();
> > +       if (IS_ERR(cand_cache))
> > +               return PTR_ERR(cand_cache);
> > +
> > +       seg = &btf_ext->core_relo_info;
> > +       for_each_btf_ext_sec(seg, sec) {
> > +               for_each_btf_ext_rec(seg, sec, relo_idx, relo) {
> > +                       struct bpf_core_spec specs_scratch[3] = {};
> > +                       struct bpf_core_relo_res targ_res = {};
> > +                       struct bpf_core_cand_list *cands = NULL;
> > +                       const void *type_key = u32_as_hash_key(relo->type_id);
> > +                       const char *sec_name = btf__name_by_offset(btf, sec->sec_name_off);
> > +
> > +                       if (relo->kind != BPF_CORE_TYPE_ID_LOCAL &&
> > +                           !hashmap__find(cand_cache, type_key, (void **)&cands)) {
> > +                               cands = btfgen_find_cands(btf, info->src_btf, relo->type_id);
> > +                               if (!cands) {
> > +                                       err = -errno;
> > +                                       goto out;
> > +                               }
> > +
> > +                               err = hashmap__set(cand_cache, type_key, cands, NULL, NULL);
> > +                               if (err)
> > +                                       goto out;
> > +                       }
> > +
> > +                       err = bpf_core_calc_relo_insn(sec_name, relo, relo_idx, btf, cands,
> > +                                                     specs_scratch, &targ_res);
>
> it feels like you forgot to submit some important patch, as I said, I
> can't find bpf_core_calc_relo_insn() anywhere
>

Yes, sorry about that. Will be in the next iteration.

>
> > +                       if (err)
> > +                               goto out;
> > +
> > +                       err = btfgen_record_reloc(info, &specs_scratch[2]);
>
> at least let's leave a comment that specs_scratch[2] is target spec
> (but that's an implementation detail, ugh...)
>

Right.

>
>
> > +                       if (err)
> > +                               goto out;
> > +               }
> > +       }
> > +
> > +out:
> > +       bpf_core_free_cand_cache(cand_cache);
> > +
> > +       return err;
> > +}
> > +
> > +/* Generate BTF from relocation information previously recorded */
> > +static struct btf *btfgen_get_btf(struct btfgen_info *info)
> > +{
> > +       return ERR_PTR(-EOPNOTSUPP);
> > +}
> > +
> > +/* Create BTF file for a set of BPF objects.
> > + *
> > + * The BTFGen algorithm is divided in two main parts: (1) collect the
> > + * BTF types that are involved in relocations and (2) generate the BTF
> > + * object using the collected types.
> > + *
> > + * In order to collect the types involved in the relocations, we parse
> > + * the BTF and BTF.ext sections of the BPF objects and use
> > + * bpf_core_calc_relo_insn() to get the target specification, this
> > + * indicates how the types and fields are used in a relocation.
> > + *
> > + * Types are recorded in different ways according to the kind of the
> > + * relocation. For field-based relocations only the members that are
> > + * actually used are saved in order to reduce the size of the generated
> > + * BTF file. For type-based and enum-based relocations the whole type is
> > + * saved.
> > + *
> > + * The second part of the algorithm generates the BTF object. It creates
> > + * an empty BTF object and fills it with the types recorded in the
> > + * previous step. This function takes care of only adding the structure
> > + * and union members that were marked as used and it also fixes up the
> > + * type IDs on the generated BTF object.
> > + */
> > +static int btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
>
> naming nit: btfgen is actually misleading. pahole is "btfgen", but
> this is actually some sort of "BTF minimizer". So something like
> "minimize_btf" would be a bit more descriptive
>

I agree that minimize_btf() is better. I think we can continue to use
btfgen_* for the different types.


> > +{
> > +       struct btfgen_info *info;
> > +       struct btf *btf_new = NULL;
> > +       int err;
> > +
> > +       info = btfgen_new_info(src_btf);
> > +       if (!info) {
> > +               p_err("failed to allocate info structure: %s", strerror(errno));
> > +               err = -errno;
> > +               goto out;
> > +       }
> > +
> > +       for (int i = 0; objspaths[i] != NULL; i++) {
> > +               p_info("Processing BPF object: %s", objspaths[i]);
> > +
> > +               err = btfgen_record_obj(info, objspaths[i]);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +       btf_new = btfgen_get_btf(info);
> > +       if (!btf_new) {
> > +               err = -errno;
> > +               p_err("error generating btf: %s", strerror(errno));
> > +               goto out;
> > +       }
> > +
> > +       p_info("Creating BTF file: %s", dst_btf);
>
> normally tools don't advertise each action through logs, unless it's
> some verbose mode. Let's dop one BTF generated per one bpftool
> invocation and drop all this descriptive logging. User will know input
> and output BTFs exactly, because they will specify it as input
> arguments (so no need to parse any output just to know what went
> where, etc).
>

> > +       err = btf_save_raw(btf_new, dst_btf);
> > +       if (err) {
> > +               p_err("error saving btf file: %s", strerror(errno));
> > +               goto out;
> > +       }
> > +
> > +out:
> > +       btf__free(btf_new);
> > +       btfgen_free_info(info);
> > +
> > +       return err;
> > +}
> > +
> >  static int do_min_core_btf(int argc, char **argv)
> >  {
> >         char src_btf_path[PATH_MAX], dst_btf_path[PATH_MAX];
> > --
> > 2.25.1
> >

[PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Mauricio Vásquez]

This commit implements the logic to record the relocation information
for the different kind of relocations.

btfgen_record_field_relo() uses the target specification to save all the
types that are involved in a field-based CO-RE relocation. In this case
types resolved and added recursively (using btfgen_put_type()).
Only the struct and union members and their types) involved in the
relocation are added to optimize the size of the generated BTF file.

On the other hand, btfgen_record_type_relo() saves the types involved in
a type-based CO-RE relocation. In this case all the members for the
struct and union types are added. This is not strictly required since
libbpf doesn't use them while performing this kind of relocation,
however that logic could change on the future. Additionally, we expect
that the number of this kind of relocations in an BPF object to be very
low, hence the impact on the size of the generated BTF should be
negligible.

Finally, btfgen_record_enumval_relo() saves the whole enum type for
enum-based relocations.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 257 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index bb9c56401ee5..7413ec808a80 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1119,9 +1119,17 @@ static int btf_save_raw(const struct btf *btf, const char *path)
 	return err;
 }
 
+struct btfgen_member {
+	struct btf_member *member;
+	int idx;
+};
+
 struct btfgen_type {
 	struct btf_type *type;
 	unsigned int id;
+	bool all_members;
+
+	struct hashmap *members;
 };
 
 struct btfgen_info {
@@ -1151,6 +1159,19 @@ static void *u32_as_hash_key(__u32 x)
 
 static void btfgen_free_type(struct btfgen_type *type)
 {
+	struct hashmap_entry *entry;
+	size_t bkt;
+
+	if (!type)
+		return;
+
+	if (!IS_ERR_OR_NULL(type->members)) {
+		hashmap__for_each_entry(type->members, entry, bkt) {
+			free(entry->value);
+		}
+		hashmap__free(type->members);
+	}
+
 	free(type);
 }
 
@@ -1199,19 +1220,252 @@ btfgen_new_info(const char *targ_btf_path)
 	return info;
 }
 
+static int btfgen_add_member(struct btfgen_type *btfgen_type,
+			     struct btf_member *btf_member, int idx)
+{
+	struct btfgen_member *btfgen_member;
+	int err;
+
+	/* create new members hashmap for this btfgen type if needed */
+	if (!btfgen_type->members) {
+		btfgen_type->members = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
+		if (IS_ERR(btfgen_type->members))
+			return PTR_ERR(btfgen_type->members);
+	}
+
+	btfgen_member = calloc(1, sizeof(*btfgen_member));
+	if (!btfgen_member)
+		return -ENOMEM;
+	btfgen_member->member = btf_member;
+	btfgen_member->idx = idx;
+	/* add btf_member as member to given btfgen_type */
+	err = hashmap__add(btfgen_type->members, uint_as_hash_key(btfgen_member->idx),
+			   btfgen_member);
+	if (err) {
+		free(btfgen_member);
+		if (err != -EEXIST)
+			return err;
+	}
+
+	return 0;
+}
+
+static struct btfgen_type *btfgen_get_type(struct btfgen_info *info, int id)
+{
+	struct btfgen_type *type = NULL;
+
+	hashmap__find(info->types, uint_as_hash_key(id), (void **)&type);
+
+	return type;
+}
+
+static struct btfgen_type *
+_btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
+		 unsigned int id, bool all_members)
+{
+	struct btfgen_type *btfgen_type, *tmp;
+	struct btf_array *array;
+	unsigned int child_id;
+	struct btf_member *m;
+	int err, i, n;
+
+	/* check if we already have this type */
+	if (hashmap__find(info->types, uint_as_hash_key(id), (void **) &btfgen_type)) {
+		if (!all_members || btfgen_type->all_members)
+			return btfgen_type;
+	} else {
+		btfgen_type = calloc(1, sizeof(*btfgen_type));
+		if (!btfgen_type)
+			return NULL;
+
+		btfgen_type->type = btf_type;
+		btfgen_type->id = id;
+
+		/* append this type to the types list before anything else */
+		err = hashmap__add(info->types, uint_as_hash_key(btfgen_type->id), btfgen_type);
+		if (err) {
+			free(btfgen_type);
+			return NULL;
+		}
+	}
+
+	/* avoid infinite recursion and yet be able to add all
+	 * fields/members for types also managed by this function
+	 */
+	btfgen_type->all_members = all_members;
+
+	/* recursively add other types needed by it */
+	switch (btf_kind(btfgen_type->type)) {
+	case BTF_KIND_UNKN:
+	case BTF_KIND_INT:
+	case BTF_KIND_FLOAT:
+	case BTF_KIND_ENUM:
+		break;
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+		/* doesn't need resolution if not adding all members */
+		if (!all_members)
+			break;
+
+		n = btf_vlen(btf_type);
+		m = btf_members(btf_type);
+		for (i = 0; i < n; i++, m++) {
+			btf_type = (struct btf_type *) btf__type_by_id(btf, m->type);
+
+			/* add all member types */
+			tmp = _btfgen_put_type(btf, info, btf_type, m->type, all_members);
+			if (!tmp)
+				return NULL;
+
+			/* add all members */
+			err = btfgen_add_member(btfgen_type, m, i);
+			if (err)
+				return NULL;
+		}
+		break;
+	case BTF_KIND_PTR:
+		if (!all_members)
+			break;
+	/* fall through */
+	/* Also add the type it's pointing to when adding all members */
+	case BTF_KIND_CONST:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_TYPEDEF:
+		child_id = btf_type->type;
+		btf_type = (struct btf_type *) btf__type_by_id(btf, child_id);
+
+		tmp = _btfgen_put_type(btf, info, btf_type, child_id, all_members);
+		if (!tmp)
+			return NULL;
+		break;
+	case BTF_KIND_ARRAY:
+		array = btf_array(btfgen_type->type);
+
+		/* add type for array type */
+		btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);
+		tmp = _btfgen_put_type(btf, info, btf_type, array->type, all_members);
+		if (!tmp)
+			return NULL;
+
+		/* add type for array's index type */
+		btf_type = (struct btf_type *) btf__type_by_id(btf, array->index_type);
+		tmp = _btfgen_put_type(btf, info, btf_type, array->index_type, all_members);
+		if (!tmp)
+			return NULL;
+		break;
+	/* tells if some other type needs to be handled */
+	default:
+		p_err("unsupported kind: %s (%d)",
+		      btf_kind_str(btfgen_type->type), btfgen_type->id);
+		errno = EINVAL;
+		return NULL;
+	}
+
+	return btfgen_type;
+}
+
+/* Put type in the list. If the type already exists it's returned, otherwise a
+ * new one is created and added to the list. This is called recursively adding
+ * all the types that are needed for the current one.
+ */
+static struct btfgen_type *
+btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
+		unsigned int id)
+{
+	return _btfgen_put_type(btf, info, btf_type, id, false);
+}
+
+/* Same as btfgen_put_type, but adding all members, from given complex type, recursively */
+static struct btfgen_type *
+btfgen_put_type_all(struct btf *btf, struct btfgen_info *info,
+		    struct btf_type *btf_type, unsigned int id)
+{
+	return _btfgen_put_type(btf, info, btf_type, id, true);
+}
+
 static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
 {
-	return -EOPNOTSUPP;
+	struct btf *btf = (struct btf *) info->src_btf;
+	struct btfgen_type *btfgen_type;
+	struct btf_member *btf_member;
+	struct btf_type *btf_type;
+	struct btf_array *array;
+	unsigned int id;
+	int idx, err;
+
+	btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
+
+	/* create btfgen_type for root type */
+	btfgen_type = btfgen_put_type(btf, info, btf_type, targ_spec->root_type_id);
+	if (!btfgen_type)
+		return -errno;
+
+	/* add types for complex types (arrays, unions, structures) */
+	for (int i = 1; i < targ_spec->raw_len; i++) {
+		/* skip typedefs and mods */
+		while (btf_is_mod(btf_type) || btf_is_typedef(btf_type)) {
+			id = btf_type->type;
+			btfgen_type = btfgen_get_type(info, id);
+			if (!btfgen_type)
+				return -ENOENT;
+			btf_type = (struct btf_type *) btf__type_by_id(btf, id);
+		}
+
+		switch (btf_kind(btf_type)) {
+		case BTF_KIND_STRUCT:
+		case BTF_KIND_UNION:
+			idx = targ_spec->raw_spec[i];
+			btf_member = btf_members(btf_type) + idx;
+			btf_type = (struct btf_type *) btf__type_by_id(btf, btf_member->type);
+
+			/* add member to relocation type */
+			err = btfgen_add_member(btfgen_type, btf_member, idx);
+			if (err)
+				return err;
+			/* put btfgen type */
+			btfgen_type = btfgen_put_type(btf, info, btf_type, btf_member->type);
+			if (!btfgen_type)
+				return -errno;
+			break;
+		case BTF_KIND_ARRAY:
+			array = btf_array(btf_type);
+			btfgen_type = btfgen_get_type(info, array->type);
+			if (!btfgen_type)
+				return -ENOENT;
+			btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);
+			break;
+		default:
+			p_err("unsupported kind: %s (%d)",
+			      btf_kind_str(btf_type), btf_type->type);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
 {
-	return -EOPNOTSUPP;
+	struct btf *btf = (struct btf *) info->src_btf;
+	struct btfgen_type *btfgen_type;
+	struct btf_type *btf_type;
+
+	btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
+
+	btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
+	return btfgen_type ? 0 : -errno;
 }
 
 static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
 {
-	return -EOPNOTSUPP;
+	struct btf *btf = (struct btf *) info->src_btf;
+	struct btfgen_type *btfgen_type;
+	struct btf_type *btf_type;
+
+	btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
+
+	btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
+	return btfgen_type ? 0 : -errno;
 }
 
 static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
-- 
2.25.1

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> This commit implements the logic to record the relocation information
> for the different kind of relocations.
>
> btfgen_record_field_relo() uses the target specification to save all the
> types that are involved in a field-based CO-RE relocation. In this case
> types resolved and added recursively (using btfgen_put_type()).
> Only the struct and union members and their types) involved in the
> relocation are added to optimize the size of the generated BTF file.
>
> On the other hand, btfgen_record_type_relo() saves the types involved in
> a type-based CO-RE relocation. In this case all the members for the

Do I understand correctly that if someone does
bpf_core_type_size(struct task_struct), you'll save not just
task_struct, but also any type that directly and indirectly referenced
from any task_struct's field, even if that is through a pointer. As
in, do you substitute forward declarations for types that are never
directly used? If not, that's going to be very suboptimal for
something like task_struct and any other type that's part of a big
cluster of types.

> struct and union types are added. This is not strictly required since
> libbpf doesn't use them while performing this kind of relocation,
> however that logic could change on the future. Additionally, we expect
> that the number of this kind of relocations in an BPF object to be very
> low, hence the impact on the size of the generated BTF should be
> negligible.
>
> Finally, btfgen_record_enumval_relo() saves the whole enum type for
> enum-based relocations.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 257 insertions(+), 3 deletions(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index bb9c56401ee5..7413ec808a80 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1119,9 +1119,17 @@ static int btf_save_raw(const struct btf *btf, const char *path)
>         return err;
>  }
>
> +struct btfgen_member {
> +       struct btf_member *member;
> +       int idx;
> +};
> +
>  struct btfgen_type {
>         struct btf_type *type;
>         unsigned int id;
> +       bool all_members;
> +
> +       struct hashmap *members;
>  };
>
>  struct btfgen_info {
> @@ -1151,6 +1159,19 @@ static void *u32_as_hash_key(__u32 x)
>
>  static void btfgen_free_type(struct btfgen_type *type)
>  {
> +       struct hashmap_entry *entry;
> +       size_t bkt;
> +
> +       if (!type)
> +               return;
> +
> +       if (!IS_ERR_OR_NULL(type->members)) {
> +               hashmap__for_each_entry(type->members, entry, bkt) {
> +                       free(entry->value);
> +               }
> +               hashmap__free(type->members);
> +       }
> +
>         free(type);
>  }
>
> @@ -1199,19 +1220,252 @@ btfgen_new_info(const char *targ_btf_path)
>         return info;
>  }
>
> +static int btfgen_add_member(struct btfgen_type *btfgen_type,
> +                            struct btf_member *btf_member, int idx)
> +{
> +       struct btfgen_member *btfgen_member;
> +       int err;
> +
> +       /* create new members hashmap for this btfgen type if needed */
> +       if (!btfgen_type->members) {
> +               btfgen_type->members = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> +               if (IS_ERR(btfgen_type->members))
> +                       return PTR_ERR(btfgen_type->members);
> +       }
> +
> +       btfgen_member = calloc(1, sizeof(*btfgen_member));
> +       if (!btfgen_member)
> +               return -ENOMEM;
> +       btfgen_member->member = btf_member;
> +       btfgen_member->idx = idx;
> +       /* add btf_member as member to given btfgen_type */
> +       err = hashmap__add(btfgen_type->members, uint_as_hash_key(btfgen_member->idx),
> +                          btfgen_member);
> +       if (err) {
> +               free(btfgen_member);
> +               if (err != -EEXIST)

why not check that such a member exists before doing btfgen_member allocation?

> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct btfgen_type *btfgen_get_type(struct btfgen_info *info, int id)
> +{
> +       struct btfgen_type *type = NULL;
> +
> +       hashmap__find(info->types, uint_as_hash_key(id), (void **)&type);

if (!hashmap__find(...))
   return NULL;

> +
> +       return type;
> +}
> +
> +static struct btfgen_type *
> +_btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
> +                unsigned int id, bool all_members)
> +{
> +       struct btfgen_type *btfgen_type, *tmp;
> +       struct btf_array *array;
> +       unsigned int child_id;
> +       struct btf_member *m;
> +       int err, i, n;
> +
> +       /* check if we already have this type */
> +       if (hashmap__find(info->types, uint_as_hash_key(id), (void **) &btfgen_type)) {
> +               if (!all_members || btfgen_type->all_members)
> +                       return btfgen_type;
> +       } else {
> +               btfgen_type = calloc(1, sizeof(*btfgen_type));
> +               if (!btfgen_type)
> +                       return NULL;
> +
> +               btfgen_type->type = btf_type;
> +               btfgen_type->id = id;
> +
> +               /* append this type to the types list before anything else */

what do you mean by "before anything else"?

> +               err = hashmap__add(info->types, uint_as_hash_key(btfgen_type->id), btfgen_type);
> +               if (err) {
> +                       free(btfgen_type);
> +                       return NULL;
> +               }
> +       }
> +
> +       /* avoid infinite recursion and yet be able to add all
> +        * fields/members for types also managed by this function
> +        */
> +       btfgen_type->all_members = all_members;
> +
> +       /* recursively add other types needed by it */
> +       switch (btf_kind(btfgen_type->type)) {
> +       case BTF_KIND_UNKN:
> +       case BTF_KIND_INT:
> +       case BTF_KIND_FLOAT:
> +       case BTF_KIND_ENUM:
> +               break;
> +       case BTF_KIND_STRUCT:
> +       case BTF_KIND_UNION:
> +               /* doesn't need resolution if not adding all members */
> +               if (!all_members)
> +                       break;
> +
> +               n = btf_vlen(btf_type);
> +               m = btf_members(btf_type);
> +               for (i = 0; i < n; i++, m++) {
> +                       btf_type = (struct btf_type *) btf__type_by_id(btf, m->type);

why `const struct btf_type *` doesn't work everywhere? You are not
modifying btf_type itself, no?

> +
> +                       /* add all member types */
> +                       tmp = _btfgen_put_type(btf, info, btf_type, m->type, all_members);
> +                       if (!tmp)
> +                               return NULL;
> +
> +                       /* add all members */
> +                       err = btfgen_add_member(btfgen_type, m, i);
> +                       if (err)
> +                               return NULL;
> +               }
> +               break;
> +       case BTF_KIND_PTR:
> +               if (!all_members)
> +                       break;
> +       /* fall through */
> +       /* Also add the type it's pointing to when adding all members */
> +       case BTF_KIND_CONST:
> +       case BTF_KIND_VOLATILE:
> +       case BTF_KIND_TYPEDEF:
> +               child_id = btf_type->type;
> +               btf_type = (struct btf_type *) btf__type_by_id(btf, child_id);
> +
> +               tmp = _btfgen_put_type(btf, info, btf_type, child_id, all_members);
> +               if (!tmp)
> +                       return NULL;
> +               break;
> +       case BTF_KIND_ARRAY:
> +               array = btf_array(btfgen_type->type);
> +
> +               /* add type for array type */
> +               btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);
> +               tmp = _btfgen_put_type(btf, info, btf_type, array->type, all_members);
> +               if (!tmp)
> +                       return NULL;
> +
> +               /* add type for array's index type */
> +               btf_type = (struct btf_type *) btf__type_by_id(btf, array->index_type);
> +               tmp = _btfgen_put_type(btf, info, btf_type, array->index_type, all_members);
> +               if (!tmp)
> +                       return NULL;
> +               break;
> +       /* tells if some other type needs to be handled */
> +       default:
> +               p_err("unsupported kind: %s (%d)",
> +                     btf_kind_str(btfgen_type->type), btfgen_type->id);
> +               errno = EINVAL;
> +               return NULL;
> +       }
> +
> +       return btfgen_type;
> +}
> +
> +/* Put type in the list. If the type already exists it's returned, otherwise a
> + * new one is created and added to the list. This is called recursively adding
> + * all the types that are needed for the current one.
> + */
> +static struct btfgen_type *
> +btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
> +               unsigned int id)
> +{
> +       return _btfgen_put_type(btf, info, btf_type, id, false);
> +}
> +
> +/* Same as btfgen_put_type, but adding all members, from given complex type, recursively */
> +static struct btfgen_type *
> +btfgen_put_type_all(struct btf *btf, struct btfgen_info *info,
> +                   struct btf_type *btf_type, unsigned int id)
> +{
> +       return _btfgen_put_type(btf, info, btf_type, id, true);
> +}

these wrappers seem unnecessary, just pass false/true in 5 call sites
below without extra wrapping of _btfgen_put_type (and call it
btfgen_put_type then)

> +
>  static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
>  {
> -       return -EOPNOTSUPP;
> +       struct btf *btf = (struct btf *) info->src_btf;
> +       struct btfgen_type *btfgen_type;
> +       struct btf_member *btf_member;
> +       struct btf_type *btf_type;
> +       struct btf_array *array;
> +       unsigned int id;
> +       int idx, err;
> +
> +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> +
> +       /* create btfgen_type for root type */
> +       btfgen_type = btfgen_put_type(btf, info, btf_type, targ_spec->root_type_id);
> +       if (!btfgen_type)
> +               return -errno;
> +
> +       /* add types for complex types (arrays, unions, structures) */
> +       for (int i = 1; i < targ_spec->raw_len; i++) {
> +               /* skip typedefs and mods */
> +               while (btf_is_mod(btf_type) || btf_is_typedef(btf_type)) {
> +                       id = btf_type->type;
> +                       btfgen_type = btfgen_get_type(info, id);
> +                       if (!btfgen_type)
> +                               return -ENOENT;
> +                       btf_type = (struct btf_type *) btf__type_by_id(btf, id);
> +               }
> +
> +               switch (btf_kind(btf_type)) {
> +               case BTF_KIND_STRUCT:
> +               case BTF_KIND_UNION:
> +                       idx = targ_spec->raw_spec[i];
> +                       btf_member = btf_members(btf_type) + idx;
> +                       btf_type = (struct btf_type *) btf__type_by_id(btf, btf_member->type);
> +
> +                       /* add member to relocation type */
> +                       err = btfgen_add_member(btfgen_type, btf_member, idx);
> +                       if (err)
> +                               return err;
> +                       /* put btfgen type */
> +                       btfgen_type = btfgen_put_type(btf, info, btf_type, btf_member->type);
> +                       if (!btfgen_type)
> +                               return -errno;
> +                       break;
> +               case BTF_KIND_ARRAY:
> +                       array = btf_array(btf_type);
> +                       btfgen_type = btfgen_get_type(info, array->type);
> +                       if (!btfgen_type)
> +                               return -ENOENT;
> +                       btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);

should index_type be added as well?

> +                       break;
> +               default:
> +                       p_err("unsupported kind: %s (%d)",
> +                             btf_kind_str(btf_type), btf_type->type);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
>  }
>
>  static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
>  {
> -       return -EOPNOTSUPP;
> +       struct btf *btf = (struct btf *) info->src_btf;
> +       struct btfgen_type *btfgen_type;
> +       struct btf_type *btf_type;
> +
> +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> +
> +       btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> +       return btfgen_type ? 0 : -errno;
>  }
>
>  static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
>  {
> -       return -EOPNOTSUPP;
> +       struct btf *btf = (struct btf *) info->src_btf;
> +       struct btfgen_type *btfgen_type;
> +       struct btf_type *btf_type;
> +
> +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> +
> +       btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> +       return btfgen_type ? 0 : -errno;
>  }
>
>  static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 2:31 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This commit implements the logic to record the relocation information
> > for the different kind of relocations.
> >
> > btfgen_record_field_relo() uses the target specification to save all the
> > types that are involved in a field-based CO-RE relocation. In this case
> > types resolved and added recursively (using btfgen_put_type()).
> > Only the struct and union members and their types) involved in the
> > relocation are added to optimize the size of the generated BTF file.
> >
> > On the other hand, btfgen_record_type_relo() saves the types involved in
> > a type-based CO-RE relocation. In this case all the members for the
>
> Do I understand correctly that if someone does
> bpf_core_type_size(struct task_struct), you'll save not just
> task_struct, but also any type that directly and indirectly referenced
> from any task_struct's field, even if that is through a pointer.

That's correct.

> As
> in, do you substitute forward declarations for types that are never
> directly used? If not, that's going to be very suboptimal for
> something like task_struct and any other type that's part of a big
> cluster of types.
>

We decided to include the whole types and all direct and indirect
types referenced from a structure field for type-based relocations.
Our reasoning is that we don't know if the matching algorithm of
libbpf could be changed to require more information in the future and
type-based relocations are few compared to field based relocations.

If you are confident enough that adding empty structures/unions is ok
then I'll update the algorithm. Actually it'll make our lives easier.

> > struct and union types are added. This is not strictly required since
> > libbpf doesn't use them while performing this kind of relocation,
> > however that logic could change on the future. Additionally, we expect
> > that the number of this kind of relocations in an BPF object to be very
> > low, hence the impact on the size of the generated BTF should be
> > negligible.
> >
> > Finally, btfgen_record_enumval_relo() saves the whole enum type for
> > enum-based relocations.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 257 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index bb9c56401ee5..7413ec808a80 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1119,9 +1119,17 @@ static int btf_save_raw(const struct btf *btf, const char *path)
> >         return err;
> >  }
> >
> > +struct btfgen_member {
> > +       struct btf_member *member;
> > +       int idx;
> > +};
> > +
> >  struct btfgen_type {
> >         struct btf_type *type;
> >         unsigned int id;
> > +       bool all_members;
> > +
> > +       struct hashmap *members;
> >  };
> >
> >  struct btfgen_info {
> > @@ -1151,6 +1159,19 @@ static void *u32_as_hash_key(__u32 x)
> >
> >  static void btfgen_free_type(struct btfgen_type *type)
> >  {
> > +       struct hashmap_entry *entry;
> > +       size_t bkt;
> > +
> > +       if (!type)
> > +               return;
> > +
> > +       if (!IS_ERR_OR_NULL(type->members)) {
> > +               hashmap__for_each_entry(type->members, entry, bkt) {
> > +                       free(entry->value);
> > +               }
> > +               hashmap__free(type->members);
> > +       }
> > +
> >         free(type);
> >  }
> >
> > @@ -1199,19 +1220,252 @@ btfgen_new_info(const char *targ_btf_path)
> >         return info;
> >  }
> >
> > +static int btfgen_add_member(struct btfgen_type *btfgen_type,
> > +                            struct btf_member *btf_member, int idx)
> > +{
> > +       struct btfgen_member *btfgen_member;
> > +       int err;
> > +
> > +       /* create new members hashmap for this btfgen type if needed */
> > +       if (!btfgen_type->members) {
> > +               btfgen_type->members = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > +               if (IS_ERR(btfgen_type->members))
> > +                       return PTR_ERR(btfgen_type->members);
> > +       }
> > +
> > +       btfgen_member = calloc(1, sizeof(*btfgen_member));
> > +       if (!btfgen_member)
> > +               return -ENOMEM;
> > +       btfgen_member->member = btf_member;
> > +       btfgen_member->idx = idx;
> > +       /* add btf_member as member to given btfgen_type */
> > +       err = hashmap__add(btfgen_type->members, uint_as_hash_key(btfgen_member->idx),
> > +                          btfgen_member);
> > +       if (err) {
> > +               free(btfgen_member);
> > +               if (err != -EEXIST)
>
> why not check that such a member exists before doing btfgen_member allocation?
>

I thought that it could be more efficient calling hashmap__add()
directly without checking and then handling the case when it was
already there. Having a second thought it seems to me that it's not
always true and depends on how many times the code follows each path,
what we don't know. I'll change it to check if it's there before.

> > +                       return err;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static struct btfgen_type *btfgen_get_type(struct btfgen_info *info, int id)
> > +{
> > +       struct btfgen_type *type = NULL;
> > +
> > +       hashmap__find(info->types, uint_as_hash_key(id), (void **)&type);
>
> if (!hashmap__find(...))
>    return NULL;
>
> > +
> > +       return type;
> > +}
> > +
> > +static struct btfgen_type *
> > +_btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
> > +                unsigned int id, bool all_members)
> > +{
> > +       struct btfgen_type *btfgen_type, *tmp;
> > +       struct btf_array *array;
> > +       unsigned int child_id;
> > +       struct btf_member *m;
> > +       int err, i, n;
> > +
> > +       /* check if we already have this type */
> > +       if (hashmap__find(info->types, uint_as_hash_key(id), (void **) &btfgen_type)) {
> > +               if (!all_members || btfgen_type->all_members)
> > +                       return btfgen_type;
> > +       } else {
> > +               btfgen_type = calloc(1, sizeof(*btfgen_type));
> > +               if (!btfgen_type)
> > +                       return NULL;
> > +
> > +               btfgen_type->type = btf_type;
> > +               btfgen_type->id = id;
> > +
> > +               /* append this type to the types list before anything else */
>
> what do you mean by "before anything else"?

Add this before starting the recursion below. I'll update the comment
to make it more clear.

>
> > +               err = hashmap__add(info->types, uint_as_hash_key(btfgen_type->id), btfgen_type);
> > +               if (err) {
> > +                       free(btfgen_type);
> > +                       return NULL;
> > +               }
> > +       }
> > +
> > +       /* avoid infinite recursion and yet be able to add all
> > +        * fields/members for types also managed by this function
> > +        */
> > +       btfgen_type->all_members = all_members;
> > +
> > +       /* recursively add other types needed by it */
> > +       switch (btf_kind(btfgen_type->type)) {
> > +       case BTF_KIND_UNKN:
> > +       case BTF_KIND_INT:
> > +       case BTF_KIND_FLOAT:
> > +       case BTF_KIND_ENUM:
> > +               break;
> > +       case BTF_KIND_STRUCT:
> > +       case BTF_KIND_UNION:
> > +               /* doesn't need resolution if not adding all members */
> > +               if (!all_members)
> > +                       break;
> > +
> > +               n = btf_vlen(btf_type);
> > +               m = btf_members(btf_type);
> > +               for (i = 0; i < n; i++, m++) {
> > +                       btf_type = (struct btf_type *) btf__type_by_id(btf, m->type);
>
> why `const struct btf_type *` doesn't work everywhere? You are not
> modifying btf_type itself, no?

Yes, `const struct btf_type *` works fine everywhere.

>
> > +
> > +                       /* add all member types */
> > +                       tmp = _btfgen_put_type(btf, info, btf_type, m->type, all_members);
> > +                       if (!tmp)
> > +                               return NULL;
> > +
> > +                       /* add all members */
> > +                       err = btfgen_add_member(btfgen_type, m, i);
> > +                       if (err)
> > +                               return NULL;
> > +               }
> > +               break;
> > +       case BTF_KIND_PTR:
> > +               if (!all_members)
> > +                       break;
> > +       /* fall through */
> > +       /* Also add the type it's pointing to when adding all members */
> > +       case BTF_KIND_CONST:
> > +       case BTF_KIND_VOLATILE:
> > +       case BTF_KIND_TYPEDEF:
> > +               child_id = btf_type->type;
> > +               btf_type = (struct btf_type *) btf__type_by_id(btf, child_id);
> > +
> > +               tmp = _btfgen_put_type(btf, info, btf_type, child_id, all_members);
> > +               if (!tmp)
> > +                       return NULL;
> > +               break;
> > +       case BTF_KIND_ARRAY:
> > +               array = btf_array(btfgen_type->type);
> > +
> > +               /* add type for array type */
> > +               btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);
> > +               tmp = _btfgen_put_type(btf, info, btf_type, array->type, all_members);
> > +               if (!tmp)
> > +                       return NULL;
> > +
> > +               /* add type for array's index type */
> > +               btf_type = (struct btf_type *) btf__type_by_id(btf, array->index_type);
> > +               tmp = _btfgen_put_type(btf, info, btf_type, array->index_type, all_members);
> > +               if (!tmp)
> > +                       return NULL;
> > +               break;
> > +       /* tells if some other type needs to be handled */
> > +       default:
> > +               p_err("unsupported kind: %s (%d)",
> > +                     btf_kind_str(btfgen_type->type), btfgen_type->id);
> > +               errno = EINVAL;
> > +               return NULL;
> > +       }
> > +
> > +       return btfgen_type;
> > +}
> > +
> > +/* Put type in the list. If the type already exists it's returned, otherwise a
> > + * new one is created and added to the list. This is called recursively adding
> > + * all the types that are needed for the current one.
> > + */
> > +static struct btfgen_type *
> > +btfgen_put_type(struct btf *btf, struct btfgen_info *info, struct btf_type *btf_type,
> > +               unsigned int id)
> > +{
> > +       return _btfgen_put_type(btf, info, btf_type, id, false);
> > +}
> > +
> > +/* Same as btfgen_put_type, but adding all members, from given complex type, recursively */
> > +static struct btfgen_type *
> > +btfgen_put_type_all(struct btf *btf, struct btfgen_info *info,
> > +                   struct btf_type *btf_type, unsigned int id)
> > +{
> > +       return _btfgen_put_type(btf, info, btf_type, id, true);
> > +}
>
> these wrappers seem unnecessary, just pass false/true in 5 call sites
> below without extra wrapping of _btfgen_put_type (and call it
> btfgen_put_type then)
>
> > +
> >  static int btfgen_record_field_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> >  {
> > -       return -EOPNOTSUPP;
> > +       struct btf *btf = (struct btf *) info->src_btf;
> > +       struct btfgen_type *btfgen_type;
> > +       struct btf_member *btf_member;
> > +       struct btf_type *btf_type;
> > +       struct btf_array *array;
> > +       unsigned int id;
> > +       int idx, err;
> > +
> > +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> > +
> > +       /* create btfgen_type for root type */
> > +       btfgen_type = btfgen_put_type(btf, info, btf_type, targ_spec->root_type_id);
> > +       if (!btfgen_type)
> > +               return -errno;
> > +
> > +       /* add types for complex types (arrays, unions, structures) */
> > +       for (int i = 1; i < targ_spec->raw_len; i++) {
> > +               /* skip typedefs and mods */
> > +               while (btf_is_mod(btf_type) || btf_is_typedef(btf_type)) {
> > +                       id = btf_type->type;
> > +                       btfgen_type = btfgen_get_type(info, id);
> > +                       if (!btfgen_type)
> > +                               return -ENOENT;
> > +                       btf_type = (struct btf_type *) btf__type_by_id(btf, id);
> > +               }
> > +
> > +               switch (btf_kind(btf_type)) {
> > +               case BTF_KIND_STRUCT:
> > +               case BTF_KIND_UNION:
> > +                       idx = targ_spec->raw_spec[i];
> > +                       btf_member = btf_members(btf_type) + idx;
> > +                       btf_type = (struct btf_type *) btf__type_by_id(btf, btf_member->type);
> > +
> > +                       /* add member to relocation type */
> > +                       err = btfgen_add_member(btfgen_type, btf_member, idx);
> > +                       if (err)
> > +                               return err;
> > +                       /* put btfgen type */
> > +                       btfgen_type = btfgen_put_type(btf, info, btf_type, btf_member->type);
> > +                       if (!btfgen_type)
> > +                               return -errno;
> > +                       break;
> > +               case BTF_KIND_ARRAY:
> > +                       array = btf_array(btf_type);
> > +                       btfgen_type = btfgen_get_type(info, array->type);
> > +                       if (!btfgen_type)
> > +                               return -ENOENT;
> > +                       btf_type = (struct btf_type *) btf__type_by_id(btf, array->type);
>
> should index_type be added as well?

This is added in _btfgen_put_type(). Here we're just updating
`btf_type` with the array's type for the next iteration of this loop.

>
> > +                       break;
> > +               default:
> > +                       p_err("unsupported kind: %s (%d)",
> > +                             btf_kind_str(btf_type), btf_type->type);
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> > +       return 0;
> >  }
> >
> >  static int btfgen_record_type_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> >  {
> > -       return -EOPNOTSUPP;
> > +       struct btf *btf = (struct btf *) info->src_btf;
> > +       struct btfgen_type *btfgen_type;
> > +       struct btf_type *btf_type;
> > +
> > +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> > +
> > +       btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> > +       return btfgen_type ? 0 : -errno;
> >  }
> >
> >  static int btfgen_record_enumval_relo(struct btfgen_info *info, struct bpf_core_spec *targ_spec)
> >  {
> > -       return -EOPNOTSUPP;
> > +       struct btf *btf = (struct btf *) info->src_btf;
> > +       struct btfgen_type *btfgen_type;
> > +       struct btf_type *btf_type;
> > +
> > +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> > +
> > +       btfgen_type = btfgen_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> > +       return btfgen_type ? 0 : -errno;
> >  }
> >
> >  static int btfgen_record_reloc(struct btfgen_info *info, struct bpf_core_spec *res)
> > --
> > 2.25.1
> >

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 8:40 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 2:31 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > This commit implements the logic to record the relocation information
> > > for the different kind of relocations.
> > >
> > > btfgen_record_field_relo() uses the target specification to save all the
> > > types that are involved in a field-based CO-RE relocation. In this case
> > > types resolved and added recursively (using btfgen_put_type()).
> > > Only the struct and union members and their types) involved in the
> > > relocation are added to optimize the size of the generated BTF file.
> > >
> > > On the other hand, btfgen_record_type_relo() saves the types involved in
> > > a type-based CO-RE relocation. In this case all the members for the
> >
> > Do I understand correctly that if someone does
> > bpf_core_type_size(struct task_struct), you'll save not just
> > task_struct, but also any type that directly and indirectly referenced
> > from any task_struct's field, even if that is through a pointer.
>
> That's correct.
>
> > As
> > in, do you substitute forward declarations for types that are never
> > directly used? If not, that's going to be very suboptimal for
> > something like task_struct and any other type that's part of a big
> > cluster of types.
> >
>
> We decided to include the whole types and all direct and indirect
> types referenced from a structure field for type-based relocations.
> Our reasoning is that we don't know if the matching algorithm of
> libbpf could be changed to require more information in the future and
> type-based relocations are few compared to field based relocations.
>

It will depend on application and which type is used in relocation.
task_struct reaches tons of types and will add a very noticeable size
to minimized BTF, for no good reason, IMO. If we discover that we do
need those types, we'll update bpftool to generate more.


> If you are confident enough that adding empty structures/unions is ok
> then I'll update the algorithm. Actually it'll make our lives easier.
>

Well, test it of course, but I think it should work.

> > > struct and union types are added. This is not strictly required since
> > > libbpf doesn't use them while performing this kind of relocation,
> > > however that logic could change on the future. Additionally, we expect
> > > that the number of this kind of relocations in an BPF object to be very
> > > low, hence the impact on the size of the generated BTF should be
> > > negligible.
> > >
> > > Finally, btfgen_record_enumval_relo() saves the whole enum type for
> > > enum-based relocations.
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > > ---
> > >  tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 257 insertions(+), 3 deletions(-)
> > >

[...]

> > > +
> > > +       btfgen_member = calloc(1, sizeof(*btfgen_member));
> > > +       if (!btfgen_member)
> > > +               return -ENOMEM;
> > > +       btfgen_member->member = btf_member;
> > > +       btfgen_member->idx = idx;
> > > +       /* add btf_member as member to given btfgen_type */
> > > +       err = hashmap__add(btfgen_type->members, uint_as_hash_key(btfgen_member->idx),
> > > +                          btfgen_member);
> > > +       if (err) {
> > > +               free(btfgen_member);
> > > +               if (err != -EEXIST)
> >
> > why not check that such a member exists before doing btfgen_member allocation?
> >
>
> I thought that it could be more efficient calling hashmap__add()
> directly without checking and then handling the case when it was
> already there. Having a second thought it seems to me that it's not
> always true and depends on how many times the code follows each path,
> what we don't know. I'll change it to check if it's there before.
>

See my other reply on this patch. Maybe you won't need a hashmap at
all if you modify btf_type in place (As in, set extra bit to mark that
type or its member is needed)? It feels a bit hacky, but this is an
internal and one specific case inside bpftool, so I think it's
justified (and it will be much cleaner and shorter code, IMO).

> > > +                       return err;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +

[...]

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Rafael David Tinoco]

>>> As in, do you substitute forward declarations for types that are
>>> never directly used? If not, that's going to be very suboptimal for
>>> something like task_struct and any other type that's part of a big
>>> cluster of types.

>> We decided to include the whole types and all direct and indirect
>> types referenced from a structure field for type-based relocations.
>> Our reasoning is that we don't know if the matching algorithm of
>> libbpf could be changed to require more information in the future and
>> type-based relocations are few compared to field based relocations.

> It will depend on application and which type is used in relocation.
> task_struct reaches tons of types and will add a very noticeable size
> to minimized BTF, for no good reason, IMO. If we discover that we do
> need those types, we'll update bpftool to generate more.

Just to see if I understood this part correctly. IIRC, we started type
based relocations support in btfgen because of this particular case:

	union kernfs_node_id {
	    struct {
	        u32 ino;
	        u32 generation;
	    };
	    u64 id;
	};

	struct kernfs_node___older_v55 {
	    const char *name;
	    union kernfs_node_id id;
	};

	struct kernfs_node___rh8 {
	    const char *name;
	    union {
	        u64 id;
	        struct {
	            union kernfs_node_id id;
	        } rh_kabi_hidden_172;
	        union { };
	    };
	};

So we have 3 situations:

(struct kernfs_node *)->id as u64

	[29] STRUCT 'kernfs_node' size=128 vlen=1
	        'id' type_id=42 bits_offset=832
	[42] TYPEDEF 'u64' type_id=10

(struct kernfs_node___older_v55 *)->id as u64 (union kernfs_node_id)->id

	[79] STRUCT 'kernfs_node' size=128 vlen=1
	        'id' type_id=69 bits_offset=832
	[69] UNION 'kernfs_node_id' size=8 vlen=2
	        '(anon)' type_id=132 bits_offset=0
	        'id' type_id=40 bits_offset=0
	[40] TYPEDEF 'u64' type_id=12

(struct kernfs_node___rh8 *)->id = (anon union)->id

	[56] STRUCT 'kernfs_node' size=128 vlen=1
	        '(anon)' type_id=24 bits_offset=832
	[24] UNION '(anon)' size=8 vlen=1
	        'id' type_id=40 bits_offset=0
	[40] TYPEDEF 'u64' type_id=11

We're finding needed BTF types, that should be added to generated BTF,
based on fields/members of CORE relo info. How we would know we had to
add the anon union of the last case if it does not exist in the local
BTF ? What is your suggestion ?

Thanks!

-rafaeldtinoco

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 10:20 PM Rafael David Tinoco
<rafaeldtinoco@gmail.com> wrote:
>
> >>> As in, do you substitute forward declarations for types that are
> >>> never directly used? If not, that's going to be very suboptimal for
> >>> something like task_struct and any other type that's part of a big
> >>> cluster of types.
>
> >> We decided to include the whole types and all direct and indirect
> >> types referenced from a structure field for type-based relocations.
> >> Our reasoning is that we don't know if the matching algorithm of
> >> libbpf could be changed to require more information in the future and
> >> type-based relocations are few compared to field based relocations.
>
> > It will depend on application and which type is used in relocation.
> > task_struct reaches tons of types and will add a very noticeable size
> > to minimized BTF, for no good reason, IMO. If we discover that we do
> > need those types, we'll update bpftool to generate more.
>
> Just to see if I understood this part correctly. IIRC, we started type
> based relocations support in btfgen because of this particular case:
>
>         union kernfs_node_id {
>             struct {
>                 u32 ino;
>                 u32 generation;
>             };
>             u64 id;
>         };
>
>         struct kernfs_node___older_v55 {
>             const char *name;
>             union kernfs_node_id id;
>         };
>
>         struct kernfs_node___rh8 {
>             const char *name;
>             union {
>                 u64 id;
>                 struct {
>                     union kernfs_node_id id;
>                 } rh_kabi_hidden_172;
>                 union { };
>             };
>         };
>
> So we have 3 situations:
>
> (struct kernfs_node *)->id as u64
>
>         [29] STRUCT 'kernfs_node' size=128 vlen=1
>                 'id' type_id=42 bits_offset=832
>         [42] TYPEDEF 'u64' type_id=10
>
> (struct kernfs_node___older_v55 *)->id as u64 (union kernfs_node_id)->id
>
>         [79] STRUCT 'kernfs_node' size=128 vlen=1
>                 'id' type_id=69 bits_offset=832
>         [69] UNION 'kernfs_node_id' size=8 vlen=2
>                 '(anon)' type_id=132 bits_offset=0
>                 'id' type_id=40 bits_offset=0
>         [40] TYPEDEF 'u64' type_id=12
>
> (struct kernfs_node___rh8 *)->id = (anon union)->id
>
>         [56] STRUCT 'kernfs_node' size=128 vlen=1
>                 '(anon)' type_id=24 bits_offset=832
>         [24] UNION '(anon)' size=8 vlen=1
>                 'id' type_id=40 bits_offset=0
>         [40] TYPEDEF 'u64' type_id=11
>
> We're finding needed BTF types, that should be added to generated BTF,
> based on fields/members of CORE relo info. How we would know we had to
> add the anon union of the last case if it does not exist in the local
> BTF ? What is your suggestion ?
>

I'd need to see real BPF program code for this situation, but if you
don't have field-based relocation that needs that anonymous union,
then it shouldn't matter if that union is there or not. I suspect you
do have field-based relocations that access fields of struct
kernfs_node___rh8 and kernfs_node___older_v55, so both structs and
necessary fields should be marked as "used" by btfgen algorithm.

> Thanks!
>
> -rafaeldtinoco

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> This commit implements the logic to record the relocation information
> for the different kind of relocations.
>
> btfgen_record_field_relo() uses the target specification to save all the
> types that are involved in a field-based CO-RE relocation. In this case
> types resolved and added recursively (using btfgen_put_type()).
> Only the struct and union members and their types) involved in the
> relocation are added to optimize the size of the generated BTF file.
>
> On the other hand, btfgen_record_type_relo() saves the types involved in
> a type-based CO-RE relocation. In this case all the members for the
> struct and union types are added. This is not strictly required since
> libbpf doesn't use them while performing this kind of relocation,
> however that logic could change on the future. Additionally, we expect
> that the number of this kind of relocations in an BPF object to be very
> low, hence the impact on the size of the generated BTF should be
> negligible.
>
> Finally, btfgen_record_enumval_relo() saves the whole enum type for
> enum-based relocations.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---

I've been thinking about this in background. This proliferation of
hashmaps to store used types and their members really adds to
complexity (and no doubt to memory usage and CPU utilization, even
though I don't think either is too big for this use case).

What if instead of keeping track of used types and members separately,
we initialize the original struct btf and its btf_type, btf_member,
btf_enum, etc types. We can carve out one bit in them to mark whether
that specific entity was used. That way you don't need any extra
hashmap maintenance. You just set or check bit on each type or its
member to figure out if it has to be in the resulting BTF.

This can be highest bit of name_off or type fields, depending on
specific case. This will work well because type IDs never use highest
bit and string offset can never be as high as to needing full 32 bits.

You'll probably want to have two copies of target BTF for this, of
course, but I think simplicity of bookkeeping trumps this
inefficiency. WDYT?

>  tools/bpf/bpftool/gen.c | 260 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 257 insertions(+), 3 deletions(-)
>

[...]

Re: [PATCH bpf-next v5 6/9] bpftool: Implement relocations recording for BTFGen

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 5:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This commit implements the logic to record the relocation information
> > for the different kind of relocations.
> >
> > btfgen_record_field_relo() uses the target specification to save all the
> > types that are involved in a field-based CO-RE relocation. In this case
> > types resolved and added recursively (using btfgen_put_type()).
> > Only the struct and union members and their types) involved in the
> > relocation are added to optimize the size of the generated BTF file.
> >
> > On the other hand, btfgen_record_type_relo() saves the types involved in
> > a type-based CO-RE relocation. In this case all the members for the
> > struct and union types are added. This is not strictly required since
> > libbpf doesn't use them while performing this kind of relocation,
> > however that logic could change on the future. Additionally, we expect
> > that the number of this kind of relocations in an BPF object to be very
> > low, hence the impact on the size of the generated BTF should be
> > negligible.
> >
> > Finally, btfgen_record_enumval_relo() saves the whole enum type for
> > enum-based relocations.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
>
> I've been thinking about this in background. This proliferation of
> hashmaps to store used types and their members really adds to
> complexity (and no doubt to memory usage and CPU utilization, even
> though I don't think either is too big for this use case).
>
> What if instead of keeping track of used types and members separately,
> we initialize the original struct btf and its btf_type, btf_member,
> btf_enum, etc types. We can carve out one bit in them to mark whether
> that specific entity was used. That way you don't need any extra
> hashmap maintenance. You just set or check bit on each type or its
> member to figure out if it has to be in the resulting BTF.
>
> This can be highest bit of name_off or type fields, depending on
> specific case. This will work well because type IDs never use highest
> bit and string offset can never be as high as to needing full 32 bits.
>
> You'll probably want to have two copies of target BTF for this, of
> course, but I think simplicity of bookkeeping trumps this
> inefficiency. WDYT?
>

It's a very nice idea indeed. I got a version working with this idea.
I keep two instances of the target BTF (as you suggested) one is only
for keeping track of the used types/members, the other one is used as
source when copying the BTF types and also to run the candidate search
algorithm and so on. Actually there is no need to use the highest bit,
I'm just setting the whole name_off to UINT32_MAX. It works fine
because that copy of the BTF isn't used anywhere else. I'm cleaning
this up and hope to send it early next week.

Thanks for all the feedback!

[PATCH bpf-next v5 7/9] bpftool: Implement btfgen_get_btf()

[Mauricio Vásquez]

The last part of the BTFGen algorithm is to create a new BTF object with
all the types that were recorded in the previous steps.

This function performs two different steps:
1. Add the types to the new BTF object by using btf__add_type(). Some
special logic around struct and unions is implemented to only add the
members that are really used in the field-based relocations. The type
ID on the new and old BTF objects is stored on a map.
2. Fix all the type IDs on the new BTF object by using the IDs saved in
the previous step.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/bpf/bpftool/gen.c | 158 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 157 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 7413ec808a80..55e6f640cbbb 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1599,10 +1599,166 @@ static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
 	return err;
 }
 
+static unsigned int btfgen_get_id(struct hashmap *ids, unsigned int old)
+{
+	uintptr_t new = 0;
+
+	/* deal with BTF_KIND_VOID */
+	if (old == 0)
+		return 0;
+
+	if (!hashmap__find(ids, uint_as_hash_key(old), (void **)&new)) {
+		/* return id for void as it's possible that the ID we're looking for is
+		 * the type of a pointer that we're not adding.
+		 */
+		return 0;
+	}
+
+	return (unsigned int)(uintptr_t)new;
+}
+
+static int btfgen_add_id(struct hashmap *ids, unsigned int old, unsigned int new)
+{
+	return hashmap__add(ids, uint_as_hash_key(old), uint_as_hash_key(new));
+}
+
 /* Generate BTF from relocation information previously recorded */
 static struct btf *btfgen_get_btf(struct btfgen_info *info)
 {
-	return ERR_PTR(-EOPNOTSUPP);
+	struct hashmap_entry *entry;
+	struct btf *btf_new = NULL;
+	struct hashmap *ids = NULL;
+	size_t bkt;
+	int err = 0;
+
+	btf_new = btf__new_empty();
+	if (libbpf_get_error(btf_new))
+		goto err_out;
+
+	ids = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
+	if (IS_ERR(ids)) {
+		errno = -PTR_ERR(ids);
+		goto err_out;
+	}
+
+	/* first pass: add all types and add their new ids to the ids map */
+	hashmap__for_each_entry(info->types, entry, bkt) {
+		struct btfgen_type *btfgen_type = entry->value;
+		struct btf_type *btf_type = btfgen_type->type;
+		int new_id;
+
+		/* we're adding BTF_KIND_VOID to the list but it can't be added to
+		 * the generated BTF object, hence we skip it here.
+		 */
+		if (btfgen_type->id == 0)
+			continue;
+
+		/* add members for struct and union */
+		if (btf_is_struct(btf_type) || btf_is_union(btf_type)) {
+			struct hashmap_entry *member_entry;
+			struct btf_type *btf_type_cpy;
+			int nmembers, index;
+			size_t new_size;
+
+			nmembers = btfgen_type->members ? hashmap__size(btfgen_type->members) : 0;
+			new_size = sizeof(struct btf_type) + nmembers * sizeof(struct btf_member);
+
+			btf_type_cpy = malloc(new_size);
+			if (!btf_type_cpy)
+				goto err_out;
+
+			/* copy header */
+			memcpy(btf_type_cpy, btf_type, sizeof(*btf_type_cpy));
+
+			/* copy only members that are needed */
+			index = 0;
+			if (nmembers > 0) {
+				size_t bkt2;
+
+				hashmap__for_each_entry(btfgen_type->members, member_entry, bkt2) {
+					struct btfgen_member *btfgen_member;
+					struct btf_member *btf_member;
+
+					btfgen_member = member_entry->value;
+					btf_member = btf_members(btf_type) + btfgen_member->idx;
+
+					memcpy(btf_members(btf_type_cpy) + index, btf_member,
+					       sizeof(struct btf_member));
+
+					index++;
+				}
+			}
+
+			/* set new vlen */
+			btf_type_cpy->info = btf_type_info(btf_kind(btf_type_cpy), nmembers,
+							   btf_kflag(btf_type_cpy));
+
+			err = btf__add_type(btf_new, info->src_btf, btf_type_cpy);
+			free(btf_type_cpy);
+		} else {
+			err = btf__add_type(btf_new, info->src_btf, btf_type);
+		}
+
+		if (err < 0)
+			goto err_out;
+
+		new_id = err;
+
+		/* add ID mapping */
+		err = btfgen_add_id(ids, btfgen_type->id, new_id);
+		if (err)
+			goto err_out;
+	}
+
+	/* second pass: fix up type ids */
+	for (unsigned int i = 0; i < btf__type_cnt(btf_new); i++) {
+		struct btf_member *btf_member;
+		struct btf_type *btf_type;
+		struct btf_param *params;
+		struct btf_array *array;
+
+		btf_type = (struct btf_type *) btf__type_by_id(btf_new, i);
+
+		switch (btf_kind(btf_type)) {
+		case BTF_KIND_STRUCT:
+		case BTF_KIND_UNION:
+			for (unsigned short j = 0; j < btf_vlen(btf_type); j++) {
+				btf_member = btf_members(btf_type) + j;
+				btf_member->type = btfgen_get_id(ids, btf_member->type);
+			}
+			break;
+		case BTF_KIND_PTR:
+		case BTF_KIND_TYPEDEF:
+		case BTF_KIND_VOLATILE:
+		case BTF_KIND_CONST:
+		case BTF_KIND_RESTRICT:
+		case BTF_KIND_FUNC:
+		case BTF_KIND_VAR:
+			btf_type->type = btfgen_get_id(ids, btf_type->type);
+			break;
+		case BTF_KIND_ARRAY:
+			array = btf_array(btf_type);
+			array->index_type = btfgen_get_id(ids, array->index_type);
+			array->type = btfgen_get_id(ids, array->type);
+			break;
+		case BTF_KIND_FUNC_PROTO:
+			btf_type->type = btfgen_get_id(ids, btf_type->type);
+			params = btf_params(btf_type);
+			for (unsigned short j = 0; j < btf_vlen(btf_type); j++)
+				params[j].type = btfgen_get_id(ids, params[j].type);
+			break;
+		default:
+			break;
+		}
+	}
+
+	hashmap__free(ids);
+	return btf_new;
+
+err_out:
+	btf__free(btf_new);
+	hashmap__free(ids);
+	return NULL;
 }
 
 /* Create BTF file for a set of BPF objects.
-- 
2.25.1

Re: [PATCH bpf-next v5 7/9] bpftool: Implement btfgen_get_btf()

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> The last part of the BTFGen algorithm is to create a new BTF object with
> all the types that were recorded in the previous steps.
>
> This function performs two different steps:
> 1. Add the types to the new BTF object by using btf__add_type(). Some
> special logic around struct and unions is implemented to only add the
> members that are really used in the field-based relocations. The type
> ID on the new and old BTF objects is stored on a map.
> 2. Fix all the type IDs on the new BTF object by using the IDs saved in
> the previous step.
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/gen.c | 158 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 7413ec808a80..55e6f640cbbb 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1599,10 +1599,166 @@ static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
>         return err;
>  }
>
> +static unsigned int btfgen_get_id(struct hashmap *ids, unsigned int old)
> +{
> +       uintptr_t new = 0;
> +
> +       /* deal with BTF_KIND_VOID */
> +       if (old == 0)
> +               return 0;
> +
> +       if (!hashmap__find(ids, uint_as_hash_key(old), (void **)&new)) {
> +               /* return id for void as it's possible that the ID we're looking for is
> +                * the type of a pointer that we're not adding.
> +                */
> +               return 0;
> +       }
> +
> +       return (unsigned int)(uintptr_t)new;
> +}
> +
> +static int btfgen_add_id(struct hashmap *ids, unsigned int old, unsigned int new)
> +{
> +       return hashmap__add(ids, uint_as_hash_key(old), uint_as_hash_key(new));
> +}
> +
>  /* Generate BTF from relocation information previously recorded */
>  static struct btf *btfgen_get_btf(struct btfgen_info *info)
>  {
> -       return ERR_PTR(-EOPNOTSUPP);
> +       struct hashmap_entry *entry;
> +       struct btf *btf_new = NULL;
> +       struct hashmap *ids = NULL;
> +       size_t bkt;
> +       int err = 0;
> +
> +       btf_new = btf__new_empty();
> +       if (libbpf_get_error(btf_new))
> +               goto err_out;
> +
> +       ids = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> +       if (IS_ERR(ids)) {
> +               errno = -PTR_ERR(ids);
> +               goto err_out;
> +       }
> +
> +       /* first pass: add all types and add their new ids to the ids map */
> +       hashmap__for_each_entry(info->types, entry, bkt) {
> +               struct btfgen_type *btfgen_type = entry->value;
> +               struct btf_type *btf_type = btfgen_type->type;
> +               int new_id;
> +
> +               /* we're adding BTF_KIND_VOID to the list but it can't be added to
> +                * the generated BTF object, hence we skip it here.
> +                */
> +               if (btfgen_type->id == 0)
> +                       continue;
> +
> +               /* add members for struct and union */
> +               if (btf_is_struct(btf_type) || btf_is_union(btf_type)) {
> +                       struct hashmap_entry *member_entry;
> +                       struct btf_type *btf_type_cpy;
> +                       int nmembers, index;
> +                       size_t new_size;
> +
> +                       nmembers = btfgen_type->members ? hashmap__size(btfgen_type->members) : 0;
> +                       new_size = sizeof(struct btf_type) + nmembers * sizeof(struct btf_member);
> +
> +                       btf_type_cpy = malloc(new_size);
> +                       if (!btf_type_cpy)
> +                               goto err_out;
> +
> +                       /* copy header */
> +                       memcpy(btf_type_cpy, btf_type, sizeof(*btf_type_cpy));
> +
> +                       /* copy only members that are needed */
> +                       index = 0;
> +                       if (nmembers > 0) {
> +                               size_t bkt2;
> +
> +                               hashmap__for_each_entry(btfgen_type->members, member_entry, bkt2) {
> +                                       struct btfgen_member *btfgen_member;
> +                                       struct btf_member *btf_member;
> +
> +                                       btfgen_member = member_entry->value;
> +                                       btf_member = btf_members(btf_type) + btfgen_member->idx;
> +
> +                                       memcpy(btf_members(btf_type_cpy) + index, btf_member,
> +                                              sizeof(struct btf_member));

you know that you are working with btf_type and btf_member, each have
just a few well known fields, why memcpy instead of just setting each
field individually? I think that would make code much easier to follow
and understand what transformations it's doing (and what it doesn't do
either).

> +
> +                                       index++;
> +                               }
> +                       }
> +
> +                       /* set new vlen */
> +                       btf_type_cpy->info = btf_type_info(btf_kind(btf_type_cpy), nmembers,
> +                                                          btf_kflag(btf_type_cpy));
> +
> +                       err = btf__add_type(btf_new, info->src_btf, btf_type_cpy);
> +                       free(btf_type_cpy);
> +               } else {
> +                       err = btf__add_type(btf_new, info->src_btf, btf_type);
> +               }
> +
> +               if (err < 0)
> +                       goto err_out;
> +
> +               new_id = err;
> +
> +               /* add ID mapping */
> +               err = btfgen_add_id(ids, btfgen_type->id, new_id);
> +               if (err)
> +                       goto err_out;
> +       }
> +
> +       /* second pass: fix up type ids */
> +       for (unsigned int i = 0; i < btf__type_cnt(btf_new); i++) {
> +               struct btf_member *btf_member;
> +               struct btf_type *btf_type;
> +               struct btf_param *params;
> +               struct btf_array *array;
> +
> +               btf_type = (struct btf_type *) btf__type_by_id(btf_new, i);
> +
> +               switch (btf_kind(btf_type)) {
> +               case BTF_KIND_STRUCT:
> +               case BTF_KIND_UNION:
> +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++) {
> +                               btf_member = btf_members(btf_type) + j;
> +                               btf_member->type = btfgen_get_id(ids, btf_member->type);
> +                       }
> +                       break;
> +               case BTF_KIND_PTR:
> +               case BTF_KIND_TYPEDEF:
> +               case BTF_KIND_VOLATILE:
> +               case BTF_KIND_CONST:
> +               case BTF_KIND_RESTRICT:
> +               case BTF_KIND_FUNC:
> +               case BTF_KIND_VAR:
> +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> +                       break;
> +               case BTF_KIND_ARRAY:
> +                       array = btf_array(btf_type);
> +                       array->index_type = btfgen_get_id(ids, array->index_type);
> +                       array->type = btfgen_get_id(ids, array->type);
> +                       break;
> +               case BTF_KIND_FUNC_PROTO:
> +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> +                       params = btf_params(btf_type);
> +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++)
> +                               params[j].type = btfgen_get_id(ids, params[j].type);
> +                       break;

did you try using btf_type_visit_type_ids() instead of this entire loop?

> +               default:
> +                       break;
> +               }
> +       }
> +
> +       hashmap__free(ids);
> +       return btf_new;
> +
> +err_out:
> +       btf__free(btf_new);
> +       hashmap__free(ids);
> +       return NULL;
>  }
>
>  /* Create BTF file for a set of BPF objects.
> --
> 2.25.1
>

Re: [PATCH bpf-next v5 7/9] bpftool: Implement btfgen_get_btf()

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 2:37 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > The last part of the BTFGen algorithm is to create a new BTF object with
> > all the types that were recorded in the previous steps.
> >
> > This function performs two different steps:
> > 1. Add the types to the new BTF object by using btf__add_type(). Some
> > special logic around struct and unions is implemented to only add the
> > members that are really used in the field-based relocations. The type
> > ID on the new and old BTF objects is stored on a map.
> > 2. Fix all the type IDs on the new BTF object by using the IDs saved in
> > the previous step.
> >
> > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > ---
> >  tools/bpf/bpftool/gen.c | 158 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 157 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 7413ec808a80..55e6f640cbbb 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1599,10 +1599,166 @@ static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
> >         return err;
> >  }
> >
> > +static unsigned int btfgen_get_id(struct hashmap *ids, unsigned int old)
> > +{
> > +       uintptr_t new = 0;
> > +
> > +       /* deal with BTF_KIND_VOID */
> > +       if (old == 0)
> > +               return 0;
> > +
> > +       if (!hashmap__find(ids, uint_as_hash_key(old), (void **)&new)) {
> > +               /* return id for void as it's possible that the ID we're looking for is
> > +                * the type of a pointer that we're not adding.
> > +                */
> > +               return 0;
> > +       }
> > +
> > +       return (unsigned int)(uintptr_t)new;
> > +}
> > +
> > +static int btfgen_add_id(struct hashmap *ids, unsigned int old, unsigned int new)
> > +{
> > +       return hashmap__add(ids, uint_as_hash_key(old), uint_as_hash_key(new));
> > +}
> > +
> >  /* Generate BTF from relocation information previously recorded */
> >  static struct btf *btfgen_get_btf(struct btfgen_info *info)
> >  {
> > -       return ERR_PTR(-EOPNOTSUPP);
> > +       struct hashmap_entry *entry;
> > +       struct btf *btf_new = NULL;
> > +       struct hashmap *ids = NULL;
> > +       size_t bkt;
> > +       int err = 0;
> > +
> > +       btf_new = btf__new_empty();
> > +       if (libbpf_get_error(btf_new))
> > +               goto err_out;
> > +
> > +       ids = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > +       if (IS_ERR(ids)) {
> > +               errno = -PTR_ERR(ids);
> > +               goto err_out;
> > +       }
> > +
> > +       /* first pass: add all types and add their new ids to the ids map */
> > +       hashmap__for_each_entry(info->types, entry, bkt) {
> > +               struct btfgen_type *btfgen_type = entry->value;
> > +               struct btf_type *btf_type = btfgen_type->type;
> > +               int new_id;
> > +
> > +               /* we're adding BTF_KIND_VOID to the list but it can't be added to
> > +                * the generated BTF object, hence we skip it here.
> > +                */
> > +               if (btfgen_type->id == 0)
> > +                       continue;
> > +
> > +               /* add members for struct and union */
> > +               if (btf_is_struct(btf_type) || btf_is_union(btf_type)) {
> > +                       struct hashmap_entry *member_entry;
> > +                       struct btf_type *btf_type_cpy;
> > +                       int nmembers, index;
> > +                       size_t new_size;
> > +
> > +                       nmembers = btfgen_type->members ? hashmap__size(btfgen_type->members) : 0;
> > +                       new_size = sizeof(struct btf_type) + nmembers * sizeof(struct btf_member);
> > +
> > +                       btf_type_cpy = malloc(new_size);
> > +                       if (!btf_type_cpy)
> > +                               goto err_out;
> > +
> > +                       /* copy header */
> > +                       memcpy(btf_type_cpy, btf_type, sizeof(*btf_type_cpy));
> > +
> > +                       /* copy only members that are needed */
> > +                       index = 0;
> > +                       if (nmembers > 0) {
> > +                               size_t bkt2;
> > +
> > +                               hashmap__for_each_entry(btfgen_type->members, member_entry, bkt2) {
> > +                                       struct btfgen_member *btfgen_member;
> > +                                       struct btf_member *btf_member;
> > +
> > +                                       btfgen_member = member_entry->value;
> > +                                       btf_member = btf_members(btf_type) + btfgen_member->idx;
> > +
> > +                                       memcpy(btf_members(btf_type_cpy) + index, btf_member,
> > +                                              sizeof(struct btf_member));
>
> you know that you are working with btf_type and btf_member, each have
> just a few well known fields, why memcpy instead of just setting each
> field individually? I think that would make code much easier to follow
> and understand what transformations it's doing (and what it doesn't do
> either).
>

Removing those memcpy() helps a lot! Do you think doing a structure
assignment would be enough? Or do you prefer to copy field by field?

> > +
> > +                                       index++;
> > +                               }
> > +                       }
> > +
> > +                       /* set new vlen */
> > +                       btf_type_cpy->info = btf_type_info(btf_kind(btf_type_cpy), nmembers,
> > +                                                          btf_kflag(btf_type_cpy));
> > +
> > +                       err = btf__add_type(btf_new, info->src_btf, btf_type_cpy);
> > +                       free(btf_type_cpy);
> > +               } else {
> > +                       err = btf__add_type(btf_new, info->src_btf, btf_type);
> > +               }
> > +
> > +               if (err < 0)
> > +                       goto err_out;
> > +
> > +               new_id = err;
> > +
> > +               /* add ID mapping */
> > +               err = btfgen_add_id(ids, btfgen_type->id, new_id);
> > +               if (err)
> > +                       goto err_out;
> > +       }
> > +
> > +       /* second pass: fix up type ids */
> > +       for (unsigned int i = 0; i < btf__type_cnt(btf_new); i++) {
> > +               struct btf_member *btf_member;
> > +               struct btf_type *btf_type;
> > +               struct btf_param *params;
> > +               struct btf_array *array;
> > +
> > +               btf_type = (struct btf_type *) btf__type_by_id(btf_new, i);
> > +
> > +               switch (btf_kind(btf_type)) {
> > +               case BTF_KIND_STRUCT:
> > +               case BTF_KIND_UNION:
> > +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++) {
> > +                               btf_member = btf_members(btf_type) + j;
> > +                               btf_member->type = btfgen_get_id(ids, btf_member->type);
> > +                       }
> > +                       break;
> > +               case BTF_KIND_PTR:
> > +               case BTF_KIND_TYPEDEF:
> > +               case BTF_KIND_VOLATILE:
> > +               case BTF_KIND_CONST:
> > +               case BTF_KIND_RESTRICT:
> > +               case BTF_KIND_FUNC:
> > +               case BTF_KIND_VAR:
> > +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> > +                       break;
> > +               case BTF_KIND_ARRAY:
> > +                       array = btf_array(btf_type);
> > +                       array->index_type = btfgen_get_id(ids, array->index_type);
> > +                       array->type = btfgen_get_id(ids, array->type);
> > +                       break;
> > +               case BTF_KIND_FUNC_PROTO:
> > +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> > +                       params = btf_params(btf_type);
> > +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++)
> > +                               params[j].type = btfgen_get_id(ids, params[j].type);
> > +                       break;
>
> did you try using btf_type_visit_type_ids() instead of this entire loop?
>

ah, that helped a lot, thanks!

> > +               default:
> > +                       break;
> > +               }
> > +       }
> > +
> > +       hashmap__free(ids);
> > +       return btf_new;
> > +
> > +err_out:
> > +       btf__free(btf_new);
> > +       hashmap__free(ids);
> > +       return NULL;
> >  }
> >
> >  /* Create BTF file for a set of BPF objects.
> > --
> > 2.25.1
> >

Re: [PATCH bpf-next v5 7/9] bpftool: Implement btfgen_get_btf()

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 8:10 AM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 2:37 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > The last part of the BTFGen algorithm is to create a new BTF object with
> > > all the types that were recorded in the previous steps.
> > >
> > > This function performs two different steps:
> > > 1. Add the types to the new BTF object by using btf__add_type(). Some
> > > special logic around struct and unions is implemented to only add the
> > > members that are really used in the field-based relocations. The type
> > > ID on the new and old BTF objects is stored on a map.
> > > 2. Fix all the type IDs on the new BTF object by using the IDs saved in
> > > the previous step.
> > >
> > > Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> > > Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> > > Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> > > Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> > > ---
> > >  tools/bpf/bpftool/gen.c | 158 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 157 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > > index 7413ec808a80..55e6f640cbbb 100644
> > > --- a/tools/bpf/bpftool/gen.c
> > > +++ b/tools/bpf/bpftool/gen.c
> > > @@ -1599,10 +1599,166 @@ static int btfgen_record_obj(struct btfgen_info *info, const char *obj_path)
> > >         return err;
> > >  }
> > >
> > > +static unsigned int btfgen_get_id(struct hashmap *ids, unsigned int old)
> > > +{
> > > +       uintptr_t new = 0;
> > > +
> > > +       /* deal with BTF_KIND_VOID */
> > > +       if (old == 0)
> > > +               return 0;
> > > +
> > > +       if (!hashmap__find(ids, uint_as_hash_key(old), (void **)&new)) {
> > > +               /* return id for void as it's possible that the ID we're looking for is
> > > +                * the type of a pointer that we're not adding.
> > > +                */
> > > +               return 0;
> > > +       }
> > > +
> > > +       return (unsigned int)(uintptr_t)new;
> > > +}
> > > +
> > > +static int btfgen_add_id(struct hashmap *ids, unsigned int old, unsigned int new)
> > > +{
> > > +       return hashmap__add(ids, uint_as_hash_key(old), uint_as_hash_key(new));
> > > +}
> > > +
> > >  /* Generate BTF from relocation information previously recorded */
> > >  static struct btf *btfgen_get_btf(struct btfgen_info *info)
> > >  {
> > > -       return ERR_PTR(-EOPNOTSUPP);
> > > +       struct hashmap_entry *entry;
> > > +       struct btf *btf_new = NULL;
> > > +       struct hashmap *ids = NULL;
> > > +       size_t bkt;
> > > +       int err = 0;
> > > +
> > > +       btf_new = btf__new_empty();
> > > +       if (libbpf_get_error(btf_new))
> > > +               goto err_out;
> > > +
> > > +       ids = hashmap__new(btfgen_hash_fn, btfgen_equal_fn, NULL);
> > > +       if (IS_ERR(ids)) {
> > > +               errno = -PTR_ERR(ids);
> > > +               goto err_out;
> > > +       }
> > > +
> > > +       /* first pass: add all types and add their new ids to the ids map */
> > > +       hashmap__for_each_entry(info->types, entry, bkt) {
> > > +               struct btfgen_type *btfgen_type = entry->value;
> > > +               struct btf_type *btf_type = btfgen_type->type;
> > > +               int new_id;
> > > +
> > > +               /* we're adding BTF_KIND_VOID to the list but it can't be added to
> > > +                * the generated BTF object, hence we skip it here.
> > > +                */
> > > +               if (btfgen_type->id == 0)
> > > +                       continue;
> > > +
> > > +               /* add members for struct and union */
> > > +               if (btf_is_struct(btf_type) || btf_is_union(btf_type)) {
> > > +                       struct hashmap_entry *member_entry;
> > > +                       struct btf_type *btf_type_cpy;
> > > +                       int nmembers, index;
> > > +                       size_t new_size;
> > > +
> > > +                       nmembers = btfgen_type->members ? hashmap__size(btfgen_type->members) : 0;
> > > +                       new_size = sizeof(struct btf_type) + nmembers * sizeof(struct btf_member);
> > > +
> > > +                       btf_type_cpy = malloc(new_size);
> > > +                       if (!btf_type_cpy)
> > > +                               goto err_out;
> > > +
> > > +                       /* copy header */
> > > +                       memcpy(btf_type_cpy, btf_type, sizeof(*btf_type_cpy));
> > > +
> > > +                       /* copy only members that are needed */
> > > +                       index = 0;
> > > +                       if (nmembers > 0) {
> > > +                               size_t bkt2;
> > > +
> > > +                               hashmap__for_each_entry(btfgen_type->members, member_entry, bkt2) {
> > > +                                       struct btfgen_member *btfgen_member;
> > > +                                       struct btf_member *btf_member;
> > > +
> > > +                                       btfgen_member = member_entry->value;
> > > +                                       btf_member = btf_members(btf_type) + btfgen_member->idx;
> > > +
> > > +                                       memcpy(btf_members(btf_type_cpy) + index, btf_member,
> > > +                                              sizeof(struct btf_member));
> >
> > you know that you are working with btf_type and btf_member, each have
> > just a few well known fields, why memcpy instead of just setting each
> > field individually? I think that would make code much easier to follow
> > and understand what transformations it's doing (and what it doesn't do
> > either).
> >
>
> Removing those memcpy() helps a lot! Do you think doing a structure
> assignment would be enough? Or do you prefer to copy field by field?
>

I think I'd prefer explicit field assignments, it's more obvious, but
yes, copying struct would work as well.

> > > +
> > > +                                       index++;
> > > +                               }
> > > +                       }
> > > +
> > > +                       /* set new vlen */
> > > +                       btf_type_cpy->info = btf_type_info(btf_kind(btf_type_cpy), nmembers,
> > > +                                                          btf_kflag(btf_type_cpy));
> > > +
> > > +                       err = btf__add_type(btf_new, info->src_btf, btf_type_cpy);
> > > +                       free(btf_type_cpy);
> > > +               } else {
> > > +                       err = btf__add_type(btf_new, info->src_btf, btf_type);
> > > +               }
> > > +
> > > +               if (err < 0)
> > > +                       goto err_out;
> > > +
> > > +               new_id = err;
> > > +
> > > +               /* add ID mapping */
> > > +               err = btfgen_add_id(ids, btfgen_type->id, new_id);
> > > +               if (err)
> > > +                       goto err_out;
> > > +       }
> > > +
> > > +       /* second pass: fix up type ids */
> > > +       for (unsigned int i = 0; i < btf__type_cnt(btf_new); i++) {
> > > +               struct btf_member *btf_member;
> > > +               struct btf_type *btf_type;
> > > +               struct btf_param *params;
> > > +               struct btf_array *array;
> > > +
> > > +               btf_type = (struct btf_type *) btf__type_by_id(btf_new, i);
> > > +
> > > +               switch (btf_kind(btf_type)) {
> > > +               case BTF_KIND_STRUCT:
> > > +               case BTF_KIND_UNION:
> > > +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++) {
> > > +                               btf_member = btf_members(btf_type) + j;
> > > +                               btf_member->type = btfgen_get_id(ids, btf_member->type);
> > > +                       }
> > > +                       break;
> > > +               case BTF_KIND_PTR:
> > > +               case BTF_KIND_TYPEDEF:
> > > +               case BTF_KIND_VOLATILE:
> > > +               case BTF_KIND_CONST:
> > > +               case BTF_KIND_RESTRICT:
> > > +               case BTF_KIND_FUNC:
> > > +               case BTF_KIND_VAR:
> > > +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> > > +                       break;
> > > +               case BTF_KIND_ARRAY:
> > > +                       array = btf_array(btf_type);
> > > +                       array->index_type = btfgen_get_id(ids, array->index_type);
> > > +                       array->type = btfgen_get_id(ids, array->type);
> > > +                       break;
> > > +               case BTF_KIND_FUNC_PROTO:
> > > +                       btf_type->type = btfgen_get_id(ids, btf_type->type);
> > > +                       params = btf_params(btf_type);
> > > +                       for (unsigned short j = 0; j < btf_vlen(btf_type); j++)
> > > +                               params[j].type = btfgen_get_id(ids, params[j].type);
> > > +                       break;
> >
> > did you try using btf_type_visit_type_ids() instead of this entire loop?
> >
>
> ah, that helped a lot, thanks!

great, you are welcome

>
> > > +               default:
> > > +                       break;
> > > +               }
> > > +       }
> > > +
> > > +       hashmap__free(ids);
> > > +       return btf_new;
> > > +
> > > +err_out:
> > > +       btf__free(btf_new);
> > > +       hashmap__free(ids);
> > > +       return NULL;
> > >  }
> > >
> > >  /* Create BTF file for a set of BPF objects.
> > > --
> > > 2.25.1
> > >

[PATCH bpf-next v5 8/9] bpftool: gen min_core_btf explanation and examples

[Mauricio Vásquez]

From: Rafael David Tinoco <rafaeldtinoco@gmail.com>

Add "min_core_btf" feature explanation and one example of how to use it
to bpftool-gen man page.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 .../bpf/bpftool/Documentation/bpftool-gen.rst | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
index bc276388f432..7aa3c29c2da0 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
@@ -25,6 +25,7 @@ GEN COMMANDS
 
 |	**bpftool** **gen object** *OUTPUT_FILE* *INPUT_FILE* [*INPUT_FILE*...]
 |	**bpftool** **gen skeleton** *FILE* [**name** *OBJECT_NAME*]
+|	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECTS(S)*
 |	**bpftool** **gen help**
 
 DESCRIPTION
@@ -149,6 +150,24 @@ DESCRIPTION
 		  (non-read-only) data from userspace, with same simplicity
 		  as for BPF side.
 
+	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECT(S)*
+		  Given one, or multiple, eBPF *OBJECT* files, generate a
+		  smaller BTF file, in the *OUTPUT* directory, to each existing
+		  BTF files in the *INPUT* directory, using the same name.
+
+		  If *INPUT* is a file, then the result BTF will be saved as a
+		  single file, with the same name, in *OUTPUT* directory.
+
+		  Generated BTF files will only contain the BTF types used by
+		  the given eBPF objects. Idea behind this is simple: Full
+		  external BTF files are big. This allows customized external
+		  BTF files generation and maximizes eBPF portability (CO-RE).
+
+		  This feature allows a particular eBPF project to embed
+		  customized BTF files in order to support older kernels,
+		  allowing code to be portable among kernels that don't support
+		  embedded BTF files but still support eBPF.
+
 	**bpftool gen help**
 		  Print short help message.
 
@@ -215,7 +234,9 @@ This is example BPF application with two BPF programs and a mix of BPF maps
 and global variables. Source code is split across two source code files.
 
 **$ clang -target bpf -g example1.bpf.c -o example1.bpf.o**
+
 **$ clang -target bpf -g example2.bpf.c -o example2.bpf.o**
+
 **$ bpftool gen object example.bpf.o example1.bpf.o example2.bpf.o**
 
 This set of commands compiles *example1.bpf.c* and *example2.bpf.c*
@@ -329,3 +350,67 @@ BPF ELF object file *example.bpf.o*.
   my_static_var: 7
 
 This is a stripped-out version of skeleton generated for above example code.
+
+*MIN_CORE_BTF*
+
+::
+
+  $ bpftool btf dump file ./input/5.4.0-91-generic.btf format raw
+
+  [1] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
+  [2] CONST '(anon)' type_id=1
+  [3] VOLATILE '(anon)' type_id=1
+  [4] ARRAY '(anon)' type_id=1 index_type_id=21 nr_elems=2
+  [5] PTR '(anon)' type_id=8
+  [6] CONST '(anon)' type_id=5
+  [7] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=(none)
+  [8] CONST '(anon)' type_id=7
+  [9] INT 'unsigned int' size=4 bits_offset=0 nr_bits=32 encoding=(none)
+  <long output>
+
+  $ bpftool btf dump file ./one.bpf.o format raw
+
+  [1] PTR '(anon)' type_id=2
+  [2] STRUCT 'trace_event_raw_sys_enter' size=64 vlen=4
+        'ent' type_id=3 bits_offset=0
+        'id' type_id=7 bits_offset=64
+        'args' type_id=9 bits_offset=128
+        '__data' type_id=12 bits_offset=512
+  [3] STRUCT 'trace_entry' size=8 vlen=4
+        'type' type_id=4 bits_offset=0
+        'flags' type_id=5 bits_offset=16
+        'preempt_count' type_id=5 bits_offset=24
+  <long output>
+
+  $ bpftool gen min_core_btf ./input/ ./output ./one.bpf.o
+
+  $ bpftool btf dump file ./output/5.4.0-91-generic.btf format raw
+
+  [1] TYPEDEF 'pid_t' type_id=6
+  [2] STRUCT 'trace_event_raw_sys_enter' size=64 vlen=1
+        'args' type_id=4 bits_offset=128
+  [3] STRUCT 'task_struct' size=9216 vlen=2
+        'pid' type_id=1 bits_offset=17920
+        'real_parent' type_id=7 bits_offset=18048
+  [4] ARRAY '(anon)' type_id=5 index_type_id=8 nr_elems=6
+  [5] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
+  [6] TYPEDEF '__kernel_pid_t' type_id=8
+  [7] PTR '(anon)' type_id=3
+  [8] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
+  <end>
+
+  Now, one may use ./output/5.4.0-91-generic.btf generated file as an external
+  BTF file fed to libbpf during eBPF object opening:
+
+  struct bpf_object *obj = NULL;
+  struct bpf_object_open_opts openopts = {};
+
+  openopts.sz = sizeof(struct bpf_object_open_opts);
+  openopts.btf_custom_path = strdup("./output/5.4.0-91-generic.btf");
+
+  obj = bpf_object__open_file("./one.bpf.o", &openopts);
+
+  ...
+
+  and allow libbpf to do all needed CO-RE relocations, to "one.bpf.o" object,
+  based on the small external BTF file.
-- 
2.25.1

Re: [PATCH bpf-next v5 8/9] bpftool: gen min_core_btf explanation and examples

[Quentin Monnet]

2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> From: Rafael David Tinoco <rafaeldtinoco@gmail.com>
> 
> Add "min_core_btf" feature explanation and one example of how to use it
> to bpftool-gen man page.
> 
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>

Thanks for this, and for the bash completion!

> ---
>  .../bpf/bpftool/Documentation/bpftool-gen.rst | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> index bc276388f432..7aa3c29c2da0 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
> @@ -25,6 +25,7 @@ GEN COMMANDS
>  
>  |	**bpftool** **gen object** *OUTPUT_FILE* *INPUT_FILE* [*INPUT_FILE*...]
>  |	**bpftool** **gen skeleton** *FILE* [**name** *OBJECT_NAME*]
> +|	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECTS(S)*
>  |	**bpftool** **gen help**
>  
>  DESCRIPTION
> @@ -149,6 +150,24 @@ DESCRIPTION
>  		  (non-read-only) data from userspace, with same simplicity
>  		  as for BPF side.
>  
> +	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECT(S)*
> +		  Given one, or multiple, eBPF *OBJECT* files, generate a
> +		  smaller BTF file, in the *OUTPUT* directory, to each existing

“directory” -> “directory or file”?

> +		  BTF files in the *INPUT* directory, using the same name.

(same)

Regarding INPUT/OUTPUT I would try to remain generic in the first
paragraph, and explodes below into more cases, like you did in the
following paragraph.

“If *INPUT* is a file...”

“If *INPUT* is a directory...”

Also for OUTPUT (I don't think this doc mentions that OUTPUT can be a
simple file, by the way).

> +
> +		  If *INPUT* is a file, then the result BTF will be saved as a
> +		  single file, with the same name, in *OUTPUT* directory.
> +
> +		  Generated BTF files will only contain the BTF types used by
> +		  the given eBPF objects.

Suggestion: I'd split here and merge the last two paragraphs on the
motivations. I would also reword them a little (“Full external BTF files
are big”: How comes? Users may not know they have all kernel symbols.
“This [...] maximized eBPF portability (CO-RE)”: What does this mean?).
Try to keep in mind that the readers for the man page are people trying
to understand how bpftool work and who barely have any idea what BTF is.

> +               Idea behind this is simple: Full
> +		  external BTF files are big. This allows customized external
> +		  BTF files generation and maximizes eBPF portability (CO-RE).
> +
> +		  This feature allows a particular eBPF project to embed
> +		  customized BTF files in order to support older kernels,
> +		  allowing code to be portable among kernels that don't support
> +		  embedded BTF files but still support eBPF.
> +
>  	**bpftool gen help**
>  		  Print short help message.
>  
> @@ -215,7 +234,9 @@ This is example BPF application with two BPF programs and a mix of BPF maps
>  and global variables. Source code is split across two source code files.
>  
>  **$ clang -target bpf -g example1.bpf.c -o example1.bpf.o**
> +
>  **$ clang -target bpf -g example2.bpf.c -o example2.bpf.o**
> +
>  **$ bpftool gen object example.bpf.o example1.bpf.o example2.bpf.o**

Good catch, thank you

>  
>  This set of commands compiles *example1.bpf.c* and *example2.bpf.c*
> @@ -329,3 +350,67 @@ BPF ELF object file *example.bpf.o*.
>    my_static_var: 7
>  
>  This is a stripped-out version of skeleton generated for above example code.
> +
> +*MIN_CORE_BTF*
> +
> +::
> +

And thanks for the example!

Note that you place everything into one literal block; this does not
follow the syntax of the rest of the document. I understand you want to
group the different commands related to min_core_btf together. To do
that, I'd suggest adding a subsection in the document:

min_core_btf
------------

**$ bpftool btf dump file ./... format raw**

::

  [1] INT ...
  [2] ...

> +  $ bpftool btf dump file ./input/5.4.0-91-generic.btf format raw
> +
> +  [1] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
> +  [2] CONST '(anon)' type_id=1
> +  [3] VOLATILE '(anon)' type_id=1
> +  [4] ARRAY '(anon)' type_id=1 index_type_id=21 nr_elems=2
> +  [5] PTR '(anon)' type_id=8
> +  [6] CONST '(anon)' type_id=5
> +  [7] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=(none)
> +  [8] CONST '(anon)' type_id=7
> +  [9] INT 'unsigned int' size=4 bits_offset=0 nr_bits=32 encoding=(none)
> +  <long output>
> +
> +  $ bpftool btf dump file ./one.bpf.o format raw
> +
> +  [1] PTR '(anon)' type_id=2
> +  [2] STRUCT 'trace_event_raw_sys_enter' size=64 vlen=4
> +        'ent' type_id=3 bits_offset=0
> +        'id' type_id=7 bits_offset=64
> +        'args' type_id=9 bits_offset=128
> +        '__data' type_id=12 bits_offset=512
> +  [3] STRUCT 'trace_entry' size=8 vlen=4
> +        'type' type_id=4 bits_offset=0
> +        'flags' type_id=5 bits_offset=16
> +        'preempt_count' type_id=5 bits_offset=24
> +  <long output>
> +
> +  $ bpftool gen min_core_btf ./input/ ./output ./one.bpf.o
> +
> +  $ bpftool btf dump file ./output/5.4.0-91-generic.btf format raw
> +
> +  [1] TYPEDEF 'pid_t' type_id=6
> +  [2] STRUCT 'trace_event_raw_sys_enter' size=64 vlen=1
> +        'args' type_id=4 bits_offset=128
> +  [3] STRUCT 'task_struct' size=9216 vlen=2
> +        'pid' type_id=1 bits_offset=17920
> +        'real_parent' type_id=7 bits_offset=18048
> +  [4] ARRAY '(anon)' type_id=5 index_type_id=8 nr_elems=6
> +  [5] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
> +  [6] TYPEDEF '__kernel_pid_t' type_id=8
> +  [7] PTR '(anon)' type_id=3
> +  [8] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> +  <end>
> +
> +  Now, one may use ./output/5.4.0-91-generic.btf generated file as an external
> +  BTF file fed to libbpf during eBPF object opening:
> +
> +  struct bpf_object *obj = NULL;
> +  struct bpf_object_open_opts openopts = {};
> +
> +  openopts.sz = sizeof(struct bpf_object_open_opts);
> +  openopts.btf_custom_path = strdup("./output/5.4.0-91-generic.btf");
> +
> +  obj = bpf_object__open_file("./one.bpf.o", &openopts);
> +
> +  ...
> +
> +  and allow libbpf to do all needed CO-RE relocations, to "one.bpf.o" object,
> +  based on the small external BTF file.

[PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Mauricio Vásquez]

This commit implements some integration tests for "BTFGen". The goal
of such tests is to verify that the generated BTF file contains the
expected types.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   4 +-
 .../selftests/bpf/progs/btfgen_btf_source.c   |  12 +
 .../bpf/progs/btfgen_primitives_array.c       |  39 +++
 .../bpf/progs/btfgen_primitives_struct.c      |  40 +++
 .../bpf/progs/btfgen_primitives_struct2.c     |  44 ++++
 .../bpf/progs/btfgen_primitives_union.c       |  32 +++
 tools/testing/selftests/bpf/test_bpftool.c    | 228 ++++++++++++++++++
 8 files changed, 399 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_btf_source.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_array.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c
 create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_union.c
 create mode 100644 tools/testing/selftests/bpf/test_bpftool.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 1dad8d617da8..308cd5b9cfc4 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -41,3 +41,4 @@ test_cpp
 *.tmp
 xdpxceiver
 xdp_redirect_multi
+test_bpftool
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 945f92d71db3..afc9bff6545d 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -38,7 +38,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_sock test_sockmap get_cgroup_id_user \
 	test_cgroup_storage \
 	test_tcpnotify_user test_sysctl \
-	test_progs-no_alu32
+	test_progs-no_alu32 \
+	test_bpftool
 
 # Also test bpf-gcc, if present
 ifneq ($(BPF_GCC),)
@@ -212,6 +213,7 @@ $(OUTPUT)/xdping: $(TESTING_HELPERS)
 $(OUTPUT)/flow_dissector_load: $(TESTING_HELPERS)
 $(OUTPUT)/test_maps: $(TESTING_HELPERS)
 $(OUTPUT)/test_verifier: $(TESTING_HELPERS)
+$(OUTPUT)/test_bpftool:
 
 BPFTOOL ?= $(DEFAULT_BPFTOOL)
 $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
diff --git a/tools/testing/selftests/bpf/progs/btfgen_btf_source.c b/tools/testing/selftests/bpf/progs/btfgen_btf_source.c
new file mode 100644
index 000000000000..772d5d1e2788
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btfgen_btf_source.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "core_reloc_types.h"
+
+// structure made of primitive types
+void f0(struct core_reloc_primitives x) {}
+
+// union made of primite types
+void f1(union a_union x) {}
+
+// arrays
+void f2(struct core_reloc_arrays x) {}
diff --git a/tools/testing/selftests/bpf/progs/btfgen_primitives_array.c b/tools/testing/selftests/bpf/progs/btfgen_primitives_array.c
new file mode 100644
index 000000000000..aa514d1bcd96
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btfgen_primitives_array.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <stdint.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	char in[256];
+	char out[256];
+} data = {};
+
+struct core_reloc_arrays_substruct {
+	int c;
+	int d;
+};
+
+struct core_reloc_arrays {
+	int a[5];
+	char b[2][3][4];
+	struct core_reloc_arrays_substruct c[3];
+	struct core_reloc_arrays_substruct d[1][2];
+	struct core_reloc_arrays_substruct f[][2];
+};
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*(dst)), src)
+
+SEC("raw_tracepoint/sys_enter")
+int test_btfgen_primitives(void *ctx)
+{
+	struct core_reloc_arrays *in = (void *)&data.in;
+	struct core_reloc_arrays *out = (void *)&data.out;
+
+	if (CORE_READ(&out->a[0], &in->a[0]))
+		return 1;
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c b/tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c
new file mode 100644
index 000000000000..b761d020fa4d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <stdint.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	char in[256];
+	char out[256];
+} data = {};
+
+enum core_reloc_primitives_enum {
+	A = 0,
+	B = 1,
+};
+
+struct core_reloc_primitives {
+	char a;
+	int b;
+	enum core_reloc_primitives_enum c;
+	void *d;
+	int (*f)(const char *);
+};
+
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*(dst)), src)
+
+SEC("raw_tracepoint/sys_enter")
+int test_btfgen_primitives(void *ctx)
+{
+	struct core_reloc_primitives *in = (void *)&data.in;
+	struct core_reloc_primitives *out = (void *)&data.out;
+
+	if (CORE_READ(&out->a, &in->a))
+		return 1;
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c b/tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c
new file mode 100644
index 000000000000..fe67c9a24a7f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* This is almost the same as btfgen_primitives_struct.c but in this one
+ * a different field is accessed
+ */
+
+#include <linux/bpf.h>
+#include <stdint.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	char in[256];
+	char out[256];
+} data = {};
+
+enum core_reloc_primitives_enum {
+	A = 0,
+	B = 1,
+};
+
+struct core_reloc_primitives {
+	char a;
+	int b;
+	enum core_reloc_primitives_enum c;
+	void *d;
+	int (*f)(const char *);
+};
+
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*(dst)), src)
+
+SEC("raw_tracepoint/sys_enter")
+int test_btfgen_primitives(void *ctx)
+{
+	struct core_reloc_primitives *in = (void *)&data.in;
+	struct core_reloc_primitives *out = (void *)&data.out;
+
+	if (CORE_READ(&out->b, &in->b))
+		return 1;
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/btfgen_primitives_union.c b/tools/testing/selftests/bpf/progs/btfgen_primitives_union.c
new file mode 100644
index 000000000000..d4690c9e963c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/btfgen_primitives_union.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <stdint.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_core_read.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	char in[256];
+	char out[256];
+} data = {};
+
+union a_union {
+	int y;
+	int z;
+};
+
+#define CORE_READ(dst, src) bpf_core_read(dst, sizeof(*(dst)), src)
+
+SEC("raw_tracepoint/sys_enter")
+int test_btfgen_primitives(void *ctx)
+{
+	union a_union *in = (void *)&data.in;
+	union a_union *out = (void *)&data.out;
+
+	if (CORE_READ(&out->y, &in->y))
+		return 1;
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/test_bpftool.c b/tools/testing/selftests/bpf/test_bpftool.c
new file mode 100644
index 000000000000..ca7facc582d5
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_bpftool.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <linux/limits.h>
+
+#include <bpf/libbpf.h>
+#include <bpf/btf.h>
+
+#include "bpf_util.h"
+
+static int run_btfgen(const char *src_btf, const char *dst_btf, const char *objspaths[])
+{
+	char command[4096];
+	int ret, i, n;
+
+	n = snprintf(command, sizeof(command),
+		     "./tools/build/bpftool/bpftool gen min_core_btf %s %s", src_btf, dst_btf);
+	assert(n >= 0 && n < sizeof(command));
+
+	for (i = 0; objspaths[i] != NULL; i++) {
+		assert(sizeof(command) - strlen(command) > strlen(objspaths[i]) + 1);
+		strcat(command, " ");
+		strcat(command, objspaths[i]);
+	}
+
+	printf("Executing bpftool: %s\n", command);
+	printf("---\n");
+	ret = system(command);
+	printf("---\n");
+	return ret;
+}
+
+struct btfgen_test {
+	const char *descr;
+	const char *src_btf;
+	const char *bpfobj[16];
+	void (*run_test)(struct btf *btf);
+};
+
+static void check_btfgen_primitive_struct(struct btf *btf)
+{
+	struct btf_member *members;
+	const struct btf_type *t;
+	int id;
+
+	assert(btf__type_cnt(btf) == 3);
+
+	id = btf__find_by_name_kind(btf, "core_reloc_primitives", BTF_KIND_STRUCT);
+	assert(id > 0);
+
+	t = btf__type_by_id(btf, id);
+	assert(btf_vlen(t) == 1);
+
+	members = btf_members(t);
+
+	id = btf__find_by_name_kind(btf, "char", BTF_KIND_INT);
+	assert(id > 0);
+
+	/* the type of the struct member must be the char */
+	assert(members[0].type == id);
+}
+
+static void check_btfgen_primitive_union(struct btf *btf)
+{
+	struct btf_member *members;
+	const struct btf_type *t;
+	int id;
+
+	/* void, a_union and int*/
+	assert(btf__type_cnt(btf) == 3);
+
+	id = btf__find_by_name_kind(btf, "a_union", BTF_KIND_UNION);
+	assert(id > 0);
+
+	t = btf__type_by_id(btf, id);
+	assert(btf_vlen(t) == 1);
+
+	members = btf_members(t);
+
+	id = btf__find_by_name_kind(btf, "int", BTF_KIND_INT);
+	assert(id > 0);
+
+	/* the type of the union member must be the integer */
+	assert(members[0].type == id);
+}
+
+static void check_btfgen_primitive_array(struct btf *btf)
+{
+	int array_id, array_type_id, array_index_type_id;
+	struct btf_array *array;
+
+	/* void, struct, array, int (array index type) and int (array type) */
+	assert(btf__type_cnt(btf) == 5);
+
+	array_id = btf__find_by_name_kind(btf, "", BTF_KIND_ARRAY);
+	assert(array_id > 0);
+
+	array = btf_array(btf__type_by_id(btf, array_id));
+
+	array_type_id = btf__find_by_name_kind(btf, "int", BTF_KIND_INT);
+	assert(array_type_id > 0);
+
+	array_index_type_id = btf__find_by_name_kind(btf, "__ARRAY_SIZE_TYPE__", BTF_KIND_INT);
+	assert(array_index_type_id > 0);
+
+	/* check that array types are the correct ones */
+	assert(array->type == array_type_id);
+	assert(array->index_type == array_index_type_id);
+}
+
+/* If there are relocations in two different BPF objects involving
+ * different members of the same struct, then the generated BTF should
+ * contain a single instance of such struct with both fields.
+ */
+static void check_btfgen_primitive_structs_different_objects(struct btf *btf)
+{
+	struct btf_member *members;
+	const struct btf_type *t;
+	int struct_id, char_id, int_id;
+
+	/* void, struct, int and char */
+	assert(btf__type_cnt(btf) == 4);
+
+	struct_id = btf__find_by_name_kind(btf, "core_reloc_primitives", BTF_KIND_STRUCT);
+	assert(struct_id > 0);
+
+	t = btf__type_by_id(btf, struct_id);
+	assert(btf_vlen(t) == 2);
+
+	members = btf_members(t);
+
+	char_id = btf__find_by_name_kind(btf, "char", BTF_KIND_INT);
+	assert(char_id > 0);
+
+	int_id = btf__find_by_name_kind(btf, "int", BTF_KIND_INT);
+	assert(int_id > 0);
+
+	for (int i = 0; i < btf_vlen(t); i++) {
+		const char *name = btf__str_by_offset(btf, members[i].name_off);
+
+		if (!strcmp("a", name))
+			assert(members[i].type == char_id);
+		else if (!strcmp("b", name))
+			assert(members[i].type == int_id);
+	}
+}
+
+static struct btfgen_test btfgen_tests[] = {
+	{
+		"primitive struct",
+		"btfgen_btf_source.o",
+		{
+			"btfgen_primitives_struct.o",
+		},
+		check_btfgen_primitive_struct,
+	},
+	{
+		"primitive union",
+		"btfgen_btf_source.o",
+		{
+			"btfgen_primitives_union.o",
+		},
+		check_btfgen_primitive_union,
+	},
+	{
+		"primitive array",
+		"btfgen_btf_source.o",
+		{
+			"btfgen_primitives_array.o",
+		},
+		check_btfgen_primitive_array,
+	},
+	{
+		"primitive structs in different objects",
+		"btfgen_btf_source.o",
+		{
+			"btfgen_primitives_struct.o",
+			"btfgen_primitives_struct2.o",
+		},
+		check_btfgen_primitive_structs_different_objects,
+	},
+};
+
+void test_gen_min_core_btf(void)
+{
+	char target_path[PATH_MAX];
+	struct btfgen_test *test;
+	struct btf *dst_btf;
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(btfgen_tests); i++) {
+		char dir_path[] = "/tmp/btfgen-XXXXXX";
+
+		test = &btfgen_tests[i];
+
+		printf("Running %s\n", test->descr);
+
+		mkdtemp(dir_path);
+
+		snprintf(target_path, sizeof(target_path), "%s/foo.btf", dir_path);
+
+		ret = run_btfgen(test->src_btf, target_path, test->bpfobj);
+		assert(ret == 0);
+
+		dst_btf = btf__parse(target_path, NULL);
+		assert(dst_btf != NULL);
+
+		test->run_test(dst_btf);
+
+		printf("Test %s: PASS\n", test->descr);
+	}
+
+	printf("%s: PASS\n", __func__);
+}
+
+int main(void)
+{
+	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
+
+	test_gen_min_core_btf();
+
+	printf("test_bpftool: OK\n");
+
+	return 0;
+}
-- 
2.25.1

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Mauricio Vásquez Bernal]

On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> This commit implements some integration tests for "BTFGen". The goal
> of such tests is to verify that the generated BTF file contains the
> expected types.
>

This is not an exhaustive list of test cases. I'm not sure if this is
the approach we should follow to implement such tests, it seems to me
that checking each generated BTF file by hand is a lot of work but I
don't have other ideas to simplify it.

I considered different options to write these tests:
1. Use core_reloc_types.h to create a "source" BTF file with a lot of
types, then run BTFGen for all test_core_reloc_*.o files and use the
generated BTF file as btf_src_file in core_reloc.c. In other words,
re-run all test_core_reloc tests using a generated BTF file as source
instead of the "btf__core_reloc_" #name ".o" one. I think this test is
great because it tests the full functionality and actually checks that
the programs are able to run using the generated file. The problem is
how do we test that the BTFGen is creating an optimized file? Just
copying the source file without any modification will make all those
tests pass. We could check that the generated file is small (by
checking the size or the number of types) but it doesn't seem a very
reliable approach to me.
2. We could write some .c files with the types we expect to have on
the generated file and compare it with the generated file. The issue
here is that comparing those BTF files doesn't seem to be too
trivial...

Do you have any suggestions about it? Thanks!

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Quentin Monnet]

2022-01-28 18:23 UTC-0500 ~ Mauricio Vásquez Bernal <mauricio@kinvolk.io>
> On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>>
>> This commit implements some integration tests for "BTFGen". The goal
>> of such tests is to verify that the generated BTF file contains the
>> expected types.
>>
> 
> This is not an exhaustive list of test cases. I'm not sure if this is
> the approach we should follow to implement such tests, it seems to me
> that checking each generated BTF file by hand is a lot of work but I
> don't have other ideas to simplify it.
> 
> I considered different options to write these tests:
> 1. Use core_reloc_types.h to create a "source" BTF file with a lot of
> types, then run BTFGen for all test_core_reloc_*.o files and use the
> generated BTF file as btf_src_file in core_reloc.c. In other words,
> re-run all test_core_reloc tests using a generated BTF file as source
> instead of the "btf__core_reloc_" #name ".o" one. I think this test is
> great because it tests the full functionality and actually checks that
> the programs are able to run using the generated file. The problem is
> how do we test that the BTFGen is creating an optimized file? Just
> copying the source file without any modification will make all those
> tests pass. We could check that the generated file is small (by
> checking the size or the number of types) but it doesn't seem a very
> reliable approach to me.

To check that the resulting BTF is optimised, one idea maybe would be to
first produce such a minimal BTF file for the program (with a manual
check) and then to expand it with additional symbols that you know are
all unnecessary to the program. Then for the test you can run bpftool to
produce the minimal BTF again and can check if any of the definitions
known as superfluous are still present.

Another solution could be to attempt to load the BTF and program by
removing any of the info from the produced BTF file, and see if the
program still loads.

Not sure if any of those solutions is easy to implement, though.

> 2. We could write some .c files with the types we expect to have on
> the generated file and compare it with the generated file. The issue
> here is that comparing those BTF files doesn't seem to be too
> trivial...
> 
> Do you have any suggestions about it? Thanks!

I'm not familiar enough with BTF to have some great suggestion here,
maybe Andrii can help.

As a side note, it's already good to have some testing for the new
feature. The CI tests are pretty limited for bpftool at the moment and
we don't test much of it, so even basic tests to make sure that the
feature is not completely broken is a good start. Then the more we
cover, the safer we are of course :).

Thanks for this work!
Quentin

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Andrii Nakryiko]

On Fri, Jan 28, 2022 at 3:23 PM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> >
> > This commit implements some integration tests for "BTFGen". The goal
> > of such tests is to verify that the generated BTF file contains the
> > expected types.
> >
>
> This is not an exhaustive list of test cases. I'm not sure if this is
> the approach we should follow to implement such tests, it seems to me
> that checking each generated BTF file by hand is a lot of work but I
> don't have other ideas to simplify it.
>
> I considered different options to write these tests:
> 1. Use core_reloc_types.h to create a "source" BTF file with a lot of
> types, then run BTFGen for all test_core_reloc_*.o files and use the
> generated BTF file as btf_src_file in core_reloc.c. In other words,
> re-run all test_core_reloc tests using a generated BTF file as source
> instead of the "btf__core_reloc_" #name ".o" one. I think this test is
> great because it tests the full functionality and actually checks that
> the programs are able to run using the generated file. The problem is
> how do we test that the BTFGen is creating an optimized file? Just
> copying the source file without any modification will make all those
> tests pass. We could check that the generated file is small (by
> checking the size or the number of types) but it doesn't seem a very
> reliable approach to me.

I think this second run after minimizing BTF is a good idea. I
wouldn't bother to check for "minimal BTF" for this case.

> 2. We could write some .c files with the types we expect to have on
> the generated file and compare it with the generated file. The issue
> here is that comparing those BTF files doesn't seem to be too
> trivial...

But I would add few realistic examples that use various combinations
of CO-RE relocations against Linux types. Then minimize BTF and
validate that BTF is what we expect.

As for how to compare BTFs. I've been wanting to do something like
btf__normalize() API to renumber and resort all the BTF types into
some "canonical" order, so that two BTFs can be actually compared and
diffed. It might be finally the time to do that.

The big complication is your decision to dump all the fields of types
that are used by type-based relocations. I'm not convinced that's the
best way to do this. I'd keep empty struct/union for such cases,
actually. That would minimize the number of types and thus BTF in
general. It also will simplify the logic of emitting minimized BTF a
bit (all_types checks won't be necessary, I think).

As I also mentioned in previous patches, for types that are only
referenced through pointer, I'd emit FWD declaration only. Or at best
empty struct/union.

With all that, after btf__minimize() operation, comparing BTFs would
be actually pretty easy, because we'll know the order of each type, so
using the
VALIDATE_RAW_BTF() (see prog_tests/btf_dedup_split.c) the tests will
be easy and clean.


One last thing, let's not add a new test binary (test_bpftool), let's
keep adding more tests into test_progs.

>
> Do you have any suggestions about it? Thanks!

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Mauricio Vásquez Bernal]

On Wed, Feb 2, 2022 at 2:50 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jan 28, 2022 at 3:23 PM Mauricio Vásquez Bernal
> <mauricio@kinvolk.io> wrote:
> >
> > On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > >
> > > This commit implements some integration tests for "BTFGen". The goal
> > > of such tests is to verify that the generated BTF file contains the
> > > expected types.
> > >
> >
> > This is not an exhaustive list of test cases. I'm not sure if this is
> > the approach we should follow to implement such tests, it seems to me
> > that checking each generated BTF file by hand is a lot of work but I
> > don't have other ideas to simplify it.
> >
> > I considered different options to write these tests:
> > 1. Use core_reloc_types.h to create a "source" BTF file with a lot of
> > types, then run BTFGen for all test_core_reloc_*.o files and use the
> > generated BTF file as btf_src_file in core_reloc.c. In other words,
> > re-run all test_core_reloc tests using a generated BTF file as source
> > instead of the "btf__core_reloc_" #name ".o" one. I think this test is
> > great because it tests the full functionality and actually checks that
> > the programs are able to run using the generated file. The problem is
> > how do we test that the BTFGen is creating an optimized file? Just
> > copying the source file without any modification will make all those
> > tests pass. We could check that the generated file is small (by
> > checking the size or the number of types) but it doesn't seem a very
> > reliable approach to me.
>
> I think this second run after minimizing BTF is a good idea. I
> wouldn't bother to check for "minimal BTF" for this case.
>

Right. Do you want this to be part of this series or can we merge later on?

> > 2. We could write some .c files with the types we expect to have on
> > the generated file and compare it with the generated file. The issue
> > here is that comparing those BTF files doesn't seem to be too
> > trivial...
>
> But I would add few realistic examples that use various combinations
> of CO-RE relocations against Linux types. Then minimize BTF and
> validate that BTF is what we expect.
>

What do you mean by "realistic examples"? Aren't the BPF programs
(that use core_reloc_types.h) I added in this commit good enough for
this test?

> As for how to compare BTFs. I've been wanting to do something like
> btf__normalize() API to renumber and resort all the BTF types into
> some "canonical" order, so that two BTFs can be actually compared and
> diffed. It might be finally the time to do that.
>
> The big complication is your decision to dump all the fields of types
> that are used by type-based relocations. I'm not convinced that's the
> best way to do this. I'd keep empty struct/union for such cases,
> actually. That would minimize the number of types and thus BTF in
> general. It also will simplify the logic of emitting minimized BTF a
> bit (all_types checks won't be necessary, I think).
>
> As I also mentioned in previous patches, for types that are only
> referenced through pointer, I'd emit FWD declaration only. Or at best
> empty struct/union.
>
> With all that, after btf__minimize() operation, comparing BTFs would
> be actually pretty easy, because we'll know the order of each type,

Why do we know the order of each type? I think the order of the types
in the generated BTF files depends on:
1. The order or the relocations on the BPF object. (I'm not sure if
the compiler generates them in the same order as they appear in the
code)
2. BTFGen implementation: types are added recursively and there is
also a hashmap in between.
3. How bpftool is invoked. bpftool gen min_core_btf ... OBJ1 OBJ2 vs
bpftool gen min_core_btf ... OBJ2 OBJ1.

What I'm saying is that given a source BTF file and a BPF object I
don't know what is the order of the output BTF file. I know we could
run the test, check the generated output and use it for the test but
it seems like "cheating" to me...

Am I missing something?

> so
> using the
> VALIDATE_RAW_BTF() (see prog_tests/btf_dedup_split.c) the tests will
> be easy and clean.
>
>
> One last thing, let's not add a new test binary (test_bpftool), let's
> keep adding more tests into test_progs.
>

Will do.

> >
> > Do you have any suggestions about it? Thanks!

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Andrii Nakryiko]

On Thu, Feb 3, 2022 at 1:17 PM Mauricio Vásquez Bernal
<mauricio@kinvolk.io> wrote:
>
> On Wed, Feb 2, 2022 at 2:50 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 3:23 PM Mauricio Vásquez Bernal
> > <mauricio@kinvolk.io> wrote:
> > >
> > > On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
> > > >
> > > > This commit implements some integration tests for "BTFGen". The goal
> > > > of such tests is to verify that the generated BTF file contains the
> > > > expected types.
> > > >
> > >
> > > This is not an exhaustive list of test cases. I'm not sure if this is
> > > the approach we should follow to implement such tests, it seems to me
> > > that checking each generated BTF file by hand is a lot of work but I
> > > don't have other ideas to simplify it.
> > >
> > > I considered different options to write these tests:
> > > 1. Use core_reloc_types.h to create a "source" BTF file with a lot of
> > > types, then run BTFGen for all test_core_reloc_*.o files and use the
> > > generated BTF file as btf_src_file in core_reloc.c. In other words,
> > > re-run all test_core_reloc tests using a generated BTF file as source
> > > instead of the "btf__core_reloc_" #name ".o" one. I think this test is
> > > great because it tests the full functionality and actually checks that
> > > the programs are able to run using the generated file. The problem is
> > > how do we test that the BTFGen is creating an optimized file? Just
> > > copying the source file without any modification will make all those
> > > tests pass. We could check that the generated file is small (by
> > > checking the size or the number of types) but it doesn't seem a very
> > > reliable approach to me.
> >
> > I think this second run after minimizing BTF is a good idea. I
> > wouldn't bother to check for "minimal BTF" for this case.
> >
>
> Right. Do you want this to be part of this series or can we merge later on?

Can be done separately, probably, but it shouldn't be a lot of code
(just call bpftool and use its output as input to core_reloc tests
again). Should be a generic part of core_reloc.c selftest.

>
> > > 2. We could write some .c files with the types we expect to have on
> > > the generated file and compare it with the generated file. The issue
> > > here is that comparing those BTF files doesn't seem to be too
> > > trivial...
> >
> > But I would add few realistic examples that use various combinations
> > of CO-RE relocations against Linux types. Then minimize BTF and
> > validate that BTF is what we expect.
> >
>
> What do you mean by "realistic examples"? Aren't the BPF programs
> (that use core_reloc_types.h) I added in this commit good enough for
> this test?

Realistic as in using real kernel types. core_reloc_types.h are
synthetic. Both are useful, but seeing big task_struct compressed to
just few relevant fields and CO-RE relocations still working would be
very convincing.

>
> > As for how to compare BTFs. I've been wanting to do something like
> > btf__normalize() API to renumber and resort all the BTF types into
> > some "canonical" order, so that two BTFs can be actually compared and
> > diffed. It might be finally the time to do that.
> >
> > The big complication is your decision to dump all the fields of types
> > that are used by type-based relocations. I'm not convinced that's the
> > best way to do this. I'd keep empty struct/union for such cases,
> > actually. That would minimize the number of types and thus BTF in
> > general. It also will simplify the logic of emitting minimized BTF a
> > bit (all_types checks won't be necessary, I think).
> >
> > As I also mentioned in previous patches, for types that are only
> > referenced through pointer, I'd emit FWD declaration only. Or at best
> > empty struct/union.
> >
> > With all that, after btf__minimize() operation, comparing BTFs would
> > be actually pretty easy, because we'll know the order of each type,
>
> Why do we know the order of each type? I think the order of the types
> in the generated BTF files depends on:
> 1. The order or the relocations on the BPF object. (I'm not sure if
> the compiler generates them in the same order as they appear in the
> code)
> 2. BTFGen implementation: types are added recursively and there is
> also a hashmap in between.
> 3. How bpftool is invoked. bpftool gen min_core_btf ... OBJ1 OBJ2 vs
> bpftool gen min_core_btf ... OBJ2 OBJ1.
>

Hm.. the order of CO-RE relocations doesn't have any bearing on
resulting BTF. btf__normalize() will make sure that order is
"canonical", which makes two BTFs comparable. E.g., all INTs will go
before STRUCTS, and within INTs they will be sorted by name, then
size, etc. That kind of an idea. So equivalent BTFs after
normalization will end up with exactly the same contents (except,
probably, for the string section, but we won't be comparing name_off
as integers, rather strings that they point to; details).

> What I'm saying is that given a source BTF file and a BPF object I
> don't know what is the order of the output BTF file. I know we could
> run the test, check the generated output and use it for the test but
> it seems like "cheating" to me...
>
> Am I missing something?

Probably that the whole point of btf__normalize() is to ensure
canonical order, see above.

>
> > so
> > using the
> > VALIDATE_RAW_BTF() (see prog_tests/btf_dedup_split.c) the tests will
> > be easy and clean.
> >
> >
> > One last thing, let's not add a new test binary (test_bpftool), let's
> > keep adding more tests into test_progs.
> >
>
> Will do.
>
> > >
> > > Do you have any suggestions about it? Thanks!

Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core_btf

[Quentin Monnet]

2022-01-28 17:33 UTC-0500 ~ Mauricio Vásquez <mauricio@kinvolk.io>
> This commit implements some integration tests for "BTFGen". The goal
> of such tests is to verify that the generated BTF file contains the
> expected types.
> 
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/testing/selftests/bpf/.gitignore        |   1 +
>  tools/testing/selftests/bpf/Makefile          |   4 +-
>  .../selftests/bpf/progs/btfgen_btf_source.c   |  12 +
>  .../bpf/progs/btfgen_primitives_array.c       |  39 +++
>  .../bpf/progs/btfgen_primitives_struct.c      |  40 +++
>  .../bpf/progs/btfgen_primitives_struct2.c     |  44 ++++
>  .../bpf/progs/btfgen_primitives_union.c       |  32 +++
>  tools/testing/selftests/bpf/test_bpftool.c    | 228 ++++++++++++++++++
>  8 files changed, 399 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/btfgen_btf_source.c
>  create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_array.c
>  create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct.c
>  create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_struct2.c
>  create mode 100644 tools/testing/selftests/bpf/progs/btfgen_primitives_union.c
>  create mode 100644 tools/testing/selftests/bpf/test_bpftool.c
> 
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 1dad8d617da8..308cd5b9cfc4 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -41,3 +41,4 @@ test_cpp
>  *.tmp
>  xdpxceiver
>  xdp_redirect_multi
> +test_bpftool
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 945f92d71db3..afc9bff6545d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -38,7 +38,8 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
>  	test_sock test_sockmap get_cgroup_id_user \
>  	test_cgroup_storage \
>  	test_tcpnotify_user test_sysctl \
> -	test_progs-no_alu32
> +	test_progs-no_alu32 \
> +	test_bpftool

Do you think we could rename this file to something like
“test_bpftool_min_core_btf”? We already have a “test_bpftool.py” and I
fear it might lead to confusion.

>  
>  # Also test bpf-gcc, if present
>  ifneq ($(BPF_GCC),)
> @@ -212,6 +213,7 @@ $(OUTPUT)/xdping: $(TESTING_HELPERS)
>  $(OUTPUT)/flow_dissector_load: $(TESTING_HELPERS)
>  $(OUTPUT)/test_maps: $(TESTING_HELPERS)
>  $(OUTPUT)/test_verifier: $(TESTING_HELPERS)
> +$(OUTPUT)/test_bpftool:
>  
>  BPFTOOL ?= $(DEFAULT_BPFTOOL)
>  $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \

> diff --git a/tools/testing/selftests/bpf/test_bpftool.c b/tools/testing/selftests/bpf/test_bpftool.c
> new file mode 100644
> index 000000000000..ca7facc582d5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_bpftool.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Not sure if it is intentional to use “GPL-2.0-only” where the other
files in your patch have “GPL-2.0”. But I don't believe it matters much
anyway.