[PATCH bpf-next v2 0/5] Subskeleton support for BPF libraries

[PATCH bpf-next v2 0/5] Subskeleton support for BPF libraries

[Delyan Kratunov]

In the quest for ever more modularity, a new need has arisen - the ability to
access data associated with a BPF library from a corresponding userspace library.
The catch is that we don't want the userspace library to know about the structure of the
final BPF object that the BPF library is linked into.

In pursuit of this modularity, this patch series introduces *subskeletons.*
Subskeletons are similar in use and design to skeletons with a couple of differences:

1. The generated storage types do not rely on contiguous storage for the library's
variables because they may be interspersed randomly throughout the final BPF object's sections.

2. Subskeletons do not own objects and instead require a loaded bpf_object* to
be passed at runtime in order to be initialized. By extension, symbols are resolved at
runtime by parsing the final object's BTF. This has the interesting effect that the same
userspace code can interoperate with the library BPF code *linked into different final objects.*

3. Subskeletons allow access to all global variables, programs, and custom maps. They also expose
the internal maps *of the final object*. This allows bpf_var_skeleton objects to contain a bpf_map**
instead of a section name.

Changes since v1:
 - Introduced new strict mode knob for single-routine-in-.text compatibility behavior, which
   disproportionately affects library objects. bpftool works in 1.0 mode so subskeleton generation
   doesn't have to worry about this now.
 - Made bpf_map_btf_value_type_id available earlier and used it wherever applicable.
 - Refactoring in bpftool gen.c per review comments.
 - Subskels now use typeof() for array and func proto globals to avoid the need for runtime split btf.
 - Expanded the subskeleton test to include arrays, custom maps, extern maps, weak symbols, and kconfigs.
 - selftests/bpf/Makefile now generates a subskel.h for every skel.h it would make.

For reference, here is a shortened subskeleton header:

#ifndef __TEST_SUBSKELETON_LIB_SUBSKEL_H__
#define __TEST_SUBSKELETON_LIB_SUBSKEL_H__

struct test_subskeleton_lib {
	struct bpf_object *obj;
	struct bpf_object_subskeleton *subskel;
	struct {
		struct bpf_map *map2;
		struct bpf_map *map1;
		struct bpf_map *data;
		struct bpf_map *rodata;
		struct bpf_map *bss;
		struct bpf_map *kconfig;
	} maps;
	struct {
		struct bpf_program *lib_perf_handler;
	} progs;
	struct test_subskeleton_lib__data {
		int *var6;
		int *var2;
		int *var5;
	} data;
	struct test_subskeleton_lib__rodata {
		int *var1;
	} rodata;
	struct test_subskeleton_lib__bss {
		struct {
			int var3_1;
			__s64 var3_2;
		} *var3;
		int *libout1;
		typeof(int[4]) *var4;
		typeof(int (*)()) *fn_ptr;
	} bss;
	struct test_subskeleton_lib__kconfig {
		_Bool *CONFIG_BPF_SYSCALL;
	} kconfig;

static inline struct test_subskeleton_lib *
test_subskeleton_lib__open(const struct bpf_object *src)
{
	struct test_subskeleton_lib *obj;
	struct bpf_object_subskeleton *s;
	int err;

	...
	s = (struct bpf_object_subskeleton *)calloc(1, sizeof(*s));
	...

	s->var_cnt = 9;
	...

	s->vars[0].name = "var6";
	s->vars[0].map = &obj->maps.data;
	s->vars[0].addr = (void**) &obj->data.var6;
  ...

	/* maps */
	...

	/* programs */
	s->prog_cnt = 1;
	...

	err = bpf_object__open_subskeleton(s);
  ...
	return obj;
}
#endif /* __TEST_SUBSKELETON_LIB_SUBSKEL_H__ */

Delyan Kratunov (5):
  libbpf: add new strict flag for .text subprograms
  libbpf: init btf_{key,value}_type_id on internal map open
  libbpf: add subskeleton scaffolding
  bpftool: add support for subskeletons
  selftests/bpf: test subskeleton functionality

 .../bpf/bpftool/Documentation/bpftool-gen.rst |  25 +
 tools/bpf/bpftool/bash-completion/bpftool     |  14 +-
 tools/bpf/bpftool/gen.c                       | 589 ++++++++++++++++--
 tools/lib/bpf/libbpf.c                        | 151 ++++-
 tools/lib/bpf/libbpf.h                        |  29 +
 tools/lib/bpf/libbpf.map                      |   2 +
 tools/lib/bpf/libbpf_legacy.h                 |   6 +
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |  10 +-
 .../selftests/bpf/prog_tests/subskeleton.c    |  83 +++
 .../selftests/bpf/progs/test_subskeleton.c    |  23 +
 .../bpf/progs/test_subskeleton_lib.c          |  56 ++
 .../bpf/progs/test_subskeleton_lib2.c         |  16 +
 13 files changed, 919 insertions(+), 86 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c

--
2.34.1

[PATCH bpf-next v2 1/5] libbpf: add new strict flag for .text subprograms

[Delyan Kratunov]

Currently, libbpf considers a single routine in .text as a program. This
is particularly confusing when it comes to library objects - a single routine
meant to be used as an extern will instead be considered a bpf_program.

This patch hides this compatibility behavior behind a libbpf_mode strict
mode flag.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c        | 7 +++++++
 tools/lib/bpf/libbpf_legacy.h | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 43161fdd44bb..b6f11ce0d6bc 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3832,7 +3832,14 @@ static bool prog_is_subprog(const struct bpf_object *obj,
 	 * .text programs are subprograms (even if they are not called from
 	 * other programs), because libbpf never explicitly supported mixing
 	 * SEC()-designated BPF programs and .text entry-point BPF programs.
+	 *
+	 * In libbpf 1.0 strict mode, we always consider .text
+	 * programs to be subprograms.
 	 */
+
+	if (libbpf_mode & LIBBPF_STRICT_TEXT_ONLY_SUBPROGRAMS)
+		return prog->sec_idx == obj->efile.text_shndx;
+
 	return prog->sec_idx == obj->efile.text_shndx && obj->nr_programs > 1;
 }
 
diff --git a/tools/lib/bpf/libbpf_legacy.h b/tools/lib/bpf/libbpf_legacy.h
index a283cf031665..388384ea97a7 100644
--- a/tools/lib/bpf/libbpf_legacy.h
+++ b/tools/lib/bpf/libbpf_legacy.h
@@ -78,6 +78,12 @@ enum libbpf_strict_mode {
 	 * in favor of BTF-defined map definitions in SEC(".maps").
 	 */
 	LIBBPF_STRICT_MAP_DEFINITIONS = 0x20,
+	/*
+	 * When enabled, always consider routines in the .text section to
+	 * be sub-programs. Previously, single routines in the .text section
+	 * would be considered a program on their own.
+	 */
+	LIBBPF_STRICT_TEXT_ONLY_SUBPROGRAMS = 0x40,
 
 	__LIBBPF_STRICT_LAST,
 };
-- 
2.34.1

Re: [PATCH bpf-next v2 1/5] libbpf: add new strict flag for .text subprograms

[Andrii Nakryiko]

On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Currently, libbpf considers a single routine in .text as a program. This
> is particularly confusing when it comes to library objects - a single routine
> meant to be used as an extern will instead be considered a bpf_program.
>
> This patch hides this compatibility behavior behind a libbpf_mode strict
> mode flag.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/lib/bpf/libbpf.c        | 7 +++++++
>  tools/lib/bpf/libbpf_legacy.h | 6 ++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 43161fdd44bb..b6f11ce0d6bc 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3832,7 +3832,14 @@ static bool prog_is_subprog(const struct bpf_object *obj,
>          * .text programs are subprograms (even if they are not called from
>          * other programs), because libbpf never explicitly supported mixing
>          * SEC()-designated BPF programs and .text entry-point BPF programs.
> +        *
> +        * In libbpf 1.0 strict mode, we always consider .text
> +        * programs to be subprograms.
>          */
> +
> +       if (libbpf_mode & LIBBPF_STRICT_TEXT_ONLY_SUBPROGRAMS)
> +               return prog->sec_idx == obj->efile.text_shndx;
> +
>         return prog->sec_idx == obj->efile.text_shndx && obj->nr_programs > 1;
>  }
>
> diff --git a/tools/lib/bpf/libbpf_legacy.h b/tools/lib/bpf/libbpf_legacy.h
> index a283cf031665..388384ea97a7 100644
> --- a/tools/lib/bpf/libbpf_legacy.h
> +++ b/tools/lib/bpf/libbpf_legacy.h
> @@ -78,6 +78,12 @@ enum libbpf_strict_mode {
>          * in favor of BTF-defined map definitions in SEC(".maps").
>          */
>         LIBBPF_STRICT_MAP_DEFINITIONS = 0x20,
> +       /*
> +        * When enabled, always consider routines in the .text section to
> +        * be sub-programs. Previously, single routines in the .text section
> +        * would be considered a program on their own.
> +        */
> +       LIBBPF_STRICT_TEXT_ONLY_SUBPROGRAMS = 0x40,

We have LIBBPF_STRICT_SEC_NAME, we can probably just rely on that one.
STRICT_SEC_NAME means (among other things) that there has to be
SEC("abc"), so there can't be BPF program in .text section. We don't
enforce that yet, but that's a separate thing.

>
>         __LIBBPF_STRICT_LAST,
>  };

> --
> 2.34.1

[PATCH bpf-next v2 2/5] libbpf: init btf_{key,value}_type_id on internal map open

[Delyan Kratunov]

For internal maps, look up the key and value btf types on
open() and not load(), so that `bpf_map_btf_value_type_id`
is usable in `bpftool gen`.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b6f11ce0d6bc..3fb9c926fe6e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1517,6 +1517,9 @@ static char *internal_map_name(struct bpf_object *obj, const char *real_name)
 	return strdup(map_name);
 }
 
+static int
+bpf_map_find_btf_info(struct bpf_object *obj, struct bpf_map *map);
+
 static int
 bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
 			      const char *real_name, int sec_idx, void *data, size_t data_sz)
@@ -1564,6 +1567,11 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
 		return err;
 	}
 
+	err = bpf_map_find_btf_info(obj, map);
+	/* intentionally ignoring err, failures are fine because of
+	 * maps like .rodata.str1.1
+	 */
+
 	if (data)
 		memcpy(map->mmaped, data, data_sz);
 
-- 
2.34.1

Re: [PATCH bpf-next v2 2/5] libbpf: init btf_{key,value}_type_id on internal map open

[Andrii Nakryiko]

On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> For internal maps, look up the key and value btf types on
> open() and not load(), so that `bpf_map_btf_value_type_id`
> is usable in `bpftool gen`.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/lib/bpf/libbpf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b6f11ce0d6bc..3fb9c926fe6e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1517,6 +1517,9 @@ static char *internal_map_name(struct bpf_object *obj, const char *real_name)
>         return strdup(map_name);
>  }
>
> +static int
> +bpf_map_find_btf_info(struct bpf_object *obj, struct bpf_map *map);
> +
>  static int
>  bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
>                               const char *real_name, int sec_idx, void *data, size_t data_sz)
> @@ -1564,6 +1567,11 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
>                 return err;
>         }
>
> +       err = bpf_map_find_btf_info(obj, map);

is there any reason to still do bpf_map_find_btf_info() in
bpf_object__create_map()? It's also non-uniform, legacy user maps
won't have btf_key_type_id/btf_value_type_id set until load, while
internal maps (and BTF-defined maps) will. Let's make it uniform and
call bpf_map_find_btf_info() from bpf_object__init_user_maps() early
on as well (and not do that at all in bpf_object__create_map())

> +       /* intentionally ignoring err, failures are fine because of
> +        * maps like .rodata.str1.1
> +        */

if the intention is to explicitly ignore error, then the most explicit
way to express this is:

(void)func_that_can_return_error(...);

> +
>         if (data)
>                 memcpy(map->mmaped, data, data_sz);
>
> --
> 2.34.1

[PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

In symmetry with bpf_object__open_skeleton(),
bpf_object__open_subskeleton() performs the actual walking and linking
of maps, progs, and globals described by bpf_*_skeleton objects.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/lib/bpf/libbpf.c   | 136 +++++++++++++++++++++++++++++++++------
 tools/lib/bpf/libbpf.h   |  29 +++++++++
 tools/lib/bpf/libbpf.map |   2 +
 3 files changed, 146 insertions(+), 21 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 3fb9c926fe6e..ba7b25b11486 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11810,6 +11810,49 @@ int libbpf_num_possible_cpus(void)
 	return tmp_cpus;
 }
 
+static int populate_skeleton_maps(const struct bpf_object* obj,
+				  struct bpf_map_skeleton* maps,
+				  size_t map_cnt)
+{
+	int i;
+
+	for (i = 0; i < map_cnt; i++) {
+		struct bpf_map **map = maps[i].map;
+		const char *name = maps[i].name;
+		void **mmaped = maps[i].mmaped;
+
+		*map = bpf_object__find_map_by_name(obj, name);
+		if (!*map) {
+			pr_warn("failed to find skeleton map '%s'\n", name);
+			return libbpf_err(-ESRCH);
+		}
+
+		/* externs shouldn't be pre-setup from user code */
+		if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
+			*mmaped = (*map)->mmaped;
+	}
+	return 0;
+}
+
+static int populate_skeleton_progs(const struct bpf_object* obj,
+				   struct bpf_prog_skeleton* progs,
+				   size_t prog_cnt)
+{
+	int i;
+
+	for (i = 0; i < prog_cnt; i++) {
+		struct bpf_program **prog = progs[i].prog;
+		const char *name = progs[i].name;
+
+		*prog = bpf_object__find_program_by_name(obj, name);
+		if (!*prog) {
+			pr_warn("failed to find skeleton program '%s'\n", name);
+			return libbpf_err(-ESRCH);
+		}
+	}
+	return 0;
+}
+
 int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 			      const struct bpf_object_open_opts *opts)
 {
@@ -11817,7 +11860,7 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 		.object_name = s->name,
 	);
 	struct bpf_object *obj;
