[PATCH v2 bpf-next 1/2] libbpf: load global data maps lazily on legacy kernels

[PATCH v2 bpf-next 1/2] libbpf: load global data maps lazily on legacy kernels

[Andrii Nakryiko]

Load global data maps lazily, if kernel is too old to support global
data. Make sure that programs are still correct by detecting if any of
the to-be-loaded programs have relocation against any of such maps.

This allows to solve the issue ([0]) with bpf_printk() and Clang
generating unnecessary and unreferenced .rodata.strX.Y sections, but it
also goes further along the CO-RE lines, allowing to have a BPF object
in which some code can work on very old kernels and relies only on BPF
maps explicitly, while other BPF programs might enjoy global variable
support. If such programs are correctly set to not load at runtime on
old kernels, bpf_object will load and function correctly now.

  [0] https://lore.kernel.org/bpf/CAK-59YFPU3qO+_pXWOH+c1LSA=8WA1yabJZfREjOEXNHAqgXNg@mail.gmail.com/

Fixes: aed659170a31 ("libbpf: Support multiple .rodata.* and .data.* BPF maps")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index af405c38aadc..27695bf31250 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5006,6 +5006,24 @@ bpf_object__create_maps(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_maps; i++) {
 		map = &obj->maps[i];
 
+		/* To support old kernels, we skip creating global data maps
+		 * (.rodata, .data, .kconfig, etc); later on, during program
+		 * loading, if we detect that at least one of the to-be-loaded
+		 * programs is referencing any global data map, we'll error
+		 * out with program name and relocation index logged.
+		 * This approach allows to accommodate Clang emitting
+		 * unnecessary .rodata.str1.1 sections for string literals,
+		 * but also it allows to have CO-RE applications that use
+		 * global variables in some of BPF programs, but not others.
+		 * If those global variable-using programs are not loaded at
+		 * runtime due to bpf_program__set_autoload(prog, false),
+		 * bpf_object loading will succeed just fine even on old
+		 * kernels.
+		 */
+		if (bpf_map__is_internal(map) &&
+		    !kernel_supports(obj, FEAT_GLOBAL_DATA))
+			continue;
+
 		retried = false;
 retry:
 		if (map->pin_path) {
@@ -5605,6 +5623,14 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
 				insn[0].src_reg = BPF_PSEUDO_MAP_IDX_VALUE;
 				insn[0].imm = relo->map_idx;
 			} else {
+				const struct bpf_map *map = &obj->maps[relo->map_idx];
+
+				if (bpf_map__is_internal(map) &&
+				    !kernel_supports(obj, FEAT_GLOBAL_DATA)) {
+					pr_warn("prog '%s': relo #%d: kernel doesn't support global data\n",
+						prog->name, i);
+					return -ENOTSUP;
+				}
 				insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
 				insn[0].imm = obj->maps[relo->map_idx].fd;
 			}
@@ -6139,6 +6165,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
 		 */
 		if (prog_is_subprog(obj, prog))
 			continue;
+		if (!prog->load)
+			continue;
 
 		err = bpf_object__relocate_calls(obj, prog);
 		if (err) {
@@ -6152,6 +6180,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
 		prog = &obj->programs[i];
 		if (prog_is_subprog(obj, prog))
 			continue;
+		if (!prog->load)
+			continue;
 		err = bpf_object__relocate_data(obj, prog);
 		if (err) {
 			pr_warn("prog '%s': failed to relocate data references: %d\n",
@@ -6939,10 +6969,6 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
 	bpf_object__for_each_map(m, obj) {
 		if (!bpf_map__is_internal(m))
 			continue;
-		if (!kernel_supports(obj, FEAT_GLOBAL_DATA)) {
-			pr_warn("kernel doesn't support global data\n");
-			return -ENOTSUP;
-		}
 		if (!kernel_supports(obj, FEAT_ARRAY_MMAP))
 			m->def.map_flags ^= BPF_F_MMAPABLE;
 	}
-- 
2.30.2

[PATCH v2 bpf-next 2/2] selftests/bpf: mix legacy (maps) and modern (vars) BPF in one test

[Andrii Nakryiko]

Add selftest that combines two BPF programs within single BPF object
file such that one of the programs is using global variables, but can be
skipped at runtime on old kernels that don't support global data.
Another BPF program is written with the goal to be runnable on very old
kernels and only relies on explicitly accessed BPF maps.

Such test, run against old kernels (e.g., libbpf CI will run it against 4.9
kernel that doesn't support global data), allows to test the approach
and ensure that libbpf doesn't make unnecessary assumption about
necessary kernel features.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
v1->v2:
  - added comments explaining why we need bpf_printk() (Song).

 .../selftests/bpf/prog_tests/legacy_printk.c  | 65 +++++++++++++++++
 .../selftests/bpf/progs/test_legacy_printk.c  | 73 +++++++++++++++++++
 2 files changed, 138 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/legacy_printk.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_legacy_printk.c

diff --git a/tools/testing/selftests/bpf/prog_tests/legacy_printk.c b/tools/testing/selftests/bpf/prog_tests/legacy_printk.c
new file mode 100644
index 000000000000..ec6e45f2a644
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/legacy_printk.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include <test_progs.h>
+#include "test_legacy_printk.skel.h"
+
+static int execute_one_variant(bool legacy)
+{
+	struct test_legacy_printk *skel;
+	int err, zero = 0, my_pid = getpid(), res, map_fd;
+
+	skel = test_legacy_printk__open();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return -errno;
+
+	bpf_program__set_autoload(skel->progs.handle_legacy, legacy);
+	bpf_program__set_autoload(skel->progs.handle_modern, !legacy);
+
+	err = test_legacy_printk__load(skel);
+	/* no ASSERT_OK, we expect one of two variants can fail here */
+	if (err)
+		goto err_out;
+
+	if (legacy) {
+		map_fd = bpf_map__fd(skel->maps.my_pid_map);
+		err = bpf_map_update_elem(map_fd, &zero, &my_pid, BPF_ANY);
+		if (!ASSERT_OK(err, "my_pid_map_update"))
+			goto err_out;
+		err = bpf_map_lookup_elem(map_fd, &zero, &res);
+	} else {
+		skel->bss->my_pid_var = my_pid;
+	}
+
+	err = test_legacy_printk__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach"))
+		goto err_out;
+
+	usleep(1); /* trigger */
+
+	if (legacy) {
+		map_fd = bpf_map__fd(skel->maps.res_map);
+		err = bpf_map_lookup_elem(map_fd, &zero, &res);
+		if (!ASSERT_OK(err, "res_map_lookup"))
+			goto err_out;
+	} else {
+		res = skel->bss->res_var;
+	}
+
+	if (!ASSERT_GT(res, 0, "res")) {
+		err = -EINVAL;
+		goto err_out;
+	}
+
+err_out:
+	test_legacy_printk__destroy(skel);
+	return err;
+}
+
+void test_legacy_printk(void)
+{
+	/* legacy variant should work everywhere */
+	ASSERT_OK(execute_one_variant(true /* legacy */), "legacy_case");
+
+	/* execute modern variant, can fail the load on old kernels */
+	execute_one_variant(false);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_legacy_printk.c b/tools/testing/selftests/bpf/progs/test_legacy_printk.c
new file mode 100644
index 000000000000..64c2d9ced529
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_legacy_printk.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <linux/bpf.h>
+#define BPF_NO_GLOBAL_DATA
+#include <bpf/bpf_helpers.h>
+
+char LICENSE[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, 1);
+} my_pid_map SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, 1);
+} res_map SEC(".maps");
+
+volatile int my_pid_var = 0;
+volatile int res_var = 0;
+
+SEC("tp/raw_syscalls/sys_enter")
+int handle_legacy(void *ctx)
+{
+	int zero = 0, *my_pid, cur_pid, *my_res;
+
+	my_pid = bpf_map_lookup_elem(&my_pid_map, &zero);
+	if (!my_pid)
+		return 1;
+
+	cur_pid = bpf_get_current_pid_tgid() >> 32;
+	if (cur_pid != *my_pid)
+		return 1;
+
+	my_res = bpf_map_lookup_elem(&res_map, &zero);
+	if (!my_res)
+		return 1;
+
+	if (*my_res == 0)
+		/* use bpf_printk() in combination with BPF_NO_GLOBAL_DATA to
+		 * force .rodata.str1.1 section that previously caused
+		 * problems on old kernels due to libbpf always tried to
+		 * create a global data map for it
+		 */
+		bpf_printk("Legacy-case bpf_printk test, pid %d\n", cur_pid);
+	*my_res = 1;
+
+	return *my_res;
+}
+
+SEC("tp/raw_syscalls/sys_enter")
+int handle_modern(void *ctx)
+{
+	int zero = 0, cur_pid;
+
+	cur_pid = bpf_get_current_pid_tgid() >> 32;
+	if (cur_pid != my_pid_var)
+		return 1;
+
+	if (res_var == 0)
+		/* we need bpf_printk() to validate libbpf logic around unused
+		 * global maps and legacy kernels; see comment in handle_legacy()
+		 */
+		bpf_printk("Modern-case bpf_printk test, pid %d\n", cur_pid);
+	res_var = 1;
+
+	return res_var;
+}
-- 
2.30.2

Re: [PATCH v2 bpf-next 1/2] libbpf: load global data maps lazily on legacy kernels

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue, 23 Nov 2021 12:01:04 -0800 you wrote:
> Load global data maps lazily, if kernel is too old to support global
> data. Make sure that programs are still correct by detecting if any of
> the to-be-loaded programs have relocation against any of such maps.
> 
> This allows to solve the issue ([0]) with bpf_printk() and Clang
> generating unnecessary and unreferenced .rodata.strX.Y sections, but it
> also goes further along the CO-RE lines, allowing to have a BPF object
> in which some code can work on very old kernels and relies only on BPF
> maps explicitly, while other BPF programs might enjoy global variable
> support. If such programs are correctly set to not load at runtime on
> old kernels, bpf_object will load and function correctly now.
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,1/2] libbpf: load global data maps lazily on legacy kernels
    https://git.kernel.org/bpf/bpf-next/c/16e0c35c6f7a
  - [v2,bpf-next,2/2] selftests/bpf: mix legacy (maps) and modern (vars) BPF in one test
    https://git.kernel.org/bpf/bpf-next/c/e4f7ac90c2b0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 bpf-next 1/2] libbpf: load global data maps lazily on legacy kernels

[Alan Maguire]

On Tue, 23 Nov 2021, Andrii Nakryiko wrote:

> Load global data maps lazily, if kernel is too old to support global
> data. Make sure that programs are still correct by detecting if any of
> the to-be-loaded programs have relocation against any of such maps.
> 
> This allows to solve the issue ([0]) with bpf_printk() and Clang
> generating unnecessary and unreferenced .rodata.strX.Y sections, but it
> also goes further along the CO-RE lines, allowing to have a BPF object
> in which some code can work on very old kernels and relies only on BPF
> maps explicitly, while other BPF programs might enjoy global variable
> support. If such programs are correctly set to not load at runtime on
> old kernels, bpf_object will load and function correctly now.
> 
>   [0] https://lore.kernel.org/bpf/CAK-59YFPU3qO+_pXWOH+c1LSA=8WA1yabJZfREjOEXNHAqgXNg@mail.gmail.com/
> 
> Fixes: aed659170a31 ("libbpf: Support multiple .rodata.* and .data.* BPF maps")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Thanks for fixing this! I ran into it today on a 4.14 kernel and verified 
that with the patch applied to latest libbpf, BPF objects with legacy maps 
loaded and ran where previously loading failed.

Tested-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index af405c38aadc..27695bf31250 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5006,6 +5006,24 @@ bpf_object__create_maps(struct bpf_object *obj)
>  	for (i = 0; i < obj->nr_maps; i++) {
>  		map = &obj->maps[i];
>  
> +		/* To support old kernels, we skip creating global data maps
> +		 * (.rodata, .data, .kconfig, etc); later on, during program
> +		 * loading, if we detect that at least one of the to-be-loaded
> +		 * programs is referencing any global data map, we'll error
> +		 * out with program name and relocation index logged.
> +		 * This approach allows to accommodate Clang emitting
> +		 * unnecessary .rodata.str1.1 sections for string literals,
> +		 * but also it allows to have CO-RE applications that use
> +		 * global variables in some of BPF programs, but not others.
> +		 * If those global variable-using programs are not loaded at
> +		 * runtime due to bpf_program__set_autoload(prog, false),
> +		 * bpf_object loading will succeed just fine even on old
> +		 * kernels.
> +		 */
> +		if (bpf_map__is_internal(map) &&
> +		    !kernel_supports(obj, FEAT_GLOBAL_DATA))
> +			continue;
> +
>  		retried = false;
>  retry:
>  		if (map->pin_path) {
> @@ -5605,6 +5623,14 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>  				insn[0].src_reg = BPF_PSEUDO_MAP_IDX_VALUE;
>  				insn[0].imm = relo->map_idx;
>  			} else {
> +				const struct bpf_map *map = &obj->maps[relo->map_idx];
> +
> +				if (bpf_map__is_internal(map) &&
> +				    !kernel_supports(obj, FEAT_GLOBAL_DATA)) {
> +					pr_warn("prog '%s': relo #%d: kernel doesn't support global data\n",
> +						prog->name, i);
> +					return -ENOTSUP;
> +				}
>  				insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
>  				insn[0].imm = obj->maps[relo->map_idx].fd;
>  			}
> @@ -6139,6 +6165,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
>  		 */
>  		if (prog_is_subprog(obj, prog))
>  			continue;
> +		if (!prog->load)
> +			continue;
>  
>  		err = bpf_object__relocate_calls(obj, prog);
>  		if (err) {
> @@ -6152,6 +6180,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
>  		prog = &obj->programs[i];
>  		if (prog_is_subprog(obj, prog))
>  			continue;
> +		if (!prog->load)
> +			continue;
>  		err = bpf_object__relocate_data(obj, prog);
>  		if (err) {
>  			pr_warn("prog '%s': failed to relocate data references: %d\n",
> @@ -6939,10 +6969,6 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
>  	bpf_object__for_each_map(m, obj) {
>  		if (!bpf_map__is_internal(m))
>  			continue;
> -		if (!kernel_supports(obj, FEAT_GLOBAL_DATA)) {
> -			pr_warn("kernel doesn't support global data\n");
> -			return -ENOTSUP;
> -		}
>  		if (!kernel_supports(obj, FEAT_ARRAY_MMAP))
>  			m->def.map_flags ^= BPF_F_MMAPABLE;
>  	}
> -- 
> 2.30.2
> 
>