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