-	int i, err;
+	int err;
 
 	/* Attempt to preserve opts->object_name, unless overriden by user
 	 * explicitly. Overwriting object name for skeletons is discouraged,
@@ -11840,37 +11883,88 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 	}
 
 	*s->obj = obj;
+	err = populate_skeleton_maps(obj, s->maps, s->map_cnt);
+	if (err) {
+		pr_warn("failed to populate skeleton maps for '%s': %d\n",
+			s->name, err);
+		return libbpf_err(err);
+	}
 
-	for (i = 0; i < s->map_cnt; i++) {
-		struct bpf_map **map = s->maps[i].map;
-		const char *name = s->maps[i].name;
-		void **mmaped = s->maps[i].mmaped;
+	err = populate_skeleton_progs(obj, s->progs, s->prog_cnt);
+	if (err) {
+		pr_warn("failed to populate skeleton progs for '%s': %d\n",
+			s->name, err);
+		return libbpf_err(err);
+	}
 
-		*map = bpf_object__find_map_by_name(obj, name);
-		if (!*map) {
-			pr_warn("failed to find skeleton map '%s'\n", name);
-			return libbpf_err(-ESRCH);
-		}
+	return 0;
+}
 
-		/* externs shouldn't be pre-setup from user code */
-		if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
-			*mmaped = (*map)->mmaped;
+int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
+{
+	int err, len, var_idx, i;
+	const char *var_name;
+	const struct bpf_map *map;
+	struct btf *btf;
+	__u32 map_type_id;
+	const struct btf_type *map_type, *var_type;
+	const struct bpf_var_skeleton *var_skel;
+	struct btf_var_secinfo *var;
+
+	if (!s->obj)
+		return libbpf_err(-EINVAL);
+
+	btf = bpf_object__btf(s->obj);
+	if (!btf)
+		return -errno;
+
+	err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
+	if (err) {
+		pr_warn("failed to populate subskeleton maps: %d\n", err);
+		return libbpf_err(err);
 	}
 
-	for (i = 0; i < s->prog_cnt; i++) {
-		struct bpf_program **prog = s->progs[i].prog;
-		const char *name = s->progs[i].name;
+	err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
+	if (err) {
+		pr_warn("failed to populate subskeleton maps: %d\n", err);
+		return libbpf_err(err);
+	}
 
-		*prog = bpf_object__find_program_by_name(obj, name);
-		if (!*prog) {
-			pr_warn("failed to find skeleton program '%s'\n", name);
-			return libbpf_err(-ESRCH);
+	for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
+		var_skel = &s->vars[var_idx];
+		map = *var_skel->map;
+		map_type_id = bpf_map__btf_value_type_id(map);
+		map_type = btf__type_by_id(btf, map_type_id);
+
+		len = btf_vlen(map_type);
+		var = btf_var_secinfos(map_type);
+		for (i = 0; i < len; i++, var++) {
+			var_type = btf__type_by_id(btf, var->type);
+			if (!var_type) {
+				pr_warn("Could not find var type for item %1$d in section %2$s",
+					i, bpf_map__name(map));
+				return libbpf_err(-EINVAL);
+			}
+			var_name = btf__name_by_offset(btf, var_type->name_off);
+			if (strcmp(var_name, var_skel->name) == 0) {
+				*var_skel->addr = (char *) map->mmaped + var->offset;
+				break;
+			}
 		}
 	}
-
 	return 0;
 }
 
+void bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s)
+{
+	if (!s)
+		return;
+	free(s->maps);
+	free(s->progs);
+	free(s->vars);
+	free(s);
+}
+
 int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
 {
 	int i, err;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c1b0c2ef14d8..1bff7647d797 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1289,6 +1289,35 @@ LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
 
+struct bpf_var_skeleton {
+	const char *name;
+	struct bpf_map **map;
+	void **addr;
+};
+
+struct bpf_object_subskeleton {
+	size_t sz; /* size of this struct, for forward/backward compatibility */
+
+	const struct bpf_object *obj;
+
+	int map_cnt;
+	int map_skel_sz; /* sizeof(struct bpf_map_skeleton) */
+	struct bpf_map_skeleton *maps;
+
+	int prog_cnt;
+	int prog_skel_sz; /* sizeof(struct bpf_prog_skeleton) */
+	struct bpf_prog_skeleton *progs;
+
+	int var_cnt;
+	int var_skel_sz; /* sizeof(struct bpf_var_skeleton) */
+	struct bpf_var_skeleton *vars;
+};
+
+LIBBPF_API int
+bpf_object__open_subskeleton(struct bpf_object_subskeleton *s);
+LIBBPF_API void
+bpf_object__destroy_subskeleton(struct bpf_object_subskeleton *s);
+
 struct gen_loader_opts {
 	size_t sz; /* size of this struct, for forward/backward compatiblity */
 	const char *data;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index df1b947792c8..d744fbb8612e 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
 
 LIBBPF_0.8.0 {
 	global:
+		bpf_object__open_subskeleton;
+		bpf_object__destroy_subskeleton;
 		libbpf_register_prog_handler;
 		libbpf_unregister_prog_handler;
 } LIBBPF_0.7.0;
-- 
2.34.1

Re: [PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> In symmetry with bpf_object__open_skeleton(),
> bpf_object__open_subskeleton() performs the actual walking and linking
> of maps, progs, and globals described by bpf_*_skeleton objects.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/lib/bpf/libbpf.c   | 136 +++++++++++++++++++++++++++++++++------
>  tools/lib/bpf/libbpf.h   |  29 +++++++++
>  tools/lib/bpf/libbpf.map |   2 +
>  3 files changed, 146 insertions(+), 21 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3fb9c926fe6e..ba7b25b11486 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11810,6 +11810,49 @@ int libbpf_num_possible_cpus(void)
>         return tmp_cpus;
>  }
>
> +static int populate_skeleton_maps(const struct bpf_object* obj,
> +                                 struct bpf_map_skeleton* maps,
> +                                 size_t map_cnt)
> +{
> +       int i;
> +
> +       for (i = 0; i < map_cnt; i++) {
> +               struct bpf_map **map = maps[i].map;
> +               const char *name = maps[i].name;
> +               void **mmaped = maps[i].mmaped;
> +
> +               *map = bpf_object__find_map_by_name(obj, name);
> +               if (!*map) {
> +                       pr_warn("failed to find skeleton map '%s'\n", name);
> +                       return libbpf_err(-ESRCH);

this is internal helper function, so no need (and it's a bit
misleading as well) to use libbpf_err() helper. Just return an error
and let user-facing API functions deal with error handling

> +               }
> +
> +               /* externs shouldn't be pre-setup from user code */
> +               if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
> +                       *mmaped = (*map)->mmaped;
> +       }
> +       return 0;
> +}
> +
> +static int populate_skeleton_progs(const struct bpf_object* obj,
> +                                  struct bpf_prog_skeleton* progs,
> +                                  size_t prog_cnt)
> +{
> +       int i;
> +
> +       for (i = 0; i < prog_cnt; i++) {
> +               struct bpf_program **prog = progs[i].prog;
> +               const char *name = progs[i].name;
> +
> +               *prog = bpf_object__find_program_by_name(obj, name);
> +               if (!*prog) {
> +                       pr_warn("failed to find skeleton program '%s'\n", name);
> +                       return libbpf_err(-ESRCH);

same about libbpf_err() use

> +               }
> +       }
> +       return 0;
> +}
> +
>  int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>                               const struct bpf_object_open_opts *opts)
>  {
> @@ -11817,7 +11860,7 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>                 .object_name = s->name,
>         );
>         struct bpf_object *obj;
> -       int i, err;
> +       int err;
>
>         /* Attempt to preserve opts->object_name, unless overriden by user
>          * explicitly. Overwriting object name for skeletons is discouraged,
> @@ -11840,37 +11883,88 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>         }
>
>         *s->obj = obj;
> +       err = populate_skeleton_maps(obj, s->maps, s->map_cnt);
> +       if (err) {
> +               pr_warn("failed to populate skeleton maps for '%s': %d\n",
> +                       s->name, err);

nit: probably fits under 100 characters on single line

> +               return libbpf_err(err);
> +       }
>
> -       for (i = 0; i < s->map_cnt; i++) {
> -               struct bpf_map **map = s->maps[i].map;
> -               const char *name = s->maps[i].name;
> -               void **mmaped = s->maps[i].mmaped;
> +       err = populate_skeleton_progs(obj, s->progs, s->prog_cnt);
> +       if (err) {
> +               pr_warn("failed to populate skeleton progs for '%s': %d\n",
> +                       s->name, err);

and here

> +               return libbpf_err(err);
> +       }
>
> -               *map = bpf_object__find_map_by_name(obj, name);
> -               if (!*map) {
> -                       pr_warn("failed to find skeleton map '%s'\n", name);
> -                       return libbpf_err(-ESRCH);
> -               }
> +       return 0;
> +}
>
> -               /* externs shouldn't be pre-setup from user code */
> -               if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
> -                       *mmaped = (*map)->mmaped;
> +int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
> +{
> +       int err, len, var_idx, i;
> +       const char *var_name;
> +       const struct bpf_map *map;
> +       struct btf *btf;
> +       __u32 map_type_id;
> +       const struct btf_type *map_type, *var_type;
> +       const struct bpf_var_skeleton *var_skel;
> +       struct btf_var_secinfo *var;
> +
> +       if (!s->obj)
> +               return libbpf_err(-EINVAL);
> +
> +       btf = bpf_object__btf(s->obj);
> +       if (!btf)
> +               return -errno;

use libbpf_err(-errno) for consistency?

> +
> +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> +       if (err) {
> +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> +               return libbpf_err(err);
>         }
>
> -       for (i = 0; i < s->prog_cnt; i++) {
> -               struct bpf_program **prog = s->progs[i].prog;
> -               const char *name = s->progs[i].name;
> +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> +       if (err) {
> +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> +               return libbpf_err(err);
> +       }
>
> -               *prog = bpf_object__find_program_by_name(obj, name);
> -               if (!*prog) {
> -                       pr_warn("failed to find skeleton program '%s'\n", name);
> -                       return libbpf_err(-ESRCH);
> +       for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
> +               var_skel = &s->vars[var_idx];
> +               map = *var_skel->map;
> +               map_type_id = bpf_map__btf_value_type_id(map);
> +               map_type = btf__type_by_id(btf, map_type_id);
> +

should we double-check that map_type is DATASEC?

> +               len = btf_vlen(map_type);
> +               var = btf_var_secinfos(map_type);
> +               for (i = 0; i < len; i++, var++) {
> +                       var_type = btf__type_by_id(btf, var->type);
> +                       if (!var_type) {

unless BTF itself is corrupted, this shouldn't ever happen. So
checking for DATASEC should be enough and this if (!var_type) is
redundant

> +                               pr_warn("Could not find var type for item %1$d in section %2$s",
> +                                       i, bpf_map__name(map));
> +                               return libbpf_err(-EINVAL);
> +                       }
> +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> +                       if (strcmp(var_name, var_skel->name) == 0) {
> +                               *var_skel->addr = (char *) map->mmaped + var->offset;

is (char *) cast necessary? C allows pointer adjustment on void *, so
shouldn't be

> +                               break;
> +                       }
>                 }
>         }
> -
>         return 0;
>  }
>

[...]

>  struct gen_loader_opts {
>         size_t sz; /* size of this struct, for forward/backward compatiblity */
>         const char *data;
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index df1b947792c8..d744fbb8612e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
>
>  LIBBPF_0.8.0 {
>         global:
> +               bpf_object__open_subskeleton;
> +               bpf_object__destroy_subskeleton;

nit: should be in alphabetic order

>                 libbpf_register_prog_handler;
>                 libbpf_unregister_prog_handler;
>  } LIBBPF_0.7.0;
> --
> 2.34.1

Re: [PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Fri, Mar 11, 2022 at 2:52 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
> >
> > In symmetry with bpf_object__open_skeleton(),
> > bpf_object__open_subskeleton() performs the actual walking and linking
> > of maps, progs, and globals described by bpf_*_skeleton objects.
> >
> > Signed-off-by: Delyan Kratunov <delyank@fb.com>
> > ---
> >  tools/lib/bpf/libbpf.c   | 136 +++++++++++++++++++++++++++++++++------
> >  tools/lib/bpf/libbpf.h   |  29 +++++++++
> >  tools/lib/bpf/libbpf.map |   2 +
> >  3 files changed, 146 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 3fb9c926fe6e..ba7b25b11486 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -11810,6 +11810,49 @@ int libbpf_num_possible_cpus(void)
> >         return tmp_cpus;
> >  }
> >
> > +static int populate_skeleton_maps(const struct bpf_object* obj,
> > +                                 struct bpf_map_skeleton* maps,
> > +                                 size_t map_cnt)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < map_cnt; i++) {
> > +               struct bpf_map **map = maps[i].map;
> > +               const char *name = maps[i].name;
> > +               void **mmaped = maps[i].mmaped;
> > +
> > +               *map = bpf_object__find_map_by_name(obj, name);
> > +               if (!*map) {
> > +                       pr_warn("failed to find skeleton map '%s'\n", name);
> > +                       return libbpf_err(-ESRCH);
>
> this is internal helper function, so no need (and it's a bit
> misleading as well) to use libbpf_err() helper. Just return an error
> and let user-facing API functions deal with error handling
>
> > +               }
> > +
> > +               /* externs shouldn't be pre-setup from user code */
> > +               if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
> > +                       *mmaped = (*map)->mmaped;
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int populate_skeleton_progs(const struct bpf_object* obj,
> > +                                  struct bpf_prog_skeleton* progs,
> > +                                  size_t prog_cnt)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < prog_cnt; i++) {
> > +               struct bpf_program **prog = progs[i].prog;
> > +               const char *name = progs[i].name;
> > +
> > +               *prog = bpf_object__find_program_by_name(obj, name);
> > +               if (!*prog) {
> > +                       pr_warn("failed to find skeleton program '%s'\n", name);
> > +                       return libbpf_err(-ESRCH);
>
> same about libbpf_err() use
>
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +
> >  int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >                               const struct bpf_object_open_opts *opts)
> >  {
> > @@ -11817,7 +11860,7 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >                 .object_name = s->name,
> >         );
> >         struct bpf_object *obj;
> > -       int i, err;
> > +       int err;
> >
> >         /* Attempt to preserve opts->object_name, unless overriden by user
> >          * explicitly. Overwriting object name for skeletons is discouraged,
> > @@ -11840,37 +11883,88 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >         }
> >
> >         *s->obj = obj;
> > +       err = populate_skeleton_maps(obj, s->maps, s->map_cnt);
> > +       if (err) {
> > +               pr_warn("failed to populate skeleton maps for '%s': %d\n",
> > +                       s->name, err);
>
> nit: probably fits under 100 characters on single line
>
> > +               return libbpf_err(err);
> > +       }
> >
> > -       for (i = 0; i < s->map_cnt; i++) {
> > -               struct bpf_map **map = s->maps[i].map;
> > -               const char *name = s->maps[i].name;
> > -               void **mmaped = s->maps[i].mmaped;
> > +       err = populate_skeleton_progs(obj, s->progs, s->prog_cnt);
> > +       if (err) {
> > +               pr_warn("failed to populate skeleton progs for '%s': %d\n",
> > +                       s->name, err);
>
> and here
>
> > +               return libbpf_err(err);
> > +       }
> >
> > -               *map = bpf_object__find_map_by_name(obj, name);
> > -               if (!*map) {
> > -                       pr_warn("failed to find skeleton map '%s'\n", name);
> > -                       return libbpf_err(-ESRCH);
> > -               }
> > +       return 0;
> > +}
> >
> > -               /* externs shouldn't be pre-setup from user code */
> > -               if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
> > -                       *mmaped = (*map)->mmaped;
> > +int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
> > +{
> > +       int err, len, var_idx, i;
> > +       const char *var_name;
> > +       const struct bpf_map *map;
> > +       struct btf *btf;
> > +       __u32 map_type_id;
> > +       const struct btf_type *map_type, *var_type;
> > +       const struct bpf_var_skeleton *var_skel;
> > +       struct btf_var_secinfo *var;
> > +
> > +       if (!s->obj)
> > +               return libbpf_err(-EINVAL);
> > +
> > +       btf = bpf_object__btf(s->obj);
> > +       if (!btf)
> > +               return -errno;
>
> use libbpf_err(-errno) for consistency?
>
> > +
> > +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> > +       if (err) {
> > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > +               return libbpf_err(err);
> >         }
> >
> > -       for (i = 0; i < s->prog_cnt; i++) {
> > -               struct bpf_program **prog = s->progs[i].prog;
> > -               const char *name = s->progs[i].name;
> > +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> > +       if (err) {
> > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > +               return libbpf_err(err);
> > +       }
> >
> > -               *prog = bpf_object__find_program_by_name(obj, name);
> > -               if (!*prog) {
> > -                       pr_warn("failed to find skeleton program '%s'\n", name);
> > -                       return libbpf_err(-ESRCH);
> > +       for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
> > +               var_skel = &s->vars[var_idx];
> > +               map = *var_skel->map;
> > +               map_type_id = bpf_map__btf_value_type_id(map);
> > +               map_type = btf__type_by_id(btf, map_type_id);
> > +
>
> should we double-check that map_type is DATASEC?
>
> > +               len = btf_vlen(map_type);
> > +               var = btf_var_secinfos(map_type);
> > +               for (i = 0; i < len; i++, var++) {
> > +                       var_type = btf__type_by_id(btf, var->type);
> > +                       if (!var_type) {
>
> unless BTF itself is corrupted, this shouldn't ever happen. So
> checking for DATASEC should be enough and this if (!var_type) is
> redundant
>
> > +                               pr_warn("Could not find var type for item %1$d in section %2$s",
> > +                                       i, bpf_map__name(map));
> > +                               return libbpf_err(-EINVAL);
> > +                       }
> > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > +                       if (strcmp(var_name, var_skel->name) == 0) {
> > +                               *var_skel->addr = (char *) map->mmaped + var->offset;
>
> is (char *) cast necessary? C allows pointer adjustment on void *, so
> shouldn't be

oh, wait, it's so that C++ compiler doesn't complain, never mind

>
> > +                               break;
> > +                       }
> >                 }
> >         }
> > -
> >         return 0;
> >  }
> >
>
> [...]
>
> >  struct gen_loader_opts {
> >         size_t sz; /* size of this struct, for forward/backward compatiblity */
> >         const char *data;
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index df1b947792c8..d744fbb8612e 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
> >
> >  LIBBPF_0.8.0 {
> >         global:
> > +               bpf_object__open_subskeleton;
> > +               bpf_object__destroy_subskeleton;
>
> nit: should be in alphabetic order
>
> >                 libbpf_register_prog_handler;
> >                 libbpf_unregister_prog_handler;
> >  } LIBBPF_0.7.0;
> > --
> > 2.34.1

Re: [PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Delyan Kratunov]

Thanks Andrii!

On Fri, 2022-03-11 at 15:08 -0800, Andrii Nakryiko wrote:
> On Fri, Mar 11, 2022 at 2:52 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> 
> > 

[...]

> > > +
> > > +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> > > +       if (err) {
> > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > +               return libbpf_err(err);
> > >         }
> > > 
> > > -       for (i = 0; i < s->prog_cnt; i++) {
> > > -               struct bpf_program **prog = s->progs[i].prog;
> > > -               const char *name = s->progs[i].name;
> > > +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> > > +       if (err) {
> > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > +               return libbpf_err(err);
> > > +       }
> > > 
> > > -               *prog = bpf_object__find_program_by_name(obj, name);
> > > -               if (!*prog) {
> > > -                       pr_warn("failed to find skeleton program '%s'\n", name);
> > > -                       return libbpf_err(-ESRCH);
> > > +       for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
> > > +               var_skel = &s->vars[var_idx];
> > > +               map = *var_skel->map;
> > > +               map_type_id = bpf_map__btf_value_type_id(map);
> > > +               map_type = btf__type_by_id(btf, map_type_id);
> > > +
> > 
> > should we double-check that map_type is DATASEC?

Sure, can do.

> > 
> > > +               len = btf_vlen(map_type);
> > > +               var = btf_var_secinfos(map_type);
> > > +               for (i = 0; i < len; i++, var++) {
> > > +                       var_type = btf__type_by_id(btf, var->type);
> > > +                       if (!var_type) {
> > 
> > unless BTF itself is corrupted, this shouldn't ever happen. So
> > checking for DATASEC should be enough and this if (!var_type) is
> > redundant
> > 
> > > +                               pr_warn("Could not find var type for item %1$d in section %2$s",
> > > +                                       i, bpf_map__name(map));
> > > +                               return libbpf_err(-EINVAL);
> > > +                       }
> > > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > > +                       if (strcmp(var_name, var_skel->name) == 0) {
> > > +                               *var_skel->addr = (char *) map->mmaped + var->offset;
> > 
> > is (char *) cast necessary? C allows pointer adjustment on void *, so
> > shouldn't be
> 
> oh, wait, it's so that C++ compiler doesn't complain, never mind

This is libbpf code, not subskel code, so it shouldn't get compiled as C++. It's
really because of -Wpointer-arith and -Werror.

> 
> > 
> > > +                               break;
> > > +                       }
> > >                 }
> > >         }
> > > -
> > >         return 0;
> > >  }
> > > 
> > 
> > [...]
> > 
> > >  struct gen_loader_opts {
> > >         size_t sz; /* size of this struct, for forward/backward compatiblity */
> > >         const char *data;
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index df1b947792c8..d744fbb8612e 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
> > > 
> > >  LIBBPF_0.8.0 {
> > >         global:
> > > +               bpf_object__open_subskeleton;
> > > +               bpf_object__destroy_subskeleton;
> > 
> > nit: should be in alphabetic order
> > 
> > >                 libbpf_register_prog_handler;
> > >                 libbpf_unregister_prog_handler;
> > >  } LIBBPF_0.7.0;
> > > --
> > > 2.34.1

Re: [PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Andrii Nakryiko]

On Fri, Mar 11, 2022 at 3:28 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Thanks Andrii!
>
> On Fri, 2022-03-11 at 15:08 -0800, Andrii Nakryiko wrote:
> > On Fri, Mar 11, 2022 at 2:52 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >
> > >
>
> [...]
>
> > > > +
> > > > +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> > > > +       if (err) {
> > > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > > +               return libbpf_err(err);
> > > >         }
> > > >
> > > > -       for (i = 0; i < s->prog_cnt; i++) {
> > > > -               struct bpf_program **prog = s->progs[i].prog;
> > > > -               const char *name = s->progs[i].name;
> > > > +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> > > > +       if (err) {
> > > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > > +               return libbpf_err(err);
> > > > +       }
> > > >
> > > > -               *prog = bpf_object__find_program_by_name(obj, name);
> > > > -               if (!*prog) {
> > > > -                       pr_warn("failed to find skeleton program '%s'\n", name);
> > > > -                       return libbpf_err(-ESRCH);
> > > > +       for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
> > > > +               var_skel = &s->vars[var_idx];
> > > > +               map = *var_skel->map;
> > > > +               map_type_id = bpf_map__btf_value_type_id(map);
> > > > +               map_type = btf__type_by_id(btf, map_type_id);
> > > > +
> > >
> > > should we double-check that map_type is DATASEC?
>
> Sure, can do.
>
> > >
> > > > +               len = btf_vlen(map_type);
> > > > +               var = btf_var_secinfos(map_type);
> > > > +               for (i = 0; i < len; i++, var++) {
> > > > +                       var_type = btf__type_by_id(btf, var->type);
> > > > +                       if (!var_type) {
> > >
> > > unless BTF itself is corrupted, this shouldn't ever happen. So
> > > checking for DATASEC should be enough and this if (!var_type) is
> > > redundant
> > >
> > > > +                               pr_warn("Could not find var type for item %1$d in section %2$s",
> > > > +                                       i, bpf_map__name(map));
> > > > +                               return libbpf_err(-EINVAL);
> > > > +                       }
> > > > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > > > +                       if (strcmp(var_name, var_skel->name) == 0) {
> > > > +                               *var_skel->addr = (char *) map->mmaped + var->offset;
> > >
> > > is (char *) cast necessary? C allows pointer adjustment on void *, so
> > > shouldn't be
> >
> > oh, wait, it's so that C++ compiler doesn't complain, never mind
>
> This is libbpf code, not subskel code, so it shouldn't get compiled as C++. It's
> really because of -Wpointer-arith and -Werror.

Oh, if it's libbpf code then it shouldn't be necessary, after all. I'm
pretty sure we assume in many places that we can do pointer arithmetic
on void *. Did you try and it didn't compile? Or you just did it
preemptively?

>
> >
> > >
> > > > +                               break;
> > > > +                       }
> > > >                 }
> > > >         }
> > > > -
> > > >         return 0;
> > > >  }
> > > >
> > >
> > > [...]
> > >
> > > >  struct gen_loader_opts {
> > > >         size_t sz; /* size of this struct, for forward/backward compatiblity */
> > > >         const char *data;
> > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > > index df1b947792c8..d744fbb8612e 100644
> > > > --- a/tools/lib/bpf/libbpf.map
> > > > +++ b/tools/lib/bpf/libbpf.map
> > > > @@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
> > > >
> > > >  LIBBPF_0.8.0 {
> > > >         global:
> > > > +               bpf_object__open_subskeleton;
> > > > +               bpf_object__destroy_subskeleton;
> > >
> > > nit: should be in alphabetic order
> > >
> > > >                 libbpf_register_prog_handler;
> > > >                 libbpf_unregister_prog_handler;
> > > >  } LIBBPF_0.7.0;
> > > > --
> > > > 2.34.1
>

[PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Delyan Kratunov]

Subskeletons are headers which require an already loaded program to
operate.

For example, when a BPF library is linked into a larger BPF object file,
the library userspace needs a way to access its own global variables
without requiring knowledge about the larger program at build time.

As a result, subskeletons require a loaded bpf_object to open().
Further, they find their own symbols in the larger program by
walking BTF type data at run time.

At this time, programs, maps, and globals are supported through
non-owning pointers.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 .../bpf/bpftool/Documentation/bpftool-gen.rst |  25 +
 tools/bpf/bpftool/bash-completion/bpftool     |  14 +-
 tools/bpf/bpftool/gen.c                       | 589 ++++++++++++++++--
 3 files changed, 564 insertions(+), 64 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-gen.rst b/tools/bpf/bpftool/Documentation/bpftool-gen.rst
index 18d646b571ec..68454ef28f58 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 subskeleton** *FILE* [**name** *OBJECT_NAME*]
 |	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECT* [*OBJECT*...]
 |	**bpftool** **gen help**
 
@@ -150,6 +151,30 @@ DESCRIPTION
 		  (non-read-only) data from userspace, with same simplicity
 		  as for BPF side.
 
+	**bpftool gen subskeleton** *FILE*
+		  Generate BPF subskeleton C header file for a given *FILE*.
+
+		  Subskeletons are similar to skeletons, except they do not own
+		  the corresponding maps, programs, or global variables. They
+		  require that the object file used to generate them is already
+		  loaded into a *bpf_object* by some other means.
+
+		  This functionality is useful when a library is included into a
+		  larger BPF program. A subskeleton for the library would have
+		  access to all objects and globals defined in it, without
+		  having to know about the larger program.
+
+		  Consequently, there are only two functions defined
+		  for subskeletons:
+
+		  - **example__open(bpf_object\*)**
+		    Instantiates a subskeleton from an already opened (but not
+		    necessarily loaded) **bpf_object**.
+
+		  - **example__destroy()**
+		    Frees the storage for the subskeleton but *does not* unload
+		    any BPF programs or maps.
+
 	**bpftool** **gen min_core_btf** *INPUT* *OUTPUT* *OBJECT* [*OBJECT*...]
 		  Generate a minimum BTF file as *OUTPUT*, derived from a given
 		  *INPUT* BTF file, containing all needed BTF types so one, or
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 958e1fd71b5c..5df8d72c5179 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -1003,13 +1003,25 @@ _bpftool()
                             ;;
                     esac
                     ;;
+                subskeleton)
+                    case $prev in
+                        $command)
+                            _filedir
+                            return 0
+                            ;;
+                        *)
+                            _bpftool_once_attr 'name'
+                            return 0
+                            ;;
+                    esac
+                    ;;
                 min_core_btf)
                     _filedir
                     return 0
                     ;;
                 *)
                     [[ $prev == $object ]] && \
-                        COMPREPLY=( $( compgen -W 'object skeleton help min_core_btf' -- "$cur" ) )
+                        COMPREPLY=( $( compgen -W 'object skeleton subskeleton help min_core_btf' -- "$cur" ) )
                     ;;
             esac
             ;;
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 145734b4fe41..3f05e3df94cf 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -64,11 +64,11 @@ static void get_obj_name(char *name, const char *file)
 	sanitize_identifier(name);
 }
 
-static void get_header_guard(char *guard, const char *obj_name)
+static void get_header_guard(char *guard, const char *obj_name, const char *suffix)
 {
 	int i;
 
-	sprintf(guard, "__%s_SKEL_H__", obj_name);
+	sprintf(guard, "__%s_%s__", obj_name, suffix);
 	for (i = 0; guard[i]; i++)
 		guard[i] = toupper(guard[i]);
 }
@@ -280,6 +280,122 @@ static int codegen_datasecs(struct bpf_object *obj, const char *obj_name)
 	return err;
 }
 
