From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-177.mta0.migadu.com (out-177.mta0.migadu.com [91.218.175.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C82F780059 for ; Fri, 22 Mar 2024 22:09:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711145390; cv=none; b=U9LSlbX11T5+1FGcKLbkJCEbq7CZXAXZ4Zsi2M320vrxNcLc2N/no3Pbz7oIjwY35uk8q/WEezx09vBXNHZbAkYXaulYXx5IOHzxr5Qv0VQYxyrHBCiPtzPjdJDbO6lefOxDvSQU/UBf5h5bzFwCp2Dsy0izeyB8DSo7fox9vlk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711145390; c=relaxed/simple; bh=TszkgPCUWlvZix+dB8/7y7tTekGK5ZwURVRpnpxzJvY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=aGJF3tjOVTEg9zr3qaApKWSoLcuOnVUFjYi3GG80Or8lcaPYx6zFe0aSCtXX/HRZfb5KDjmJpL56WwhtnEsNlBkGxplex3coEP1JjJfE/IG4oE6UfHqJmUuFgnjOQEpnLVbCB/pQDG39/IEE7BqK5ydfQfAXtT3FZ9VM6Sr8sSE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=AE7svXE0; arc=none smtp.client-ip=91.218.175.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="AE7svXE0" Message-ID: <54ce4078-70d4-443a-8bc4-4ffede1d0405@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1711145385; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dmO8mzs2DBT4acUi3A7ldcAQEH+XWQ1XobtGzlvBkqM=; b=AE7svXE0iPEbv8AgZb5p5kRqcgGM7sDx6PjpDGHbWOdS0PCrWzC8tlMqnebQv+huaoIuNY O0zrVU/4PrUOkbLWeAA+/HLB7PMnPX3ndpjTcZpUs5EfB1zKST+KcSqjnmwQbp2LFQB/9y tVZQ/ijxBltyCpvrWh9hEzVhHppFsuc= Date: Fri, 22 Mar 2024 15:09:37 -0700 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH bpf-next v2 3/5] libbpf: Handle .llvm. symbol properly Content-Language: en-GB To: Andrii Nakryiko Cc: bpf@vger.kernel.org, Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , kernel-team@fb.com, Martin KaFai Lau References: <20240321200058.2218328-1-yonghong.song@linux.dev> <20240321200114.2219721-1-yonghong.song@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yonghong Song In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT On 3/22/24 2:50 PM, Andrii Nakryiko wrote: > On Thu, Mar 21, 2024 at 1:01 PM Yonghong Song 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.' ([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.' >> 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.' 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 >> --- >> 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 >>