[PATCH bpf-next v2 0/5] bpf: Fix a couple of test failures with LTO kernel

[PATCH bpf-next v2 0/5] bpf: Fix a couple of test failures with LTO kernel

[Yonghong Song]

With a LTO kernel built with clang, with one of earlier version of kernel,
I encountered two test failures, ksyms and kprobe_multi_bench_attach/kernel.
Now with latest bpf-next, only kprobe_multi_bench_attach/kernel failed.
But it is possible in the future ksyms selftest may fail again.

Both test failures are due to static variable/function renaming
due to cross-file inlining. For Ksyms failure, the solution is
to strip .llvm.<hash> suffixes for symbols in /proc/kallsyms before
comparing against the ksym in bpf program.
For kprobe_multi_bench_attach/kernel failure, the solution is
to either provide names in /proc/kallsyms to the kernel or
ignore those names who have .llvm.<hash> suffix since the kernel
sym name comparison is against /proc/kallsyms.

Please see each individual patches for details.

Yonghong Song (5):
  selftests/bpf: Replace CHECK with ASSERT macros for ksyms test
  libbpf: Mark libbpf_kallsyms_parse static function
  libbpf: Handle <orig_name>.llvm.<hash> symbol properly
  selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO
    kernel
  selftests/bpf: Add a selftest with available_filter_functions_addrs

 tools/lib/bpf/libbpf.c                        |  66 +++++++-
 tools/lib/bpf/libbpf_internal.h               |   2 -
 .../bpf/prog_tests/kprobe_multi_test.c        | 151 ++++++++++++++++++
 .../testing/selftests/bpf/prog_tests/ksyms.c  |  30 ++--
 4 files changed, 224 insertions(+), 25 deletions(-)

-- 
2.43.0

[PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Yonghong Song]

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  | 30 ++++++-------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
index b295969b263b..6a86d1f07800 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,33 @@ 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))
-		return;
-	if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
+	if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "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))
-		return;
-	if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
+	if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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

Re: [PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Jiri Olsa]

On Thu, Mar 21, 2024 at 01:01:03PM -0700, Yonghong Song 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>

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  .../testing/selftests/bpf/prog_tests/ksyms.c  | 30 ++++++-------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> index b295969b263b..6a86d1f07800 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,33 @@ 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))
> -		return;
> -	if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
> +	if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "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))
> -		return;
> -	if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
> +	if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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
> 
> 

Re: [PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Andrii Nakryiko]

On Thu, Mar 21, 2024 at 1:01 PM 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  | 30 ++++++-------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> index b295969b263b..6a86d1f07800 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,33 @@ 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))
> -               return;
> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "bpf_link_fops"))
>                 return;

it's best to keep each individual equality test as ASSERT_EQ(), that
way we get more specific and detailed error (with expected vs actual
value), which here we'll just get "wanted true but got false", and
then we'd have to debug some more which of the conditions it is. So
please keep the original granularity of checks everywhere.

>
>         err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
> -               return;
> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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
>

Re: [PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Yonghong Song]

On 3/22/24 9:13 AM, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 1:01 PM 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  | 30 ++++++-------------
>>   1 file changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> index b295969b263b..6a86d1f07800 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,33 @@ 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))
>> -               return;
>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
>> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "bpf_link_fops"))
>>                  return;
> it's best to keep each individual equality test as ASSERT_EQ(), that
> way we get more specific and detailed error (with expected vs actual
> value), which here we'll just get "wanted true but got false", and
> then we'd have to debug some more which of the conditions it is. So
> please keep the original granularity of checks everywhere.

Do you mean

         if (ASSERT_EQ(err, -EINVAL, "..."))
                 return;
         if (ASSERT_EQ(err, -ENOENT, "..."))
                 return;

This does not really work, for example if err = 0, it will declare test 
failure.


>
>>          err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
>> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>> -               return;
>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
>> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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
>>

Re: [PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Andrii Nakryiko]

On Fri, Mar 22, 2024 at 9:41 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/22/24 9:13 AM, Andrii Nakryiko wrote:
> > On Thu, Mar 21, 2024 at 1:01 PM 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  | 30 ++++++-------------
> >>   1 file changed, 9 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
> >> index b295969b263b..6a86d1f07800 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,33 @@ 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))
> >> -               return;
> >> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
> >> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "bpf_link_fops"))
> >>                  return;
> > it's best to keep each individual equality test as ASSERT_EQ(), that
> > way we get more specific and detailed error (with expected vs actual
> > value), which here we'll just get "wanted true but got false", and
> > then we'd have to debug some more which of the conditions it is. So
> > please keep the original granularity of checks everywhere.
>
> Do you mean
>
>          if (ASSERT_EQ(err, -EINVAL, "..."))
>                  return;
>          if (ASSERT_EQ(err, -ENOENT, "..."))
>                  return;
>
> This does not really work, for example if err = 0, it will declare test
> failure.


if (!ASSERT_NEQ(err, -EINVAL, "..."))
    return;
if (!ASSERT_NEQ(err, -NOENT, "..."))
    return;

It's mirroring original logic. We had two independent checks there? In
this case the condition is "not equal" (ASSERT_NEQ).

>
>
> >
> >>          err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
> >> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
> >> -               return;
> >> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
> >> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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
> >>

Re: [PATCH bpf-next v2 1/5] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

[Yonghong Song]

On 3/22/24 9:48 AM, Andrii Nakryiko wrote:
> On Fri, Mar 22, 2024 at 9:41 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/22/24 9:13 AM, Andrii Nakryiko wrote:
>>> On Thu, Mar 21, 2024 at 1:01 PM 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  | 30 ++++++-------------
>>>>    1 file changed, 9 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>>> index b295969b263b..6a86d1f07800 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,33 @@ 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))
>>>> -               return;
>>>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
>>>> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "bpf_link_fops"))
>>>>                   return;
>>> it's best to keep each individual equality test as ASSERT_EQ(), that
>>> way we get more specific and detailed error (with expected vs actual
>>> value), which here we'll just get "wanted true but got false", and
>>> then we'd have to debug some more which of the conditions it is. So
>>> please keep the original granularity of checks everywhere.
>> Do you mean
>>
>>           if (ASSERT_EQ(err, -EINVAL, "..."))
>>                   return;
>>           if (ASSERT_EQ(err, -ENOENT, "..."))
>>                   return;
>>
>> This does not really work, for example if err = 0, it will declare test
>> failure.
>
> if (!ASSERT_NEQ(err, -EINVAL, "..."))
>      return;
> if (!ASSERT_NEQ(err, -NOENT, "..."))
>      return;
>
> It's mirroring original logic. We had two independent checks there? In
> this case the condition is "not equal" (ASSERT_NEQ).

I actually tried this as well when I am experimenting ASSERT_EQ
and somehow I concluded that !ASSERT_NEQ not working either.

Now, I actully tried with adding err = 0, -EINVAL and -ENOENT
and verified it indeed works. I don't know how I concluded
it was not working before... Sad.

