linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts
@ 2021-07-13 12:42 Shuyi Cheng
  2021-07-13 12:42 ` [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts' Shuyi Cheng
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-13 12:42 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
  Cc: netdev, bpf, linux-kernel, kernel-janitors, Shuyi Cheng

This patch set adds the ability to point to a custom BTF for the
purposes of BPF CO-RE relocations. This is useful for using BPF CO-RE
on old kernels that don't yet natively support kernel (vmlinux) BTF
and thus libbpf needs application's help in locating kernel BTF
generated separately from the kernel itself. This was already possible
to do through bpf_object__load's attribute struct, but that makes it
inconvenient to use with BPF skeleton, which only allows to specify
bpf_object_open_opts during the open step. Thus, add the ability to
override vmlinux BTF at open time.

Patch #1 adds libbpf changes. 
Patch #2 fixes pre-existing memory leak detected during the code review. 
Patch #3 switches existing selftests to using open_opts for custom BTF.

Changelog:
----------

v3: https://lore.kernel.org/bpf/CAEf4BzY2cdT44bfbMus=gei27ViqGE1BtGo6XrErSsOCnqtVJg@mail.gmail.com/T/#m877eed1d4cf0a1d3352d3f3d6c5ff158be45c542
v3->v4:
--- Follow Andrii's suggestion to modify cover letter description.
--- Delete function bpf_object__load_override_btf.
--- Follow Dan's suggestion to add fixes tag and modify commit msg to patch #2.
--- Add pathch #3 to switch existing selftests to using open_opts.

v2: https://lore.kernel.org/bpf/CAEf4Bza_ua+tjxdhyy4nZ8Boeo+scipWmr_1xM1pC6N5wyuhAA@mail.gmail.com/T/#mf9cf86ae0ffa96180ac29e4fd12697eb70eccd0f
v2->v3:
--- Load the BTF specified by btf_custom_path to btf_vmlinux_override 
    instead of btf_bmlinux.
--- Fix the memory leak that may be introduced by the second version 
    of the patch.
--- Add a new patch to fix the possible memory leak caused by 
    obj->kconfig.

v1: https://lore.kernel.org/bpf/CAEf4BzaGjEC4t1OefDo11pj2-HfNy0BLhs_G2UREjRNTmb2u=A@mail.gmail.com/t/#m4d9f7c6761fbd2b436b5dfe491cd864b70225804
v1->v2:
-- Change custom_btf_path to btf_custom_path.
-- If the length of btf_custom_path of bpf_obj_open_opts is too long, 
   return ERR_PTR(-ENAMETOOLONG).
-- Add `custom BTF is in addition to vmlinux BTF`
   with btf_custom_path field.

Shuyi Cheng (3):
  libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
  libbpf: Fix the possible memory leak on error
  selftests/bpf: switches existing selftests to using open_opts for custom BTF

 tools/lib/bpf/libbpf.c                             | 42 +++++++++++++++++-----
 tools/lib/bpf/libbpf.h                             |  9 ++++-
 .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++------
 .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 +++++++--------
 4 files changed, 66 insertions(+), 35 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
  2021-07-13 12:42 [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Shuyi Cheng
@ 2021-07-13 12:42 ` Shuyi Cheng
  2021-07-16  4:51   ` Andrii Nakryiko
  2021-07-13 12:42 ` [PATCH bpf-next v4 2/3] libbpf: Fix the possible memory leak on error Shuyi Cheng
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-13 12:42 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
  Cc: netdev, bpf, linux-kernel, kernel-janitors, Shuyi Cheng

btf_custom_path allows developers to load custom BTF, and subsequent 
CO-RE will use custom BTF for relocation.

Learn from Andrii's comments in [0], add the btf_custom_path parameter
to bpf_obj_open_opts, you can directly use the skeleton's
<objname>_bpf__open_opts function to pass in the btf_custom_path
parameter.

