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

[Alan Maguire <alan.maguire@oracle.com>]

On 08/12/2023 00:05, 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.
>

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.

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.

> 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
> 
> 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;
> - 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).
> 
> 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.

Thanks!

Alan

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