>
>>
>>>>           err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
>>>> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>>>> -               return;
>>>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
>>>> +       if (!ASSERT_TRUE(err != -EINVAL && err != -ENOENT, "__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
>>>>

[PATCH bpf-next v2 2/5] libbpf: Mark libbpf_kallsyms_parse static function

[Yonghong Song]

Currently libbpf_kallsyms_parse() function is declared as a global
function but actually it is not a API and there is no external
users in bpftool/bpf-selftests. So let us mark the function as
static.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/libbpf.c          | 2 +-
 tools/lib/bpf/libbpf_internal.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 86df0d50cba7..a7a89269148c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7982,7 +7982,7 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
 	return 0;
 }
 
-int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
+static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
 {
 	char sym_type, sym_name[500];
 	unsigned long long sym_addr;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 864b36177424..b1bbbdcb7792 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -521,8 +521,6 @@ __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
 typedef int (*kallsyms_cb_t)(unsigned long long sym_addr, char sym_type,
 			     const char *sym_name, void *ctx);
 
-int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *arg);
-
 /* handle direct returned errors */
 static inline int libbpf_err(int ret)
 {
-- 
2.43.0

Re: [PATCH bpf-next v2 2/5] libbpf: Mark libbpf_kallsyms_parse static function

[Jiri Olsa]

On Thu, Mar 21, 2024 at 01:01:08PM -0700, Yonghong Song wrote:
> Currently libbpf_kallsyms_parse() function is declared as a global
> function but actually it is not a API and there is no external
> users in bpftool/bpf-selftests. So let us mark the function as
> static.
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c          | 2 +-
>  tools/lib/bpf/libbpf_internal.h | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 86df0d50cba7..a7a89269148c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7982,7 +7982,7 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
>  	return 0;
>  }
>  
> -int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
> +static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>  {
>  	char sym_type, sym_name[500];
>  	unsigned long long sym_addr;
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 864b36177424..b1bbbdcb7792 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -521,8 +521,6 @@ __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
>  typedef int (*kallsyms_cb_t)(unsigned long long sym_addr, char sym_type,
>  			     const char *sym_name, void *ctx);
>  

there's also kallsyms_cb_t which should be moved to libbpf.c,
or perhaps removed and unwinded in libbpf_kallsyms_parse

jirka

> -int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *arg);
> -
>  /* handle direct returned errors */
>  static inline int libbpf_err(int ret)
>  {
> -- 
> 2.43.0
> 
> 

Re: [PATCH bpf-next v2 2/5] libbpf: Mark libbpf_kallsyms_parse static function

[Yonghong Song]

On 3/22/24 5:37 AM, Jiri Olsa wrote:
> On Thu, Mar 21, 2024 at 01:01:08PM -0700, Yonghong Song wrote:
>> Currently libbpf_kallsyms_parse() function is declared as a global
>> function but actually it is not a API and there is no external
>> users in bpftool/bpf-selftests. So let us mark the function as
>> static.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c          | 2 +-
>>   tools/lib/bpf/libbpf_internal.h | 2 --
>>   2 files changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 86df0d50cba7..a7a89269148c 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -7982,7 +7982,7 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
>>   	return 0;
>>   }
>>   
>> -int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>> +static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>>   {
>>   	char sym_type, sym_name[500];
>>   	unsigned long long sym_addr;
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index 864b36177424..b1bbbdcb7792 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -521,8 +521,6 @@ __s32 btf__find_by_name_kind_own(const struct btf *btf, const char *type_name,
>>   typedef int (*kallsyms_cb_t)(unsigned long long sym_addr, char sym_type,
>>   			     const char *sym_name, void *ctx);
>>   
> there's also kallsyms_cb_t which should be moved to libbpf.c,
> or perhaps removed and unwinded in libbpf_kallsyms_parse

Thanks for suggestion, will make the change in the next revision.

>
> jirka
>
>> -int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *arg);
>> -
>>   /* handle direct returned errors */
>>   static inline int libbpf_err(int ret)
>>   {
>> -- 
>> 2.43.0
>>
>>

[PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song]

With CONFIG_LTO_CLANG_THIN enabled, with some of previous
version of kernel code base ([1]), I hit the following
error:
   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>' ([2]) to 'bpf_link_fops' variable.
Such renaming caused a problem in libbpf if 'bpf_link_fops'
is used in bpf prog as a ksym as 'bpf_link_fops' does not
match any symbol in /proc/kallsyms.

To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
is caused by clang lto kernel and to process such symbols properly.

With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
I cannot reproduce the above failure any more. But such an issue
could happen with other symbols.

For example, with my current kernel, I got the following from
/proc/kallsyms:
  ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
  ffffffff85f0a500 d tk_core.llvm.726630847145216431
  ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
  ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300

I could not easily create a selftest to test newly-added
libbpf functionality with a static C test since I do not know
which symbol is cross-file inlined. But based on my particular kernel,
the following test change can run successfully.

  diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
  index 6a86d1f07800..904a103f7b1d 100644
  --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
  +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
  @@ -42,6 +42,7 @@ void test_ksyms(void)
          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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
          ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");

   cleanup:
  diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
  index 6c9cbb5a3bdf..fe91eef54b66 100644
  --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
  +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
  @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
   __u64 out__bpf_link_fops1 = -1;
   __u64 out__btf_size = -1;
   __u64 out__per_cpu_start = -1;
  +__u64 out__fake_dst_ops = -1;

   extern const void bpf_link_fops __ksym;
   extern const void __start_BTF __ksym;
   extern const void __stop_BTF __ksym;
   extern const void __per_cpu_start __ksym;
  +extern const void fake_dst_ops __ksym;
   /* non-existing symbol, weak, default to zero */
   extern const void bpf_link_fops1 __ksym __weak;

  @@ -23,6 +25,7 @@ int handler(const void *ctx)
          out__bpf_link_fops = (__u64)&bpf_link_fops;
          out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
          out__per_cpu_start = (__u64)&__per_cpu_start;
  +       out__fake_dst_ops = (__u64)&fake_dst_ops;

          out__bpf_link_fops1 = (__u64)&bpf_link_fops1;

This patch fixed the issue in libbpf such that if clang lto kernel
is enabled and the symbol resolution is for ksym's,
the suffix '.llvm.<hash>' will be ignored during comparison of
bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.

Note that currently kernel does not support gcc build with lto.

  [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
  [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a7a89269148c..8c3861192bc8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -664,6 +664,7 @@ struct bpf_object {
 	bool loaded;
 	bool has_subcalls;
 	bool has_rodata;
+	bool need_kallsyms;
 
 	struct bpf_gen *gen_loader;
 
@@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
 	return err;
 }
 
+static 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_CLANG=y", 18)) {
+			check_lto = 1;
+			break;
+		}
+	}
+
+	gzclose(file);
+out:
+	return check_lto;
+}
+
 static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
 		       const char *sym_name, void *ctx)
 {
+	int lto_enabled = check_lto_kernel();
+	char orig_name[PATH_MAX], *res;
 	struct bpf_object *obj = ctx;
 	const struct btf_type *t;
 	struct extern_desc *ext;
 
-	ext = find_extern_by_name(obj, sym_name);
+	/* Only check static variables in data sections */
+	if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
+		strcpy(orig_name, sym_name);
+		res = strstr(orig_name, ".llvm.");
+		if (res) {
+			*res = '\0';
+			pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
+				 sym_name, orig_name);
+		}
+		ext = find_extern_by_name(obj, orig_name);
+	} else {
+		ext = find_extern_by_name(obj, sym_name);
+	}
 	if (!ext || ext->type != EXT_KSYM)
 		return 0;
 
@@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
 			return -EINVAL;
 	}
 	if (need_kallsyms) {
+		obj->need_kallsyms = true;
 		err = bpf_object__read_kallsyms_file(obj);
+		obj->need_kallsyms = false;
 		if (err)
 			return -EINVAL;
 	}
-- 
2.43.0

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Alexei Starovoitov]

On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
> version of kernel code base ([1]), I hit the following
> error:
>    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>' ([2]) to 'bpf_link_fops' variable.
> Such renaming caused a problem in libbpf if 'bpf_link_fops'
> is used in bpf prog as a ksym as 'bpf_link_fops' does not
> match any symbol in /proc/kallsyms.
>
> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
> is caused by clang lto kernel and to process such symbols properly.
>
> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
> I cannot reproduce the above failure any more. But such an issue
> could happen with other symbols.
>
> For example, with my current kernel, I got the following from
> /proc/kallsyms:
>   ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>   ffffffff85f0a500 d tk_core.llvm.726630847145216431
>   ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>   ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>
> I could not easily create a selftest to test newly-added
> libbpf functionality with a static C test since I do not know
> which symbol is cross-file inlined. But based on my particular kernel,
> the following test change can run successfully.
>
>   diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   index 6a86d1f07800..904a103f7b1d 100644
>   --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   @@ -42,6 +42,7 @@ void test_ksyms(void)
>           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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>           ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>
>    cleanup:
>   diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   index 6c9cbb5a3bdf..fe91eef54b66 100644
>   --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>   +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>    __u64 out__bpf_link_fops1 = -1;
>    __u64 out__btf_size = -1;
>    __u64 out__per_cpu_start = -1;
>   +__u64 out__fake_dst_ops = -1;
>
>    extern const void bpf_link_fops __ksym;
>    extern const void __start_BTF __ksym;
>    extern const void __stop_BTF __ksym;
>    extern const void __per_cpu_start __ksym;
>   +extern const void fake_dst_ops __ksym;
>    /* non-existing symbol, weak, default to zero */
>    extern const void bpf_link_fops1 __ksym __weak;
>
>   @@ -23,6 +25,7 @@ int handler(const void *ctx)
>           out__bpf_link_fops = (__u64)&bpf_link_fops;
>           out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>           out__per_cpu_start = (__u64)&__per_cpu_start;
>   +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>
>           out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>
> This patch fixed the issue in libbpf such that if clang lto kernel
> is enabled and the symbol resolution is for ksym's,
> the suffix '.llvm.<hash>' will be ignored during comparison of
> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>
> Note that currently kernel does not support gcc build with lto.
>
>   [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>   [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a7a89269148c..8c3861192bc8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -664,6 +664,7 @@ struct bpf_object {
>         bool loaded;
>         bool has_subcalls;
>         bool has_rodata;
> +       bool need_kallsyms;
>
>         struct bpf_gen *gen_loader;
>
> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>         return err;
>  }
>
> +static 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_CLANG=y", 18)) {
> +                       check_lto = 1;
> +                       break;
> +               }
> +       }
> +
> +       gzclose(file);
> +out:
> +       return check_lto;
> +}
> +
>  static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>                        const char *sym_name, void *ctx)
>  {
> +       int lto_enabled = check_lto_kernel();
> +       char orig_name[PATH_MAX], *res;
>         struct bpf_object *obj = ctx;
>         const struct btf_type *t;
>         struct extern_desc *ext;
>
> -       ext = find_extern_by_name(obj, sym_name);
> +       /* Only check static variables in data sections */
> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {

why bother grepping config.gz ?
I see no harm in doing below strstr unconditionally.

> +               strcpy(orig_name, sym_name);
> +               res = strstr(orig_name, ".llvm.");
> +               if (res) {
> +                       *res = '\0';
> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
> +                                sym_name, orig_name);
> +               }
> +               ext = find_extern_by_name(obj, orig_name);
> +       } else {
> +               ext = find_extern_by_name(obj, sym_name);
> +       }
>         if (!ext || ext->type != EXT_KSYM)
>                 return 0;
>
> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>                         return -EINVAL;
>         }
>         if (need_kallsyms) {
> +               obj->need_kallsyms = true;
>                 err = bpf_object__read_kallsyms_file(obj);
> +               obj->need_kallsyms = false;
>                 if (err)
>                         return -EINVAL;
>         }
> --
> 2.43.0
>

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song]