Prior to this, there was also a developer who provided a patch with
similar functions. It is a pity that the follow-up did not continue to
advance. See [1].

	[0]https://lore.kernel.org/bpf/CAEf4BzbJZLjNoiK8_VfeVg_Vrg=9iYFv+po-38SMe=UzwDKJ=Q@mail.gmail.com/#t
	[1]https://yhbt.net/lore/all/CAEf4Bzbgw49w2PtowsrzKQNcxD4fZRE6AKByX-5-dMo-+oWHHA@mail.gmail.com/

Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
---
 tools/lib/bpf/libbpf.c | 36 ++++++++++++++++++++++++++++++------
 tools/lib/bpf/libbpf.h |  9 ++++++++-
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1e04ce7..6e11a7b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -498,6 +498,13 @@ struct bpf_object {
 	 * it at load time.
 	 */
 	struct btf *btf_vmlinux;
+	/* Path to the custom BTF to be used for BPF CO-RE relocations.
+	 * This custom BTF completely replaces the use of vmlinux BTF
+	 * for the purpose of CO-RE relocations.
+	 * NOTE: any other BPF feature (e.g., fentry/fexit programs,
+	 * struct_ops, etc) will need actual kernel BTF at /sys/kernel/btf/vmlinux.
+	 */
+	char *btf_custom_path;
 	/* vmlinux BTF override for CO-RE relocations */
 	struct btf *btf_vmlinux_override;
 	/* Lazily initialized kernel module BTFs */
@@ -2645,10 +2652,6 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
 	struct bpf_program *prog;
 	int i;
 
-	/* CO-RE relocations need kernel BTF */
-	if (obj->btf_ext && obj->btf_ext->core_relo_info.len)
-		return true;
-
 	/* Support for typed ksyms needs kernel BTF */
 	for (i = 0; i < obj->nr_extern; i++) {
 		const struct extern_desc *ext;
@@ -2665,6 +2668,13 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
 			return true;
 	}
 
+	/* CO-RE relocations need kernel BTF, only when btf_custom_path
+	 * is not specified
+	 */
+	if (obj->btf_ext && obj->btf_ext->core_relo_info.len
+		&& !obj->btf_custom_path)
+		return true;
+
 	return false;
 }
 
