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