On 3/21/24 2:54 PM, Alexei Starovoitov wrote:
> On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
>> version of kernel code base ([1]), I hit the following
>> error:
>>     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>' ([2]) to 'bpf_link_fops' variable.
>> Such renaming caused a problem in libbpf if 'bpf_link_fops'
>> is used in bpf prog as a ksym as 'bpf_link_fops' does not
>> match any symbol in /proc/kallsyms.
>>
>> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
>> is caused by clang lto kernel and to process such symbols properly.
>>
>> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
>> I cannot reproduce the above failure any more. But such an issue
>> could happen with other symbols.
>>
>> For example, with my current kernel, I got the following from
>> /proc/kallsyms:
>>    ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>>    ffffffff85f0a500 d tk_core.llvm.726630847145216431
>>    ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>>    ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>>
>> I could not easily create a selftest to test newly-added
>> libbpf functionality with a static C test since I do not know
>> which symbol is cross-file inlined. But based on my particular kernel,
>> the following test change can run successfully.
>>
>>    diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    index 6a86d1f07800..904a103f7b1d 100644
>>    --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    @@ -42,6 +42,7 @@ void test_ksyms(void)
>>            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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>>            ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>>
>>     cleanup:
>>    diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    index 6c9cbb5a3bdf..fe91eef54b66 100644
>>    --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>>     __u64 out__bpf_link_fops1 = -1;
>>     __u64 out__btf_size = -1;
>>     __u64 out__per_cpu_start = -1;
>>    +__u64 out__fake_dst_ops = -1;
>>
>>     extern const void bpf_link_fops __ksym;
>>     extern const void __start_BTF __ksym;
>>     extern const void __stop_BTF __ksym;
>>     extern const void __per_cpu_start __ksym;
>>    +extern const void fake_dst_ops __ksym;
>>     /* non-existing symbol, weak, default to zero */
>>     extern const void bpf_link_fops1 __ksym __weak;
>>
>>    @@ -23,6 +25,7 @@ int handler(const void *ctx)
>>            out__bpf_link_fops = (__u64)&bpf_link_fops;
>>            out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>>            out__per_cpu_start = (__u64)&__per_cpu_start;
>>    +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>>
>>            out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>>
>> This patch fixed the issue in libbpf such that if clang lto kernel
>> is enabled and the symbol resolution is for ksym's,
>> the suffix '.llvm.<hash>' will be ignored during comparison of
>> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>>
>> Note that currently kernel does not support gcc build with lto.
>>
>>    [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>>    [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index a7a89269148c..8c3861192bc8 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -664,6 +664,7 @@ struct bpf_object {
>>          bool loaded;
>>          bool has_subcalls;
>>          bool has_rodata;
>> +       bool need_kallsyms;
>>
>>          struct bpf_gen *gen_loader;
>>
>> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>>          return err;
>>   }
>>
>> +static 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_CLANG=y", 18)) {
>> +                       check_lto = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       gzclose(file);
>> +out:
>> +       return check_lto;
>> +}
>> +
>>   static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>>                         const char *sym_name, void *ctx)
>>   {
>> +       int lto_enabled = check_lto_kernel();
>> +       char orig_name[PATH_MAX], *res;
>>          struct bpf_object *obj = ctx;
>>          const struct btf_type *t;
>>          struct extern_desc *ext;
>>
>> -       ext = find_extern_by_name(obj, sym_name);
>> +       /* Only check static variables in data sections */
>> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> why bother grepping config.gz ?
> I see no harm in doing below strstr unconditionally.

Do you mean we skip condition
	sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
all together?

For condition sym_type == 'd', Andrii suggested (in private discussion)
to focus on data first since that is the issue we hitted. Of course
we could do all symbols here too.

For condition obj->need_kallsyms, I can remove this one.

For lto_enabled == 1, the main goal is to avoid extra overhead for
not-lto kernels.

I guess that the overhead is not that bad since typically symbol name
is not long. So removing all conditions seems indeed a viable solution.

>
>> +               strcpy(orig_name, sym_name);
>> +               res = strstr(orig_name, ".llvm.");
>> +               if (res) {
>> +                       *res = '\0';
>> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
>> +                                sym_name, orig_name);
>> +               }
>> +               ext = find_extern_by_name(obj, orig_name);
>> +       } else {
>> +               ext = find_extern_by_name(obj, sym_name);
>> +       }
>>          if (!ext || ext->type != EXT_KSYM)
>>                  return 0;
>>
>> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>>                          return -EINVAL;
>>          }
>>          if (need_kallsyms) {
>> +               obj->need_kallsyms = true;
>>                  err = bpf_object__read_kallsyms_file(obj);
>> +               obj->need_kallsyms = false;
>>                  if (err)
>>                          return -EINVAL;
>>          }
>> --
>> 2.43.0
>>

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Alexei Starovoitov]

On Thu, Mar 21, 2024 at 4:55 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> >>   static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
> >>                         const char *sym_name, void *ctx)
> >>   {
> >> +       int lto_enabled = check_lto_kernel();
> >> +       char orig_name[PATH_MAX], *res;
> >>          struct bpf_object *obj = ctx;
> >>          const struct btf_type *t;
> >>          struct extern_desc *ext;
> >>
> >> -       ext = find_extern_by_name(obj, sym_name);
> >> +       /* Only check static variables in data sections */
> >> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> > why bother grepping config.gz ?
> > I see no harm in doing below strstr unconditionally.
>
> Do you mean we skip condition
>         sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
> all together?
>
> For condition sym_type == 'd', Andrii suggested (in private discussion)
> to focus on data first since that is the issue we hitted. Of course
> we could do all symbols here too.
>
> For condition obj->need_kallsyms, I can remove this one.
>
> For lto_enabled == 1, the main goal is to avoid extra overhead for
> not-lto kernels.
>
> I guess that the overhead is not that bad since typically symbol name
> is not long. So removing all conditions seems indeed a viable solution.

I was suggesting to remove the last lto_enabled == 1 check and
check_lto_kernel() since it will simplify the patch significantly and
won't cause any slowdown.
The first two checks... I'm not sure.
The less corner cases the better. strstr is fast enough.
Ok to remove them all.

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Andrii Nakryiko]

On Thu, Mar 21, 2024 at 5:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Mar 21, 2024 at 4:55 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> > >>   static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
> > >>                         const char *sym_name, void *ctx)
> > >>   {
> > >> +       int lto_enabled = check_lto_kernel();
> > >> +       char orig_name[PATH_MAX], *res;
> > >>          struct bpf_object *obj = ctx;
> > >>          const struct btf_type *t;
> > >>          struct extern_desc *ext;
> > >>
> > >> -       ext = find_extern_by_name(obj, sym_name);
> > >> +       /* Only check static variables in data sections */
> > >> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> > > why bother grepping config.gz ?
> > > I see no harm in doing below strstr unconditionally.
> >
> > Do you mean we skip condition
> >         sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
> > all together?
> >
> > For condition sym_type == 'd', Andrii suggested (in private discussion)
> > to focus on data first since that is the issue we hitted. Of course
> > we could do all symbols here too.
> >
> > For condition obj->need_kallsyms, I can remove this one.
> >
> > For lto_enabled == 1, the main goal is to avoid extra overhead for
> > not-lto kernels.
> >
> > I guess that the overhead is not that bad since typically symbol name
> > is not long. So removing all conditions seems indeed a viable solution.
>
> I was suggesting to remove the last lto_enabled == 1 check and
> check_lto_kernel() since it will simplify the patch significantly and
> won't cause any slowdown.

+1, grepping Kconfig seems like an overkill

> The first two checks... I'm not sure.

I'd say we shouldn't do this for functions, because if LLVM rewrites
them, then usually that means that function signature is changed,
which seems dangerous to just silently resolve. I'd start with data
symbols for now.

> The less corner cases the better. strstr is fast enough.

yep, I doubt it would be noticeable if we do strstr()

> Ok to remove them all.

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song]

On 3/21/24 5:17 PM, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 5:02 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Thu, Mar 21, 2024 at 4:55 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>>    static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>>>>>                          const char *sym_name, void *ctx)
>>>>>    {
>>>>> +       int lto_enabled = check_lto_kernel();
>>>>> +       char orig_name[PATH_MAX], *res;
>>>>>           struct bpf_object *obj = ctx;
>>>>>           const struct btf_type *t;
>>>>>           struct extern_desc *ext;
>>>>>
>>>>> -       ext = find_extern_by_name(obj, sym_name);
>>>>> +       /* Only check static variables in data sections */
>>>>> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
>>>> why bother grepping config.gz ?
>>>> I see no harm in doing below strstr unconditionally.
>>> Do you mean we skip condition
>>>          sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
>>> all together?
>>>
>>> For condition sym_type == 'd', Andrii suggested (in private discussion)
>>> to focus on data first since that is the issue we hitted. Of course
>>> we could do all symbols here too.
>>>
>>> For condition obj->need_kallsyms, I can remove this one.
>>>
>>> For lto_enabled == 1, the main goal is to avoid extra overhead for
>>> not-lto kernels.
>>>
>>> I guess that the overhead is not that bad since typically symbol name
>>> is not long. So removing all conditions seems indeed a viable solution.
>> I was suggesting to remove the last lto_enabled == 1 check and
>> check_lto_kernel() since it will simplify the patch significantly and
>> won't cause any slowdown.
> +1, grepping Kconfig seems like an overkill
>
>> The first two checks... I'm not sure.
> I'd say we shouldn't do this for functions, because if LLVM rewrites
> them, then usually that means that function signature is changed,
> which seems dangerous to just silently resolve. I'd start with data
> symbols for now.

For this particular suffix '.llvm.<hash>', function signature will
not change. But considering all funcitons annotated with __ksym are
kfunc's and I cannot see currently how kfunc's could be cross-file
inlined.

It might be possible a kernel function is attached with __ksym as
bpf program wants to know the address of that kernel function.
But this should be a really corner case.

So agree and let me keep sym_type == 'd' condition only.

>
>> The less corner cases the better. strstr is fast enough.
> yep, I doubt it would be noticeable if we do strstr()
>> Ok to remove them all.

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Andrii Nakryiko]

