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

[Yonghong Song <yonghong.song@linux.dev>]

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