Date: Mon, 20 May 2024 10:21:13 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Yonghong Song <yonghong.song@...ux.dev>
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 Mon, May 20, 2024 at 10:01 AM Yonghong Song <yonghong.song@...uxdev> 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.

> 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