On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
> version of kernel code base ([1]), I hit the following
> error:
>    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>' ([2]) to 'bpf_link_fops' variable.
> Such renaming caused a problem in libbpf if 'bpf_link_fops'
> is used in bpf prog as a ksym as 'bpf_link_fops' does not
> match any symbol in /proc/kallsyms.
>
> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
> is caused by clang lto kernel and to process such symbols properly.
>
> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
> I cannot reproduce the above failure any more. But such an issue
> could happen with other symbols.
>
> For example, with my current kernel, I got the following from
> /proc/kallsyms:
>   ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>   ffffffff85f0a500 d tk_core.llvm.726630847145216431
>   ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>   ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>
> I could not easily create a selftest to test newly-added
> libbpf functionality with a static C test since I do not know
> which symbol is cross-file inlined. But based on my particular kernel,
> the following test change can run successfully.
>
>   diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   index 6a86d1f07800..904a103f7b1d 100644
>   --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   @@ -42,6 +42,7 @@ void test_ksyms(void)
>           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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>           ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>
>    cleanup:
>   diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   index 6c9cbb5a3bdf..fe91eef54b66 100644
>   --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>   +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>    __u64 out__bpf_link_fops1 = -1;
>    __u64 out__btf_size = -1;
>    __u64 out__per_cpu_start = -1;
>   +__u64 out__fake_dst_ops = -1;
>
>    extern const void bpf_link_fops __ksym;
>    extern const void __start_BTF __ksym;
>    extern const void __stop_BTF __ksym;
>    extern const void __per_cpu_start __ksym;
>   +extern const void fake_dst_ops __ksym;
>    /* non-existing symbol, weak, default to zero */
>    extern const void bpf_link_fops1 __ksym __weak;
>
>   @@ -23,6 +25,7 @@ int handler(const void *ctx)
>           out__bpf_link_fops = (__u64)&bpf_link_fops;
>           out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>           out__per_cpu_start = (__u64)&__per_cpu_start;
>   +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>
>           out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>
> This patch fixed the issue in libbpf such that if clang lto kernel
> is enabled and the symbol resolution is for ksym's,
> the suffix '.llvm.<hash>' will be ignored during comparison of
> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>
> Note that currently kernel does not support gcc build with lto.
>
>   [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>   [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a7a89269148c..8c3861192bc8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -664,6 +664,7 @@ struct bpf_object {
>         bool loaded;
>         bool has_subcalls;
>         bool has_rodata;
> +       bool need_kallsyms;
>
>         struct bpf_gen *gen_loader;
>
> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>         return err;
>  }
>
> +static 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_CLANG=y", 18)) {
> +                       check_lto = 1;
> +                       break;
> +               }
> +       }
> +
> +       gzclose(file);
> +out:
> +       return check_lto;
> +}
> +
>  static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>                        const char *sym_name, void *ctx)
>  {
> +       int lto_enabled = check_lto_kernel();
> +       char orig_name[PATH_MAX], *res;
>         struct bpf_object *obj = ctx;
>         const struct btf_type *t;
>         struct extern_desc *ext;
>
> -       ext = find_extern_by_name(obj, sym_name);
> +       /* Only check static variables in data sections */
> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> +               strcpy(orig_name, sym_name);
> +               res = strstr(orig_name, ".llvm.");
> +               if (res) {
> +                       *res = '\0';
> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
> +                                sym_name, orig_name);
> +               }
> +               ext = find_extern_by_name(obj, orig_name);

let's teach find_extern_by_name() to take string length so we don't
have to do those strcpy, we can just pass "effective length"

I'll take a look at the patch set tomorrow, ran out of time today, sorry.

> +       } else {
> +               ext = find_extern_by_name(obj, sym_name);
> +       }
>         if (!ext || ext->type != EXT_KSYM)
>                 return 0;
>
> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>                         return -EINVAL;
>         }
>         if (need_kallsyms) {
> +               obj->need_kallsyms = true;
>                 err = bpf_object__read_kallsyms_file(obj);
> +               obj->need_kallsyms = false;
>                 if (err)
>                         return -EINVAL;
>         }
> --
> 2.43.0
>

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song]

On 3/21/24 5:18 PM, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
>> version of kernel code base ([1]), I hit the following
>> error:
>>     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>' ([2]) to 'bpf_link_fops' variable.
>> Such renaming caused a problem in libbpf if 'bpf_link_fops'
>> is used in bpf prog as a ksym as 'bpf_link_fops' does not
>> match any symbol in /proc/kallsyms.
>>
>> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
>> is caused by clang lto kernel and to process such symbols properly.
>>
>> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
>> I cannot reproduce the above failure any more. But such an issue
>> could happen with other symbols.
>>
>> For example, with my current kernel, I got the following from
>> /proc/kallsyms:
>>    ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>>    ffffffff85f0a500 d tk_core.llvm.726630847145216431
>>    ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>>    ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>>
>> I could not easily create a selftest to test newly-added
>> libbpf functionality with a static C test since I do not know
>> which symbol is cross-file inlined. But based on my particular kernel,
>> the following test change can run successfully.
>>
>>    diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    index 6a86d1f07800..904a103f7b1d 100644
>>    --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    @@ -42,6 +42,7 @@ void test_ksyms(void)
>>            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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>>            ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>>
>>     cleanup:
>>    diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    index 6c9cbb5a3bdf..fe91eef54b66 100644
>>    --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>>     __u64 out__bpf_link_fops1 = -1;
>>     __u64 out__btf_size = -1;
>>     __u64 out__per_cpu_start = -1;
>>    +__u64 out__fake_dst_ops = -1;
>>
>>     extern const void bpf_link_fops __ksym;
>>     extern const void __start_BTF __ksym;
>>     extern const void __stop_BTF __ksym;
>>     extern const void __per_cpu_start __ksym;
>>    +extern const void fake_dst_ops __ksym;
>>     /* non-existing symbol, weak, default to zero */
>>     extern const void bpf_link_fops1 __ksym __weak;
>>
>>    @@ -23,6 +25,7 @@ int handler(const void *ctx)
>>            out__bpf_link_fops = (__u64)&bpf_link_fops;
>>            out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>>            out__per_cpu_start = (__u64)&__per_cpu_start;
>>    +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>>
>>            out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>>
>> This patch fixed the issue in libbpf such that if clang lto kernel
>> is enabled and the symbol resolution is for ksym's,
>> the suffix '.llvm.<hash>' will be ignored during comparison of
>> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>>
>> Note that currently kernel does not support gcc build with lto.
>>
>>    [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>>    [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index a7a89269148c..8c3861192bc8 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -664,6 +664,7 @@ struct bpf_object {
>>          bool loaded;
>>          bool has_subcalls;
>>          bool has_rodata;
>> +       bool need_kallsyms;
>>
>>          struct bpf_gen *gen_loader;
>>
>> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>>          return err;
>>   }
>>
>> +static 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_CLANG=y", 18)) {
>> +                       check_lto = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       gzclose(file);
>> +out:
>> +       return check_lto;
>> +}
>> +
>>   static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>>                         const char *sym_name, void *ctx)
>>   {
>> +       int lto_enabled = check_lto_kernel();
>> +       char orig_name[PATH_MAX], *res;
>>          struct bpf_object *obj = ctx;
>>          const struct btf_type *t;
>>          struct extern_desc *ext;
>>
>> -       ext = find_extern_by_name(obj, sym_name);
>> +       /* Only check static variables in data sections */
>> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
>> +               strcpy(orig_name, sym_name);
>> +               res = strstr(orig_name, ".llvm.");
>> +               if (res) {
>> +                       *res = '\0';
>> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
>> +                                sym_name, orig_name);
>> +               }
>> +               ext = find_extern_by_name(obj, orig_name);
> let's teach find_extern_by_name() to take string length so we don't
> have to do those strcpy, we can just pass "effective length"

good idea!

>
> I'll take a look at the patch set tomorrow, ran out of time today, sorry.

thanks.

>
>> +       } else {
>> +               ext = find_extern_by_name(obj, sym_name);
>> +       }
>>          if (!ext || ext->type != EXT_KSYM)
>>                  return 0;
>>
>> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>>                          return -EINVAL;
>>          }
>>          if (need_kallsyms) {
>> +               obj->need_kallsyms = true;
>>                  err = bpf_object__read_kallsyms_file(obj);
>> +               obj->need_kallsyms = false;
>>                  if (err)
>>                          return -EINVAL;
>>          }
>> --
>> 2.43.0
>>

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Andrii Nakryiko]

