Date: Mon, 20 May 2024 11:35:29 -0700
From: Yonghong Song <yonghong.song@...ux.dev>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Ivan Babrou <ivan@...udflare.com>, bpf <bpf@...r.kernel.org>,
 kernel-team <kernel-team@...udflare.com>,
 linux-kernel <linux-kernel@...r.kernel.org>,
 clang-built-linux <llvm@...ts.linux.dev>
Subject: Re: bpftool does not print full names with LLVM 17 and newer


On 5/20/24 11:21 AM, Alexei Starovoitov wrote:
> On Mon, May 20, 2024 at 10:01 AM Yonghong Song <yonghong.song@...ux.dev> wrote:
>>
>> On 5/17/24 5:33 PM, Alexei Starovoitov wrote:
>>> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@...udflare.com> wrote:
>>>> Hello,
>>>>
>>>> We recently bumped LLVM used for bpftool compilation from 15 to 18 and
>>>> our alerting system notified us about some unknown bpf programs. It
>>>> turns out, the names were truncated to 15 chars, whereas before they
>>>> were longer.
>>>>
>>>> After some investigation, I was able to see that the following code:
>>>>
>>>>       diff --git a/src/common.c b/src/common.c
>>>>       index 958e92a..ac38506 100644
>>>>       --- a/src/common.c
>>>>       +++ b/src/common.c
>>>>       @@ -435,7 +435,9 @@ void get_prog_full_name(const struct
>>>> bpf_prog_info *prog_info, int prog_fd,
>>>>           if (!prog_btf)
>>>>               goto copy_name;
>>>>
>>>>       +    printf("[0] finfo.type_id = %x\n", finfo.type_id);
>>>>           func_type = btf__type_by_id(prog_btf, finfo.type_id);
>>>>       +    printf("[1] finfo.type_id = %x\n", finfo.type_id);
>>>>           if (!func_type || !btf_is_func(func_type))
>>>>               goto copy_name;
>>>>
>>>> When ran under gdb, shows:
>>>>
>>>>       (gdb) b common.c:439
>>>>       Breakpoint 1 at 0x16859: file common.c, line 439.
>>>>
>>>>       (gdb) r
>>>>       3403: tracing  [0] finfo.type_id = 0
>>>>
>>>>       Breakpoint 1, get_prog_full_name (prog_info=0x7fffffffe160,
>>>> prog_fd=3, name_buff=0x7fffffffe030 "", buff_len=128) at common.c:439
>>>>       439        func_type = btf__type_by_id(prog_btf, finfo.type_id);
>>>>       (gdb) print finfo
>>>>       $1 = {insn_off = 0, type_id = 1547}
>>>>
>>>>
>>>> Notice that finfo.type_id is printed as zero, but in gdb it is in fact 1547.
>>>>
>>>> Disassembly difference looks like this:
>>>>
>>>>       -    8b 75 cc                 mov    -0x34(%rbp),%esi
>>>>       -    e8 47 8d 02 00           call   3f5b0 <btf__type_by_id>
>>>>       +    31 f6                    xor    %esi,%esi
>>>>       +    e8 a9 8c 02 00           call   3f510 <btf__type_by_id>
>>>>
>>>> This can be avoided if one removes "const" during finfo initialization:
>>>>
>>>>       const struct bpf_func_info finfo = {};
>>>>
>>>> This seems like a pretty annoying miscompilation, and hopefully
>>>> there's a way to make clang complain about this loudly, but that's
>>>> outside of my expertise. There might be other places like this that we
>>>> just haven't noticed yet.
>>>>
>>>> I can send a patch to fix this particular issue, but I'm hoping for a
>>>> more comprehensive approach from people who know better.
>>> Wow. Great catch. Please send a patch to fix bpftool and,
>> Indeed, removing 'const' modifier should allow correct code
>> generation.
>>
>>> I agree, llvm should be warning about such footgun,
>>> but the way ptr_to_u64() is written is probably silencing it.
>> Yes, ptr_to_u64() cast a 'ptr to const value' to a __u64
>> which later could be used as 'ptr to value' where the 'value'
>> could be changed.
>>
>>> We probably should drop 'const' from it:
>>> static inline __u64 ptr_to_u64(const void *ptr)
>>>
>>> and maybe add a flavor of ptr_to_u64 with extra check
>>> that the arg doesn't have a const modifier.
>>> __builtin_types_compatible_p(typeof(ptr), void *)
>>> should do the trick.
>> I guess we could introduce ptr_non_const_to_u64() like
>>
>> static inline __u64 ptr_non_const_to_u64(void *ptr)
>> {
>>           static_assert(__builtin_types_compatible_p(typeof(ptr), void *), "expect type void *");
>>           return (__u64)(unsigned long)ptr;
>> }
>>
>> and add additional check in ptr_to_u64() like
>>
>> static inline __u64 ptr_to_u64(const void *ptr)
>> {
>>          static_assert(__builtin_types_compatible_p(typeof(ptr), const void *), "expect type const void *");
>>          return (__u64)(unsigned long)ptr;
>> }
>>
>> But I am not sure how useful they are. If users declare the variable as 'const'
>> and use ptr_to_u64(), compilation will succeed but the result could be wrong.
> I mean to flip the default. Make ptr_to_u64(void *) and
> assert when 'const void *' is passed,
> and introduce const_ptr_to_u64(const void *)
> and use it in a few cases where data is indeed const.
>
> And do the same in libbpf and bpftool.

Okay, this is better. Forcing people to think about
const vs. non-const where in most cases people
will just use ptr_to_u64(void *) flavor.

>
>> Compiler could do the following analysis:
>>     (1) ptr_to_u64() argument is a constant and the result is __u64 (let us say u64_val = ptr_to_u64(...)).
>>     (2) u64_val has address taken and its content may be modified in the current function or
>>         through the function call. If this is true, compiler might warn. This will require some
>>         analysis and the warning may not be always true (esp. it requires inter-procedural analysis and
>>         in this case, bpf_prog_get_info_by_fd() eventually goes into the library/kernel so compiler has no
>>         way to know whether the value could change).
>> So I guess it will be very hard for compiler to warn for this particular case.
> indeed.

Powered by blists - more mailing lists