* [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* 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 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
* [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 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 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
* 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