Re: bpftool does not print full names with LLVM 17 and newer

[Yonghong Song]

On 5/20/24 11:21 AM, Alexei Starovoitov wrote:
> On Mon, May 20, 2024 at 10:01 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > On 5/17/24 5:33 PM, Alexei Starovoitov wrote:
> > > On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@xxxxxxxxxxxxxx> 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.