+static bool btf_is_ptr_to_func_proto(const struct btf *btf,
+				     const struct btf_type *v)
+{
+	return btf_is_ptr(v) && btf_is_func_proto(btf__type_by_id(btf, v->type));
+}
+
+static int codegen_subskel_datasecs(struct bpf_object *obj, const char *obj_name)
+{
+	struct btf *btf = bpf_object__btf(obj);
+	struct btf_dump *d;
+	struct bpf_map *map;
+	const struct btf_type *sec, *var;
+	const struct btf_var_secinfo *sec_var;
+	int i, err, vlen;
+	char map_ident[256], var_ident[256], sec_ident[256];
+	bool strip_mods = false;
+	const char *sec_name, *var_name;
+	__u32 var_type_id;
+
+	d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
+	err = libbpf_get_error(d);
+	if (err)
+		return err;
+
+	bpf_object__for_each_map(map, obj) {
+		/* only generate definitions for memory-mapped internal maps */
+		if (!bpf_map__is_internal(map))
+			continue;
+		if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
+			continue;
+		if (!get_map_ident(map, map_ident, sizeof(map_ident)))
+			continue;
+
+		sec = find_type_for_map(btf, map_ident);
+		if (!sec)
+			continue;
+
+		sec_name = btf__name_by_offset(btf, sec->name_off);
+		if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident)))
+			continue;
+
+		if (strcmp(sec_name, ".kconfig") != 0)
+			strip_mods = true;
+
+		printf("	struct %s__%s {\n", obj_name, sec_ident);
+
+		sec_var = btf_var_secinfos(sec);
+		vlen = btf_vlen(sec);
+		for (i = 0; i < vlen; i++, sec_var++) {
+			DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
+				.field_name = var_ident,
+				.indent_level = 2,
+				.strip_mods = strip_mods,
+			);
+
+			var = btf__type_by_id(btf, sec_var->type);
+			var_name = btf__name_by_offset(btf, var->name_off);
+			var_type_id = var->type;
+
+			/* static variables are not exposed through BPF skeleton */
+			if (btf_var(var)->linkage == BTF_VAR_STATIC)
+				continue;
+
+			/* leave the first character for a * prefix, size - 2
+			 * as a result as well
+			 */
+			var_ident[0] = '\0';
+			var_ident[1] = '\0';
+			strncat(var_ident + 1, var_name, sizeof(var_ident) - 2);
+
+			/* sanitize variable name, e.g., for static vars inside
+			 * a function, it's name is '<function name>.<variable name>',
+			 * which we'll turn into a '<function name>_<variable name>'.
+			 */
+			sanitize_identifier(var_ident + 1);
+			var_ident[0] = ' ';
+
+			/* The datasec member has KIND_VAR but we want the
+			 * underlying type of the variable (e.g. KIND_INT).
+			 */
+			var = btf__type_by_id(btf, var->type);
+
+			/* Prepend * to the field name to make it a pointer. */
+			var_ident[0] = '*';
+
+			printf("\t\t");
+			/* Func and array members require special handling.
+			 * Instead of producing `typename *var`, they produce
+			 * `typeof(typename) *var`. This allows us to keep a
+			 * similar syntax where the identifier is just prefixed
+			 * by *, allowing us to ignore C declaration minutae.
+			 */
+			if (btf_is_array(var) ||
+			    btf_is_ptr_to_func_proto(btf, var)) {
+				printf("typeof(");
+				/* print the type inside typeof() without a name */
+				opts.field_name = "";
+				err = btf_dump__emit_type_decl(d, var_type_id, &opts);
+				if (err)
+					goto out;
+				printf(") %s", var_ident);
+			} else {
+				err = btf_dump__emit_type_decl(d, var_type_id, &opts);
+				if (err)
+					goto out;
+			}
+			printf(";\n");
+		}
+		printf("	} %s;\n", sec_ident);
+	}
+
+out:
+	btf_dump__free(d);
+	return err;
+}
+
 static void codegen(const char *template, ...)
 {
 	const char *src, *end;
@@ -727,10 +843,93 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
 	return err;
 }
 
