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