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

[Yonghong Song <yonghong.song@linux.dev>]

On 12/7/23 4:05 PM, Eduard Zingerman wrote:
> For certain program context types, the verifier applies the
> verifier.c:convert_ctx_access() transformation.
> It modifies ST/STX/LDX instructions that access program context.
> convert_ctx_access() updates the offset field of these instructions
> changing "virtual" offset by offset corresponding to data
> representation in the running kernel.
>
> For this transformation to be applicable access to the context field
> shouldn't use pointer arithmetics. For example, consider the read of
> __sk_buff->pkt_type field.
> If translated as a single ST instruction:
>
>      r0 = *(u32 *)(r1 + 4);
>
> The verifier would accept such code and patch the offset in the
> instruction, however, if translated as a pair of instructions:
>
>      r1 += 4;
>      r0 = *(u32 *)(r1 + 0);
>
> The verifier would reject such code.
>
> Occasionally clang shuffling code during compilation might break
> verifier expectations and cause verification errors, e.g. as in [0].
> Technically, this happens because each field read/write represented in
> LLVM IR as two operations: address lookup + memory access,
> and the compiler is free to move and substitute those independently.
> For example, LLVM can rewrite C code below:
>
>      __u32 v;
>      if (...)
>        v = sk_buff->pkt_type;
>      else
>        v = sk_buff->mark;
>
> As if it was written as so:
>
>      __u32 v, *p;
>      if (...)
>        p = &sk_buff->pkt_type;  // r0 = 4; (offset of pkt_type)
>      else
>        p = &sk_buff->mark;      // r0 = 8; (offset of mark)
>      v = *p;                    // r1 += r0;
>                                 // r0 = *(u32 *)(r1 + 0)
>
> Which is a valid rewrite from the point of view of C semantics but won't
> pass verification, because convert_ctx_access() can no longer replace
> offset in 'r0 = *(u32 *)(r1 + 0)' with a constant.
>
> Recently, attribute preserve_static_offset was added to
> clang [1] to tackle this problem. From its documentation:
>
>    Clang supports the ``__attribute__((preserve_static_offset))``
>    attribute for the BPF target. This attribute may be attached to a
>    struct or union declaration. Reading or writing fields of types having
>    such annotation is guaranteed to generate LDX/ST/STX instruction with
>    an offset corresponding to the field.
>
> The convert_ctx_access() transformation is applied when the context
> parameter has one of the following types:
> - __sk_buff
> - bpf_cgroup_dev_ctx
> - bpf_perf_event_data
> - bpf_sk_lookup
> - bpf_sock
> - bpf_sock_addr
> - bpf_sock_ops
> - bpf_sockopt
> - bpf_sysctl
> - sk_msg_md
> - sk_reuseport_md
> - xdp_md

All context types are defined in include/linux/bpf_types.h.
The context type bpf_nf_ctx is missing.

>
>  From my understanding, BPF programs typically access definitions of
> these types in two ways:
> - via uapi headers linux/bpf.h and linux/bpf_perf_event.h;

and bpf_nf_ctx is defined in include/net/netfilter/nf_bpf_link.h
and rely on vmlinux.h to provide the ctx struct definition.

> - via vmlinux.h.
>
> This RFC seeks to mark with preserve_static_offset the definitions of
> the relevant context types within uapi headers.
>
> The attribute is abstracted by '__bpf_ctx' macro.
> As bpf.h and bpf_perf_event.h do not share any common include files,
> this RFC opts to copy the same definition of '__bpf_ctx' in both
> headers to avoid adding a new uapi header.
> (Another tempting location for '__bpf_ctx' is compiler_types.h /
>   compiler-clang.h, but these headers are not exported as uapi).

Previously I think we might use similar mechanism like vmlinux.h
with push/pop preserve_static_offset attributes. But looks like
there are many other structures in uapi bpf.h do not need
preserve_static_offset. So I think your approach sounds okay.

>
> 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))?

The number of context types is limited, I would just go through
the first approach with hard coding the list of ctx types and
mark them with preserve_static_offset attribute in vmlinux.h.

>
> Please suggest if any of the options above sound reasonable.
>
> [0] https://lore.kernel.org/bpf/CAA-VZPmxh8o8EBcJ=m-DH4ytcxDFmo0JKsm1p1gf40kS0CE3NQ@mail.gmail.com/T/#m4b9ce2ce73b34f34172328f975235fc6f19841b6
> [1] 030b8cb1561d ("[BPF] Attribute preserve_static_offset for structs")
>      git@github.com:llvm/llvm-project.git
>
> Eduard Zingerman (1):
>    bpf: Mark virtual BPF context structures as preserve_static_offset
>
>   include/uapi/linux/bpf.h                  | 28 ++++++++++++++---------
>   include/uapi/linux/bpf_perf_event.h       |  8 ++++++-
>   tools/include/uapi/linux/bpf.h            | 28 ++++++++++++++---------
>   tools/include/uapi/linux/bpf_perf_event.h |  8 ++++++-
>   4 files changed, 48 insertions(+), 24 deletions(-)
>