On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
> version of kernel code base ([1]), I hit the following
> error:
>    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>' ([2]) to 'bpf_link_fops' variable.
> Such renaming caused a problem in libbpf if 'bpf_link_fops'
> is used in bpf prog as a ksym as 'bpf_link_fops' does not
> match any symbol in /proc/kallsyms.
>
> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
> is caused by clang lto kernel and to process such symbols properly.
>
> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
> I cannot reproduce the above failure any more. But such an issue
> could happen with other symbols.
>
> For example, with my current kernel, I got the following from
> /proc/kallsyms:
>   ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>   ffffffff85f0a500 d tk_core.llvm.726630847145216431
>   ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>   ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>
> I could not easily create a selftest to test newly-added
> libbpf functionality with a static C test since I do not know
> which symbol is cross-file inlined. But based on my particular kernel,
> the following test change can run successfully.
>
>   diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   index 6a86d1f07800..904a103f7b1d 100644
>   --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>   @@ -42,6 +42,7 @@ void test_ksyms(void)
>           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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>           ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>
>    cleanup:
>   diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   index 6c9cbb5a3bdf..fe91eef54b66 100644
>   --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>   +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>   @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>    __u64 out__bpf_link_fops1 = -1;
>    __u64 out__btf_size = -1;
>    __u64 out__per_cpu_start = -1;
>   +__u64 out__fake_dst_ops = -1;
>
>    extern const void bpf_link_fops __ksym;
>    extern const void __start_BTF __ksym;
>    extern const void __stop_BTF __ksym;
>    extern const void __per_cpu_start __ksym;
>   +extern const void fake_dst_ops __ksym;
>    /* non-existing symbol, weak, default to zero */
>    extern const void bpf_link_fops1 __ksym __weak;
>
>   @@ -23,6 +25,7 @@ int handler(const void *ctx)
>           out__bpf_link_fops = (__u64)&bpf_link_fops;
>           out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>           out__per_cpu_start = (__u64)&__per_cpu_start;
>   +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>
>           out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>
> This patch fixed the issue in libbpf such that if clang lto kernel
> is enabled and the symbol resolution is for ksym's,
> the suffix '.llvm.<hash>' will be ignored during comparison of
> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>
> Note that currently kernel does not support gcc build with lto.
>
>   [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>   [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a7a89269148c..8c3861192bc8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -664,6 +664,7 @@ struct bpf_object {
>         bool loaded;
>         bool has_subcalls;
>         bool has_rodata;
> +       bool need_kallsyms;
>
>         struct bpf_gen *gen_loader;
>
> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>         return err;
>  }
>
> +static 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_CLANG=y", 18)) {
> +                       check_lto = 1;
> +                       break;
> +               }
> +       }
> +
> +       gzclose(file);
> +out:
> +       return check_lto;
> +}
> +
>  static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>                        const char *sym_name, void *ctx)
>  {
> +       int lto_enabled = check_lto_kernel();
> +       char orig_name[PATH_MAX], *res;
>         struct bpf_object *obj = ctx;
>         const struct btf_type *t;
>         struct extern_desc *ext;
>
> -       ext = find_extern_by_name(obj, sym_name);
> +       /* Only check static variables in data sections */
> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> +               strcpy(orig_name, sym_name);
> +               res = strstr(orig_name, ".llvm.");
> +               if (res) {
> +                       *res = '\0';
> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
> +                                sym_name, orig_name);
> +               }
> +               ext = find_extern_by_name(obj, orig_name);
> +       } else {
> +               ext = find_extern_by_name(obj, sym_name);
> +       }
>         if (!ext || ext->type != EXT_KSYM)
>                 return 0;
>
> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>                         return -EINVAL;
>         }
>         if (need_kallsyms) {
> +               obj->need_kallsyms = true;
>                 err = bpf_object__read_kallsyms_file(obj);
> +               obj->need_kallsyms = false;

I'm not clear on why you need this obj->need_kallsyms? kallsyms_cb is
used for just this use case, it seems, it should be fine without this
extra temporary flag.

Ideally we should also switch to the iterator approach for kallsyms,
just like we did with elf symbols iterator (see elf_sym_iter in
elf.c), it would be a cleaner approach. Let me know if you are
interested in doing this as well (it's not mandatory for the changes
in this patch set, just to be clear).

>                 if (err)
>                         return -EINVAL;
>         }
> --
> 2.43.0
>

Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song]

On 3/22/24 2:50 PM, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> With CONFIG_LTO_CLANG_THIN enabled, with some of previous
>> version of kernel code base ([1]), I hit the following
>> error:
>>     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>' ([2]) to 'bpf_link_fops' variable.
>> Such renaming caused a problem in libbpf if 'bpf_link_fops'
>> is used in bpf prog as a ksym as 'bpf_link_fops' does not
>> match any symbol in /proc/kallsyms.
>>
>> To fix this issue, libbpf needs to understand that suffix '.llvm.<hash>'
>> is caused by clang lto kernel and to process such symbols properly.
>>
>> With latest bpf-next code base built with CONFIG_LTO_CLANG_THIN,
>> I cannot reproduce the above failure any more. But such an issue
>> could happen with other symbols.
>>
>> For example, with my current kernel, I got the following from
>> /proc/kallsyms:
>>    ffffffff84782154 d __func__.net_ratelimit.llvm.6135436931166841955
>>    ffffffff85f0a500 d tk_core.llvm.726630847145216431
>>    ffffffff85fdb960 d __fs_reclaim_map.llvm.10487989720912350772
>>    ffffffff864c7300 d fake_dst_ops.llvm.54750082607048300
>>
>> I could not easily create a selftest to test newly-added
>> libbpf functionality with a static C test since I do not know
>> which symbol is cross-file inlined. But based on my particular kernel,
>> the following test change can run successfully.
>>
>>    diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    index 6a86d1f07800..904a103f7b1d 100644
>>    --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>>    @@ -42,6 +42,7 @@ void test_ksyms(void)
>>            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_NEQ(data->out__fake_dst_ops, 0, "fake_dst_ops");
>>            ASSERT_EQ(data->out__per_cpu_start, per_cpu_start_addr, "__per_cpu_start");
>>
>>     cleanup:
>>    diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    index 6c9cbb5a3bdf..fe91eef54b66 100644
>>    --- a/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c
>>    @@ -9,11 +9,13 @@ __u64 out__bpf_link_fops = -1;
>>     __u64 out__bpf_link_fops1 = -1;
>>     __u64 out__btf_size = -1;
>>     __u64 out__per_cpu_start = -1;
>>    +__u64 out__fake_dst_ops = -1;
>>
>>     extern const void bpf_link_fops __ksym;
>>     extern const void __start_BTF __ksym;
>>     extern const void __stop_BTF __ksym;
>>     extern const void __per_cpu_start __ksym;
>>    +extern const void fake_dst_ops __ksym;
>>     /* non-existing symbol, weak, default to zero */
>>     extern const void bpf_link_fops1 __ksym __weak;
>>
>>    @@ -23,6 +25,7 @@ int handler(const void *ctx)
>>            out__bpf_link_fops = (__u64)&bpf_link_fops;
>>            out__btf_size = (__u64)(&__stop_BTF - &__start_BTF);
>>            out__per_cpu_start = (__u64)&__per_cpu_start;
>>    +       out__fake_dst_ops = (__u64)&fake_dst_ops;
>>
>>            out__bpf_link_fops1 = (__u64)&bpf_link_fops1;
>>
>> This patch fixed the issue in libbpf such that if clang lto kernel
>> is enabled and the symbol resolution is for ksym's,
>> the suffix '.llvm.<hash>' will be ignored during comparison of
>> bpf prog ksym vs. symbols in /proc/kallsyms, this resolved the issue.
>>
>> Note that currently kernel does not support gcc build with lto.
>>
>>    [1] https://lore.kernel.org/bpf/20240302165017.1627295-1-yonghong.song@linux.dev/
>>    [2] https://github.com/llvm/llvm-project/blob/release/18.x/llvm/include/llvm/IR/ModuleSummaryIndex.h#L1714-L1719
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c | 64 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index a7a89269148c..8c3861192bc8 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -664,6 +664,7 @@ struct bpf_object {
>>          bool loaded;
>>          bool has_subcalls;
>>          bool has_rodata;
>> +       bool need_kallsyms;
>>
>>          struct bpf_gen *gen_loader;
>>
>> @@ -8016,14 +8017,73 @@ static int libbpf_kallsyms_parse(kallsyms_cb_t cb, void *ctx)
>>          return err;
>>   }
>>
>> +static 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_CLANG=y", 18)) {
>> +                       check_lto = 1;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       gzclose(file);
>> +out:
>> +       return check_lto;
>> +}
>> +
>>   static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>>                         const char *sym_name, void *ctx)
>>   {
>> +       int lto_enabled = check_lto_kernel();
>> +       char orig_name[PATH_MAX], *res;
>>          struct bpf_object *obj = ctx;
>>          const struct btf_type *t;
>>          struct extern_desc *ext;
>>
>> -       ext = find_extern_by_name(obj, sym_name);
>> +       /* Only check static variables in data sections */
>> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
>> +               strcpy(orig_name, sym_name);
>> +               res = strstr(orig_name, ".llvm.");
>> +               if (res) {
>> +                       *res = '\0';
>> +                       pr_debug("extern (ksym) '%s': use original name '%s' for comparison\n",
>> +                                sym_name, orig_name);
>> +               }
>> +               ext = find_extern_by_name(obj, orig_name);
>> +       } else {
>> +               ext = find_extern_by_name(obj, sym_name);
>> +       }
>>          if (!ext || ext->type != EXT_KSYM)
>>                  return 0;
>>
>> @@ -8322,7 +8382,9 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
>>                          return -EINVAL;
>>          }
>>          if (need_kallsyms) {
>> +               obj->need_kallsyms = true;
>>                  err = bpf_object__read_kallsyms_file(obj);
>> +               obj->need_kallsyms = false;
> I'm not clear on why you need this obj->need_kallsyms? kallsyms_cb is
> used for just this use case, it seems, it should be fine without this
> extra temporary flag.

yes, I realized later so I responded to Alexei's comment that it is okay
to remove it. Originally I thought bpf_object__read_kallsyms_file()
might be an API function hence the above obj->need_kallsyms.
Apparently, it is not.

>
> Ideally we should also switch to the iterator approach for kallsyms,
> just like we did with elf symbols iterator (see elf_sym_iter in
> elf.c), it would be a cleaner approach. Let me know if you are
> interested in doing this as well (it's not mandatory for the changes
> in this patch set, just to be clear).

I will probably finish the core functionality first. I think this
can be a follow-up.

>
>>                  if (err)
>>                          return -EINVAL;
>>          }
>> --
>> 2.43.0
>>