+static void
+codegen_maps_skeleton(struct bpf_object *obj, size_t map_cnt, bool mmaped)
+{
+	struct bpf_map *map;
+	char ident[256];
+	size_t i;
+
+	if (map_cnt) {
+		codegen("\
+			\n\
+									    \n\
+				/* maps */				    \n\
+				s->map_cnt = %zu;			    \n\
+				s->map_skel_sz = sizeof(*s->maps);	    \n\
+				s->maps = (struct bpf_map_skeleton *)calloc(s->map_cnt, s->map_skel_sz);\n\
+				if (!s->maps)				    \n\
+					goto err;			    \n\
+			",
+			map_cnt
+		);
+		i = 0;
+		bpf_object__for_each_map(map, obj) {
+			if (!get_map_ident(map, ident, sizeof(ident)))
+				continue;
+
+			codegen("\
+				\n\
+									    \n\
+					s->maps[%zu].name = \"%s\";	    \n\
+					s->maps[%zu].map = &obj->maps.%s;   \n\
+				",
+				i, bpf_map__name(map), i, ident);
+			/* memory-mapped internal maps */
+			if (mmaped && bpf_map__is_internal(map) &&
+			    (bpf_map__map_flags(map) & BPF_F_MMAPABLE)) {
+				printf("\ts->maps[%zu].mmaped = (void **)&obj->%s;\n",
+				       i, ident);
+			}
+			i++;
+		}
+	}
+}
+
+static void
+codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_links)
+{
+	struct bpf_program *prog;
+	int i;
+
+	if (prog_cnt) {
+		codegen("\
+			\n\
+									    \n\
+				/* programs */				    \n\
+				s->prog_cnt = %zu;			    \n\
+				s->prog_skel_sz = sizeof(*s->progs);	    \n\
+				s->progs = (struct bpf_prog_skeleton *)calloc(s->prog_cnt, s->prog_skel_sz);\n\
+				if (!s->progs)				    \n\
+					goto err;			    \n\
+			",
+			prog_cnt
+		);
+		i = 0;
+		bpf_object__for_each_program(prog, obj) {
+			codegen("\
+				\n\
+									    \n\
+					s->progs[%1$zu].name = \"%2$s\";    \n\
+					s->progs[%1$zu].prog = &obj->progs.%2$s;\n\
+				",
+				i, bpf_program__name(prog));
+
+			if (populate_links)
+				codegen("\
+					\n\
+						s->progs[%1$zu].link = &obj->links.%2$s;\n\
+					",
+					i, bpf_program__name(prog));
+			i++;
+		}
+	}
+}
+
 static int do_skeleton(int argc, char **argv)
 {
 	char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
-	size_t i, map_cnt = 0, prog_cnt = 0, file_sz, mmap_sz;
+	size_t map_cnt = 0, prog_cnt = 0, file_sz, mmap_sz;
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
 	char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data;
 	struct bpf_object *obj = NULL;
@@ -821,7 +1020,7 @@ static int do_skeleton(int argc, char **argv)
 		prog_cnt++;
 	}
 
