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?
>
> [0] https://github.com/systemd/systemd/pull/24164#issuecomment-1201806413
>
>
> [...]