[PATCH bpf-next v2 4/5] selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO kernel

[Yonghong Song]

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
  #117/1   kprobe_multi_bench_attach/kernel:FAIL

There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
are renamed in /proc/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.

The kernel bpf_kprobe_multi_link_attach() and ftrace_lookup_symbols() try
to find addresses based on /proc/kallsyms, hence the current test failed
with LTO kenrel.

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:
  #117/1   kprobe_multi_bench_attach/kernel:OK

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/prog_tests/kprobe_multi_test.c     | 12 ++++++++++++
 1 file changed, 12 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..f6130f4f3d88 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -345,6 +345,9 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
 	FILE *f;
 	int err = 0;
 
+	if (!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 +396,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
 		if (!strncmp(name, "__ftrace_invalid_address__",
 			     sizeof("__ftrace_invalid_address__") - 1))
 			continue;
+		/*
+		 * In certain cases, e.g., clang lto kernel, the 'name' here
+		 * may be different from the one in /proc/kallsyms due to
+		 * /proc/kallsyms name might be "<name>.llvm.<hash>" instead
+		 * of "<name>". Exclude these 'name's since they will cause
+		 * later kprobe_multi_attach failure.
+		 */
+		if (ksym_get_addr(name) == 0)
+			continue;
 
 		err = hashmap__add(map, name, 0);
 		if (err == -EEXIST) {
-- 
2.43.0

Re: [PATCH bpf-next v2 4/5] selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO kernel

[Jiri Olsa]

On Thu, Mar 21, 2024 at 01:01:19PM -0700, Yonghong Song wrote:
> 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
>   #117/1   kprobe_multi_bench_attach/kernel:FAIL
> 
> There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
> are renamed in /proc/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.
> 
> The kernel bpf_kprobe_multi_link_attach() and ftrace_lookup_symbols() try
> to find addresses based on /proc/kallsyms, hence the current test failed
> with LTO kenrel.
> 
> 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:
>   #117/1   kprobe_multi_bench_attach/kernel:OK
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  .../selftests/bpf/prog_tests/kprobe_multi_test.c     | 12 ++++++++++++
>  1 file changed, 12 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..f6130f4f3d88 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> @@ -345,6 +345,9 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>  	FILE *f;
>  	int err = 0;
>  
> +	if (!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 +396,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>  		if (!strncmp(name, "__ftrace_invalid_address__",
>  			     sizeof("__ftrace_invalid_address__") - 1))
>  			continue;
> +		/*
> +		 * In certain cases, e.g., clang lto kernel, the 'name' here
> +		 * may be different from the one in /proc/kallsyms due to
> +		 * /proc/kallsyms name might be "<name>.llvm.<hash>" instead
> +		 * of "<name>". Exclude these 'name's since they will cause
> +		 * later kprobe_multi_attach failure.
> +		 */
> +		if (ksym_get_addr(name) == 0)
> +			continue;

curious how many symbols like that are there?

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

>  
>  		err = hashmap__add(map, name, 0);
>  		if (err == -EEXIST) {
> -- 
> 2.43.0
> 
> 

Re: [PATCH bpf-next v2 4/5] selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO kernel

[Yonghong Song]

On 3/22/24 5:37 AM, Jiri Olsa wrote:
> On Thu, Mar 21, 2024 at 01:01:19PM -0700, Yonghong Song wrote:
>> 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
>>    #117/1   kprobe_multi_bench_attach/kernel:FAIL
>>
>> There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
>> are renamed in /proc/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.
>>
>> The kernel bpf_kprobe_multi_link_attach() and ftrace_lookup_symbols() try
>> to find addresses based on /proc/kallsyms, hence the current test failed
>> with LTO kenrel.
>>
>> 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:
>>    #117/1   kprobe_multi_bench_attach/kernel:OK
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   .../selftests/bpf/prog_tests/kprobe_multi_test.c     | 12 ++++++++++++
>>   1 file changed, 12 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..f6130f4f3d88 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>> @@ -345,6 +345,9 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>>   	FILE *f;
>>   	int err = 0;
>>   
>> +	if (!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 +396,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>>   		if (!strncmp(name, "__ftrace_invalid_address__",
>>   			     sizeof("__ftrace_invalid_address__") - 1))
>>   			continue;
>> +		/*
>> +		 * In certain cases, e.g., clang lto kernel, the 'name' here
>> +		 * may be different from the one in /proc/kallsyms due to
>> +		 * /proc/kallsyms name might be "<name>.llvm.<hash>" instead
>> +		 * of "<name>". Exclude these 'name's since they will cause
>> +		 * later kprobe_multi_attach failure.
>> +		 */
>> +		if (ksym_get_addr(name) == 0)
>> +			continue;
> curious how many symbols like that are there?

The number of entries in /sys/kernel/tracing/available_filter_functions: 50654
After existing filtering ('arch_cpu_idle') etc: 50513 (filtering 141)
After above ksym_get_addr(name) check: 49437 (further filtering 1076)

>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> jirka
>
>>   
>>   		err = hashmap__add(map, name, 0);
>>   		if (err == -EEXIST) {
>> -- 
>> 2.43.0
>>
>>

Re: [PATCH bpf-next v2 4/5] selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO kernel

[Andrii Nakryiko]

On Fri, Mar 22, 2024 at 9:01 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/22/24 5:37 AM, Jiri Olsa wrote:
> > On Thu, Mar 21, 2024 at 01:01:19PM -0700, Yonghong Song wrote:
> >> 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
> >>    #117/1   kprobe_multi_bench_attach/kernel:FAIL
> >>
> >> There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
> >> are renamed in /proc/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.
> >>
> >> The kernel bpf_kprobe_multi_link_attach() and ftrace_lookup_symbols() try
> >> to find addresses based on /proc/kallsyms, hence the current test failed
> >> with LTO kenrel.
> >>
> >> 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:
> >>    #117/1   kprobe_multi_bench_attach/kernel:OK
> >>
> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >> ---
> >>   .../selftests/bpf/prog_tests/kprobe_multi_test.c     | 12 ++++++++++++
> >>   1 file changed, 12 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..f6130f4f3d88 100644
> >> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
> >> @@ -345,6 +345,9 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
> >>      FILE *f;
> >>      int err = 0;
> >>
> >> +    if (!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 +396,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
> >>              if (!strncmp(name, "__ftrace_invalid_address__",
> >>                           sizeof("__ftrace_invalid_address__") - 1))
> >>                      continue;
> >> +            /*
> >> +             * In certain cases, e.g., clang lto kernel, the 'name' here
> >> +             * may be different from the one in /proc/kallsyms due to
> >> +             * /proc/kallsyms name might be "<name>.llvm.<hash>" instead
> >> +             * of "<name>". Exclude these 'name's since they will cause
> >> +             * later kprobe_multi_attach failure.
> >> +             */
> >> +            if (ksym_get_addr(name) == 0)
> >> +                    continue;
> > curious how many symbols like that are there?
>
> The number of entries in /sys/kernel/tracing/available_filter_functions: 50654
> After existing filtering ('arch_cpu_idle') etc: 50513 (filtering 141)
> After above ksym_get_addr(name) check: 49437 (further filtering 1076)
>

alternatively, you could have found matching func.llvm.* for any func
in available_filter_functions. Have you considered that?

> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> >
> > jirka
> >
> >>
> >>              err = hashmap__add(map, name, 0);
> >>              if (err == -EEXIST) {
> >> --
> >> 2.43.0
> >>
> >>

Re: [PATCH bpf-next v2 4/5] selftests/bpf: Fix kprobe_multi_bench_attach test failure with LTO kernel

[Yonghong Song]

On 3/22/24 2:53 PM, Andrii Nakryiko wrote:
> On Fri, Mar 22, 2024 at 9:01 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/22/24 5:37 AM, Jiri Olsa wrote:
>>> On Thu, Mar 21, 2024 at 01:01:19PM -0700, Yonghong Song wrote:
>>>> 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
>>>>     #117/1   kprobe_multi_bench_attach/kernel:FAIL
>>>>
>>>> There are multiple symbols in /sys/kernel/debug/tracing/available_filter_functions
>>>> are renamed in /proc/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.
>>>>
>>>> The kernel bpf_kprobe_multi_link_attach() and ftrace_lookup_symbols() try
>>>> to find addresses based on /proc/kallsyms, hence the current test failed
>>>> with LTO kenrel.
>>>>
>>>> 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:
>>>>     #117/1   kprobe_multi_bench_attach/kernel:OK
>>>>
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>>    .../selftests/bpf/prog_tests/kprobe_multi_test.c     | 12 ++++++++++++
>>>>    1 file changed, 12 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..f6130f4f3d88 100644
>>>> --- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>>>> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
>>>> @@ -345,6 +345,9 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>>>>       FILE *f;
>>>>       int err = 0;
>>>>
>>>> +    if (!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 +396,15 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
>>>>               if (!strncmp(name, "__ftrace_invalid_address__",
>>>>                            sizeof("__ftrace_invalid_address__") - 1))
>>>>                       continue;
>>>> +            /*
>>>> +             * In certain cases, e.g., clang lto kernel, the 'name' here
>>>> +             * may be different from the one in /proc/kallsyms due to
>>>> +             * /proc/kallsyms name might be "<name>.llvm.<hash>" instead
>>>> +             * of "<name>". Exclude these 'name's since they will cause
>>>> +             * later kprobe_multi_attach failure.
>>>> +             */
>>>> +            if (ksym_get_addr(name) == 0)
>>>> +                    continue;
>>> curious how many symbols like that are there?
>> The number of entries in /sys/kernel/tracing/available_filter_functions: 50654
>> After existing filtering ('arch_cpu_idle') etc: 50513 (filtering 141)
>> After above ksym_get_addr(name) check: 49437 (further filtering 1076)
>>
> alternatively, you could have found matching func.llvm.* for any func
> in available_filter_functions. Have you considered that?

Looks like you prefer not skipping those functions who have .llvm.* in
/proc/kallsyms in this patch set. Yes, I can do that.

>
>>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>> jirka
>>>
>>>>               err = hashmap__add(map, name, 0);
>>>>               if (err == -EEXIST) {
>>>> --
>>>> 2.43.0
>>>>
>>>>

[PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Yonghong Song]

The current kprobe_multi_bench_attach/kernel test
reads sym names from /sys/kernel/tracing/available_filter_functions.
Some names do not agree with the corresponding entries in /proc/kallsyms
since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
Actually, if we pass symbol names in /proc/kallsyms,
kprobe_multi_attach will be okay.

This patch added a new subtest where addresses are retrieved from
/sys/kernel/tracing/available_filter_functions_addrs, and use the
address to consule /proc/kallsyms to get the function name.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/kprobe_multi_test.c        | 139 ++++++++++++++++++
 1 file changed, 139 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 f6130f4f3d88..142309ced146 100644
--- a/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c
@@ -438,6 +438,102 @@ static int get_syms(char ***symsp, size_t *cntp, bool kernel)
 	return err;
 }
 
+static int get_syms_from_addr(char ***symsp, size_t *cntp)
+{
+	char *name = NULL, **syms = NULL;
+	size_t cap = 0, cnt = 0, i;
+	struct hashmap *map;
+	struct ksym *ks;
+	char buf[256];
+	FILE *f;
+	int err = 0;
+	void *addr;
+
+	if (!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
+	 * interface.
+	 * Filtering out duplicates by using hashmap__add, which won't
+	 * add existing entry.
+	 */
+
+	if (access("/sys/kernel/tracing/trace", F_OK) == 0)
+		f = fopen("/sys/kernel/tracing/available_filter_functions_addrs", "r");
+	else
+		f = fopen("/sys/kernel/debug/tracing/available_filter_functions_addrs", "r");
+
+	if (!f)
+		return -ENOENT;
+
+	map = hashmap__new(symbol_hash, symbol_equal, NULL);
+	if (IS_ERR(map)) {
+		err = libbpf_get_error(map);
+		goto error;
+	}
+
+	while (fgets(buf, sizeof(buf), f)) {
+		if (strchr(buf, '['))
+			continue;
+
+		if (sscanf(buf, "%p$*[^\n]\n", &addr) != 1)
+			continue;
+
+		ks = ksym_search((long)addr);
+		if (!ks)
+			return -EINVAL;
+
+		name = ks->name;
+
+		/*
+		 * We attach to almost all kernel functions and some of them
+		 * will cause 'suspicious RCU usage' when fprobe is attached
+		 * to them. Filter out the current culprits - arch_cpu_idle
+		 * default_idle and rcu_* functions.
+		 */
+		if (!strcmp(name, "arch_cpu_idle"))
+			continue;
+		if (!strcmp(name, "default_idle"))
+			continue;
+		if (!strncmp(name, "rcu_", 4))
+			continue;
+		if (!strcmp(name, "bpf_dispatcher_xdp_func"))
+			continue;
+		if (!strncmp(name, "__ftrace_invalid_address__",
+			     sizeof("__ftrace_invalid_address__") - 1))
+			continue;
+
+		err = hashmap__add(map, name, 0);
+		if (err == -EEXIST) {
+			err = 0;
+			continue;
+		}
+		if (err)
+			goto error;
+
+		err = libbpf_ensure_mem((void **) &syms, &cap,
+					sizeof(*syms), cnt + 1);
+		if (err)
+			goto error;
+
+		syms[cnt++] = name;
+	}
+
+	*symsp = syms;
+	*cntp = cnt;
+
+error:
+	fclose(f);
+	hashmap__free(map);
+	if (err) {
+		for (i = 0; i < cnt; i++)
+			free(syms[i]);
+		free(syms);
+	}
+	return err;
+}
+
 static void test_kprobe_multi_bench_attach(bool kernel)
 {
 	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
@@ -521,6 +617,47 @@ static void test_attach_override(void)
 	kprobe_multi_override__destroy(skel);
 }
 
+static void test_attach_kernel_addrs_to_sym(void)
+{
+	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
+	struct kprobe_multi_empty *skel;
+	struct bpf_link *link;
+	char **syms = NULL;
+	size_t cnt = 0;
+	int i, err;
+
+	err = get_syms_from_addr(&syms, &cnt);
+	if (err == -ENOENT) {
+		test__skip();
+		return;
+	}
+	if (!ASSERT_OK(err, "get_syms_from_addr"))
+		return;
+
+	skel = kprobe_multi_empty__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
+		goto cleanup;
+
+	opts.syms = (const char **) syms;
+	opts.cnt = cnt;
+
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_empty,
+						     NULL, &opts);
+
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
+		goto cleanup;
+
+	bpf_link__destroy(link);
+
+cleanup:
+	kprobe_multi_empty__destroy(skel);
+	if (syms) {
+		for (i = 0; i < cnt; i++)
+			free(syms[i]);
+		free(syms);
+	}
+}
+
 void serial_test_kprobe_multi_bench_attach(void)
 {
 	if (test__start_subtest("kernel"))
@@ -550,4 +687,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_fails();
 	if (test__start_subtest("attach_override"))
 		test_attach_override();
+	if (test__start_subtest("kernel_addrs_to_sym"))
+		test_attach_kernel_addrs_to_sym();
 }
-- 
2.43.0

Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Jiri Olsa]

On Thu, Mar 21, 2024 at 01:01:24PM -0700, Yonghong Song wrote:
> The current kprobe_multi_bench_attach/kernel test
> reads sym names from /sys/kernel/tracing/available_filter_functions.
> Some names do not agree with the corresponding entries in /proc/kallsyms
> since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
> Actually, if we pass symbol names in /proc/kallsyms,
> kprobe_multi_attach will be okay.
> 
> This patch added a new subtest where addresses are retrieved from
> /sys/kernel/tracing/available_filter_functions_addrs, and use the
> address to consule /proc/kallsyms to get the function name.

hm, I don't understand the reason for this test.. AFAICS test_kprobe_multi_bench_attach
is doing that already, just reading available_filter_functions file

both available_filter_functions_addrs and available_filter_functions have the
same functions, there's just extra addresses in available_filter_functions_addrs

> +	*symsp = syms;
> +	*cntp = cnt;
> +
> +error:
> +	fclose(f);
> +	hashmap__free(map);
> +	if (err) {
> +		for (i = 0; i < cnt; i++)
> +			free(syms[i]);
> +		free(syms);
> +	}
> +	return err;
> +}
> +
>  static void test_kprobe_multi_bench_attach(bool kernel)
>  {
>  	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> @@ -521,6 +617,47 @@ static void test_attach_override(void)
>  	kprobe_multi_override__destroy(skel);
>  }

there's lot of duplicated code in both
  get_syms_from_addr/get_syms
  test_attach_kernel_addrs_to_sym/test_kprobe_multi_bench_attach

would be great to put it together

>  
> +static void test_attach_kernel_addrs_to_sym(void)
> +{
> +	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
> +	struct kprobe_multi_empty *skel;
> +	struct bpf_link *link;
> +	char **syms = NULL;
> +	size_t cnt = 0;
> +	int i, err;
> +
> +	err = get_syms_from_addr(&syms, &cnt);
> +	if (err == -ENOENT) {
> +		test__skip();
> +		return;
> +	}
> +	if (!ASSERT_OK(err, "get_syms_from_addr"))
> +		return;
> +
> +	skel = kprobe_multi_empty__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
> +		goto cleanup;
> +
> +	opts.syms = (const char **) syms;
> +	opts.cnt = cnt;
> +
> +	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_empty,
> +						     NULL, &opts);
> +
> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
> +		goto cleanup;
> +
> +	bpf_link__destroy(link);
> +
> +cleanup:
> +	kprobe_multi_empty__destroy(skel);
> +	if (syms) {
> +		for (i = 0; i < cnt; i++)
> +			free(syms[i]);
> +		free(syms);
> +	}
> +}
> +
>  void serial_test_kprobe_multi_bench_attach(void)
>  {
>  	if (test__start_subtest("kernel"))
> @@ -550,4 +687,6 @@ void test_kprobe_multi_test(void)
>  		test_attach_api_fails();
>  	if (test__start_subtest("attach_override"))
>  		test_attach_override();
> +	if (test__start_subtest("kernel_addrs_to_sym"))
> +		test_attach_kernel_addrs_to_sym();

we moved the bench subtests to serial_test_kprobe_multi_bench_attach,
not to clash with others in parallel mode

jirka

Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Yonghong Song]

On 3/22/24 5:26 AM, Jiri Olsa wrote:
> On Thu, Mar 21, 2024 at 01:01:24PM -0700, Yonghong Song wrote:
>> The current kprobe_multi_bench_attach/kernel test
>> reads sym names from /sys/kernel/tracing/available_filter_functions.
>> Some names do not agree with the corresponding entries in /proc/kallsyms
>> since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
>> Actually, if we pass symbol names in /proc/kallsyms,
>> kprobe_multi_attach will be okay.
>>
>> This patch added a new subtest where addresses are retrieved from
>> /sys/kernel/tracing/available_filter_functions_addrs, and use the
>> address to consule /proc/kallsyms to get the function name.
> hm, I don't understand the reason for this test.. AFAICS test_kprobe_multi_bench_attach
> is doing that already, just reading available_filter_functions file
>
> both available_filter_functions_addrs and available_filter_functions have the
> same functions, there's just extra addresses in available_filter_functions_addrs

The goal is to include those kernel functions filtered in patch 4.
But we cannot use the names from available_filter_functions[_addrs],
and we need to get names from /proc/kallsyms. Hence this patch.
This will test if we give names (<name>.llvm.<hash>) to kernel
for kprobe_multi_attach, things will be okay.

>
>> +	*symsp = syms;
>> +	*cntp = cnt;
>> +
>> +error:
>> +	fclose(f);
>> +	hashmap__free(map);
>> +	if (err) {
>> +		for (i = 0; i < cnt; i++)
>> +			free(syms[i]);
>> +		free(syms);
>> +	}
>> +	return err;
>> +}
>> +
>>   static void test_kprobe_multi_bench_attach(bool kernel)
>>   {
>>   	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
>> @@ -521,6 +617,47 @@ static void test_attach_override(void)
>>   	kprobe_multi_override__destroy(skel);
>>   }
> there's lot of duplicated code in both
>    get_syms_from_addr/get_syms
>    test_attach_kernel_addrs_to_sym/test_kprobe_multi_bench_attach
>
> would be great to put it together

I will give a try in the next revision.

>
>>   
>> +static void test_attach_kernel_addrs_to_sym(void)
>> +{
>> +	LIBBPF_OPTS(bpf_kprobe_multi_opts, opts);
>> +	struct kprobe_multi_empty *skel;
>> +	struct bpf_link *link;
>> +	char **syms = NULL;
>> +	size_t cnt = 0;
>> +	int i, err;
>> +
>> +	err = get_syms_from_addr(&syms, &cnt);
>> +	if (err == -ENOENT) {
>> +		test__skip();
>> +		return;
>> +	}
>> +	if (!ASSERT_OK(err, "get_syms_from_addr"))
>> +		return;
>> +
>> +	skel = kprobe_multi_empty__open_and_load();
>> +	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
>> +		goto cleanup;
>> +
>> +	opts.syms = (const char **) syms;
>> +	opts.cnt = cnt;
>> +
>> +	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_kprobe_empty,
>> +						     NULL, &opts);
>> +
>> +	if (!ASSERT_OK_PTR(link, "bpf_program__attach_kprobe_multi_opts"))
>> +		goto cleanup;
>> +
>> +	bpf_link__destroy(link);
>> +
>> +cleanup:
>> +	kprobe_multi_empty__destroy(skel);
>> +	if (syms) {
>> +		for (i = 0; i < cnt; i++)
>> +			free(syms[i]);
>> +		free(syms);
>> +	}
>> +}
>> +
>>   void serial_test_kprobe_multi_bench_attach(void)
>>   {
>>   	if (test__start_subtest("kernel"))
>> @@ -550,4 +687,6 @@ void test_kprobe_multi_test(void)
>>   		test_attach_api_fails();
>>   	if (test__start_subtest("attach_override"))
>>   		test_attach_override();
>> +	if (test__start_subtest("kernel_addrs_to_sym"))
>> +		test_attach_kernel_addrs_to_sym();
> we moved the bench subtests to serial_test_kprobe_multi_bench_attach,
> not to clash with others in parallel mode

Okay, I will put it in serial mode in next revision.

>
> jirka

Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Andrii Nakryiko]

On Fri, Mar 22, 2024 at 9:07 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/22/24 5:26 AM, Jiri Olsa wrote:
> > On Thu, Mar 21, 2024 at 01:01:24PM -0700, Yonghong Song wrote:
> >> The current kprobe_multi_bench_attach/kernel test
> >> reads sym names from /sys/kernel/tracing/available_filter_functions.
> >> Some names do not agree with the corresponding entries in /proc/kallsyms
> >> since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
> >> Actually, if we pass symbol names in /proc/kallsyms,
> >> kprobe_multi_attach will be okay.
> >>
> >> This patch added a new subtest where addresses are retrieved from
> >> /sys/kernel/tracing/available_filter_functions_addrs, and use the
> >> address to consule /proc/kallsyms to get the function name.
> > hm, I don't understand the reason for this test.. AFAICS test_kprobe_multi_bench_attach
> > is doing that already, just reading available_filter_functions file
> >
> > both available_filter_functions_addrs and available_filter_functions have the
> > same functions, there's just extra addresses in available_filter_functions_addrs
>
> The goal is to include those kernel functions filtered in patch 4.
> But we cannot use the names from available_filter_functions[_addrs],
> and we need to get names from /proc/kallsyms. Hence this patch.
> This will test if we give names (<name>.llvm.<hash>) to kernel
> for kprobe_multi_attach, things will be okay.
>

for patch #4 it would be good to not skip those *.llvm.* functions,
but find the full name using kallsyms. While here we can use address
based multi-attachment, while getting addresses from
available_filter_functions_addrs? That way we'll have a test that
benchmarks both symbol lookup paths in the kernel (where user provides
symbol names as strings) and address-based lookup (where user provides
raw addresses).

> >
> >> +    *symsp = syms;
> >> +    *cntp = cnt;
> >> +
> >> +error:
> >> +    fclose(f);
> >> +    hashmap__free(map);
> >> +    if (err) {
> >> +            for (i = 0; i < cnt; i++)
> >> +                    free(syms[i]);
> >> +            free(syms);
> >> +    }
> >> +    return err;
> >> +}
> >> +

[...]

Re: [PATCH bpf-next v2 5/5] selftests/bpf: Add a selftest with available_filter_functions_addrs

[Yonghong Song]

On 3/22/24 2:58 PM, Andrii Nakryiko wrote:
> On Fri, Mar 22, 2024 at 9:07 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/22/24 5:26 AM, Jiri Olsa wrote:
>>> On Thu, Mar 21, 2024 at 01:01:24PM -0700, Yonghong Song wrote:
>>>> The current kprobe_multi_bench_attach/kernel test
>>>> reads sym names from /sys/kernel/tracing/available_filter_functions.
>>>> Some names do not agree with the corresponding entries in /proc/kallsyms
>>>> since the corresponding /proc/kallsyms syms have suffix '.llvm.<hash>'.
>>>> Actually, if we pass symbol names in /proc/kallsyms,
>>>> kprobe_multi_attach will be okay.
>>>>
>>>> This patch added a new subtest where addresses are retrieved from
>>>> /sys/kernel/tracing/available_filter_functions_addrs, and use the
>>>> address to consule /proc/kallsyms to get the function name.
>>> hm, I don't understand the reason for this test.. AFAICS test_kprobe_multi_bench_attach
>>> is doing that already, just reading available_filter_functions file
>>>
>>> both available_filter_functions_addrs and available_filter_functions have the
>>> same functions, there's just extra addresses in available_filter_functions_addrs
>> The goal is to include those kernel functions filtered in patch 4.
>> But we cannot use the names from available_filter_functions[_addrs],
>> and we need to get names from /proc/kallsyms. Hence this patch.
>> This will test if we give names (<name>.llvm.<hash>) to kernel
>> for kprobe_multi_attach, things will be okay.
>>
> for patch #4 it would be good to not skip those *.llvm.* functions,
> but find the full name using kallsyms. While here we can use address

Okay, I will change patch #4 to lookup /proc/kallsyms to find the
full name. If we did that, then current patch #5 is not needed.

> based multi-attachment, while getting addresses from
> available_filter_functions_addrs? That way we'll have a test that
> benchmarks both symbol lookup paths in the kernel (where user provides
> symbol names as strings) and address-based lookup (where user provides
> raw addresses).

yes, we can use available_filter_functions_addrs
to have addrs available to kernel to do multi-attach.
will do. This will be yet another bench test.

>
>>>> +    *symsp = syms;
>>>> +    *cntp = cnt;
>>>> +
>>>> +error:
>>>> +    fclose(f);
>>>> +    hashmap__free(map);
>>>> +    if (err) {
>>>> +            for (i = 0; i < cnt; i++)
>>>> +                    free(syms[i]);
>>>> +            free(syms);
>>>> +    }
>>>> +    return err;
>>>> +}
>>>> +
> [...]