-	get_header_guard(header_guard, obj_name);
+	get_header_guard(header_guard, obj_name, "SKEL_H");
 	if (use_loader) {
 		codegen("\
 		\n\
@@ -1024,66 +1223,10 @@ static int do_skeleton(int argc, char **argv)
 		",
 		obj_name
 	);
-	if (map_cnt) {
-		codegen("\
-			\n\
-									    \n\
-				/* maps */				    \n\
-				s->map_cnt = %zu;			    \n\
-				s->map_skel_sz = sizeof(*s->maps);	    \n\
-				s->maps = (struct bpf_map_skeleton *)calloc(s->map_cnt, s->map_skel_sz);\n\
-				if (!s->maps)				    \n\
-					goto err;			    \n\
-			",
-			map_cnt
-		);
-		i = 0;
-		bpf_object__for_each_map(map, obj) {
-			if (!get_map_ident(map, ident, sizeof(ident)))
-				continue;
 
-			codegen("\
-				\n\
-									    \n\
-					s->maps[%zu].name = \"%s\";	    \n\
-					s->maps[%zu].map = &obj->maps.%s;   \n\
-				",
-				i, bpf_map__name(map), i, ident);
-			/* memory-mapped internal maps */
-			if (bpf_map__is_internal(map) &&
-			    (bpf_map__map_flags(map) & BPF_F_MMAPABLE)) {
-				printf("\ts->maps[%zu].mmaped = (void **)&obj->%s;\n",
-				       i, ident);
-			}
-			i++;
-		}
-	}
-	if (prog_cnt) {
-		codegen("\
-			\n\
-									    \n\
-				/* programs */				    \n\
-				s->prog_cnt = %zu;			    \n\
-				s->prog_skel_sz = sizeof(*s->progs);	    \n\
-				s->progs = (struct bpf_prog_skeleton *)calloc(s->prog_cnt, s->prog_skel_sz);\n\
-				if (!s->progs)				    \n\
-					goto err;			    \n\
-			",
-			prog_cnt
-		);
-		i = 0;
-		bpf_object__for_each_program(prog, obj) {
-			codegen("\
-				\n\
-									    \n\
-					s->progs[%1$zu].name = \"%2$s\";    \n\
-					s->progs[%1$zu].prog = &obj->progs.%2$s;\n\
-					s->progs[%1$zu].link = &obj->links.%2$s;\n\
-				",
-				i, bpf_program__name(prog));
-			i++;
-		}
-	}
+	codegen_maps_skeleton(obj, map_cnt, true /*mmaped*/);
+	codegen_progs_skeleton(obj, prog_cnt, true /*populate_links*/);
+
 	codegen("\
 		\n\
 									    \n\
@@ -1141,6 +1284,324 @@ static int do_skeleton(int argc, char **argv)
 	return err;
 }
 
+/* Subskeletons are like skeletons, except they don't own the bpf_object,
+ * associated maps, links, etc. Instead, they know about the existence of
+ * variables, maps, programs and are able to find their locations
+ * _at runtime_ from an already loaded bpf_object.
+ *
+ * This allows for library-like BPF objects to have userspace counterparts
+ * with access to their own items without having to know anything about the
+ * final BPF object that the library was linked into.
+ */
+static int do_subskeleton(int argc, char **argv)
+{
+	char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SUBSKEL_H__")];
+	size_t i, len, file_sz, map_cnt = 0, prog_cnt = 0, mmap_sz, var_cnt = 0, var_idx = 0;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	char obj_name[MAX_OBJ_NAME_LEN] = "", *obj_data;
+	struct bpf_object *obj = NULL;
+	const char *file, *var_name;
+	char ident[256], var_ident[256];
+	int fd, err = -1, map_type_id;
+	const struct bpf_map *map;
+	struct bpf_program *prog;
+	struct btf *btf;
+	const struct btf_type *map_type, *var_type;
+	const struct btf_var_secinfo *var;
+	struct stat st;
+
+	if (!REQ_ARGS(1)) {
+		usage();
+		return -1;
+	}
+	file = GET_ARG();
+
+	while (argc) {
+		if (!REQ_ARGS(2))
+			return -1;
+
+		if (is_prefix(*argv, "name")) {
+			NEXT_ARG();
+
+			if (obj_name[0] != '\0') {
+				p_err("object name already specified");
+				return -1;
+			}
+
+			strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
+			obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
+		} else {
+			p_err("unknown arg %s", *argv);
+			return -1;
+		}
+
+		NEXT_ARG();
+	}
+
+	if (argc) {
+		p_err("extra unknown arguments");
+		return -1;
+	}
+
+	if (use_loader) {
+		p_err("cannot use loader for subskeletons");
+		return -1;
+	}
+
+	if (stat(file, &st)) {
+		p_err("failed to stat() %s: %s", file, strerror(errno));
+		return -1;
+	}
+	file_sz = st.st_size;
+	mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE));
+	fd = open(file, O_RDONLY);
+	if (fd < 0) {
+		p_err("failed to open() %s: %s", file, strerror(errno));
+		return -1;
+	}
+	obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (obj_data == MAP_FAILED) {
+		obj_data = NULL;
+		p_err("failed to mmap() %s: %s", file, strerror(errno));
+		goto out;
+	}
+	if (obj_name[0] == '\0')
+		get_obj_name(obj_name, file);
+
+	/* The empty object name allows us to use bpf_map__name and produce
+	 * ELF section names out of it. (".data" instead of "obj.data")
+	 */
+	opts.object_name = "";
+	obj = bpf_object__open_mem(obj_data, file_sz, &opts);
+	if (!obj) {
+		char err_buf[256];
+
+		libbpf_strerror(errno, err_buf, sizeof(err_buf));
+		p_err("failed to open BPF object file: %s", err_buf);
+		obj = NULL;
+		goto out;
+	}
+
+	btf = bpf_object__btf(obj);
+	if (!btf) {
+		err = -1;
+		p_err("need btf type information for %s", obj_name);
+		goto out;
+	}
+
+	bpf_object__for_each_program(prog, obj) {
+		prog_cnt++;
+	}
+
+	/* First, count how many variables we have to find.
+	 * We need this in advance so the subskel can allocate the right
+	 * amount of storage.
+	 */
+	bpf_object__for_each_map(map, obj) {
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+
+		/* Also count all maps that have a name */
+		map_cnt++;
+
+		if (!bpf_map__is_internal(map))
+			continue;
+		if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
+			continue;
+
+		map_type_id = bpf_map__btf_value_type_id(map);
+		if (map_type_id < 0) {
+			err = map_type_id;
+			goto out;
+		}
+		map_type = btf__type_by_id(btf, map_type_id);
+
+		var = btf_var_secinfos(map_type);
+		len = btf_vlen(map_type);
+		for (i = 0; i < len; i++, var++) {
+			var_type = btf__type_by_id(btf, var->type);
+
+			if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
+				continue;
+
+			var_cnt++;
+		}
+	}
+
+	get_header_guard(header_guard, obj_name, "SUBSKEL_H");
+	codegen("\
+	\n\
+	/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
+								    \n\
+	/* THIS FILE IS AUTOGENERATED! */			    \n\
+	#ifndef %2$s						    \n\
+	#define %2$s						    \n\
+								    \n\
+	#include <errno.h>					    \n\
+	#include <stdlib.h>					    \n\
+	#include <bpf/libbpf.h>					    \n\
+								    \n\
+	struct %1$s {						    \n\
+		struct bpf_object *obj;				    \n\
+		struct bpf_object_subskeleton *subskel;		    \n\
+	", obj_name, header_guard);
+
+	if (map_cnt) {
+		printf("\tstruct {\n");
+		bpf_object__for_each_map(map, obj) {
+			if (!get_map_ident(map, ident, sizeof(ident)))
+				continue;
+			printf("\t\tstruct bpf_map *%s;\n", ident);
+		}
+		printf("\t} maps;\n");
+	}
+
+	if (prog_cnt) {
+		printf("\tstruct {\n");
+		bpf_object__for_each_program(prog, obj) {
+			printf("\t\tstruct bpf_program *%s;\n",
+				bpf_program__name(prog));
+		}
+		printf("\t} progs;\n");
+	}
+
+	err = codegen_subskel_datasecs(obj, obj_name);
+	if (err)
+		goto out;
+
+	/* emit code that will allocate enough storage for all symbols */
+	codegen("\
+		\n\
+									    \n\
+		#ifdef __cplusplus					    \n\
+			static inline struct %1$s *open(const struct bpf_object *src);\n\
+			static inline void destroy(struct %1$s *skel);\n\
+		#endif /* __cplusplus */				    \n\
+		};							    \n\
+									    \n\
+		static inline void					    \n\
+		%1$s__destroy(struct %1$s *skel)			    \n\
+		{							    \n\
+			if (!skel)					    \n\
+				return;					    \n\
+			if (skel->subskel)				    \n\
+				bpf_object__destroy_subskeleton(skel->subskel);\n\
+			free(skel);					    \n\
+		}							    \n\
+									    \n\
+		static inline struct %1$s *				    \n\
+		%1$s__open(const struct bpf_object *src)		    \n\
+		{							    \n\
+			struct %1$s *obj;				    \n\
+			struct bpf_object_subskeleton *s;		    \n\
+			int err;					    \n\
+									    \n\
+			obj = (struct %1$s *)calloc(1, sizeof(*obj));	    \n\
+			if (!obj) {					    \n\
+				errno = ENOMEM;				    \n\
+				goto err;				    \n\
+			}						    \n\
+			s = (struct bpf_object_subskeleton *)calloc(1, sizeof(*s));\n\
+			if (!s) {					    \n\
+				errno = ENOMEM;				    \n\
+				goto err;				    \n\
+			}						    \n\
+			s->sz = sizeof(*s);				    \n\
+			s->obj = src;					    \n\
+			s->var_skel_sz = sizeof(*s->vars);		    \n\
+			obj->subskel = s;				    \n\
+									    \n\
+			/* vars */					    \n\
+			s->var_cnt = %2$d;				    \n\
+			s->vars = (struct bpf_var_skeleton *)calloc(%2$d, sizeof(*s->vars));\n\
+			if (!s->vars) {					    \n\
+				errno = ENOMEM;				    \n\
+				goto err;				    \n\
+			}						    \n\
+									    \n\
+		",
+		obj_name, var_cnt
+	);
+
+	/* walk through each symbol and emit the runtime representation */
+	bpf_object__for_each_map(map, obj) {
+		if (!bpf_map__is_internal(map))
+			continue;
+
+		if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
+			continue;
+
+		if (!get_map_ident(map, ident, sizeof(ident)))
+			continue;
+
+		map_type_id = bpf_map__btf_value_type_id(map);
+		if (map_type_id < 0)
+			/* skip over internal maps with no type*/
+			continue;
+
+		map_type = btf__type_by_id(btf, map_type_id);
+		var = btf_var_secinfos(map_type);
+		len = btf_vlen(map_type);
+		for (i = 0; i < len; i++, var++) {
+			var_type = btf__type_by_id(btf, var->type);
+			var_name = btf__name_by_offset(btf, var_type->name_off);
+
+			if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
+				continue;
+
+			var_ident[0] = '\0';
+			strncat(var_ident, var_name, sizeof(var_ident) - 1);
+			sanitize_identifier(var_ident);
+
+			/* Note that we use the dot prefix in .data as the
+			 * field access operator i.e. maps%s becomes maps.data
+			 */
+			codegen("\
+			\n\
+				s->vars[%4$d].name = \"%1$s\";		    \n\
+				s->vars[%4$d].map = &obj->maps%3$s;	    \n\
+				s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
+			", var_ident, ident, bpf_map__name(map), var_idx);
+
+			var_idx++;
+		}
+	}
+
+	codegen_maps_skeleton(obj, map_cnt, false /*mmaped*/);
+	codegen_progs_skeleton(obj, prog_cnt, false /*links*/);
+
+	codegen("\
+		\n\
+									    \n\
+			err = bpf_object__open_subskeleton(s);		    \n\
+			if (err) {					    \n\
+				errno = err;				    \n\
+				goto err;				    \n\
+			}						    \n\
+									    \n\
+			return obj;					    \n\
+		err:							    \n\
+			%1$s__destroy(obj);				    \n\
+			return NULL;					    \n\
+		}							    \n\
+									    \n\
+		#ifdef __cplusplus					    \n\
+		struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\
+		void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\
+		#endif /* __cplusplus */				    \n\
+									    \n\
+		#endif /* %2$s */					    \n\
+		",
+		obj_name, header_guard);
+	err = 0;
+out:
+	bpf_object__close(obj);
+	if (obj_data)
+		munmap(obj_data, mmap_sz);
+	close(fd);
+	return err;
+}
+
 static int do_object(int argc, char **argv)
 {
 	struct bpf_linker *linker;
@@ -1192,6 +1653,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 subskeleton FILE [name OBJECT_NAME]\n"
 		"       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
 		"       %1$s %2$s help\n"
 		"\n"
@@ -1788,6 +2250,7 @@ static int do_min_core_btf(int argc, char **argv)
 static const struct cmd cmds[] = {
 	{ "object",		do_object },
 	{ "skeleton",		do_skeleton },
+	{ "subskeleton",	do_subskeleton },
 	{ "min_core_btf",	do_min_core_btf},
 	{ "help",		do_help },
 	{ 0 }
-- 
2.34.1

Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Subskeletons are headers which require an already loaded program to
> operate.
>
> For example, when a BPF library is linked into a larger BPF object file,
> the library userspace needs a way to access its own global variables
> without requiring knowledge about the larger program at build time.
>
> As a result, subskeletons require a loaded bpf_object to open().
> Further, they find their own symbols in the larger program by
> walking BTF type data at run time.
>
> At this time, programs, maps, and globals are supported through
> non-owning pointers.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  .../bpf/bpftool/Documentation/bpftool-gen.rst |  25 +
>  tools/bpf/bpftool/bash-completion/bpftool     |  14 +-
>  tools/bpf/bpftool/gen.c                       | 589 ++++++++++++++++--
>  3 files changed, 564 insertions(+), 64 deletions(-)
>

Few comments below, but overall looks great!

[...]

> +static bool btf_is_ptr_to_func_proto(const struct btf *btf,
> +                                    const struct btf_type *v)
> +{
> +       return btf_is_ptr(v) && btf_is_func_proto(btf__type_by_id(btf, v->type));
> +}
> +
> +static int codegen_subskel_datasecs(struct bpf_object *obj, const char *obj_name)
> +{
> +       struct btf *btf = bpf_object__btf(obj);
> +       struct btf_dump *d;
> +       struct bpf_map *map;
> +       const struct btf_type *sec, *var;
> +       const struct btf_var_secinfo *sec_var;
> +       int i, err, vlen;
> +       char map_ident[256], var_ident[256], sec_ident[256];
> +       bool strip_mods = false;
> +       const char *sec_name, *var_name;
> +       __u32 var_type_id;
> +
> +       d = btf_dump__new(btf, codegen_btf_dump_printf, NULL, NULL);
> +       err = libbpf_get_error(d);

nit: no need to use libbpf_get_error() in libbpf 1.0 mode

> +       if (err)
> +               return err;
> +
> +       bpf_object__for_each_map(map, obj) {
> +               /* only generate definitions for memory-mapped internal maps */
> +               if (!bpf_map__is_internal(map))
> +                       continue;
> +               if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> +                       continue;
> +               if (!get_map_ident(map, map_ident, sizeof(map_ident)))
> +                       continue;

extract these three checks into helper and reuse from codegen_asserts
and codegen_datasecs?


> +
> +               sec = find_type_for_map(btf, map_ident);
> +               if (!sec)
> +                       continue;
> +
> +               sec_name = btf__name_by_offset(btf, sec->name_off);
> +               if (!get_datasec_ident(sec_name, sec_ident, sizeof(sec_ident)))
> +                       continue;
> +
> +               if (strcmp(sec_name, ".kconfig") != 0)
> +                       strip_mods = true;
> +
> +               printf("        struct %s__%s {\n", obj_name, sec_ident);
> +
> +               sec_var = btf_var_secinfos(sec);
> +               vlen = btf_vlen(sec);
> +               for (i = 0; i < vlen; i++, sec_var++) {
> +                       DECLARE_LIBBPF_OPTS(btf_dump_emit_type_decl_opts, opts,
> +                               .field_name = var_ident,
> +                               .indent_level = 2,
> +                               .strip_mods = strip_mods,
> +                       );
> +
> +                       var = btf__type_by_id(btf, sec_var->type);
> +                       var_name = btf__name_by_offset(btf, var->name_off);
> +                       var_type_id = var->type;
> +
> +                       /* static variables are not exposed through BPF skeleton */
> +                       if (btf_var(var)->linkage == BTF_VAR_STATIC)
> +                               continue;
> +
> +                       /* leave the first character for a * prefix, size - 2
> +                        * as a result as well
> +                        */
> +                       var_ident[0] = '\0';
> +                       var_ident[1] = '\0';
> +                       strncat(var_ident + 1, var_name, sizeof(var_ident) - 2);
> +
> +                       /* sanitize variable name, e.g., for static vars inside
> +                        * a function, it's name is '<function name>.<variable name>',
> +                        * which we'll turn into a '<function name>_<variable name>'.
> +                        */
> +                       sanitize_identifier(var_ident + 1);

btw, I think we don't need sanitization anymore. We needed it for
static variables (they would be of the form <func_name>.<var_name> for
static variables inside the functions), but now it's just unnecessary
complication

> +                       var_ident[0] = ' ';
> +
> +                       /* The datasec member has KIND_VAR but we want the
> +                        * underlying type of the variable (e.g. KIND_INT).
> +                        */
> +                       var = btf__type_by_id(btf, var->type);

you need to use skip_mods_and_typedefs() or equivalent to skip any
const/volatile/restrict modifiers before checking btf_is_array()

> +
> +                       /* Prepend * to the field name to make it a pointer. */
> +                       var_ident[0] = '*';
> +
> +                       printf("\t\t");
> +                       /* Func and array members require special handling.
> +                        * Instead of producing `typename *var`, they produce
> +                        * `typeof(typename) *var`. This allows us to keep a
> +                        * similar syntax where the identifier is just prefixed
> +                        * by *, allowing us to ignore C declaration minutae.
> +                        */
> +                       if (btf_is_array(var) ||
> +                           btf_is_ptr_to_func_proto(btf, var)) {
> +                               printf("typeof(");
> +                               /* print the type inside typeof() without a name */
> +                               opts.field_name = "";
> +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> +                               if (err)
> +                                       goto out;
> +                               printf(") %s", var_ident);
> +                       } else {
> +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> +                               if (err)
> +                                       goto out;
> +                       }
> +                       printf(";\n");

we can simplify this a bit around var_ident and two
btf_dump__emit_type_decl() invocations. We know that we are handling
"non-uniform" C syntax for array and func pointer, so we don't need to
use opts.field_name. Doing this (schematically) should work (taking
into account no need for sanitization as well):

if (btf_is_array() || btf_is_ptr_to_func_proto())
    printf("typeof(");
btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
printf(" *%s", var_name);

or did I miss some corner case?

> +               }
> +               printf("        } %s;\n", sec_ident);
> +       }
> +
> +out:
> +       btf_dump__free(d);
> +       return err;
> +}
> +
>  static void codegen(const char *template, ...)
>  {
>         const char *src, *end;
> @@ -727,10 +843,93 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
>         return err;
>  }
>
> +static void
> +codegen_maps_skeleton(struct bpf_object *obj, size_t map_cnt, bool mmaped)
> +{
> +       struct bpf_map *map;
> +       char ident[256];
> +       size_t i;
> +
> +       if (map_cnt) {

invert if, return early, reduce nestedness?

> +               codegen("\
> +                       \n\
> +                                                                           \n\
> +                               /* maps */                                  \n\
> +                               s->map_cnt = %zu;                           \n\
> +                               s->map_skel_sz = sizeof(*s->maps);          \n\
> +                               s->maps = (struct bpf_map_skeleton *)calloc(s->map_cnt, s->map_skel_sz);\n\
> +                               if (!s->maps)                               \n\
> +                                       goto err;                           \n\
> +                       ",
> +                       map_cnt
> +               );
> +               i = 0;
> +               bpf_object__for_each_map(map, obj) {
> +                       if (!get_map_ident(map, ident, sizeof(ident)))
> +                               continue;
> +
> +                       codegen("\
> +                               \n\
> +                                                                           \n\
> +                                       s->maps[%zu].name = \"%s\";         \n\
> +                                       s->maps[%zu].map = &obj->maps.%s;   \n\
> +                               ",
> +                               i, bpf_map__name(map), i, ident);
> +                       /* memory-mapped internal maps */
> +                       if (mmaped && bpf_map__is_internal(map) &&
> +                           (bpf_map__map_flags(map) & BPF_F_MMAPABLE)) {
> +                               printf("\ts->maps[%zu].mmaped = (void **)&obj->%s;\n",
> +                                      i, ident);
> +                       }
> +                       i++;
> +               }
> +       }
> +}
> +
> +static void
> +codegen_progs_skeleton(struct bpf_object *obj, size_t prog_cnt, bool populate_links)
> +{
> +       struct bpf_program *prog;
> +       int i;
> +
> +       if (prog_cnt) {

same as above, reduce nestedness?

> +               codegen("\
> +                       \n\
> +                                                                           \n\
> +                               /* programs */                              \n\
> +                               s->prog_cnt = %zu;                          \n\
> +                               s->prog_skel_sz = sizeof(*s->progs);        \n\
> +                               s->progs = (struct bpf_prog_skeleton *)calloc(s->prog_cnt, s->prog_skel_sz);\n\
> +                               if (!s->progs)                              \n\
> +                                       goto err;                           \n\
> +                       ",
> +                       prog_cnt
> +               );
> +               i = 0;
> +               bpf_object__for_each_program(prog, obj) {
> +                       codegen("\
> +                               \n\
> +                                                                           \n\
> +                                       s->progs[%1$zu].name = \"%2$s\";    \n\
> +                                       s->progs[%1$zu].prog = &obj->progs.%2$s;\n\
> +                               ",
> +                               i, bpf_program__name(prog));
> +
> +                       if (populate_links)

nit: {} ?

> +                               codegen("\
> +                                       \n\
> +                                               s->progs[%1$zu].link = &obj->links.%2$s;\n\
> +                                       ",
> +                                       i, bpf_program__name(prog));
> +                       i++;
> +               }
> +       }
> +}
> +

[...]

> +       if (!REQ_ARGS(1)) {
> +               usage();
> +               return -1;
> +       }
> +       file = GET_ARG();
> +
> +       while (argc) {
> +               if (!REQ_ARGS(2))
> +                       return -1;
> +
> +               if (is_prefix(*argv, "name")) {
> +                       NEXT_ARG();
> +
> +                       if (obj_name[0] != '\0') {
> +                               p_err("object name already specified");
> +                               return -1;
> +                       }
> +
> +                       strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
> +                       obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';

we don't know the name of the final object, why would we allow to set
any object name at all?

> +               } else {
> +                       p_err("unknown arg %s", *argv);
> +                       return -1;
> +               }
> +
> +               NEXT_ARG();
> +       }
> +
> +       if (argc) {
> +               p_err("extra unknown arguments");
> +               return -1;
> +       }
> +
> +       if (use_loader) {
> +               p_err("cannot use loader for subskeletons");
> +               return -1;
> +       }
> +
> +       if (stat(file, &st)) {
> +               p_err("failed to stat() %s: %s", file, strerror(errno));
> +               return -1;
> +       }
> +       file_sz = st.st_size;
> +       mmap_sz = roundup(file_sz, sysconf(_SC_PAGE_SIZE));
> +       fd = open(file, O_RDONLY);
> +       if (fd < 0) {
> +               p_err("failed to open() %s: %s", file, strerror(errno));
> +               return -1;
> +       }
> +       obj_data = mmap(NULL, mmap_sz, PROT_READ, MAP_PRIVATE, fd, 0);
> +       if (obj_data == MAP_FAILED) {
> +               obj_data = NULL;
> +               p_err("failed to mmap() %s: %s", file, strerror(errno));
> +               goto out;
> +       }
> +       if (obj_name[0] == '\0')
> +               get_obj_name(obj_name, file);
> +
> +       /* The empty object name allows us to use bpf_map__name and produce
> +        * ELF section names out of it. (".data" instead of "obj.data")
> +        */
> +       opts.object_name = "";

yep, like this. So that "name" argument "support" above is bogus,
let's remove it

> +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> +       if (!obj) {
> +               char err_buf[256];
> +
> +               libbpf_strerror(errno, err_buf, sizeof(err_buf));
> +               p_err("failed to open BPF object file: %s", err_buf);
> +               obj = NULL;
> +               goto out;
> +       }
> +

[...]

> +               for (i = 0; i < len; i++, var++) {
> +                       var_type = btf__type_by_id(btf, var->type);
> +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> +
> +                       if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
> +                               continue;
> +
> +                       var_ident[0] = '\0';
> +                       strncat(var_ident, var_name, sizeof(var_ident) - 1);
> +                       sanitize_identifier(var_ident);
> +
> +                       /* Note that we use the dot prefix in .data as the
> +                        * field access operator i.e. maps%s becomes maps.data
> +                        */
> +                       codegen("\
> +                       \n\
> +                               s->vars[%4$d].name = \"%1$s\";              \n\
> +                               s->vars[%4$d].map = &obj->maps%3$s;         \n\
> +                               s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
> +                       ", var_ident, ident, bpf_map__name(map), var_idx);

map reference should be using ident, not bpf_map__name(), as it refers
to a field. The way it is now it shouldn't work for custom
.data.my_section case (do you have a test for this?) You shouldn't
need bpf_map__name() here at all.

> +
> +                       var_idx++;
> +               }
> +       }
> +
> +       codegen_maps_skeleton(obj, map_cnt, false /*mmaped*/);
> +       codegen_progs_skeleton(obj, prog_cnt, false /*links*/);
> +
> +       codegen("\
> +               \n\
> +                                                                           \n\
> +                       err = bpf_object__open_subskeleton(s);              \n\
> +                       if (err) {                                          \n\
> +                               errno = err;                                \n\

see below, best setting errno after __destroy() call (and errno has to
be positive), see below

> +                               goto err;                                   \n\
> +                       }                                                   \n\
> +                                                                           \n\
> +                       return obj;                                         \n\
> +               err:                                                        \n\
> +                       %1$s__destroy(obj);                                 \n\

errno = -err here

> +                       return NULL;                                        \n\
> +               }                                                           \n\
> +                                                                           \n\
> +               #ifdef __cplusplus                                          \n\
> +               struct %1$s *%1$s::open(const struct bpf_object *src) { return %1$s__open(src); }\n\
> +               void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }\n\
> +               #endif /* __cplusplus */                                    \n\
> +                                                                           \n\
> +               #endif /* %2$s */                                           \n\
> +               ",
> +               obj_name, header_guard);
> +       err = 0;
> +out:
> +       bpf_object__close(obj);
> +       if (obj_data)
> +               munmap(obj_data, mmap_sz);
> +       close(fd);
> +       return err;
> +}
> +
>  static int do_object(int argc, char **argv)
>  {
>         struct bpf_linker *linker;
> @@ -1192,6 +1653,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 subskeleton FILE [name OBJECT_NAME]\n"

[name OBJECT_NAME] should be supported

>                 "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
>                 "       %1$s %2$s help\n"
>                 "\n"
> @@ -1788,6 +2250,7 @@ static int do_min_core_btf(int argc, char **argv)
>  static const struct cmd cmds[] = {
>         { "object",             do_object },
>         { "skeleton",           do_skeleton },
> +       { "subskeleton",        do_subskeleton },
>         { "min_core_btf",       do_min_core_btf},
>         { "help",               do_help },
>         { 0 }
> --
> 2.34.1

Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Delyan Kratunov]

Thanks, Andrii! The nits/refactors are straightforward but have some questions
below:

On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > +
> > +                       /* sanitize variable name, e.g., for static vars inside
> > +                        * a function, it's name is '<function name>.<variable name>',
> > +                        * which we'll turn into a '<function name>_<variable name>'.
> > +                        */
> > +                       sanitize_identifier(var_ident + 1);
> 
> btw, I think we don't need sanitization anymore. We needed it for
> static variables (they would be of the form <func_name>.<var_name> for
> static variables inside the functions), but now it's just unnecessary
> complication

How would we handle static variables inside functions in libraries then?

> 
> > +                       var_ident[0] = ' ';
> > +
> > +                       /* The datasec member has KIND_VAR but we want the
> > +                        * underlying type of the variable (e.g. KIND_INT).
> > +                        */
> > +                       var = btf__type_by_id(btf, var->type);
> 
> you need to use skip_mods_and_typedefs() or equivalent to skip any
> const/volatile/restrict modifiers before checking btf_is_array()

Good catch!

> 
> > +
> > +                       /* Prepend * to the field name to make it a pointer. */
> > +                       var_ident[0] = '*';
> > +
> > +                       printf("\t\t");
> > +                       /* Func and array members require special handling.
> > +                        * Instead of producing `typename *var`, they produce
> > +                        * `typeof(typename) *var`. This allows us to keep a
> > +                        * similar syntax where the identifier is just prefixed
> > +                        * by *, allowing us to ignore C declaration minutae.
> > +                        */
> > +                       if (btf_is_array(var) ||
> > +                           btf_is_ptr_to_func_proto(btf, var)) {
> > +                               printf("typeof(");
> > +                               /* print the type inside typeof() without a name */
> > +                               opts.field_name = "";
> > +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > +                               if (err)
> > +                                       goto out;
> > +                               printf(") %s", var_ident);
> > +                       } else {
> > +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > +                               if (err)
> > +                                       goto out;
> > +                       }
> > +                       printf(";\n");
> 
> we can simplify this a bit around var_ident and two
> btf_dump__emit_type_decl() invocations. We know that we are handling
> "non-uniform" C syntax for array and func pointer, so we don't need to
> use opts.field_name. Doing this (schematically) should work (taking
> into account no need for sanitization as well):
> 
> if (btf_is_array() || btf_is_ptr_to_func_proto())
>     printf("typeof(");
> btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
> printf(" *%s", var_name);
> 
> or did I miss some corner case?

You didn't close the "typeof" :) 

if (btf_is_array() || btf_is_ptr_to_func_proto())
     printf("typeof(");
btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
if (btf_is_array() || btf_is_ptr_to_func_proto())
     printf(")");
printf(" *%s", var_name);

If you feel that's easier to understand, sure. I don't love it but it's
understandable enough.

[...]


> we don't know the name of the final object, why would we allow to set
> any object name at all?

We don't really care about the final object name but we do need an object name
for the subskeleton. The subskeleton type name, header guard etc all use it.
We can say that it's always taken from the file name, but giving the user the
option to override it feels right, given the parallel with skeletons (and what
would we do if the file name is a pipe from a subshell invocation?).

> > 
> > +
> > +       /* The empty object name allows us to use bpf_map__name and produce
> > +        * ELF section names out of it. (".data" instead of "obj.data")
> > +        */
> > +       opts.object_name = "";
> 
> yep, like this. So that "name" argument "support" above is bogus,
> let's remove it

See above, it changes real things.

> 
> > +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > +       if (!obj) {
> > +               char err_buf[256];
> > +
> > +               libbpf_strerror(errno, err_buf, sizeof(err_buf));
> > +               p_err("failed to open BPF object file: %s", err_buf);
> > +               obj = NULL;
> > +               goto out;
> > +       }
> > +
> 
> [...]
> 
> > +               for (i = 0; i < len; i++, var++) {
> > +                       var_type = btf__type_by_id(btf, var->type);
> > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > +
> > +                       if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
> > +                               continue;
> > +
> > +                       var_ident[0] = '\0';
> > +                       strncat(var_ident, var_name, sizeof(var_ident) - 1);
> > +                       sanitize_identifier(var_ident);
> > +
> > +                       /* Note that we use the dot prefix in .data as the
> > +                        * field access operator i.e. maps%s becomes maps.data
> > +                        */
> > +                       codegen("\
> > +                       \n\
> > +                               s->vars[%4$d].name = \"%1$s\";              \n\
> > +                               s->vars[%4$d].map = &obj->maps%3$s;         \n\
> > +                               s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
> > +                       ", var_ident, ident, bpf_map__name(map), var_idx);
> 
> map reference should be using ident, not bpf_map__name(), as it refers
> to a field. The way it is now it shouldn't work for custom
> .data.my_section case (do you have a test for this?) You shouldn't
> need bpf_map__name() here at all.

Good catch, I'll add a .data.custom test.

[...]

> 
> > +               "       %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n"
> 
> [name OBJECT_NAME] should be supported

Not sure what you mean by "supported" here.

> 
> >                 "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
> >                 "       %1$s %2$s help\n"
> >                 "\n"
> > @@ -1788,6 +2250,7 @@ static int do_min_core_btf(int argc, char **argv)
> >  static const struct cmd cmds[] = {
> >         { "object",             do_object },
> >         { "skeleton",           do_skeleton },
> > +       { "subskeleton",        do_subskeleton },
> >         { "min_core_btf",       do_min_core_btf},
> >         { "help",               do_help },
> >         { 0 }
> > --
> > 2.34.1

-- Delyan

Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Delyan Kratunov]

On Mon, 2022-03-14 at 16:13 -0700, Delyan Kratunov wrote:
> Thanks, Andrii! The nits/refactors are straightforward but have some questions
> below:
> 
> On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > > +
> > > +                       /* sanitize variable name, e.g., for static vars inside
> > > +                        * a function, it's name is '<function name>.<variable name>',
> > > +                        * which we'll turn into a '<function name>_<variable name>'.
> > > +                        */
> > > +                       sanitize_identifier(var_ident + 1);
> > 
> > btw, I think we don't need sanitization anymore. We needed it for
> > static variables (they would be of the form <func_name>.<var_name> for
> > static variables inside the functions), but now it's just unnecessary
> > complication
> 
> How would we handle static variables inside functions in libraries then?

Ah, just realized 31332ccb7562 (bpftool: Stop emitting static variables in BPF
skeleton) stopped that. I'll remove the sanitization then.

[...]


> > we don't know the name of the final object, why would we allow to set
> > any object name at all?
> 
> We don't really care about the final object name but we do need an object name
> for the subskeleton. The subskeleton type name, header guard etc all use it.
> We can say that it's always taken from the file name, but giving the user the
> option to override it feels right, given the parallel with skeletons (and what
> would we do if the file name is a pipe from a subshell invocation?).

In particular, with the current test setup, if we don't use the name param, we'd
end up inferring names like test_subskeleton_lib_linked3 from the filename. The
tests case is a bit contrived and we can work around it but there may be other
similar situations out there.


-- Delyan

Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Tue, Mar 15, 2022 at 10:28 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Mon, 2022-03-14 at 16:13 -0700, Delyan Kratunov wrote:
> > Thanks, Andrii! The nits/refactors are straightforward but have some questions
> > below:
> >
> > On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > > > +
> > > > +                       /* sanitize variable name, e.g., for static vars inside
> > > > +                        * a function, it's name is '<function name>.<variable name>',
> > > > +                        * which we'll turn into a '<function name>_<variable name>'.
> > > > +                        */
> > > > +                       sanitize_identifier(var_ident + 1);
> > >
> > > btw, I think we don't need sanitization anymore. We needed it for
> > > static variables (they would be of the form <func_name>.<var_name> for
> > > static variables inside the functions), but now it's just unnecessary
> > > complication
> >
> > How would we handle static variables inside functions in libraries then?
>
> Ah, just realized 31332ccb7562 (bpftool: Stop emitting static variables in BPF
> skeleton) stopped that. I'll remove the sanitization then.
>

yep

> [...]
>
>
> > > we don't know the name of the final object, why would we allow to set
> > > any object name at all?
> >
> > We don't really care about the final object name but we do need an object name
> > for the subskeleton. The subskeleton type name, header guard etc all use it.
> > We can say that it's always taken from the file name, but giving the user the
> > option to override it feels right, given the parallel with skeletons (and what
> > would we do if the file name is a pipe from a subshell invocation?).
>
> In particular, with the current test setup, if we don't use the name param, we'd
> end up inferring names like test_subskeleton_lib_linked3 from the filename. The
> tests case is a bit contrived and we can work around it but there may be other
> similar situations out there.
>

yep


>
> -- Delyan

Re: [PATCH bpf-next v2 4/5] bpftool: add support for subskeletons

[Andrii Nakryiko]

On Mon, Mar 14, 2022 at 4:18 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> Thanks, Andrii! The nits/refactors are straightforward but have some questions
> below:
>
> On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote:
> > > +
> > > +                       /* sanitize variable name, e.g., for static vars inside
> > > +                        * a function, it's name is '<function name>.<variable name>',
> > > +                        * which we'll turn into a '<function name>_<variable name>'.
> > > +                        */
> > > +                       sanitize_identifier(var_ident + 1);
> >
> > btw, I think we don't need sanitization anymore. We needed it for
> > static variables (they would be of the form <func_name>.<var_name> for
> > static variables inside the functions), but now it's just unnecessary
> > complication
>
> How would we handle static variables inside functions in libraries then?

That's the thing, we don't expose them (anymore) in skeletons. So we
skip anything static. Anything global should have a unique and valid C
identifier name.

>
> >
> > > +                       var_ident[0] = ' ';
> > > +
> > > +                       /* The datasec member has KIND_VAR but we want the
> > > +                        * underlying type of the variable (e.g. KIND_INT).
> > > +                        */
> > > +                       var = btf__type_by_id(btf, var->type);
> >
> > you need to use skip_mods_and_typedefs() or equivalent to skip any
> > const/volatile/restrict modifiers before checking btf_is_array()
>
> Good catch!
>
> >
> > > +
> > > +                       /* Prepend * to the field name to make it a pointer. */
> > > +                       var_ident[0] = '*';
> > > +
> > > +                       printf("\t\t");
> > > +                       /* Func and array members require special handling.
> > > +                        * Instead of producing `typename *var`, they produce
> > > +                        * `typeof(typename) *var`. This allows us to keep a
> > > +                        * similar syntax where the identifier is just prefixed
> > > +                        * by *, allowing us to ignore C declaration minutae.
> > > +                        */
> > > +                       if (btf_is_array(var) ||
> > > +                           btf_is_ptr_to_func_proto(btf, var)) {
> > > +                               printf("typeof(");
> > > +                               /* print the type inside typeof() without a name */
> > > +                               opts.field_name = "";
> > > +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > > +                               if (err)
> > > +                                       goto out;
> > > +                               printf(") %s", var_ident);
> > > +                       } else {
> > > +                               err = btf_dump__emit_type_decl(d, var_type_id, &opts);
> > > +                               if (err)
> > > +                                       goto out;
> > > +                       }
> > > +                       printf(";\n");
> >
> > we can simplify this a bit around var_ident and two
> > btf_dump__emit_type_decl() invocations. We know that we are handling
> > "non-uniform" C syntax for array and func pointer, so we don't need to
> > use opts.field_name. Doing this (schematically) should work (taking
> > into account no need for sanitization as well):
> >
> > if (btf_is_array() || btf_is_ptr_to_func_proto())
> >     printf("typeof(");
> > btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
> > printf(" *%s", var_name);
> >
> > or did I miss some corner case?
>
> You didn't close the "typeof" :)

Eagle eye :)

>
> if (btf_is_array() || btf_is_ptr_to_func_proto())
>      printf("typeof(");
> btf_dump__emit_type_decl(... /* opts.field_name stays NULL */);
> if (btf_is_array() || btf_is_ptr_to_func_proto())
>      printf(")");
> printf(" *%s", var_name);
>
> If you feel that's easier to understand, sure. I don't love it but it's
> understandable enough.

all the string buffer manipulations seem worse (you can also have a
variable to record the decision whether to use typeof or not, so you
don't have to repeat verbose is_array || is_ptr_to_func_proto check)

>
> [...]
>
>
> > we don't know the name of the final object, why would we allow to set
> > any object name at all?
>
> We don't really care about the final object name but we do need an object name
> for the subskeleton. The subskeleton type name, header guard etc all use it.
> We can say that it's always taken from the file name, but giving the user the
> option to override it feels right, given the parallel with skeletons (and what
> would we do if the file name is a pipe from a subshell invocation?).

Ah, I see, it's sort of like "library name" in this case. Yeah, makes
sense, missed that part. I was too much focused on not letting it get
into map names :)

>
> > >
> > > +
> > > +       /* The empty object name allows us to use bpf_map__name and produce
> > > +        * ELF section names out of it. (".data" instead of "obj.data")
> > > +        */
> > > +       opts.object_name = "";
> >
> > yep, like this. So that "name" argument "support" above is bogus,
> > let's remove it
>
> See above, it changes real things.

yep, my bad

>
> >
> > > +       obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > > +       if (!obj) {
> > > +               char err_buf[256];
> > > +
> > > +               libbpf_strerror(errno, err_buf, sizeof(err_buf));
> > > +               p_err("failed to open BPF object file: %s", err_buf);
> > > +               obj = NULL;
> > > +               goto out;
> > > +       }
> > > +
> >
> > [...]
> >
> > > +               for (i = 0; i < len; i++, var++) {
> > > +                       var_type = btf__type_by_id(btf, var->type);
> > > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > > +
> > > +                       if (btf_var(var_type)->linkage == BTF_VAR_STATIC)
> > > +                               continue;
> > > +
> > > +                       var_ident[0] = '\0';
> > > +                       strncat(var_ident, var_name, sizeof(var_ident) - 1);
> > > +                       sanitize_identifier(var_ident);
> > > +
> > > +                       /* Note that we use the dot prefix in .data as the
> > > +                        * field access operator i.e. maps%s becomes maps.data
> > > +                        */
> > > +                       codegen("\
> > > +                       \n\
> > > +                               s->vars[%4$d].name = \"%1$s\";              \n\
> > > +                               s->vars[%4$d].map = &obj->maps%3$s;         \n\
> > > +                               s->vars[%4$d].addr = (void**) &obj->%2$s.%1$s;\n\
> > > +                       ", var_ident, ident, bpf_map__name(map), var_idx);
> >
> > map reference should be using ident, not bpf_map__name(), as it refers
> > to a field. The way it is now it shouldn't work for custom
> > .data.my_section case (do you have a test for this?) You shouldn't
> > need bpf_map__name() here at all.
>
> Good catch, I'll add a .data.custom test.
>
> [...]
>
> >
> > > +               "       %1$s %2$s subskeleton FILE [name OBJECT_NAME]\n"
> >
> > [name OBJECT_NAME] should be supported
>
> Not sure what you mean by "supported" here.

"not" was missing, but we just concluded that it is indeed needed

>
> >
> > >                 "       %1$s %2$s min_core_btf INPUT OUTPUT OBJECT [OBJECT...]\n"
> > >                 "       %1$s %2$s help\n"
> > >                 "\n"
> > > @@ -1788,6 +2250,7 @@ static int do_min_core_btf(int argc, char **argv)
> > >  static const struct cmd cmds[] = {
> > >         { "object",             do_object },
> > >         { "skeleton",           do_skeleton },
> > > +       { "subskeleton",        do_subskeleton },
> > >         { "min_core_btf",       do_min_core_btf},
> > >         { "help",               do_help },
> > >         { 0 }
> > > --
> > > 2.34.1
>
> -- Delyan
>

[PATCH bpf-next v2 5/5] selftests/bpf: test subskeleton functionality

[Delyan Kratunov]

This patch changes the selftests/bpf Makefile to  also generate
a subskel.h for every skel.h it would have normally generated.

Separately, it also introduces a new subskeleton test which tests
library objects, externs, weak symbols, kconfigs, and user maps.

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/testing/selftests/bpf/.gitignore        |  1 +
 tools/testing/selftests/bpf/Makefile          | 10 ++-
 .../selftests/bpf/prog_tests/subskeleton.c    | 83 +++++++++++++++++++
 .../selftests/bpf/progs/test_subskeleton.c    | 23 +++++
 .../bpf/progs/test_subskeleton_lib.c          | 56 +++++++++++++
 .../bpf/progs/test_subskeleton_lib2.c         | 16 ++++
 6 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index a7eead8820a0..595565eb68c0 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -31,6 +31,7 @@ test_tcp_check_syncookie_user
 test_sysctl
 xdping
 test_cpp
+*.subskel.h
 *.skel.h
 *.lskel.h
 /no_alu32
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index fe12b4f5fe20..9f7b22faedd6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -328,6 +328,12 @@ SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
 		linked_vars.skel.h linked_maps.skel.h
 
+# In the subskeleton case, we want the test_subskeleton_lib.subskel.h file
+# but that's created as a side-effect of the skel.h generation.
+LINKED_SKELS += test_subskeleton.skel.h test_subskeleton_lib.skel.h
+test_subskeleton.skel.h-deps := test_subskeleton_lib2.o test_subskeleton_lib.o test_subskeleton.o
+test_subskeleton_lib.skel.h-deps := test_subskeleton_lib2.o test_subskeleton_lib.o
+
 LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
 	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
 	map_ptr_kern.c core_kern.c core_kern_overflow.c
@@ -404,6 +410,7 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
 	$(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
 	$(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
+	$(Q)$$(BPFTOOL) gen subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$(@:.skel.h=.subskel.h)
 
 $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
@@ -421,6 +428,7 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
 	$(Q)diff $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
 	$$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
 	$(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
+	$(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)
 endif
 
 # ensure we set up tests.h header generation rule just once
@@ -557,6 +565,6 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o \
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
 	feature bpftool							\
-	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h no_alu32 bpf_gcc bpf_testmod.ko)
+	$(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h no_alu32 bpf_gcc bpf_testmod.ko)
 
 .PHONY: docs docs-clean
diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton.c b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
new file mode 100644
index 000000000000..9cbe17281f17
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+#include "test_subskeleton.skel.h"
+#include "test_subskeleton_lib.subskel.h"
+
+void subskeleton_lib_setup(struct bpf_object *obj)
+{
+	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
+
+	if (!ASSERT_OK_PTR(lib, "open subskeleton"))
+		goto out;
+
+	*lib->rodata.var1 = 1;
+	*lib->data.var2 = 2;
+	lib->bss.var3->var3_1 = 3;
+	lib->bss.var3->var3_2 = 4;
+
+out:
+	test_subskeleton_lib__destroy(lib);
+}
+
+int subskeleton_lib_subresult(struct bpf_object *obj)
+{
+	struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
+	int result;
+
+	if (!ASSERT_OK_PTR(lib, "open subskeleton")) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = *lib->bss.libout1;
+	ASSERT_EQ(result, 1 + 2 + 3 + 4 + 5 + 6, "lib subresult");
+
+	ASSERT_OK_PTR(lib->progs.lib_perf_handler, "lib_perf_handler");
+	ASSERT_STREQ(bpf_program__name(lib->progs.lib_perf_handler),
+		     "lib_perf_handler", "program name");
+
+	ASSERT_OK_PTR(lib->maps.map1, "map1");
+	ASSERT_STREQ(bpf_map__name(lib->maps.map1), "map1", "map name");
+
+	ASSERT_EQ(*lib->data.var5, 5, "__weak var5");
+	ASSERT_EQ(*lib->data.var6, 6, "extern var6");
+	ASSERT_TRUE(*lib->kconfig.CONFIG_BPF_SYSCALL, "CONFIG_BPF_SYSCALL");
+
+out:
+	test_subskeleton_lib__destroy(lib);
+	return result;
+}
+
+void test_subskeleton(void)
+{
+	int err, result;
+	struct test_subskeleton *skel;
+
+	skel = test_subskeleton__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	skel->rodata->rovar1 = 10;
+	skel->rodata->var1 = 1;
+	subskeleton_lib_setup(skel->obj);
+
+	err = test_subskeleton__load(skel);
+	if (!ASSERT_OK(err, "skel_load"))
+		goto cleanup;
+
+
+	err = test_subskeleton__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach"))
+		goto cleanup;
+
+	/* trigger tracepoint */
+	usleep(1);
+
+	result = subskeleton_lib_subresult(skel->obj) * 10;
+	ASSERT_EQ(skel->bss->out1, result, "unexpected calculation");
+
+cleanup:
+	test_subskeleton__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton.c b/tools/testing/selftests/bpf/progs/test_subskeleton.c
new file mode 100644
index 000000000000..5bd5452b41cd
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_subskeleton.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* volatile to force a read, compiler may assume 0 otherwise */
+const volatile int rovar1;
+int out1;
+
+/* Override weak symbol in test_subskeleton_lib */
+int var5 = 5;
+
+extern int lib_routine(void);
+
+SEC("raw_tp/sys_enter")
+int handler1(const void *ctx)
+{
+	out1 = lib_routine() * rovar1;
+	return 0;
+}
+
+char LICENSE[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
new file mode 100644
index 000000000000..665338006e33
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <stdbool.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* volatile to force a read */
+const volatile int var1;
+volatile int var2 = 1;
+struct {
+	int var3_1;
+	__s64 var3_2;
+} var3;
+int libout1;
+
+extern volatile bool CONFIG_BPF_SYSCALL __kconfig;
+
+int var4[4];
+
+__weak int var5 SEC(".data");
+extern int var6;
+int (*fn_ptr)(void);
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, __u32);
+	__type(value, __u32);
+	__uint(max_entries, 16);
+} map1 SEC(".maps");
+
+extern struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, __u32);
+	__type(value, __u32);
+	__uint(max_entries, 16);
+} map2 SEC(".maps");
+
+int lib_routine(void)
+{
+	__u32 key = 1, value = 2;
+
+	(void) CONFIG_BPF_SYSCALL;
+	bpf_map_update_elem(&map2, &key, &value, BPF_ANY);
+
+	libout1 = var1 + var2 + var3.var3_1 + var3.var3_2 + var5 + var6;
+	return libout1;
+}
+
+SEC("perf_event")
+int lib_perf_handler(struct pt_regs *ctx)
+{
+	return 0;
+}
+
+char LICENSE[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
new file mode 100644
index 000000000000..cbff92674b76
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, __u32);
+	__type(value, __u32);
+	__uint(max_entries, 16);
+} map2 SEC(".maps");
+
+int var6 = 6;
+
+char LICENSE[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf-next v2 5/5] selftests/bpf: test subskeleton functionality

[Andrii Nakryiko]

On Thu, Mar 10, 2022 at 4:12 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> This patch changes the selftests/bpf Makefile to  also generate
> a subskel.h for every skel.h it would have normally generated.
>
> Separately, it also introduces a new subskeleton test which tests
> library objects, externs, weak symbols, kconfigs, and user maps.
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/testing/selftests/bpf/.gitignore        |  1 +
>  tools/testing/selftests/bpf/Makefile          | 10 ++-
>  .../selftests/bpf/prog_tests/subskeleton.c    | 83 +++++++++++++++++++
>  .../selftests/bpf/progs/test_subskeleton.c    | 23 +++++
>  .../bpf/progs/test_subskeleton_lib.c          | 56 +++++++++++++
>  .../bpf/progs/test_subskeleton_lib2.c         | 16 ++++
>  6 files changed, 188 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index a7eead8820a0..595565eb68c0 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -31,6 +31,7 @@ test_tcp_check_syncookie_user
>  test_sysctl
>  xdping
>  test_cpp
> +*.subskel.h
>  *.skel.h
>  *.lskel.h
>  /no_alu32
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index fe12b4f5fe20..9f7b22faedd6 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -328,6 +328,12 @@ SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
>  LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h          \
>                 linked_vars.skel.h linked_maps.skel.h
>
> +# In the subskeleton case, we want the test_subskeleton_lib.subskel.h file
> +# but that's created as a side-effect of the skel.h generation.
> +LINKED_SKELS += test_subskeleton.skel.h test_subskeleton_lib.skel.h

this can be part of LINKED_SKELS list above, no need to split
definition. The comment above is very useful, but can stay near -deps
definitions without hurting understanding, IMO

> +test_subskeleton.skel.h-deps := test_subskeleton_lib2.o test_subskeleton_lib.o test_subskeleton.o
> +test_subskeleton_lib.skel.h-deps := test_subskeleton_lib2.o test_subskeleton_lib.o
> +
>  LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
>         test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
>         map_ptr_kern.c core_kern.c core_kern_overflow.c
> @@ -404,6 +410,7 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
>         $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
>         $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> +       $(Q)$$(BPFTOOL) gen subskeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$(@:.skel.h=.subskel.h)

we shouldn't need or use name for subskeleton (in real life you won't
know the name of the final bpf_object)

>
>  $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> @@ -421,6 +428,7 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
>         $(Q)diff $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
>         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
>         $(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
> +       $(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)

probably don't need subskel for LSKELS (and it just adds race when we
generate both skeleton and light skeleton for the same object file)

>  endif
>
>  # ensure we set up tests.h header generation rule just once
> @@ -557,6 +565,6 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o \
>  EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \
>         prog_tests/tests.h map_tests/tests.h verifier/tests.h           \
>         feature bpftool                                                 \
> -       $(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h no_alu32 bpf_gcc bpf_testmod.ko)
> +       $(addprefix $(OUTPUT)/,*.o *.skel.h *.lskel.h *.subskel.h no_alu32 bpf_gcc bpf_testmod.ko)
>
>  .PHONY: docs docs-clean
> diff --git a/tools/testing/selftests/bpf/prog_tests/subskeleton.c b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> new file mode 100644
> index 000000000000..9cbe17281f17
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/subskeleton.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> +
> +#include <test_progs.h>
> +#include "test_subskeleton.skel.h"
> +#include "test_subskeleton_lib.subskel.h"
> +
> +void subskeleton_lib_setup(struct bpf_object *obj)

static?

> +{
> +       struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +
> +       if (!ASSERT_OK_PTR(lib, "open subskeleton"))
> +               goto out;

nit: can return early, if open failed that lib should be NULL, right?

> +
> +       *lib->rodata.var1 = 1;
> +       *lib->data.var2 = 2;
> +       lib->bss.var3->var3_1 = 3;
> +       lib->bss.var3->var3_2 = 4;
> +
> +out:
> +       test_subskeleton_lib__destroy(lib);
> +}
> +
> +int subskeleton_lib_subresult(struct bpf_object *obj)

static?

> +{
> +       struct test_subskeleton_lib *lib = test_subskeleton_lib__open(obj);
> +       int result;
> +
> +       if (!ASSERT_OK_PTR(lib, "open subskeleton")) {
> +               result = -EINVAL;
> +               goto out;
> +       }

same, just return ?


> +
> +       result = *lib->bss.libout1;
> +       ASSERT_EQ(result, 1 + 2 + 3 + 4 + 5 + 6, "lib subresult");
> +
> +       ASSERT_OK_PTR(lib->progs.lib_perf_handler, "lib_perf_handler");
> +       ASSERT_STREQ(bpf_program__name(lib->progs.lib_perf_handler),
> +                    "lib_perf_handler", "program name");
> +
> +       ASSERT_OK_PTR(lib->maps.map1, "map1");
> +       ASSERT_STREQ(bpf_map__name(lib->maps.map1), "map1", "map name");
> +
> +       ASSERT_EQ(*lib->data.var5, 5, "__weak var5");
> +       ASSERT_EQ(*lib->data.var6, 6, "extern var6");
> +       ASSERT_TRUE(*lib->kconfig.CONFIG_BPF_SYSCALL, "CONFIG_BPF_SYSCALL");
> +
> +out:
> +       test_subskeleton_lib__destroy(lib);
> +       return result;
> +}
> +
> +void test_subskeleton(void)
> +{
> +       int err, result;
> +       struct test_subskeleton *skel;
> +
> +       skel = test_subskeleton__open();
> +       if (!ASSERT_OK_PTR(skel, "skel_open"))
> +               return;
> +
> +       skel->rodata->rovar1 = 10;
> +       skel->rodata->var1 = 1;
> +       subskeleton_lib_setup(skel->obj);
> +
> +       err = test_subskeleton__load(skel);
> +       if (!ASSERT_OK(err, "skel_load"))
> +               goto cleanup;
> +
> +

nit: extra empty line

> +       err = test_subskeleton__attach(skel);
> +       if (!ASSERT_OK(err, "skel_attach"))
> +               goto cleanup;
> +
> +       /* trigger tracepoint */
> +       usleep(1);
> +
> +       result = subskeleton_lib_subresult(skel->obj) * 10;
> +       ASSERT_EQ(skel->bss->out1, result, "unexpected calculation");
> +
> +cleanup:
> +       test_subskeleton__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton.c b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> new file mode 100644
> index 000000000000..5bd5452b41cd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +/* volatile to force a read, compiler may assume 0 otherwise */
> +const volatile int rovar1;
> +int out1;
> +
> +/* Override weak symbol in test_subskeleton_lib */
> +int var5 = 5;
> +

can you please add CONFIG_BPF_SYSCALL here as well, to check that
externs are properly "merged" and found, even if they overlap between
library and app BPF code

> +extern int lib_routine(void);
> +
> +SEC("raw_tp/sys_enter")
> +int handler1(const void *ctx)
> +{
> +       out1 = lib_routine() * rovar1;
> +       return 0;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> new file mode 100644
> index 000000000000..665338006e33
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> +
> +#include <stdbool.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +/* volatile to force a read */
> +const volatile int var1;
> +volatile int var2 = 1;
> +struct {
> +       int var3_1;
> +       __s64 var3_2;
> +} var3;
> +int libout1;
> +
> +extern volatile bool CONFIG_BPF_SYSCALL __kconfig;
> +
> +int var4[4];
> +
> +__weak int var5 SEC(".data");
> +extern int var6;
> +int (*fn_ptr)(void);
> +

libbpf supports .data.my_custom_name and .rodata.my_custom_whatever,
let's have a variable to test this also works?


> +struct {
> +       __uint(type, BPF_MAP_TYPE_HASH);
> +       __type(key, __u32);
> +       __type(value, __u32);
> +       __uint(max_entries, 16);
> +} map1 SEC(".maps");
> +
> +extern struct {
> +       __uint(type, BPF_MAP_TYPE_HASH);
> +       __type(key, __u32);
> +       __type(value, __u32);
> +       __uint(max_entries, 16);
> +} map2 SEC(".maps");
> +
> +int lib_routine(void)
> +{
> +       __u32 key = 1, value = 2;
> +
> +       (void) CONFIG_BPF_SYSCALL;
> +       bpf_map_update_elem(&map2, &key, &value, BPF_ANY);
> +
> +       libout1 = var1 + var2 + var3.var3_1 + var3.var3_2 + var5 + var6;
> +       return libout1;
> +}
> +
> +SEC("perf_event")
> +int lib_perf_handler(struct pt_regs *ctx)
> +{
> +       return 0;
> +}
> +
> +char LICENSE[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c b/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
> new file mode 100644
> index 000000000000..cbff92674b76
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_HASH);
> +       __type(key, __u32);
> +       __type(value, __u32);
> +       __uint(max_entries, 16);
> +} map2 SEC(".maps");
> +

let's move this into progs/test_subskeleton.c instead. It will
simulate a bit more complicated scenario, where library expects
application to define and provide a map, but the library itself
doesn't define it. It should work just fine right now (I think), but
just in case let's double check that having only "extern map" in the
library works.

> +int var6 = 6;
> +
> +char LICENSE[] SEC("license") = "GPL";
> --
> 2.34.1

Re: [PATCH bpf-next v2 5/5] selftests/bpf: test subskeleton functionality

[Delyan Kratunov]

On Fri, 2022-03-11 at 15:40 -0800, Andrii Nakryiko wrote:
> 
> we shouldn't need or use name for subskeleton (in real life you won't
> know the name of the final bpf_object)

Let's have this discussion in the bpftool email thread. Happy to remove the name
in the Makefile and fall back on the filename though.

> > 
> >  $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> >         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > @@ -421,6 +428,7 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
> >         $(Q)diff $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
> >         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> >         $(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
> > +       $(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)
> 
> probably don't need subskel for LSKELS (and it just adds race when we
> generate both skeleton and light skeleton for the same object file)

We're not generating subskels for LSKELS, that's just confusing diff output.
This is under the $(TRUNNER_BPF_SKELS_LINKED) outputs.

> 
> can you please add CONFIG_BPF_SYSCALL here as well, to check that
> externs are properly "merged" and found, even if they overlap between
> library and app BPF code

Sure.

> 
> libbpf supports .data.my_custom_name and .rodata.my_custom_whatever,
> let's have a variable to test this also works?

Sure.

> 
> let's move this into progs/test_subskeleton.c instead. It will
> simulate a bit more complicated scenario, where library expects
> application to define and provide a map, but the library itself
> doesn't define it. It should work just fine right now (I think), but
> just in case let's double check that having only "extern map" in the
> library works.

This fails to even open in bpftool:

libbpf: map 'map2': unsupported map linkage extern.
Error: failed to open BPF object file: Operation not supported

If we think this is valuable enough to support, let's tackle it separately after
the bulk of this functionality is merged?


-- Delyan

Re: [PATCH bpf-next v2 5/5] selftests/bpf: test subskeleton functionality

[Andrii Nakryiko]

On Mon, Mar 14, 2022 at 4:50 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Fri, 2022-03-11 at 15:40 -0800, Andrii Nakryiko wrote:
> >
> > we shouldn't need or use name for subskeleton (in real life you won't
> > know the name of the final bpf_object)
>
> Let's have this discussion in the bpftool email thread. Happy to remove the name
> in the Makefile and fall back on the filename though.
>

It's fine, keep it, you explained why we need it.

> > >
> > >  $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > >         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > > @@ -421,6 +428,7 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > >         $(Q)diff $$(@:.skel.h=.linked2.o) $$(@:.skel.h=.linked3.o)
> > >         $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > >         $(Q)$$(BPFTOOL) gen skeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$@
> > > +       $(Q)$$(BPFTOOL) gen subskeleton $$(@:.skel.h=.linked3.o) name $$(notdir $$(@:.skel.h=)) > $$(@:.skel.h=.subskel.h)
> >
> > probably don't need subskel for LSKELS (and it just adds race when we
> > generate both skeleton and light skeleton for the same object file)
>
> We're not generating subskels for LSKELS, that's just confusing diff output.
> This is under the $(TRUNNER_BPF_SKELS_LINKED) outputs.

