Re: [PATCH bpf-next 0/1] use preserve_static_offset in bpf uapi headers

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2023-12-08 at 12:27 +0000, Alan Maguire wrote:
[...]
> Sorry if this is a digression, but I'm trying to understand how
> this might intersect with vmlinux.h's
>
> #pragma clang attribute push (__attribute__((preserve_access_index)),
> apply_to = record
>
> Since that is currently applied to all structures in vmlinux.h, does
> that protect us from the above scenario when BPF code is compiled and
> #include's vmlinux.h (I suspect not from what you say below but just
> wanted to check)? I realize we get extra relocation info that we don't
> need since the offsets for these BPF context structures are recalcuated
> by the verifier, but given that clang needs to record the relocations,
> does it also constrain the generated code to avoid these "increment
> pointer, use zero offset" instruction patterns? Or can they still occur
> with preserve_access_index applied to the structure? Sorry, might be a
> naive question but it's not clear to me how (if at all) the mechanisms
> might interact.

Unfortunately preserve_access_index does not save us from this problem.
This is the case because field reads and writes are split as two LLVM
IR instructions: getelementptr to get an address, and load/store
to/from that address. The preserve_access_index transformation
rewrites the getelementptr but does not touch load/store.

For example, consider the following C code:

    /* #define __ctx __attribute__((preserve_static_offset)) */
    /* #define __pai */
    #define __ctx
    #define __pai __attribute__((preserve_access_index))

    extern int magic2(int);

    struct bpf_sock {
      int bound_dev_if;
      int family;
    } __ctx __pai;

    struct bpf_sockopt {
      int _;
      struct bpf_sock *sk;
      int level;
      int optlen;
    } __ctx __pai;

    int known_load_sink_example_1(struct bpf_sockopt *ctx)
    {
      unsigned g = 0;
      switch (ctx->level) {
      case 10:
        g = magic2(ctx->sk->family);
        break;
      case 20:
        g = magic2(ctx->optlen);
        break;
      }
      return g % 2;
    }

Here is how it is compiled:

    $ clang -g -O2 --target=bpf -mcpu=v3 -c e3.c -o - | llvm-objdump --no-show-raw-insn -Sdr -
    ...
    0000000000000000 <known_load_sink_example_1>:
    ;   switch (ctx->level) {
           0:   r2 = *(u32 *)(r1 + 0x10)
            0000000000000000:  CO-RE <byte_off> [2] struct bpf_sockopt::level (0:2)
           1:   if w2 == 0x14 goto +0x5 <LBB0_3>
           2:   w0 = 0x0
    ;   switch (ctx->level) {
           3:   if w2 != 0xa goto +0x8 <LBB0_5>
    ;     g = magic2(ctx->sk->family);
           4:   r1 = *(u64 *)(r1 + 0x8)
            0000000000000020:  CO-RE <byte_off> [2] struct bpf_sockopt::sk (0:1)
           5:   r2 = 0x4
            0000000000000028:  CO-RE <byte_off> [7] struct bpf_sock::family (0:1)
           6:   goto +0x1 <LBB0_4>

    0000000000000038 <LBB0_3>:
           7:   r2 = 0x14
            0000000000000038:  CO-RE <byte_off> [2] struct bpf_sockopt::optlen (0:3)

    0000000000000040 <LBB0_4>:
           8:   r1 += r2
           9:   r1 = *(u32 *)(r1 + 0x0)  <---------------- verifier error would
          10:   call -0x1                                  be reported for this insn
            0000000000000050:  R_BPF_64_32  magic2
    ;   return g % 2;
          11:   w0 &= 0x1

    0000000000000060 <LBB0_5>:
          12:   exit

> The reason I ask is if it was safe to assume that code generation would
> avoid such patterns with preserve_access_index, it might avoid needing
> to update vmlinux.h generation.

In current LLVM implementation preserve_static_offset has priority
over preserve_access_index. So changing __pai/__ctx definitions above helps.
(And this priority of one attribute over the other was the reason to
 have preserve_static_offset as an attribute, not as
 btf_decl_tag("preserve_static_offset"). Although that is unfortunate
 for vmlinux.h, as we already have means to preserve decl tags).

[...]

> > How to add the same definitions in vmlinux.h is an open question,
> > and most likely requires bpftool modification:
> > - Hard code generation of __bpf_ctx based on type names?
> > - Mark context types with some special
> >   __attribute__((btf_decl_tag("preserve_static_offset")))
> >   and convert it to __attribute__((preserve_static_offset))?
>
> To me it seems like whatever mechanism supports identification of such
> structures would need to live in vmlinux BTF as ideally it should be
> possible to generate vmlinux.h purely from that BTF. That seems to argue
> for the declaration tag approach.

Tbh, I like the decl tag approach a bit more too.
Although macro definition would be somewhat ridiculous:

    #if __has_attribute(preserve_static_offset) && defined(__bpf__)
    #define __bpf_ctx __attribute__((preserve_static_offset)) \
                      __attribute__((btf_decl_tag("preserve_static_offset")))
    #else
    #define __bpf_ctx
    #endif

Thanks,
Eduard