On Tue, Aug 2, 2022 at 6:49 PM James Hilliard <james.hilliard1@xxxxxxxxx> wrote:
>
> On Tue, Aug 2, 2022 at 6:29 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Aug 2, 2022 at 3:06 PM James Hilliard <james.hilliard1@xxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 2, 2022 at 3:29 PM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Aug 1, 2022 at 4:35 PM James Hilliard <james.hilliard1@xxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Aug 1, 2022 at 4:52 PM Andrii Nakryiko
> > > > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Sun, Jul 31, 2022 at 7:20 PM James Hilliard
> > > > > > <james.hilliard1@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Sun, Jul 10, 2022 at 2:22 PM Jose E. Marchesi
> > > > > > > <jose.marchesi@xxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > >>> On Sun, Jul 10, 2022 at 3:38 AM Jose E. Marchesi
> > > > > > > > >>> <jose.marchesi@xxxxxxxxxx> wrote:
> > > > > > > > >>>>
> > > > > > > > >>>>
> > > > > > > > >>>> > On Sat, Jul 9, 2022 at 4:41 PM Jose E. Marchesi
> > > > > > > > >>>> > <jose.marchesi@xxxxxxxxxx> wrote:
> > > > > > > > >>>> >>
> > > > > > > > >>>> >>
> > > > > > > > >>>> >> > On Sat, Jul 9, 2022 at 2:32 PM James Hilliard <james.hilliard1@xxxxxxxxx> wrote:
> > > > > > > > >>>> >> >>
> > > > > > > > >>>> >> >> On Sat, Jul 9, 2022 at 2:21 PM Jose E. Marchesi
> > > > > > > > >>>> >> >> <jose.marchesi@xxxxxxxxxx> wrote:
> > > > > > > > >>>> >> >> >
> > > > > > > > >>>> >> >> >
> > > > > >
> > > > > > Please trim your replies (and I don't know what your email client did,
> > > > > > but it completely ruined nested quote formatting)
> > > > >
> > > > > Yeah, not sure what happened there.
> > > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > >>>>
> > > > > > > > >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > > > > >>>> index e89cc9c885b3..887b78780099 100644
> > > > > > > > >>>> --- a/tools/lib/bpf/libbpf.c
> > > > > > > > >>>> +++ b/tools/lib/bpf/libbpf.c
> > > > > > > > >>>> @@ -1591,6 +1591,10 @@ static int bpf_object__init_global_data_maps(struct bpf_object *obj)
> > > > > > > > >>>> for (sec_idx = 1; sec_idx < obj->efile.sec_cnt; sec_idx++) {
> > > > > > > > >>>> sec_desc = &obj->efile.secs[sec_idx];
> > > > > > > > >>>>
> > > > > > > > >>>> + /* Skip recognized sections with size 0. */
> > > > > > > > >>>> + if (sec_desc->data && sec_desc->data->d_size == 0)
> > > > > > > > >>>> + continue;
> > > > > > > > >>>> +
> > > > > > > > >>>> switch (sec_desc->sec_type) {
> > > > > > > > >>>> case SEC_DATA:
> > > > > > > > >>>> sec_name = elf_sec_name(obj, elf_sec_by_idx(obj, sec_idx));
> > > > > > > > >>>
> > > > > > > > >>> Ok, skeleton is now getting generated successfully, however it differs from the
> > > > > > > > >>> clang version so there's a build error when we include/use the header:
> > > > > > > > >>> ../src/core/restrict-ifaces.c: In function ‘prepare_restrict_ifaces_bpf’:
> > > > > > > > >>> ../src/core/restrict-ifaces.c:45:14: error: ‘struct
> > > > > > > > >>> restrict_ifaces_bpf’ has no member named ‘rodata’; did you mean
> > > > > > > > >>> ‘data’?
> > > > > > > > >>> 45 | obj->rodata->is_allow_list = is_allow_list;
> > > > > > > > >>> | ^~~~~~
> > > > > > > > >>> | data
> > > > > > > > >>>
> > > > > > > > >>> The issue appears to be that clang generates "rodata" members in
> > > > > > > > >>> restrict_ifaces_bpf while with gcc we get "data" members instead.
> > > > > > > > >>
> > > > > > > > >> This is because the BPF GCC port is putting the
> > > > > > > > >>
> > > > > > > > >> const volatile unsigned char is_allow_list = 0;
> > > > > > > > >>
> > > > > > > > >> in a .data section instead of .rodata, due to the `volatile'. The
> > > > > > > > >> x86_64 GCC seems to use .rodata.
> > > > > > > > >>
> > > > > > > > >> Looking at why the PBF port does this...
> > > > > > > > >
> > > > > > > > > So, turns out GCC puts zero-initialized `const volatile' variables in
> > > > > > > > > .data sections (and not .rodata) in all the targets I have tried, like
> > > > > > > > > x86_64 and aarch64.
> > > > > > > > >
> > > > > > > > > So this is a LLVM and GCC divergence :/
> > > > > > > >
> > > > > > > > See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25521.
> > > > > > > >
> > > > > > > > You may try, as a workaround:
> > > > > > > >
> > > > > > > > __attribute__((section(".rodata"))) const volatile unsigned char is_allow_list = 0;
> > > > > > > >
> > > > > > > > But that will use permissions "aw" for the .rodata section (and you will
> > > > > > > > get a warning from the assembler.) It may be problematic for libbpf.
> > > > > > >
> > > > > > > So rather than try to force gcc to use the incorrect llvm .rodata
> > > > > > > section it looks
> > > > > > > like we can instead just force llvm to use the correct .data section like this:
> > > > > > > https://github.com/systemd/systemd/pull/24164
> > > > > > >
> > > > > >
> > > > > > There is a huge difference between variables in .rodata and .data.
> > > > > > .rodata variable's value is known to the BPF verifier at verification
> > > > > > time and this knowledge will be used to decide which code paths are
> > > > > > always or never taken (as one example). It's a crucial property and
> > > > > > important guarantee.
> > > > > >
> > > > > > If you don't care about that property, don't declare the variable as `const`.
> > > > > >
> > > > > > So no, it's not llvm putting `const` variable into .rodata
> > > > > > incorrectly, but GCC is trying to be smart and just because variable
> > > > > > is declared volatile is putting *const* variable into read-write .data
> > > > > > section. It's declared as const, and yes it's volatile to make sure
> > > > > > that compiler isn't too smart about optimizing away read operations.
> > > > >
> > > > > Isn't const volatile generating a .rodata section(like llvm is doing) a spec
> > > > > violation?
> > > > > https://github.com/llvm/llvm-project/issues/56468
> > > > >
> > > > > > But it's still a const read-only variable from the perspective of that
> > > > > > BPF C code.
> > > > > >
> > > > > > If you don't care about the read-only nature of that variable, drop
> > > > > > the const and make it into a non-read-only variable.
> > > > > >
> > > > > > And please stop proposing hacks to be added to perfectly valid systemd
> > > > > > BPF source code (I replied on [0] as well).
> > > > >
> > > > > From my understanding gcc is correctly putting a const volatile variable
> > > > > in .data while llvm is incorrectly putting it in .rodata, is the gcc behavior
> > > > > here invalid or is the llvm behavior invalid?
> > > >
> > > > From link you left to C standard, it does seem like that side-note
> > > > *implies* that const volatile should be put into .data, but it's a)
> > > > implied b) is quite arguable about assumptions that this data has to
> > > > be in modifiable section, and c) entire CO-RE feature detection and
> > > > guarding relies on having `const volatile` variables in .rodata and
> > > > mark them as read-only for BPF verifier to allow dead code
> > > > elimination. Changing c) would break entire CO-RE ecosystem.
> > >
> > > Well it does appear that llvm is fixing the behavior to be in line with gcc:
> > > https://reviews.llvm.org/D131012
> > >
> > > >
> > > > Seems like this issue was raised by Ulrich Drepper back in 2005 ([0])
> > > > and he was also confused about GCC's behavior, btw.
> > > >
> > > > So either way, at the very least for BPF target we can't change this
> > > > and I still think it's more logical to put const variables into
> > > > .rodata, regardless of side notes in C standard.
> > >
> > > GCC does put const variables in .rodata, just not const volatile variables.
> > >
> >
> > It's still const. Volatile doesn't change constness of a variable.
> > We've been discussing this over and over, it gets a bit tiring, tbh.
> >
> > Aaron Ballman seems to agree, quoting him from [0]:
> >
> > The footnote cited isn't a normative requirement (I can't see any
> > normative requirements that say we can't put a const volatile object
> > into a read only section), so it's debatable just how much of a bug
> > this is from a standards conformance perspective. I will have to
> > inquire on the WG14 reflectors to see if there's something I've
> > missed but I believe that C17 6.7.3p5 is meant to point out that
> > only lvalue are qualified; rvalues are not because lvalue conversion
> > strips the qualifiers. I think the footnote is mostly a nod towards
> > the fact that volatile qualified objects may change their value in
> > ways unknown to the compiler so storing it in a read-only section
> > of memory is a bit questionable. But I'm curious if the committee
> > tells me I've missed something there.
> >
> >
> > [0] https://github.com/llvm/llvm-project/issues/56468#issuecomment-1203146308
> >
> > > >
> > > >
> > > > As for systemd's program and its is_allow_list ([1]), to unblock
> > > > yourself you can drop const because systemd doesn't rely on read-only
> > > > guarantees of that variable anyways. It's much more critical in
> > > > feature-detection use cases. But let's try to converge discussion in
> > > > one place (preferably here), it's quite inconvenient to either reply
> > > > the same thing twice here and on Github, or cross-reference lore and
> > > > Github.
> > >
> > > Hmm, are you sure:
> > > const __u8 is_allow_list SEC(".rodata") = 0;
> > >
> > > doesn't provide equivalent behavior to:
> > > const volatile __u8 is_allow_list = 0;
> >
> > Is there anything preventing you from experimenting with this yourself?..
>
> Wasn't sure how to reproduce the issue properly.
>
Then please start asking specific questions about how to repro issues
we are talking about, instead of randomly throwing various ideas
around and seeing what sticks.
> >
> > Let's see:
> >
> > $ git diff
> > diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c
> > b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> > index 99dac5292b41..386785a45af0 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> > @@ -31,6 +31,8 @@ void test_skeleton(void)
> > if (CHECK(skel->kconfig, "skel_kconfig", "kconfig is mmaped()!\n"))
> > goto cleanup;
> >
> > + skel->rodata->should_trap = false;
> > +
> > bss = skel->bss;
> > data = skel->data;
> > data_dyn = skel->data_dyn;
> > diff --git a/tools/testing/selftests/bpf/progs/test_skeleton.c
> > b/tools/testing/selftests/bpf/progs/test_skeleton.c
> > index 1a4e93f6d9df..eee26bc82525 100644
> > --- a/tools/testing/selftests/bpf/progs/test_skeleton.c
> > +++ b/tools/testing/selftests/bpf/progs/test_skeleton.c
> > @@ -26,6 +26,8 @@ const volatile struct {
> > const int in6;
> > } in = {};
> >
> > +const bool should_trap SEC(".rodata") = true;
> > +
> > /* .data section */
> > int out1 = -1;
> > long long out2 = -1;
> > @@ -58,6 +60,10 @@ int handler(const void *ctx)
> > {
> > int i;
> >
> > + while (should_trap)
> > + {
> > + }
> > +
> > out1 = in1;
> > out2 = in2;
> > out3 = in3;
> >
> >
> > Result: selftests stop compiling. Why?
> >
> > $ llvm-objdump -d test_skeleton.linked1.o
> >
> > test_skeleton.linked1.o: file format elf64-bpf
> >
> > Disassembly of section raw_tp/sys_enter:
> >
> > 0000000000000000 <handler>:
> > 0: 05 00 ff ff 00 00 00 00 goto -1 <handler>
> >
> > Because compiler optimized away everything with while (true) {}. There
> > is no global data at all anymore.
> >
> > This is what I keep telling you, const volatile is the only thing
> > preventing the compiler from making wrong assumptions. And neither
> >
> > const bool should_trap SEC(".rodata") = true;
> >
> > nor
> >
> > const bool should_trap = true;
> >
> > work.
> >
> > But
> >
> > const volatile bool should_trap = true;
> >
> > does work.
> >
> > If you have some more speculative proposals, please investigate them
> > yourself and report back with the results. But either way there is a
> > lot of BPF code written with reliance on `const volatile`, both open
> > source and not, so please stop actively trying to break BPF ecosystem
> > with proposals like moving `const volatile` to .data, just because
> > it's more convenient for you in GCC. Thank you.
>
> So it does at least seem I can force GCC to use .rodata like this for
> systemd, maybe that's the best approach for now?:
>
> const volatile __u8 is_allow_list SEC(".rodata") = 0;
>
> It does emit an assembler warning but otherwise appears to work:
> Generating src/core/bpf/restrict_ifaces/restrict-ifaces.bpf.unstripped.o
> with a custom command
> /tmp/ccM2b7jP.s: Assembler messages:
> /tmp/ccM2b7jP.s:87: Warning: setting incorrect section attributes for .rodata
>
> I updated my systemd PR with that.
>
For Nth time, don't do this. Drop the const for systemd. And please
start paying attention to feedback.
Please also start with learning how to build selftests/bpf with Clang
and looking at various examples so that you can at least look at all
the different features we rely on in the BPF world, instead of trying
to tune one small piece (systemd's BPF programs) to your liking. If
you are serious about making GCC BPF backend viable, you'll have to
understand BPF a bit better.
That would be a better use of everyone's time, instead of you going
behind our backs and requesting Clang to break the entire BPF
ecosystem just because GCC is doing something differently, like you
and Jose E. Marchesi did with [0] and [1].
[0] https://github.com/llvm/llvm-project/issues/56468
[1] https://reviews.llvm.org/D131012
> >
> > >
> > > The used attribute in the SEC macro supposedly ensures that:
> > > The compiler must emit the definition even if it appears to be unused, and
> > > it must not apply optimizations which depend on fully understanding how
> > > the entity is used.
> > >
> > > Or maybe the retain attribute along with used would be sufficient to allow
> > > us to drop volatile in these cases?:
> > > https://clang.llvm.org/docs/AttributeReference.html#retain
> > >
> > > >
> > > >
> > > > [0] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25521
> > > > [1] https://github.com/systemd/systemd/pull/24164#issuecomment-1203207372
> > > >
> > > > >
> > > > > >
> > > > > > [0] https://github.com/systemd/systemd/pull/24164#issuecomment-1201806413
> > > > > >
> > > > > >
> > > > > > [...]
