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?..
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.
>
> 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
> > > >
> > > >
> > > > [...]
