bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Luigi Rizzo <lrizzo@google.com>
Cc: Luigi Rizzo <rizzo@iet.unipi.it>, bpf <bpf@vger.kernel.org>,
	Petar Penkov <ppenkov@google.com>,
	Andrii Nakryiko <andriin@fb.com>,
	Stanislav Fomichev <sdf@google.com>
Subject: Re: libbpf/bpftool inconsistent handling og .data and .bss ?
Date: Wed, 7 Oct 2020 13:40:31 -0700	[thread overview]
Message-ID: <CAEf4BzYRiF00B+4=u8r-z+RN3bVWeV_h==4f_JJJZ133PhGAog@mail.gmail.com> (raw)
In-Reply-To: <CAMOZA0JFYEYmLqAQu=km624nZfY8epPEpmqqsdUigzp+jFsymQ@mail.gmail.com>

On Wed, Oct 7, 2020 at 1:31 PM Luigi Rizzo <lrizzo@google.com> wrote:
>
> TL;DR; there seems to be a compiler bug with clang-10 and -O2
> when struct are in .data -- details below.
>
> On Wed, Oct 7, 2020 at 8:35 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Oct 7, 2020 at 9:03 AM Luigi Rizzo <rizzo@iet.unipi.it> wrote:
> > >
> > > I am experiencing some weirdness in global variables handling
> > > in bpftool and libbpf, as described below.
> ...
> > > 2. .bss overrides from userspace are not seen in bpf at runtime
> > >
> > >     In foo_bpf.c I have "int x = 0;"
> > >     In the userspace program, before foo_bpf__load(), I do
> > >        obj->bss->x = 1
> > >     but after attach, the bpf code does not see the change, ie
> > >         "if (x == 0) { .. } else { .. }"
> > >     always takes the first branch.
> > >
> > >     If I initialize "int x = 2" and then do
> > >        obj->data->x = 1
> > >     the update is seen correctly ie
> > >           "if (x == 2) { .. } else { .. }"
> > >      takes one or the other depending on whether userspace overrides
> > >      the value before foo_bpf__load()
> >
> > This is quite surprising, given we have explicit selftests validating
> > that all this works. And it seems to work. Please check
> > prog_tests/skeleton.c and progs/test_skeleton.c. Can you try running
> > it and confirm that it works in your setup?
>
> Ah, this was non intuitive but obvious in hindsight:
>
> .bss is zeroed by the kernel after load(), and since my program
> changed the value before foo_bpf__load() , the memory was overwritten
> with 0s. I could confirm this by printing the value after load.
>
> If I update obj->data-><something> after __load(),
> or even after __attach() given that userspace mmaps .bss and .data,
> everything works as expected both for scalars and structs.

Check prog_tests/skeleton.c again, it sets .data, .bss, and .rodata
before the load. And checks that those values are preserved after
load. So .bss, if you initialize it manually, shouldn't zero-out what
you set.

>
> > >
> > > 3. .data overrides do not seem to work for non-scalar types
> > >     In foo_bpf.c I have
> > >           struct one { int a; }; // type also visible to userspace
> > >           struct one x { .a = 2 }; // avoid bugs #1 and #2
> > >     If in userspace I do
> > >           obj->data->x.a = 1
> > >     the update is not seen in the kernel, ie
> > >             "if (x.a == 2) { .. } else { .. }"
> > >      always takes the first branch
> > >
> >
> > Similarly, the same skeleton selftest tests this situation. So please
> > check selftests first and report if selftests for some reason don't
> > work in your case.
>
> Actually test_skeleton.c does _not_ test for struct in .data,
> only in .rodata and .bss

It doesn't matter which section it's in, I meant it's testing struct
field accesses from at least one of global data sections.

>
> There seems to be a compiler error, at least with clang-10 and -O2
>
> Note how the struct case the compiler uses '2' as immediate value
> when reading, whereas in the scalar case it correctly dereferences
> the pointer to the variable

It would be useful to include your original source code, especially
the variable declaration parts. I suspect that you declared your
struct variable as a static variable? In that case Clang will assume
nothing can change the value and can inline values like 2. So either
make sure you have a global variable declaration or use `static
volatile`. See how `const volatile` is used throughout all selftests
when working with the .rodata section.

[...]

  reply	other threads:[~2020-10-07 20:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07 14:01 libbpf/bpftool inconsistent handling og .data and .bss ? Luigi Rizzo
2020-10-07 15:58 ` Yonghong Song
2020-10-07 18:35 ` Andrii Nakryiko
2020-10-07 20:31   ` Luigi Rizzo
2020-10-07 20:40     ` Andrii Nakryiko [this message]
2020-10-07 21:29       ` Luigi Rizzo
2020-10-07 22:26         ` Andrii Nakryiko
2020-10-08  1:33           ` Yonghong Song
2020-10-10 22:49         ` Luigi Rizzo
2020-10-10 23:11           ` Andrii Nakryiko
2020-10-11  0:31             ` Luigi Rizzo
2020-10-11  1:36               ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzYRiF00B+4=u8r-z+RN3bVWeV_h==4f_JJJZ133PhGAog@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=lrizzo@google.com \
    --cc=ppenkov@google.com \
    --cc=rizzo@iet.unipi.it \
    --cc=sdf@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).