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

[Yonghong Song]

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
>
> Currently, libbpf does not handle relocation inside .rodata
> section, so they content remains 0.
>
> That is why you see the issue with pointer as NULL.
>
> With array size of 1, compiler does not bother to put it into
> rodata section.
>
> I *guess* that it works in the macro due to some kind of heuristics,
> e.g., nested blocks, etc, and llvm did not promote the array init value
> to rodata. I will double check whether llvm can complete prevent
> such transformation.
>
> Maybe in the future libbpf is able to handle relocations for
> rodata section too. But for the time being, please just consider to use 
> either macro, or the explicit array assignment.

Digging into the compiler, the compiler tries to make *const* initial
value into rodata section if the initial value size > 64, so in
this case, macro does not work either. I think this is how you
discovered the issue. The llvm does not provide target hooks to 
influence this transformation.

So, there are two workarounds,
(1).    __u64 param_working[2];
        param_working[0] = (__u64)str1;
        param_working[1] = (__u64)str2;
(2). BPF_SEQ_PRINTF(seq, "%s ", str1);
     BPF_SEQ_PRINTF(seq, "%s", str2);

In practice, if you have at least one non-const format argument,
you should be fine. But if all format arguments are constant, then
none of them should be strings. Maybe we could change marco
   unsigned long long ___param[] = { args };
to declare an array explicitly and then have a loop to
assign each array element?

> Thanks for the reproducer!
>
> > I attached below a repro with a dummy selftest that I expect should pass
> > but fails to pass with the latest clang and bpf-next. Hopefully, the
> > logic should be simple: I try to print two strings from pointers in an
> > array using bpf_seq_printf but depending on how the array is initialized
> > the helper either receives the string pointers or NULL pointers:
> >
> > test_bug:FAIL:read unexpected read: actual 'str1= str2= str1=STR1
> > str2=STR2 ' != expected 'str1=STR1 str2=STR2 str1=STR1 str2=STR2 '
> >
> > Signed-off-by: Florent Revest <revest@xxxxxxxxxxxx>
> > ---
> >   tools/testing/selftests/bpf/prog_tests/bug.c | 41 +++++++++++++++++++
> >   tools/testing/selftests/bpf/progs/test_bug.c | 43 ++++++++++++++++++++
> >   2 files changed, 84 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/bug.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/test_bug.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bug.c 
> > b/tools/testing/selftests/bpf/prog_tests/bug.c
> > new file mode 100644
> > index 000000000000..4b0fafd936b7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/bug.c
> > @@ -0,0 +1,41 @@
> > +#include <test_progs.h>
> > +#include "test_bug.skel.h"
> > +
> > +static int duration;
> > +
> > +void test_bug(void)
> > +{
> > +    struct test_bug *skel;
> > +    struct bpf_link *link;
> > +    char buf[64] = {};
> > +    int iter_fd, len;
> > +
> > +    skel = test_bug__open_and_load();
> > +    if (CHECK(!skel, "test_bug__open_and_load",
> > +          "skeleton open_and_load failed\n"))
> > +        goto destroy;
> > +
> > +    link = bpf_program__attach_iter(skel->progs.bug, NULL);
> > +    if (CHECK(IS_ERR(link), "attach_iter", "attach_iter failed\n"))
> > +        goto destroy;
> > +
> > +    iter_fd = bpf_iter_create(bpf_link__fd(link));
> > +    if (CHECK(iter_fd < 0, "create_iter", "create_iter failed\n"))
> > +        goto free_link;
> > +
> > +    len = read(iter_fd, buf, sizeof(buf));
> > +    CHECK(len < 0, "read", "read failed: %s\n", strerror(errno));
> > +    // BUG: We expect the strings to be printed in both cases but 
> > only the
> > +    // second case works.
> > +    // actual 'str1= str2= str1=STR1 str2=STR2 '
> > +    // != expected 'str1=STR1 str2=STR2 str1=STR1 str2=STR2 '
> > +    ASSERT_STREQ(buf, "str1=STR1 str2=STR2 str1=STR1 str2=STR2 ", 
> > "read");
> > +
> > +    close(iter_fd);
> > +
> > +free_link:
> > +    bpf_link__destroy(link);
> > +destroy:
> > +    test_bug__destroy(skel);
> > +}
> > +
> > diff --git a/tools/testing/selftests/bpf/progs/test_bug.c 
> > b/tools/testing/selftests/bpf/progs/test_bug.c
> > new file mode 100644
> > index 000000000000..c41e69483785
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_bug.c
> > @@ -0,0 +1,43 @@
> > +#include "bpf_iter.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("iter/task")
> > +int bug(struct bpf_iter__task *ctx)
> > +{
> > +    struct seq_file *seq = ctx->meta->seq;
> > +
> > +    /* We want to print two strings */
> > +    static const char fmt[] = "str1=%s str2=%s ";
> > +    static char str1[] = "STR1";
> > +    static char str2[] = "STR2";
> > +
> > +    /*
> > +     * Because bpf_seq_printf takes parameters to its format 
> > specifiers in
> > +     * an array, we need to stuff pointers to str1 and str2 in a u64 
> > array.
> > +     */
> > +
> > +    /* First, we try a one-liner array initialization. Note that this is
> > +     * what the BPF_SEQ_PRINTF macro does under the hood. */
> > +    __u64 param_not_working[] = { (__u64)str1, (__u64)str2 };
> > +    /* But we also try a field by field initialization of the array. We
> > +     * would expect the arrays and the behavior to be exactly the 
> > same. */
> > +    __u64 param_working[2];
> > +    param_working[0] = (__u64)str1;
> > +    param_working[1] = (__u64)str2;
> > +
> > +    /* For convenience, only print once */
> > +    if (ctx->meta->seq_num != 0)
> > +        return 0;
> > +
> > +    /* Using the one-liner array of params, it does not print the 
> > strings */
> > +    bpf_seq_printf(seq, fmt, sizeof(fmt),
> > +               param_not_working, sizeof(param_not_working));
> > +    /* Using the field-by-field array of params, it prints the 
> > strings */
> > +    bpf_seq_printf(seq, fmt, sizeof(fmt),
> > +               param_working, sizeof(param_working));
> > +
> > +    return 0;
> > +}