On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song <yonghong.song@xxxxxxxxx> 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@xxxxxxxxx/ > [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@xxxxxxxxx> > --- > 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 >
