bpftool does not print full names with LLVM 17 and newer

bpftool does not print full names with LLVM 17 and newer

[Ivan Babrou]

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.

Thanks.

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

[Alexei Starovoitov]

On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@cloudflare.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,
I agree, llvm should be warning about such footgun,
but the way ptr_to_u64() is written is probably silencing it.
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.

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

[Yonghong Song]

On 5/17/24 5:33 PM, Alexei Starovoitov wrote:
> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@cloudflare.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.

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.

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

[Alexei Starovoitov]

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

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@linux.dev> wrote:
>>
>> On 5/17/24 5:33 PM, Alexei Starovoitov wrote:
>>> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@cloudflare.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.

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

[Ivan Babrou]

On Fri, May 17, 2024 at 4:33 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@cloudflare.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,
> I agree, llvm should be warning about such footgun,
> but the way ptr_to_u64() is written is probably silencing it.
> 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.

In bpftool there's just two call sites that are unhappy if I remove
"const" in the arguments:

* this problematic one
* "GPL" literal passed

I'll send the patch to drop "const" from the struct initialization
today or tomorrow (it works great in our internal build), but I'll
leave the bigger change to you. There seem to be many places in libbpf
and I'm far from being a C expert to drive this change.

I managed to bisect clang to find the commit that introduced the change:

* https://github.com/llvm/llvm-project/commit/0b2d5b967d98

I also mentioned the commit author and they have some ideas about
UBSAN catching this (it doesn't in the current state):

* https://mastodon.ivan.computer/@mastodon/112465898861074834

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

[Yonghong Song]

On 5/20/24 12:44 PM, Ivan Babrou wrote:
> On Fri, May 17, 2024 at 4:33 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Fri, May 17, 2024 at 2:51 PM Ivan Babrou <ivan@cloudflare.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,
>> I agree, llvm should be warning about such footgun,
>> but the way ptr_to_u64() is written is probably silencing it.
>> 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.
> In bpftool there's just two call sites that are unhappy if I remove
> "const" in the arguments:
>
> * this problematic one
> * "GPL" literal passed
>
> I'll send the patch to drop "const" from the struct initialization

Yes, this should work as we discussed earlier. Thanks!

> today or tomorrow (it works great in our internal build), but I'll
> leave the bigger change to you. There seem to be many places in libbpf
> and I'm far from being a C expert to drive this change.

As discussed in below link. It is not easy for compiler to deduce whether
an undefined behavior is triggered or not. The additional
const_ptr_to_u64() serves the purpose to force patch author and reviewer
to double check whether the 'u64' value may eventually invalidate
'const' property or not.

>
> I managed to bisect clang to find the commit that introduced the change:
>
> * https://github.com/llvm/llvm-project/commit/0b2d5b967d98
>
> I also mentioned the commit author and they have some ideas about
> UBSAN catching this (it doesn't in the current state):
>
> * https://mastodon.ivan.computer/@mastodon/112465898861074834
>