@@ -7554,7 +7564,7 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		   const struct bpf_object_open_opts *opts)
 {
-	const char *obj_name, *kconfig;
+	const char *obj_name, *kconfig, *btf_tmp_path;
 	struct bpf_program *prog;
 	struct bpf_object *obj;
 	char tmp_name[64];
@@ -7584,6 +7594,19 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 	obj = bpf_object__new(path, obj_buf, obj_buf_sz, obj_name);
 	if (IS_ERR(obj))
 		return obj;
+
+	btf_tmp_path = OPTS_GET(opts, btf_custom_path, NULL);
+	if (btf_tmp_path) {
+		if (strlen(btf_tmp_path) >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto out;
+		}
+		obj->btf_custom_path = strdup(btf_tmp_path);
+		if (!obj->btf_custom_path) {
+			err = -ENOMEM;
+			goto out;
+		}
+	}
 
 	kconfig = OPTS_GET(opts, kconfig, NULL);
 	if (kconfig) {
@@ -8055,7 +8078,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 	err = err ? : bpf_object__sanitize_maps(obj);
 	err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
 	err = err ? : bpf_object__create_maps(obj);
-	err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
+	err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : attr->target_btf_path);
 	err = err ? : bpf_object__load_progs(obj, attr->log_level);
 
 	if (obj->gen_loader) {
@@ -8702,6 +8725,7 @@ void bpf_object__close(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_maps; i++)
 		bpf_map__destroy(&obj->maps[i]);
 
+	zfree(&obj->btf_custom_path);
 	zfree(&obj->kconfig);
 	zfree(&obj->externs);
 	obj->nr_extern = 0;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6e61342..102ee8e 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -94,8 +94,15 @@ struct bpf_object_open_opts {
 	 * system Kconfig for CONFIG_xxx externs.
 	 */
 	const char *kconfig;
+	/* Path to the custom BTF to be used for BPF CO-RE relocations.
+	 * This custom BTF completely replaces the use of vmlinux BTF
+	 * for the purpose of CO-RE relocations.
+	 * NOTE: any other BPF feature (e.g., fentry/fexit programs,
+	 * struct_ops, etc) will need actual kernel BTF at /sys/kernel/btf/vmlinux.
+	 */
+	char *btf_custom_path;
 };
-#define bpf_object_open_opts__last_field kconfig
+#define bpf_object_open_opts__last_field btf_custom_path
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v4 2/3] libbpf: Fix the possible memory leak on error
  2021-07-13 12:42 [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Shuyi Cheng
  2021-07-13 12:42 ` [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts' Shuyi Cheng
@ 2021-07-13 12:42 ` Shuyi Cheng
  2021-07-13 12:42 ` [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF Shuyi Cheng
  2021-07-16  4:49 ` [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Andrii Nakryiko
  3 siblings, 0 replies; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-13 12:42 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
  Cc: netdev, bpf, linux-kernel, kernel-janitors, Shuyi Cheng, Dan Carpenter

If the strdup() fails then we need to call bpf_object__close(obj) to
avoid a resource leak.

Fixes: 166750b ("libbpf: Support libbpf-provided extern variables")

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
---
 tools/lib/bpf/libbpf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6e11a7b..9d80794 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7611,8 +7611,10 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 	kconfig = OPTS_GET(opts, kconfig, NULL);
 	if (kconfig) {
 		obj->kconfig = strdup(kconfig);
-		if (!obj->kconfig)
-			return ERR_PTR(-ENOMEM);
+		if (!obj->kconfig) {
+			err = -ENOMEM;
+			goto out;
+		}
 	}
 
 	err = bpf_object__elf_init(obj);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF
  2021-07-13 12:42 [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Shuyi Cheng
  2021-07-13 12:42 ` [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts' Shuyi Cheng
  2021-07-13 12:42 ` [PATCH bpf-next v4 2/3] libbpf: Fix the possible memory leak on error Shuyi Cheng
@ 2021-07-13 12:42 ` Shuyi Cheng
  2021-07-16  4:53   ` Andrii Nakryiko
  2021-07-16 20:27   ` Andrii Nakryiko
  2021-07-16  4:49 ` [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Andrii Nakryiko
  3 siblings, 2 replies; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-13 12:42 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh
  Cc: netdev, bpf, linux-kernel, kernel-janitors, Shuyi Cheng

This patch mainly replaces the bpf_object_load_attr of 
the core_autosize.c and core_reloc.c files with bpf_object_open_opts.

Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
---
 .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++++---------
 .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 ++++++++++------------
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/core_autosize.c b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
index 981c251..d163342 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_autosize.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
@@ -54,7 +54,7 @@ void test_core_autosize(void)
 	int err, fd = -1, zero = 0;
 	int char_id, short_id, int_id, long_long_id, void_ptr_id, id;
 	struct test_core_autosize* skel = NULL;
-	struct bpf_object_load_attr load_attr = {};
+	struct bpf_object_open_opts open_opts = {};
 	struct bpf_program *prog;
 	struct bpf_map *bss_map;
 	struct btf *btf = NULL;
@@ -125,9 +125,11 @@ void test_core_autosize(void)
 	fd = -1;
 
 	/* open and load BPF program with custom BTF as the kernel BTF */
-	skel = test_core_autosize__open();
+	open_opts.btf_custom_path = btf_file;
+	open_opts.sz = sizeof(struct bpf_object_open_opts);
+	skel = test_core_autosize__open_opts(&open_opts);
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
-		return;
+		goto cleanup;
 
 	/* disable handle_signed() for now */
 	prog = bpf_object__find_program_by_name(skel->obj, "handle_signed");
@@ -135,9 +137,7 @@ void test_core_autosize(void)
 		goto cleanup;
 	bpf_program__set_autoload(prog, false);
 
-	load_attr.obj = skel->obj;
-	load_attr.target_btf_path = btf_file;
-	err = bpf_object__load_xattr(&load_attr);
+	err = bpf_object__load(skel->obj);
 	if (!ASSERT_OK(err, "prog_load"))
 		goto cleanup;
 
@@ -204,13 +204,13 @@ void test_core_autosize(void)
 	skel = NULL;
 
 	/* now re-load with handle_signed() enabled, it should fail loading */
-	skel = test_core_autosize__open();
+	open_opts.btf_custom_path = btf_file;
+	open_opts.sz = sizeof(struct bpf_object_open_opts);
+	skel = test_core_autosize__open_opts(&opts);
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
-		return;
+		goto cleanup;
 
-	load_attr.obj = skel->obj;
-	load_attr.target_btf_path = btf_file;
-	err = bpf_object__load_xattr(&load_attr);
+	err = bpf_object__load(skel);
 	if (!ASSERT_ERR(err, "bad_prog_load"))
 		goto cleanup;
 
diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
index d02e064..10eb2407 100644
--- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
+++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
@@ -816,7 +816,7 @@ static size_t roundup_page(size_t sz)
 void test_core_reloc(void)
 {
 	const size_t mmap_sz = roundup_page(sizeof(struct data));
-	struct bpf_object_load_attr load_attr = {};
+	struct bpf_object_open_opts open_opts = {};
 	struct core_reloc_test_case *test_case;
 	const char *tp_name, *probe_name;
 	int err, i, equal;
@@ -846,9 +846,17 @@ void test_core_reloc(void)
 				continue;
 		}
 
-		obj = bpf_object__open_file(test_case->bpf_obj_file, NULL);
+		if (test_case->btf_src_file) {
+			err = access(test_case->btf_src_file, R_OK);
+			if (!ASSERT_OK(err, "btf_src_file"))
+				goto cleanup;
+		}
+
+		open_opts.btf_custom_path = test_case->btf_src_file;
+		open_opts.sz = sizeof(struct bpf_object_open_opts);
+		obj = bpf_object__open_file(test_case->bpf_obj_file, &open_opts);
 		if (!ASSERT_OK_PTR(obj, "obj_open"))
-			continue;
+			goto cleanup;
 
 		probe_name = "raw_tracepoint/sys_enter";
 		tp_name = "sys_enter";
@@ -861,18 +869,8 @@ void test_core_reloc(void)
 		if (CHECK(!prog, "find_probe",
 			  "prog '%s' not found\n", probe_name))
 			goto cleanup;
-
-
-		if (test_case->btf_src_file) {
-			err = access(test_case->btf_src_file, R_OK);
-			if (!ASSERT_OK(err, "btf_src_file"))
-				goto cleanup;
-		}
-
-		load_attr.obj = obj;
-		load_attr.log_level = 0;
-		load_attr.target_btf_path = test_case->btf_src_file;
-		err = bpf_object__load_xattr(&load_attr);
+
+		err = bpf_object__load(obj);
 		if (err) {
 			if (!test_case->fails)
 				ASSERT_OK(err, "obj_load");
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts
  2021-07-13 12:42 [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Shuyi Cheng
                   ` (2 preceding siblings ...)
  2021-07-13 12:42 ` [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF Shuyi Cheng
@ 2021-07-16  4:49 ` Andrii Nakryiko
  3 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-07-16  4:49 UTC (permalink / raw)
  To: Shuyi Cheng
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors

On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
<chengshuyi@linux.alibaba.com> wrote:
>
> This patch set adds the ability to point to a custom BTF for the
> purposes of BPF CO-RE relocations. This is useful for using BPF CO-RE
> on old kernels that don't yet natively support kernel (vmlinux) BTF
> and thus libbpf needs application's help in locating kernel BTF
> generated separately from the kernel itself. This was already possible
> to do through bpf_object__load's attribute struct, but that makes it
> inconvenient to use with BPF skeleton, which only allows to specify
> bpf_object_open_opts during the open step. Thus, add the ability to
> override vmlinux BTF at open time.
>
> Patch #1 adds libbpf changes.
> Patch #2 fixes pre-existing memory leak detected during the code review.
> Patch #3 switches existing selftests to using open_opts for custom BTF.
>

LGTM with some minor things I'll adjust while applying (which I'll
point out in respective patches). So no need to re-send anything.
Thanks.

> Changelog:
> ----------
>
> v3: https://lore.kernel.org/bpf/CAEf4BzY2cdT44bfbMus=gei27ViqGE1BtGo6XrErSsOCnqtVJg@mail.gmail.com/T/#m877eed1d4cf0a1d3352d3f3d6c5ff158be45c542
> v3->v4:
> --- Follow Andrii's suggestion to modify cover letter description.
> --- Delete function bpf_object__load_override_btf.
> --- Follow Dan's suggestion to add fixes tag and modify commit msg to patch #2.
> --- Add pathch #3 to switch existing selftests to using open_opts.
>
> v2: https://lore.kernel.org/bpf/CAEf4Bza_ua+tjxdhyy4nZ8Boeo+scipWmr_1xM1pC6N5wyuhAA@mail.gmail.com/T/#mf9cf86ae0ffa96180ac29e4fd12697eb70eccd0f
> v2->v3:
> --- Load the BTF specified by btf_custom_path to btf_vmlinux_override
>     instead of btf_bmlinux.
> --- Fix the memory leak that may be introduced by the second version
>     of the patch.
> --- Add a new patch to fix the possible memory leak caused by
>     obj->kconfig.
>
> v1: https://lore.kernel.org/bpf/CAEf4BzaGjEC4t1OefDo11pj2-HfNy0BLhs_G2UREjRNTmb2u=A@mail.gmail.com/t/#m4d9f7c6761fbd2b436b5dfe491cd864b70225804
> v1->v2:
> -- Change custom_btf_path to btf_custom_path.
> -- If the length of btf_custom_path of bpf_obj_open_opts is too long,
>    return ERR_PTR(-ENAMETOOLONG).
> -- Add `custom BTF is in addition to vmlinux BTF`
>    with btf_custom_path field.
>
> Shuyi Cheng (3):
>   libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
>   libbpf: Fix the possible memory leak on error
>   selftests/bpf: switches existing selftests to using open_opts for custom BTF
>
>  tools/lib/bpf/libbpf.c                             | 42 +++++++++++++++++-----
>  tools/lib/bpf/libbpf.h                             |  9 ++++-
>  .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++------
>  .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 +++++++--------
>  4 files changed, 66 insertions(+), 35 deletions(-)
>
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
  2021-07-13 12:42 ` [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts' Shuyi Cheng
@ 2021-07-16  4:51   ` Andrii Nakryiko
  2021-07-17  1:15     ` Shuyi Cheng
  0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2021-07-16  4:51 UTC (permalink / raw)
  To: Shuyi Cheng
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors

On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
<chengshuyi@linux.alibaba.com> wrote:
>
> btf_custom_path allows developers to load custom BTF, and subsequent
> CO-RE will use custom BTF for relocation.
>
> Learn from Andrii's comments in [0], add the btf_custom_path parameter
> to bpf_obj_open_opts, you can directly use the skeleton's
> <objname>_bpf__open_opts function to pass in the btf_custom_path
> parameter.
>
> Prior to this, there was also a developer who provided a patch with
> similar functions. It is a pity that the follow-up did not continue to
> advance. See [1].
>
>         [0]https://lore.kernel.org/bpf/CAEf4BzbJZLjNoiK8_VfeVg_Vrg=9iYFv+po-38SMe=UzwDKJ=Q@mail.gmail.com/#t
>         [1]https://yhbt.net/lore/all/CAEf4Bzbgw49w2PtowsrzKQNcxD4fZRE6AKByX-5-dMo-+oWHHA@mail.gmail.com/
>
> Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
> ---
>  tools/lib/bpf/libbpf.c | 36 ++++++++++++++++++++++++++++++------
>  tools/lib/bpf/libbpf.h |  9 ++++++++-
>  2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1e04ce7..6e11a7b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -498,6 +498,13 @@ struct bpf_object {
>          * it at load time.
>          */
>         struct btf *btf_vmlinux;
> +       /* Path to the custom BTF to be used for BPF CO-RE relocations.
> +        * This custom BTF completely replaces the use of vmlinux BTF
> +        * for the purpose of CO-RE relocations.
> +        * NOTE: any other BPF feature (e.g., fentry/fexit programs,
> +        * struct_ops, etc) will need actual kernel BTF at /sys/kernel/btf/vmlinux.
> +        */

this comment completely duplicates the one from bpf_object_open_opts,
I'll remove or shorten it

> +       char *btf_custom_path;
>         /* vmlinux BTF override for CO-RE relocations */
>         struct btf *btf_vmlinux_override;
>         /* Lazily initialized kernel module BTFs */
> @@ -2645,10 +2652,6 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
>         struct bpf_program *prog;
>         int i;
>
> -       /* CO-RE relocations need kernel BTF */
> -       if (obj->btf_ext && obj->btf_ext->core_relo_info.len)
> -               return true;
> -
>         /* Support for typed ksyms needs kernel BTF */
>         for (i = 0; i < obj->nr_extern; i++) {
>                 const struct extern_desc *ext;
> @@ -2665,6 +2668,13 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
>                         return true;
>         }
>
> +       /* CO-RE relocations need kernel BTF, only when btf_custom_path
> +        * is not specified
> +        */
> +       if (obj->btf_ext && obj->btf_ext->core_relo_info.len
> +               && !obj->btf_custom_path)
> +               return true;

not sure why you moved it, I'll move it back to minimize code churn

> +
>         return false;
>  }
>

[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF
  2021-07-13 12:42 ` [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF Shuyi Cheng
@ 2021-07-16  4:53   ` Andrii Nakryiko
  2021-07-16 20:27   ` Andrii Nakryiko
  1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-07-16  4:53 UTC (permalink / raw)
  To: Shuyi Cheng
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors

On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
<chengshuyi@linux.alibaba.com> wrote:
>
> This patch mainly replaces the bpf_object_load_attr of
> the core_autosize.c and core_reloc.c files with bpf_object_open_opts.
>
> Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
> ---
>  .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++++---------
>  .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 ++++++++++------------
>  2 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/core_autosize.c b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
> index 981c251..d163342 100644
> --- a/tools/testing/selftests/bpf/prog_tests/core_autosize.c
> +++ b/tools/testing/selftests/bpf/prog_tests/core_autosize.c
> @@ -54,7 +54,7 @@ void test_core_autosize(void)
>         int err, fd = -1, zero = 0;
>         int char_id, short_id, int_id, long_long_id, void_ptr_id, id;
>         struct test_core_autosize* skel = NULL;
> -       struct bpf_object_load_attr load_attr = {};
> +       struct bpf_object_open_opts open_opts = {};
>         struct bpf_program *prog;
>         struct bpf_map *bss_map;
>         struct btf *btf = NULL;
> @@ -125,9 +125,11 @@ void test_core_autosize(void)
>         fd = -1;
>
>         /* open and load BPF program with custom BTF as the kernel BTF */
> -       skel = test_core_autosize__open();
> +       open_opts.btf_custom_path = btf_file;
> +       open_opts.sz = sizeof(struct bpf_object_open_opts);
> +       skel = test_core_autosize__open_opts(&open_opts);
>         if (!ASSERT_OK_PTR(skel, "skel_open"))
> -               return;
> +               goto cleanup;
>
>         /* disable handle_signed() for now */
>         prog = bpf_object__find_program_by_name(skel->obj, "handle_signed");
> @@ -135,9 +137,7 @@ void test_core_autosize(void)
>                 goto cleanup;
>         bpf_program__set_autoload(prog, false);
>
> -       load_attr.obj = skel->obj;
> -       load_attr.target_btf_path = btf_file;
> -       err = bpf_object__load_xattr(&load_attr);
> +       err = bpf_object__load(skel->obj);
>         if (!ASSERT_OK(err, "prog_load"))
>                 goto cleanup;
>
> @@ -204,13 +204,13 @@ void test_core_autosize(void)
>         skel = NULL;
>
>         /* now re-load with handle_signed() enabled, it should fail loading */
> -       skel = test_core_autosize__open();
> +       open_opts.btf_custom_path = btf_file;
> +       open_opts.sz = sizeof(struct bpf_object_open_opts);

For opts structs libbpf provides DECLARE_LIBBPF_OPTS macro for their
initialization which zeroes the struct out and sets its sz
automatically. So I'll switch to using that instead. All the rest
looks good.

> +       skel = test_core_autosize__open_opts(&opts);
>         if (!ASSERT_OK_PTR(skel, "skel_open"))
> -               return;
> +               goto cleanup;
>
> -       load_attr.obj = skel->obj;
> -       load_attr.target_btf_path = btf_file;
> -       err = bpf_object__load_xattr(&load_attr);
> +       err = bpf_object__load(skel);
>         if (!ASSERT_ERR(err, "bad_prog_load"))
>                 goto cleanup;
>

[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF
  2021-07-13 12:42 ` [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF Shuyi Cheng
  2021-07-16  4:53   ` Andrii Nakryiko
@ 2021-07-16 20:27   ` Andrii Nakryiko
  2021-07-17  1:39     ` Shuyi Cheng
  1 sibling, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2021-07-16 20:27 UTC (permalink / raw)
  To: Shuyi Cheng
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors

On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
<chengshuyi@linux.alibaba.com> wrote:
>
> This patch mainly replaces the bpf_object_load_attr of
> the core_autosize.c and core_reloc.c files with bpf_object_open_opts.
>
> Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
> ---
>  .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++++---------
>  .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 ++++++++++------------
>  2 files changed, 24 insertions(+), 26 deletions(-)
>

So I applied this, but it's obvious you haven't bothered even
*building* selftests, because it had at least one compilation warning
and one compilation *error*, not building test_progs at all. I've
noted stuff I fixed (and still remember) below. I understand it might
be your first kernel contribution, but it's not acceptable to submit
patches that don't build. Next time please be more thorough.

[...]

>
> -       load_attr.obj = skel->obj;
> -       load_attr.target_btf_path = btf_file;
> -       err = bpf_object__load_xattr(&load_attr);
> +       err = bpf_object__load(skel);

This didn't compile outright, because it should have been
test_core_autosize__load(skel).

>         if (!ASSERT_ERR(err, "bad_prog_load"))
>                 goto cleanup;
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> index d02e064..10eb2407 100644
> --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
> @@ -816,7 +816,7 @@ static size_t roundup_page(size_t sz)
>  void test_core_reloc(void)
>  {
>         const size_t mmap_sz = roundup_page(sizeof(struct data));
> -       struct bpf_object_load_attr load_attr = {};
> +       struct bpf_object_open_opts open_opts = {};
>         struct core_reloc_test_case *test_case;
>         const char *tp_name, *probe_name;
>         int err, i, equal;
> @@ -846,9 +846,17 @@ void test_core_reloc(void)
>                                 continue;
>                 }
>
> -               obj = bpf_object__open_file(test_case->bpf_obj_file, NULL);
> +               if (test_case->btf_src_file) {
> +                       err = access(test_case->btf_src_file, R_OK);
> +                       if (!ASSERT_OK(err, "btf_src_file"))
> +                               goto cleanup;
> +               }
> +
> +               open_opts.btf_custom_path = test_case->btf_src_file;

This was reporting a valid warning about dropping const modifier. For
good reason, becyase btf_custom_path in open_opts should have been
`const char *`, I fixed that.

> +               open_opts.sz = sizeof(struct bpf_object_open_opts);
> +               obj = bpf_object__open_file(test_case->bpf_obj_file, &open_opts);
>                 if (!ASSERT_OK_PTR(obj, "obj_open"))
> -                       continue;
> +                       goto cleanup;
>
>                 probe_name = "raw_tracepoint/sys_enter";
>                 tp_name = "sys_enter";

[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts'
  2021-07-16  4:51   ` Andrii Nakryiko
@ 2021-07-17  1:15     ` Shuyi Cheng
  0 siblings, 0 replies; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-17  1:15 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors



On 7/16/21 12:51 PM, Andrii Nakryiko wrote:
> On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
> <chengshuyi@linux.alibaba.com> wrote:
>>
>> btf_custom_path allows developers to load custom BTF, and subsequent
>> CO-RE will use custom BTF for relocation.
>>
>> Learn from Andrii's comments in [0], add the btf_custom_path parameter
>> to bpf_obj_open_opts, you can directly use the skeleton's
>> <objname>_bpf__open_opts function to pass in the btf_custom_path
>> parameter.
>>
>> Prior to this, there was also a developer who provided a patch with
>> similar functions. It is a pity that the follow-up did not continue to
>> advance. See [1].
>>
>>          [0]https://lore.kernel.org/bpf/CAEf4BzbJZLjNoiK8_VfeVg_Vrg=9iYFv+po-38SMe=UzwDKJ=Q@mail.gmail.com/#t
>>          [1]https://yhbt.net/lore/all/CAEf4Bzbgw49w2PtowsrzKQNcxD4fZRE6AKByX-5-dMo-+oWHHA@mail.gmail.com/
>>
>> Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 36 ++++++++++++++++++++++++++++++------
>>   tools/lib/bpf/libbpf.h |  9 ++++++++-
>>   2 files changed, 38 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 1e04ce7..6e11a7b 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -498,6 +498,13 @@ struct bpf_object {
>>           * it at load time.
>>           */
>>          struct btf *btf_vmlinux;
>> +       /* Path to the custom BTF to be used for BPF CO-RE relocations.
>> +        * This custom BTF completely replaces the use of vmlinux BTF
>> +        * for the purpose of CO-RE relocations.
>> +        * NOTE: any other BPF feature (e.g., fentry/fexit programs,
>> +        * struct_ops, etc) will need actual kernel BTF at /sys/kernel/btf/vmlinux.
>> +        */
> 
> this comment completely duplicates the one from bpf_object_open_opts,
> I'll remove or shorten it
> 
>> +       char *btf_custom_path;
>>          /* vmlinux BTF override for CO-RE relocations */
>>          struct btf *btf_vmlinux_override;
>>          /* Lazily initialized kernel module BTFs */
>> @@ -2645,10 +2652,6 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
>>          struct bpf_program *prog;
>>          int i;
>>
>> -       /* CO-RE relocations need kernel BTF */
>> -       if (obj->btf_ext && obj->btf_ext->core_relo_info.len)
>> -               return true;
>> -
>>          /* Support for typed ksyms needs kernel BTF */
>>          for (i = 0; i < obj->nr_extern; i++) {
>>                  const struct extern_desc *ext;
>> @@ -2665,6 +2668,13 @@ static bool obj_needs_vmlinux_btf(const struct bpf_object *obj)
>>                          return true;
>>          }
>>
>> +       /* CO-RE relocations need kernel BTF, only when btf_custom_path
>> +        * is not specified
>> +        */
>> +       if (obj->btf_ext && obj->btf_ext->core_relo_info.len
>> +               && !obj->btf_custom_path)
>> +               return true;
> 
> not sure why you moved it, I'll move it back to minimize code churn

You're right. 👍

regards,
Shuyi
> 
>> +
>>          return false;
>>   }
>>
> 
> [...]
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF
  2021-07-16 20:27   ` Andrii Nakryiko
@ 2021-07-17  1:39     ` Shuyi Cheng
  0 siblings, 0 replies; 10+ messages in thread
From: Shuyi Cheng @ 2021-07-17  1:39 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Song Liu, Yonghong Song, john fastabend, KP Singh, Networking,
	bpf, open list, kernel-janitors



On 7/17/21 4:27 AM, Andrii Nakryiko wrote:
> On Tue, Jul 13, 2021 at 5:43 AM Shuyi Cheng
> <chengshuyi@linux.alibaba.com> wrote:
>>
>> This patch mainly replaces the bpf_object_load_attr of
>> the core_autosize.c and core_reloc.c files with bpf_object_open_opts.
>>
>> Signed-off-by: Shuyi Cheng <chengshuyi@linux.alibaba.com>
>> ---
>>   .../selftests/bpf/prog_tests/core_autosize.c       | 22 ++++++++---------
>>   .../testing/selftests/bpf/prog_tests/core_reloc.c  | 28 ++++++++++------------
>>   2 files changed, 24 insertions(+), 26 deletions(-)
>>
> 
> So I applied this, but it's obvious you haven't bothered even
> *building* selftests, because it had at least one compilation warning
> and one compilation *error*, not building test_progs at all. I've
> noted stuff I fixed (and still remember) below. I understand it might
> be your first kernel contribution, but it's not acceptable to submit
> patches that don't build. Next time please be more thorough.
> 

I'm very sorry, it was my fault. Although I learned a lot from libbpf, 
there is still a lot to learn and improve. Thank you very much for your 
advice and the very powerful libbpf.

regards,
Shuyi

> [...]
> 
>>
>> -       load_attr.obj = skel->obj;
>> -       load_attr.target_btf_path = btf_file;
>> -       err = bpf_object__load_xattr(&load_attr);
>> +       err = bpf_object__load(skel);
> 
> This didn't compile outright, because it should have been
> test_core_autosize__load(skel).
> 
>>          if (!ASSERT_ERR(err, "bad_prog_load"))
>>                  goto cleanup;
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/core_reloc.c b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
>> index d02e064..10eb2407 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/core_reloc.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/core_reloc.c
>> @@ -816,7 +816,7 @@ static size_t roundup_page(size_t sz)
>>   void test_core_reloc(void)
>>   {
>>          const size_t mmap_sz = roundup_page(sizeof(struct data));
>> -       struct bpf_object_load_attr load_attr = {};
>> +       struct bpf_object_open_opts open_opts = {};
>>          struct core_reloc_test_case *test_case;
>>          const char *tp_name, *probe_name;
>>          int err, i, equal;
>> @@ -846,9 +846,17 @@ void test_core_reloc(void)
>>                                  continue;
>>                  }
>>
>> -               obj = bpf_object__open_file(test_case->bpf_obj_file, NULL);
>> +               if (test_case->btf_src_file) {
>> +                       err = access(test_case->btf_src_file, R_OK);
>> +                       if (!ASSERT_OK(err, "btf_src_file"))
>> +                               goto cleanup;
>> +               }
>> +
>> +               open_opts.btf_custom_path = test_case->btf_src_file;
> 
> This was reporting a valid warning about dropping const modifier. For
> good reason, becyase btf_custom_path in open_opts should have been
> `const char *`, I fixed that.
> 
>> +               open_opts.sz = sizeof(struct bpf_object_open_opts);
>> +               obj = bpf_object__open_file(test_case->bpf_obj_file, &open_opts);
>>                  if (!ASSERT_OK_PTR(obj, "obj_open"))
>> -                       continue;
>> +                       goto cleanup;
>>
>>                  probe_name = "raw_tracepoint/sys_enter";
>>                  tp_name = "sys_enter";
> 
> [...]
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-07-17  1:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 12:42 [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Shuyi Cheng
2021-07-13 12:42 ` [PATCH bpf-next v4 1/3] libbpf: Introduce 'btf_custom_path' to 'bpf_obj_open_opts' Shuyi Cheng
2021-07-16  4:51   ` Andrii Nakryiko
2021-07-17  1:15     ` Shuyi Cheng
2021-07-13 12:42 ` [PATCH bpf-next v4 2/3] libbpf: Fix the possible memory leak on error Shuyi Cheng
2021-07-13 12:42 ` [PATCH bpf-next v4 3/3] selftests/bpf: Switches existing selftests to using open_opts for custom BTF Shuyi Cheng
2021-07-16  4:53   ` Andrii Nakryiko
2021-07-16 20:27   ` Andrii Nakryiko
2021-07-17  1:39     ` Shuyi Cheng
2021-07-16  4:49 ` [PATCH bpf-next v4 0/3] Add btf_custom_path in bpf_obj_open_opts Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).