* [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel
@ 2024-03-02 16:50 Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test Yonghong Song
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Yonghong Song @ 2024-03-02 16:50 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
With a LTO kernel built with clang, I encountered two test failures,
ksyms and kprobe_multi_bench_attach/kernel. Both test failures are
due to static variable/function renaming due to cross-file inlining.
The solution is to either skip the test or filter out those renamed
functions. A helper function check_lto_kernel() is introduced to
identify whether the underlying kernel is built with LTO or not.
Please see each individual patches for details.
Yonghong Song (4):
selftests/bpf: Replace CHECK with ASSERT macros for ksyms test
selftests/bpf: Add check_lto_kernel() helper
selftests/bpf: Fix possible ksyms test failure with LTO kernel
selftests/bpf: Fix possible kprobe_multi_bench_attach test failure
with LTO kernel
.../bpf/prog_tests/kprobe_multi_test.c | 7 +++
.../testing/selftests/bpf/prog_tests/ksyms.c | 42 +++++++++--------
tools/testing/selftests/bpf/testing_helpers.c | 47 +++++++++++++++++++
tools/testing/selftests/bpf/testing_helpers.h | 1 +
4 files changed, 78 insertions(+), 19 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test
2024-03-02 16:50 [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel Yonghong Song
@ 2024-03-02 16:50 ` Yonghong Song
2024-03-05 0:17 ` Andrii Nakryiko
2024-03-02 16:50 ` [PATCH bpf-next 2/4] selftests/bpf: Add check_lto_kernel() helper Yonghong Song
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2024-03-02 16:50 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
I am going to modify ksyms test later so take this opportunity
to replace old CHECK macros with new ASSERT macros.
No functionality change.
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
.../testing/selftests/bpf/prog_tests/ksyms.c | 38 +++++++++----------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
index b295969b263b..e081f8bf3f17 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
@@ -5,8 +5,6 @@
#include "test_ksyms.skel.h"
#include <sys/stat.h>
-static int duration;
-
void test_ksyms(void)
{
const char *btf_path = "/sys/kernel/btf/vmlinux";
@@ -18,43 +16,45 @@ void test_ksyms(void)
int err;
err = kallsyms_find("bpf_link_fops", &link_fops_addr);
- if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
+ if (err == -EINVAL) {
+ ASSERT_TRUE(false, "kallsyms_fopen for bpf_link_fops");
return;
- if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
+ }
+ if (err == -ENOENT) {
+ ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
return;
+ }
err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
- if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
+ if (err == -EINVAL) {
+ ASSERT_TRUE(false, "kallsyms_fopen for __per_cpu_start");
return;
- if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
+ }
+ if (err == -ENOENT) {
+ ASSERT_TRUE(false, "ksym_find for __per_cpu_start");
return;
+ }
- if (CHECK(stat(btf_path, &st), "stat_btf", "err %d\n", errno))
+ if (!ASSERT_OK(stat(btf_path, &st), "stat_btf"))
return;
btf_size = st.st_size;
skel = test_ksyms__open_and_load();
- if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n"))
+ if (!ASSERT_OK_PTR(skel, "test_ksyms__open_and_load"))
return;
err = test_ksyms__attach(skel);
- if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+ if (!ASSERT_OK(err, "test_ksyms__attach"))
goto cleanup;
/* trigger tracepoint */
usleep(1);
data = skel->data;
- CHECK(data->out__bpf_link_fops != link_fops_addr, "bpf_link_fops",
- "got 0x%llx, exp 0x%llx\n",
- data->out__bpf_link_fops, link_fops_addr);
- CHECK(data->out__bpf_link_fops1 != 0, "bpf_link_fops1",
- "got %llu, exp %llu\n", data->out__bpf_link_fops1, (__u64)0);
- CHECK(data->out__btf_size != btf_size, "btf_size",
- "got %llu, exp %llu\n", data->out__btf_size, btf_size);
- CHECK(data->out__per_cpu_start != per_cpu_start_addr, "__per_cpu_start",
- "got %llu, exp %llu\n", data->out__per_cpu_start,
- per_cpu_start_addr);
+ ASSERT_EQ(data->out__bpf_link_fops, link_fops_addr, "bpf_link_fops");
+ ASSERT_EQ(data->out__bpf_link_fops1, 0, "bpf_link_fops1");
+ ASSERT_EQ(data->out__btf_size, btf_size, "btf_size");
+ ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
cleanup:
test_ksyms__destroy(skel);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf-next 2/4] selftests/bpf: Add check_lto_kernel() helper
2024-03-02 16:50 [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test Yonghong Song
@ 2024-03-02 16:50 ` Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 4/4] selftests/bpf: Fix possible kprobe_multi_bench_attach " Yonghong Song
3 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2024-03-02 16:50 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
Add check_lto_kernel() helper to detect whether the underlying kernel
enabled CONFIG_LTO or not. The function check_lto_kernel() can be
used by selftests to handle some lto-specific situations.
The code is heavily borrowed from libbpf function
bpf_object__read_kconfig_file().
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
tools/testing/selftests/bpf/testing_helpers.c | 47 +++++++++++++++++++
tools/testing/selftests/bpf/testing_helpers.h | 1 +
2 files changed, 48 insertions(+)
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 28b6646662af..3f74f73843cf 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -5,6 +5,8 @@
#include <stdlib.h>
#include <string.h>
#include <errno.h>
+#include <zlib.h>
+#include <sys/utsname.h>
#include <bpf/bpf.h>
#include <bpf/libbpf.h>
#include "test_progs.h"
@@ -475,3 +477,48 @@ bool is_jit_enabled(void)
return enabled;
}
+
+int check_lto_kernel(void)
+{
+ static int check_lto = 2;
+ char buf[PATH_MAX];
+ struct utsname uts;
+ gzFile file;
+ int len;
+
+ if (check_lto != 2)
+ return check_lto;
+
+ uname(&uts);
+ len = snprintf(buf, PATH_MAX, "/boot/config-%s", uts.release);
+ if (len < 0) {
+ check_lto = -EINVAL;
+ goto out;
+ } else if (len >= PATH_MAX) {
+ check_lto = -ENAMETOOLONG;
+ goto out;
+ }
+
+ /* gzopen also accepts uncompressed files. */
+ file = gzopen(buf, "re");
+ if (!file)
+ file = gzopen("/proc/config.gz", "re");
+
+ if (!file) {
+ check_lto = -ENOENT;
+ goto out;
+ }
+
+ check_lto = 0;
+ while (gzgets(file, buf, sizeof(buf))) {
+ /* buf also contains '\n', skip it during comparison. */
+ if (!strncmp(buf, "CONFIG_LTO=y", 12)) {
+ check_lto = 1;
+ break;
+ }
+ }
+
+ gzclose(file);
+out:
+ return check_lto;
+}
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index d55f6ab12433..57683b3a1280 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -55,5 +55,6 @@ struct bpf_insn;
int get_xlated_program(int fd_prog, struct bpf_insn **buf, __u32 *cnt);
int testing_prog_flags(void);
bool is_jit_enabled(void);
+int check_lto_kernel(void);
#endif /* __TESTING_HELPERS_H */
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel
2024-03-02 16:50 [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 2/4] selftests/bpf: Add check_lto_kernel() helper Yonghong Song
@ 2024-03-02 16:50 ` Yonghong Song
2024-03-05 5:37 ` Alexei Starovoitov
2024-03-02 16:50 ` [PATCH bpf-next 4/4] selftests/bpf: Fix possible kprobe_multi_bench_attach " Yonghong Song
3 siblings, 1 reply; 9+ messages in thread
From: Yonghong Song @ 2024-03-02 16:50 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
In my locally build clang LTO kernel (enabling CONFIG_LTO and
CONFIG_LTO_CLANG_THIN), ksyms test failed like:
test_ksyms:PASS:kallsyms_fopen 0 nsec
test_ksyms:FAIL:ksym_find symbol 'bpf_link_fops' not found
#118 ksyms:FAIL
The reason is that 'bpf_link_fops' is renamed to
bpf_link_fops.llvm.8325593422554671469
Due to cross-file inlining, the static variable 'bpf_link_fops'
in syscall.c is used by a function in another file. To avoid
potential duplicated names, the llvm added suffix '.llvm.<hash>'.
To fix the failure, we can skip this test with LTO kernel
if the symbol 'bpf_link_fops' is not found in kallsyms.
After this patch, with the same LTO kernel:
#118 ksyms:SKIP
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
tools/testing/selftests/bpf/prog_tests/ksyms.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
index e081f8bf3f17..cd81f190c5d7 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
@@ -21,7 +21,11 @@ void test_ksyms(void)
return;
}
if (err == -ENOENT) {
- ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
+ /* bpf_link_fops might be renamed to bpf_link_fops.llvm.<hash> in LTO kernel. */
+ if (check_lto_kernel() == 1)
+ test__skip();
+ else
+ ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
return;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH bpf-next 4/4] selftests/bpf: Fix possible kprobe_multi_bench_attach test failure with LTO kernel
2024-03-02 16:50 [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel Yonghong Song
` (2 preceding siblings ...)
2024-03-02 16:50 ` [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel Yonghong Song
@ 2024-03-02 16:50 ` Yonghong Song
3 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2024-03-02 16:50 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
In my locally build clang LTO kernel (enabling CONFIG_LTO and
CONFIG_LTO_CLANG_THIN), kprobe_multi_bench_attach/kernel subtest
failed like:
test_kprobe_multi_bench_attach:PASS:get_syms 0 nsec
test_kprobe_multi_bench_attach:PASS:kprobe_multi_empty__open_and_load 0 nsec
libbpf: prog 'test_kprobe_empty': failed to attach: No such process
test_kprobe_multi_bench_attach:FAIL:bpf_program__attach_kprobe_multi_opts unexpected error: -3
#114/1 kprobe_multi_bench_attach/kernel:FAIL
There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
are renamed in kallsyms due to cross file inlining. One example is for
static function __access_remote_vm in mm/memory.c.
In a non-LTO kernel, we have the following call stack:
ptrace_access_vm (global, kernel/ptrace.c)
access_remote_vm (global, mm/memory.c)
__access_remote_vm (static, mm/memory.c)
With LTO kernel, it is possible that access_remote_vm() is inlined by
ptrace_access_vm(). So we end up with the following call stack:
ptrace_access_vm (global, kernel/ptrace.c)
__access_remote_vm (static, mm/memory.c)
The compiler renames __access_remote_vm to __access_remote_vm.llvm.<hash>
to prevent potential name collision.
This patch removed __access_remote_vm and other similar functions from
kprobe_multi_attach by checking if the symbol like __access_remote_vm
does not exist in kallsyms with LTO kernel. The test succeeded after this change:
#114/1 kprobe_multi_bench_attach/kernel:OK
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
index 05000810e28e..2aad5f9cdb84 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -341,10 +341,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
size_t cap = 0, cnt = 0, i;
char *name = NULL, **syms = NULL;
struct hashmap *map;
+ bool lto_kernel;
char buf[256];
FILE *f;
int err = 0;
+ lto_kernel = kernel && check_lto_kernel() == 1;
+ if (lto_kernel && !ASSERT_OK(load_kallsyms(), "load_kallsyms"))
+ return -EINVAL;
+
/*
* The available_filter_functions contains many duplicates,
* but other than that all symbols are usable in kprobe multi
@@ -393,6 +398,8 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
if (!strncmp(name, "__ftrace_invalid_address__",
sizeof("__ftrace_invalid_address__") - 1))
continue;
+ if (lto_kernel && ksym_get_addr(name) == 0)
+ continue;
err = hashmap__add(map, name, 0);
if (err == -EEXIST) {
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test
2024-03-02 16:50 ` [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test Yonghong Song
@ 2024-03-05 0:17 ` Andrii Nakryiko
2024-03-05 7:21 ` Yonghong Song
0 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2024-03-05 0:17 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
On Sat, Mar 2, 2024 at 8:50 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> I am going to modify ksyms test later so take this opportunity
> to replace old CHECK macros with new ASSERT macros.
> No functionality change.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
> .../testing/selftests/bpf/prog_tests/ksyms.c | 38 +++++++++----------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> index b295969b263b..e081f8bf3f17 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> @@ -5,8 +5,6 @@
> #include "test_ksyms.skel.h"
> #include <sys/stat.h>
>
> -static int duration;
> -
> void test_ksyms(void)
> {
> const char *btf_path = "/sys/kernel/btf/vmlinux";
> @@ -18,43 +16,45 @@ void test_ksyms(void)
> int err;
>
> err = kallsyms_find("bpf_link_fops", &link_fops_addr);
> - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
> + if (err == -EINVAL) {
> + ASSERT_TRUE(false, "kallsyms_fopen for bpf_link_fops");
should this (and few other cases below) be ASSERT_EQ()/ASSERT_NEQ()
(whichever makes sense, I can't reason about CHECK() conditions).
ASSERT_TRUE(false) is a last resort way, we have more meaningful checks.
> return;
> - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
> + }
> + if (err == -ENOENT) {
> + ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
> return;
> + }
>
> err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
> - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
> + if (err == -EINVAL) {
> + ASSERT_TRUE(false, "kallsyms_fopen for __per_cpu_start");
> return;
> - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
> + }
> + if (err == -ENOENT) {
> + ASSERT_TRUE(false, "ksym_find for __per_cpu_start");
> return;
> + }
>
> - if (CHECK(stat(btf_path, &st), "stat_btf", "err %d\n", errno))
> + if (!ASSERT_OK(stat(btf_path, &st), "stat_btf"))
> return;
> btf_size = st.st_size;
>
> skel = test_ksyms__open_and_load();
> - if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n"))
> + if (!ASSERT_OK_PTR(skel, "test_ksyms__open_and_load"))
> return;
>
> err = test_ksyms__attach(skel);
> - if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> + if (!ASSERT_OK(err, "test_ksyms__attach"))
> goto cleanup;
>
> /* trigger tracepoint */
> usleep(1);
>
> data = skel->data;
> - CHECK(data->out__bpf_link_fops != link_fops_addr, "bpf_link_fops",
> - "got 0x%llx, exp 0x%llx\n",
> - data->out__bpf_link_fops, link_fops_addr);
> - CHECK(data->out__bpf_link_fops1 != 0, "bpf_link_fops1",
> - "got %llu, exp %llu\n", data->out__bpf_link_fops1, (__u64)0);
> - CHECK(data->out__btf_size != btf_size, "btf_size",
> - "got %llu, exp %llu\n", data->out__btf_size, btf_size);
> - CHECK(data->out__per_cpu_start != per_cpu_start_addr, "__per_cpu_start",
> - "got %llu, exp %llu\n", data->out__per_cpu_start,
> - per_cpu_start_addr);
> + ASSERT_EQ(data->out__bpf_link_fops, link_fops_addr, "bpf_link_fops");
> + ASSERT_EQ(data->out__bpf_link_fops1, 0, "bpf_link_fops1");
> + ASSERT_EQ(data->out__btf_size, btf_size, "btf_size");
> + ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>
> cleanup:
> test_ksyms__destroy(skel);
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel
2024-03-02 16:50 ` [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel Yonghong Song
@ 2024-03-05 5:37 ` Alexei Starovoitov
2024-03-05 7:24 ` Yonghong Song
0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2024-03-05 5:37 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau
On Sat, Mar 2, 2024 at 8:50 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> In my locally build clang LTO kernel (enabling CONFIG_LTO and
> CONFIG_LTO_CLANG_THIN), ksyms test failed like:
> test_ksyms:PASS:kallsyms_fopen 0 nsec
> test_ksyms:FAIL:ksym_find symbol 'bpf_link_fops' not found
> #118 ksyms:FAIL
>
> The reason is that 'bpf_link_fops' is renamed to
> bpf_link_fops.llvm.8325593422554671469
> Due to cross-file inlining, the static variable 'bpf_link_fops'
> in syscall.c is used by a function in another file. To avoid
> potential duplicated names, the llvm added suffix '.llvm.<hash>'.
>
> To fix the failure, we can skip this test with LTO kernel
> if the symbol 'bpf_link_fops' is not found in kallsyms.
>
> After this patch, with the same LTO kernel:
> #118 ksyms:SKIP
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
> tools/testing/selftests/bpf/prog_tests/ksyms.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> index e081f8bf3f17..cd81f190c5d7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> @@ -21,7 +21,11 @@ void test_ksyms(void)
> return;
> }
> if (err == -ENOENT) {
> - ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
> + /* bpf_link_fops might be renamed to bpf_link_fops.llvm.<hash> in LTO kernel. */
> + if (check_lto_kernel() == 1)
> + test__skip();
> + else
> + ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
I'm afraid LTO breakage is bigger than this.
pid_iter program as part of bpftool is using bpf_link_fops too.
I suspect that part of bpftool feature is broken on LTO kernel.
We need to find a solution instead of 'skip' a selftest.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test
2024-03-05 0:17 ` Andrii Nakryiko
@ 2024-03-05 7:21 ` Yonghong Song
0 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2024-03-05 7:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
On 3/4/24 4:17 PM, Andrii Nakryiko wrote:
> On Sat, Mar 2, 2024 at 8:50 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> I am going to modify ksyms test later so take this opportunity
>> to replace old CHECK macros with new ASSERT macros.
>> No functionality change.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>> .../testing/selftests/bpf/prog_tests/ksyms.c | 38 +++++++++----------
>> 1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> index b295969b263b..e081f8bf3f17 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> @@ -5,8 +5,6 @@
>> #include "test_ksyms.skel.h"
>> #include <sys/stat.h>
>>
>> -static int duration;
>> -
>> void test_ksyms(void)
>> {
>> const char *btf_path = "/sys/kernel/btf/vmlinux";
>> @@ -18,43 +16,45 @@ void test_ksyms(void)
>> int err;
>>
>> err = kallsyms_find("bpf_link_fops", &link_fops_addr);
>> - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>> + if (err == -EINVAL) {
>> + ASSERT_TRUE(false, "kallsyms_fopen for bpf_link_fops");
> should this (and few other cases below) be ASSERT_EQ()/ASSERT_NEQ()
> (whichever makes sense, I can't reason about CHECK() conditions).
The below 'err == -ENOENT' case will later be modified in Patch 3 where I cannot
do ASSERT_EQ(err, -ENOENT) which is why I do 'err == -EINVAL' here. But you have
a good point that ASSERT_EQ/NEQ is easier to understand. Will fix it in the next
revision.
>
> ASSERT_TRUE(false) is a last resort way, we have more meaningful checks.
>
>> return;
>> - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
>> + }
>> + if (err == -ENOENT) {
>> + ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
>> return;
>> + }
>>
>> err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
>> - if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>> + if (err == -EINVAL) {
>> + ASSERT_TRUE(false, "kallsyms_fopen for __per_cpu_start");
>> return;
>> - if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
>> + }
>> + if (err == -ENOENT) {
>> + ASSERT_TRUE(false, "ksym_find for __per_cpu_start");
>> return;
>> + }
[...]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel
2024-03-05 5:37 ` Alexei Starovoitov
@ 2024-03-05 7:24 ` Yonghong Song
0 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2024-03-05 7:24 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Kernel Team, Martin KaFai Lau
On 3/4/24 9:37 PM, Alexei Starovoitov wrote:
> On Sat, Mar 2, 2024 at 8:50 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> In my locally build clang LTO kernel (enabling CONFIG_LTO and
>> CONFIG_LTO_CLANG_THIN), ksyms test failed like:
>> test_ksyms:PASS:kallsyms_fopen 0 nsec
>> test_ksyms:FAIL:ksym_find symbol 'bpf_link_fops' not found
>> #118 ksyms:FAIL
>>
>> The reason is that 'bpf_link_fops' is renamed to
>> bpf_link_fops.llvm.8325593422554671469
>> Due to cross-file inlining, the static variable 'bpf_link_fops'
>> in syscall.c is used by a function in another file. To avoid
>> potential duplicated names, the llvm added suffix '.llvm.<hash>'.
>>
>> To fix the failure, we can skip this test with LTO kernel
>> if the symbol 'bpf_link_fops' is not found in kallsyms.
>>
>> After this patch, with the same LTO kernel:
>> #118 ksyms:SKIP
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>> tools/testing/selftests/bpf/prog_tests/ksyms.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> index e081f8bf3f17..cd81f190c5d7 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> @@ -21,7 +21,11 @@ void test_ksyms(void)
>> return;
>> }
>> if (err == -ENOENT) {
>> - ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
>> + /* bpf_link_fops might be renamed to bpf_link_fops.llvm.<hash> in LTO kernel. */
>> + if (check_lto_kernel() == 1)
>> + test__skip();
>> + else
>> + ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
> I'm afraid LTO breakage is bigger than this.
> pid_iter program as part of bpftool is using bpf_link_fops too.
> I suspect that part of bpftool feature is broken on LTO kernel.
> We need to find a solution instead of 'skip' a selftest.
Thanks for pointing this out! Let me do some investigation on
how to resolve this.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-05 7:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-02 16:50 [PATCH bpf-next 0/4] selftests/bpf: Fix a couple of test failures with LTO kernel Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test Yonghong Song
2024-03-05 0:17 ` Andrii Nakryiko
2024-03-05 7:21 ` Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 2/4] selftests/bpf: Add check_lto_kernel() helper Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 3/4] selftests/bpf: Fix possible ksyms test failure with LTO kernel Yonghong Song
2024-03-05 5:37 ` Alexei Starovoitov
2024-03-05 7:24 ` Yonghong Song
2024-03-02 16:50 ` [PATCH bpf-next 4/4] selftests/bpf: Fix possible kprobe_multi_bench_attach " Yonghong Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.