| Message ID | 20240321200114.2219721-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix a couple of test failures with LTO kernel | expand |
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 >
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 >>
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.
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.
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 >
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.
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 >>
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 >
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 >>
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; }
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(-)