indeed confusing, never mind then

>
> >
> > can you please add CONFIG_BPF_SYSCALL here as well, to check that
> > externs are properly "merged" and found, even if they overlap between
> > library and app BPF code
>
> Sure.
>
> >
> > libbpf supports .data.my_custom_name and .rodata.my_custom_whatever,
> > let's have a variable to test this also works?
>
> Sure.
>
> >
> > let's move this into progs/test_subskeleton.c instead. It will
> > simulate a bit more complicated scenario, where library expects
> > application to define and provide a map, but the library itself
> > doesn't define it. It should work just fine right now (I think), but
> > just in case let's double check that having only "extern map" in the
> > library works.
>
> This fails to even open in bpftool:
>
> libbpf: map 'map2': unsupported map linkage extern.
> Error: failed to open BPF object file: Operation not supported
>
> If we think this is valuable enough to support, let's tackle it separately after
> the bulk of this functionality is merged?

yep, totally. It's not super critical to support, but seems like a
useful use case for library to be able to access some pre-agreed map
in the final BPF app

>
>
> -- Delyan

Re: [PATCH bpf-next v2 0/5] Subskeleton support for BPF libraries

[Yonghong Song]

On 3/10/22 4:11 PM, Delyan Kratunov wrote:
> In the quest for ever more modularity, a new need has arisen - the ability to
> access data associated with a BPF library from a corresponding userspace library.
> The catch is that we don't want the userspace library to know about the structure of the
> final BPF object that the BPF library is linked into.
> 
> In pursuit of this modularity, this patch series introduces *subskeletons.*
> Subskeletons are similar in use and design to skeletons with a couple of differences:
> 
> 1. The generated storage types do not rely on contiguous storage for the library's
> variables because they may be interspersed randomly throughout the final BPF object's sections.
> 
> 2. Subskeletons do not own objects and instead require a loaded bpf_object* to
> be passed at runtime in order to be initialized. By extension, symbols are resolved at
> runtime by parsing the final object's BTF. This has the interesting effect that the same
> userspace code can interoperate with the library BPF code *linked into different final objects.*
> 
> 3. Subskeletons allow access to all global variables, programs, and custom maps. They also expose
> the internal maps *of the final object*. This allows bpf_var_skeleton objects to contain a bpf_map**
> instead of a section name.
> 
> Changes since v1:
>   - Introduced new strict mode knob for single-routine-in-.text compatibility behavior, which
>     disproportionately affects library objects. bpftool works in 1.0 mode so subskeleton generation
>     doesn't have to worry about this now.
>   - Made bpf_map_btf_value_type_id available earlier and used it wherever applicable.
>   - Refactoring in bpftool gen.c per review comments.
>   - Subskels now use typeof() for array and func proto globals to avoid the need for runtime split btf.
>   - Expanded the subskeleton test to include arrays, custom maps, extern maps, weak symbols, and kconfigs.
>   - selftests/bpf/Makefile now generates a subskel.h for every skel.h it would make.
> 
> For reference, here is a shortened subskeleton header:
> 
> #ifndef __TEST_SUBSKELETON_LIB_SUBSKEL_H__
> #define __TEST_SUBSKELETON_LIB_SUBSKEL_H__
> 
> struct test_subskeleton_lib {
> 	struct bpf_object *obj;
> 	struct bpf_object_subskeleton *subskel;
> 	struct {
> 		struct bpf_map *map2;
> 		struct bpf_map *map1;
> 		struct bpf_map *data;
> 		struct bpf_map *rodata;
> 		struct bpf_map *bss;
> 		struct bpf_map *kconfig;
> 	} maps;
> 	struct {
> 		struct bpf_program *lib_perf_handler;
> 	} progs;
> 	struct test_subskeleton_lib__data {
> 		int *var6;
> 		int *var2;
> 		int *var5;
> 	} data;
> 	struct test_subskeleton_lib__rodata {
> 		int *var1;
> 	} rodata;
> 	struct test_subskeleton_lib__bss {
> 		struct {
> 			int var3_1;
> 			__s64 var3_2;
> 		} *var3;
> 		int *libout1;
> 		typeof(int[4]) *var4;
> 		typeof(int (*)()) *fn_ptr;
> 	} bss;
> 	struct test_subskeleton_lib__kconfig {
> 		_Bool *CONFIG_BPF_SYSCALL;
> 	} kconfig;
> 
> static inline struct test_subskeleton_lib *
> test_subskeleton_lib__open(const struct bpf_object *src)
> {
> 	struct test_subskeleton_lib *obj;
> 	struct bpf_object_subskeleton *s;
> 	int err;
> 
> 	...
> 	s = (struct bpf_object_subskeleton *)calloc(1, sizeof(*s));
> 	...
> 
> 	s->var_cnt = 9;
> 	...
> 
> 	s->vars[0].name = "var6";
> 	s->vars[0].map = &obj->maps.data;
> 	s->vars[0].addr = (void**) &obj->data.var6;
>    ...
> 
> 	/* maps */
> 	...
> 
> 	/* programs */
> 	s->prog_cnt = 1;
> 	...
> 
> 	err = bpf_object__open_subskeleton(s);
>    ...
> 	return obj;
> }
> #endif /* __TEST_SUBSKELETON_LIB_SUBSKEL_H__ */

When I tried to build the patch set with parallel mode (-j),
    make -C tools/testing/selftests/bpf -j
I hit the following errors:

/bin/sh: line 1: 3484984 Bus error               (core dumped) 
/home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/sbin/bpftool 
gen skeleton 
/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.linked3.o 
name test_ksyms_weak > 
/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h
make: *** [Makefile:496: 
/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h] 
Error 135
make: *** Deleting file 
'/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h'
make: *** Waiting for unfinished jobs....
make: Leaving directory 
'/home/yhs/work/bpf-next/tools/testing/selftests/bpf'

Probably some make file related issues.
I didn't hit this issue before without this patch set.

> 
> Delyan Kratunov (5):
>    libbpf: add new strict flag for .text subprograms
>    libbpf: init btf_{key,value}_type_id on internal map open
>    libbpf: add subskeleton scaffolding
>    bpftool: add support for subskeletons
>    selftests/bpf: test subskeleton functionality
> 
>   .../bpf/bpftool/Documentation/bpftool-gen.rst |  25 +
>   tools/bpf/bpftool/bash-completion/bpftool     |  14 +-
>   tools/bpf/bpftool/gen.c                       | 589 ++++++++++++++++--
>   tools/lib/bpf/libbpf.c                        | 151 ++++-
>   tools/lib/bpf/libbpf.h                        |  29 +
>   tools/lib/bpf/libbpf.map                      |   2 +
>   tools/lib/bpf/libbpf_legacy.h                 |   6 +
>   tools/testing/selftests/bpf/.gitignore        |   1 +
>   tools/testing/selftests/bpf/Makefile          |  10 +-
>   .../selftests/bpf/prog_tests/subskeleton.c    |  83 +++
>   .../selftests/bpf/progs/test_subskeleton.c    |  23 +
>   .../bpf/progs/test_subskeleton_lib.c          |  56 ++
>   .../bpf/progs/test_subskeleton_lib2.c         |  16 +
>   13 files changed, 919 insertions(+), 86 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/subskeleton.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_subskeleton_lib2.c
> 
> --
> 2.34.1

Re: [PATCH bpf-next v2 0/5] Subskeleton support for BPF libraries

[Delyan Kratunov]

On Thu, 2022-03-10 at 21:10 -0800, Yonghong Song wrote:
> 
> On 3/10/22 4:11 PM, Delyan Kratunov wrote:
[..]
> 
> When I tried to build the patch set with parallel mode (-j),
>     make -C tools/testing/selftests/bpf -j
> I hit the following errors:
> 
> /bin/sh: line 1: 3484984 Bus error               (core dumped) 
> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/sbin/bpftool 
> gen skeleton 
> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.linked3.o 
> name test_ksyms_weak > 
> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h
> make: *** [Makefile:496: 
> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h] 
> Error 135
> make: *** Deleting file 
> '/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h'
> make: *** Waiting for unfinished jobs....
> make: Leaving directory 
> '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
> 
> Probably some make file related issues.
> I didn't hit this issue before without this patch set.

Hm, that's interesting, can you reproduce it? I build everything with -j and
have not seen any bpftool issues. I also use ASAN for bpftool and that's not
complaining about anything either.

SIGBUS suggests a memory mapped file was not there. I'll try and come up with
ways that can happen, especially given that it's a `gen skeleton` invocation,
which I haven't changed at all.

--Delyan

Re: [PATCH bpf-next v2 0/5] Subskeleton support for BPF libraries

[Yonghong Song]

On 3/11/22 10:18 AM, Delyan Kratunov wrote:
> On Thu, 2022-03-10 at 21:10 -0800, Yonghong Song wrote:
>>
>> On 3/10/22 4:11 PM, Delyan Kratunov wrote:
> [..]
>>
>> When I tried to build the patch set with parallel mode (-j),
>>      make -C tools/testing/selftests/bpf -j
>> I hit the following errors:
>>
>> /bin/sh: line 1: 3484984 Bus error               (core dumped)
>> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/tools/sbin/bpftool
>> gen skeleton
>> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.linked3.o
>> name test_ksyms_weak >
>> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h
>> make: *** [Makefile:496:
>> /home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h]
>> Error 135
>> make: *** Deleting file
>> '/home/yhs/work/bpf-next/tools/testing/selftests/bpf/test_ksyms_weak.skel.h'
>> make: *** Waiting for unfinished jobs....
>> make: Leaving directory
>> '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
>>
>> Probably some make file related issues.
>> I didn't hit this issue before without this patch set.
> 
> Hm, that's interesting, can you reproduce it? I build everything with -j and
> have not seen any bpftool issues. I also use ASAN for bpftool and that's not
> complaining about anything either.

I can reproduce it in 50% of tries with command line:
   make -C tools/testing/selftests/bpf -j

> 
> SIGBUS suggests a memory mapped file was not there. I'll try and come up with
> ways that can happen, especially given that it's a `gen skeleton` invocation,
> which I haven't changed at all.
> 
> --Delyan