Re: [BUG] One-liner array initialization with two pointers in BPF results in NULLs

[Florent Revest <revest@chromium.org>]

On Wed, Mar 10, 2021 at 10:51 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Wed, Mar 10, 2021 at 12:12 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Wed, Mar 10, 2021 at 8:59 AM Yonghong Song <yhs@fb.com> wrote:
> > > On 3/10/21 3:48 AM, Florent Revest wrote:
> > > > On Wed, Mar 10, 2021 at 6:16 AM Yonghong Song <yhs@fb.com> wrote:
> > > >> On 3/9/21 7:43 PM, Yonghong Song wrote:
> > > >>> On 3/9/21 5:54 PM, Florent Revest wrote:
> > > >>>> I noticed that initializing an array of pointers using this syntax:
> > > >>>> __u64 array[] = { (__u64)&var1, (__u64)&var2 };
> > > >>>> (which is a fairly common operation with macros such as BPF_SEQ_PRINTF)
> > > >>>> always results in array[0] and array[1] being NULL.
> > > >>>>
> > > >>>> Interestingly, if the array is only initialized with one pointer, ex:
> > > >>>> __u64 array[] = { (__u64)&var1 };
> > > >>>> Then array[0] will not be NULL.
> > > >>>>
> > > >>>> Or if the array is initialized field by field, ex:
> > > >>>> __u64 array[2];
> > > >>>> array[0] = (__u64)&var1;
> > > >>>> array[1] = (__u64)&var2;
> > > >>>> Then array[0] and array[1] will not be NULL either.
> > > >>>>
> > > >>>> I'm assuming that this should have something to do with relocations
> > > >>>> and might be a bug in clang or in libbpf but because I don't know much
> > > >>>> about these, I thought that reporting could be a good first step. :)
> > > >>>
> > > >>> Thanks for reporting. What you guess is correct, this is due to
> > > >>> relocations :-(
> > > >>>
> > > >>> The compiler notoriously tend to put complex initial values into
> > > >>> rodata section. For example, for
> > > >>>      __u64 array[] = { (__u64)&var1, (__u64)&var2 };
> > > >>> the compiler will put
> > > >>>      { (__u64)&var1, (__u64)&var2 }
> > > >>> into rodata section.
> > > >>>
> > > >>> But &var1 and &var2 themselves need relocation since they are
> > > >>> address of static variables which will sit inside .data section.
> > > >>>
> > > >>> So in the elf file, you will see the following relocations:
> > > >>>
> > > >>> RELOCATION RECORDS FOR [.rodata]:
> > > >>> OFFSET           TYPE                     VALUE
> > > >>> 0000000000000018 R_BPF_64_64              .data
> > > >>> 0000000000000020 R_BPF_64_64              .data
> > > >
> > > > Right :) Thank you for the explanations Yonghong!
> > > >
> > > >>> Currently, libbpf does not handle relocation inside .rodata
> > > >>> section, so they content remains 0.
> > > >
> > > > Just for my own edification, why is .rodata relocation not yet handled
> > > > in libbpf ? Is it because of a read-only mapping that makes it more
> > > > difficult ?
> > >
> > > We don't have this use case before. In general, people do not put
> > > string pointers in init code in the declaration. I think
> > > bpf_seq_printf() is special about this and hence triggering
> > > the issue.

Fair enough, the only reasonable usecase that I can think of is a
selftest like the one I wrote for bpf_snprintf and the macro in
bpf_tracing.h will be a good enough workaround for that.

> > > To support relocation of rodata section, kernel needs to be
> > > involved and this is actually more complicated as
> >
> > Exactly. It would be trivial for libbpf to support it, but it needs to
> > resolve to the actual in-kernel address of a map (plus offset), which
> > libbpf has no way of knowing.

Ah right, I see now, thanks! Indeed this would be quite complex and
probably not very useful.

> Having said that, libbpf should probably error out when such
> relocation is present, because there is no way the application with
> such relocations is going to be correct.

Good point, it would have helped me notice the problem earlier. :)