[RFC v3 0/3] use preserve_static_offset in bpf uapi headers

[RFC v3 0/3] use preserve_static_offset in bpf uapi headers

[Eduard Zingerman]

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

For context types that are not subject of the convert_ctx_access(),
namely:
- bpf_nf_ctx
- bpf_raw_tracepoint_args
- pt_regs

Verifier simply denies access via modified pointer in
__check_ptr_off_reg() function with error message:
"dereference of modified %s ptr R%d off=%d disallowed\n".

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 header include/net/netfilter/nf_bpf_link.h;
- via vmlinux.h.

This RFC seeks to mark with preserve_static_offset the definitions of
the relevant context types within uapi headers, and modify bpftool to
emit said attribute in generated vmlinux.h.

In 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.

Note:
This RFC does not handle type pt_regs used for kprobes/
This type is defined in architecture specific headers like
arch/x86/include/asm/ptrace.h and is hidden behind typedef
bpf_user_pt_regs_t in include/uapi/asm-generic/bpf_perf_event.h.
There are two ways to handle struct pt_regs:
1. Modify all architecture specific ptrace.h files to use __bpf_ctx;
2. Add annotated forward declaration for pt_regs in
   include/uapi/asm-generic/bpf_perf_event.h, e.g. as follows:

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

    struct __bpf_ctx pt_regs;

    #undef __bpf_ctx

    #include <linux/ptrace.h>

    /* Export kernel pt_regs structure */
    typedef struct pt_regs bpf_user_pt_regs_t;

Unfortunately, it might be the case that option (2) is not sufficient,
as at-least BPF selftests access pt_regs either via vmlinux.h or by
directly including ptrace.h.

If option (1) is to be implemented, it feels unreasonable to continue
copying definition of __bpf_ctx macro from file to file.
Given absence of common uapi exported headers between bpf.h and
bpf_perf_event.h/ptrace.h, it looks like a new uapi header would have
to be added, e.g. include/uapi/bpf_compiler.h.
For the moment this header would contain only the definition for
__bpf_ctx, and would be included in bpf.h, nf_bpf_link.h and
architecture specific ptrace.h.

Please advise.

Changelog:
- V2 [3] -> V3:
  - bpftool now generates preserve_static_offset when
    btf_decl_tag("preserve_static_offset") is present
    (as discussed with Quentin);
  - bpftool now correctly filters BTF types that need preserve
    static offset annotation when commands like
    "bpftool btf dump map pinned ... value format c"
    are used;
  - changes in __bpf_ctx definition to include said decl tag;
  - test_bpftool.py extended to check for preserve static offset
    in "bpftool btf dump map pinned ... value format c" output.
- V1 [2] -> V2:
  - changes to bpftool to generate preserve_static_offset
    (by hard-coding context type names as suggested by Yonghong
     and Alexei, including BPF_NO_PRESERVE_STATIC_OFFSET
     option suggested by Alan);
  - added "#undef __bpf_ctx" in relevant headers (Yonghong);
  - added __bpf_ctx for the missing context types (Yonghong);

[1] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
[2] https://lore.kernel.org/bpf/20231208000531.19179-1-eddyz87@gmail.com/
[3] https://lore.kernel.org/bpf/20231212023136.7021-1-eddyz87@gmail.com/

Eduard Zingerman (3):
  bpf: Mark virtual BPF context structures as preserve_static_offset
  bpftool: add attribute preserve_static_offset for context types
  selftests/bpf: verify bpftool emits preserve_static_offset

 include/net/netfilter/nf_bpf_link.h           |  12 +-
 include/uapi/linux/bpf.h                      |  34 +++--
 include/uapi/linux/bpf_perf_event.h           |  12 +-
 tools/bpf/bpftool/btf.c                       | 135 ++++++++++++++++++
 tools/include/uapi/linux/bpf.h                |  34 +++--
 tools/include/uapi/linux/bpf_perf_event.h     |  12 +-
 .../bpf/progs/dummy_no_context_btf.c          |  12 ++
 .../selftests/bpf/progs/dummy_prog_with_map.c |  65 +++++++++
 .../selftests/bpf/progs/dummy_sk_buff_user.c  |  29 ++++
 tools/testing/selftests/bpf/test_bpftool.py   | 100 +++++++++++++
 10 files changed, 418 insertions(+), 27 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_no_context_btf.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_prog_with_map.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c

-- 
2.42.1

[RFC v3 1/3] bpf: Mark virtual BPF context structures as preserve_static_offset

[Eduard Zingerman]

Add __attribute__((preserve_static_offset)) for the following BPF
related structures:
- __sk_buff (*)
- bpf_cgroup_dev_ctx (*)
- bpf_nf_ctx
- bpf_perf_event_data (*)
- bpf_raw_tracepoint_args
- bpf_sk_lookup (*)
- bpf_sock (*)
- bpf_sock_addr (*)
- bpf_sock_ops (*)
- bpf_sockopt (*)
- bpf_sysctl (*)
- sk_msg_md (*)
- sk_reuseport_md (*)
- xdp_md (*)

Access to structures marked with (*) is rewritten by BPF verifier.
(See verifier.c:convert_ctx_access). The rewrite requires that offsets
used in access to fields of these structures are constant values.
For the rest of the structures verifier just disallows access
via modified context pointer in the following code path:

  check_mem_access
    check_ptr_off_reg
      __check_ptr_off_reg
        if (!fixed_off_ok && reg->off)
          "dereference of modified %s ptr R%d off=%d disallowed\n"

Attribute preserve_static_offset [0] is a hint to clang that
ensures that constant offsets are used.

Attribute __attribute__((btf_decl_tag("preserve_static_offset")))
would be used as a marker for bpftool to emit preserve_static_offset
attribute when vmlinux.h is generated from BTF.

Type 'pt_regs' is not handled yet.

Note: !defined(__cplusplus) is necessary only to make
tools/testing/selftests/bpf/test_cpp.cpp selftest compile:
this test includes bpf.h and is compiled as C++.
Without !defined(__cplusplus) the following error message
is reported: "error: 'btf_decl_tag' attribute ignored".

Apparently, when compiling C++, clang's implementation of
__has_attribute returns true for attributes declared as COnly.
This is possibly incorrect [1], but even if fixed in newer clang
versions would be necessary for backwards compatibility.

[0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
[1] https://discourse.llvm.org/t/when-compiling-c-has-attribute-returns-1-for-attributes-declared-as-conly-in-attr-td/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 include/net/netfilter/nf_bpf_link.h       | 12 +++++++-
 include/uapi/linux/bpf.h                  | 34 +++++++++++++++--------
 include/uapi/linux/bpf_perf_event.h       | 12 +++++++-
 tools/include/uapi/linux/bpf.h            | 34 +++++++++++++++--------
 tools/include/uapi/linux/bpf_perf_event.h | 12 +++++++-
 5 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/include/net/netfilter/nf_bpf_link.h b/include/net/netfilter/nf_bpf_link.h
index 6c984b0ea838..7308efd4454f 100644
--- a/include/net/netfilter/nf_bpf_link.h
+++ b/include/net/netfilter/nf_bpf_link.h
@@ -1,9 +1,17 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((preserve_static_offset))
+#elif __has_attribute(btf_decl_tag) && !defined(__cplusplus)
+#define __bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_nf_ctx {
 	const struct nf_hook_state *state;
 	struct sk_buff *skb;
-};
+} __bpf_ctx;
 
 #if IS_ENABLED(CONFIG_NETFILTER_BPF_LINK)
 int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
@@ -13,3 +21,5 @@ static inline int bpf_nf_link_attach(const union bpf_attr *attr, struct bpf_prog
 	return -EOPNOTSUPP;
 }
 #endif
+
+#undef __bpf_ctx
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e0545201b55f..351c39842b7b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -69,6 +69,14 @@ enum {
 /* BPF has 10 general purpose 64-bit registers and stack frame. */
 #define MAX_BPF_REG	__MAX_BPF_REG
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((preserve_static_offset))
+#elif __has_attribute(btf_decl_tag) && !defined(__cplusplus)
+#define __bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_insn {
 	__u8	code;		/* opcode */
 	__u8	dst_reg:4;	/* dest register */
@@ -6190,7 +6198,7 @@ struct __sk_buff {
 	__u8  tstamp_type;
 	__u32 :24;		/* Padding, future use. */
 	__u64 hwtstamp;
-};
+} __bpf_ctx;
 
 struct bpf_tunnel_key {
 	__u32 tunnel_id;
@@ -6271,7 +6279,7 @@ struct bpf_sock {
 	__u32 dst_ip6[4];
 	__u32 state;
 	__s32 rx_queue_mapping;
-};
+} __bpf_ctx;
 
 struct bpf_tcp_sock {
 	__u32 snd_cwnd;		/* Sending congestion window		*/
@@ -6379,7 +6387,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
-};
+} __bpf_ctx;
 
 /* DEVMAP map-value layout
  *
@@ -6429,7 +6437,7 @@ struct sk_msg_md {
 	__u32 size;		/* Total size of sk_msg */
 
 	__bpf_md_ptr(struct bpf_sock *, sk); /* current socket */
-};
+} __bpf_ctx;
 
 struct sk_reuseport_md {
 	/*
@@ -6468,7 +6476,7 @@ struct sk_reuseport_md {
 	 */
 	__bpf_md_ptr(struct bpf_sock *, sk);
 	__bpf_md_ptr(struct bpf_sock *, migrating_sk);
-};
+} __bpf_ctx;
 
 #define BPF_TAG_SIZE	8
 
@@ -6678,7 +6686,7 @@ struct bpf_sock_addr {
 				 * Stored in network byte order.
 				 */
 	__bpf_md_ptr(struct bpf_sock *, sk);
-};
+} __bpf_ctx;
 
 /* User bpf_sock_ops struct to access socket values and specify request ops
  * and their replies.
@@ -6761,7 +6769,7 @@ struct bpf_sock_ops {
 				 * been written yet.
 				 */
 	__u64 skb_hwtstamp;
-};
+} __bpf_ctx;
 
 /* Definitions for bpf_sock_ops_cb_flags */
 enum {
@@ -7034,11 +7042,11 @@ struct bpf_cgroup_dev_ctx {
 	__u32 access_type;
 	__u32 major;
 	__u32 minor;
-};
+} __bpf_ctx;
 
 struct bpf_raw_tracepoint_args {
 	__u64 args[0];
-};
+} __bpf_ctx;
 
 /* DIRECT:  Skip the FIB rules and go to FIB table associated with device
  * OUTPUT:  Do lookup from egress perspective; default is ingress
@@ -7245,7 +7253,7 @@ struct bpf_sysctl {
 	__u32	file_pos;	/* Sysctl file position to read from, write to.
 				 * Allows 1,2,4-byte read an 4-byte write.
 				 */
-};
+} __bpf_ctx;
 
 struct bpf_sockopt {
 	__bpf_md_ptr(struct bpf_sock *, sk);
@@ -7256,7 +7264,7 @@ struct bpf_sockopt {
 	__s32	optname;
 	__s32	optlen;
 	__s32	retval;
-};
+} __bpf_ctx;
 
 struct bpf_pidns_info {
 	__u32 pid;
@@ -7280,7 +7288,7 @@ struct bpf_sk_lookup {
 	__u32 local_ip6[4];	/* Network byte order */
 	__u32 local_port;	/* Host byte order */
 	__u32 ingress_ifindex;		/* The arriving interface. Determined by inet_iif. */
-};
+} __bpf_ctx;
 
 /*
  * struct btf_ptr is used for typed pointer representation; the
@@ -7406,4 +7414,6 @@ struct bpf_iter_num {
 	__u64 __opaque[1];
 } __attribute__((aligned(8)));
 
+#undef __bpf_ctx
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/include/uapi/linux/bpf_perf_event.h b/include/uapi/linux/bpf_perf_event.h
index eb1b9d21250c..4f3f1e906f96 100644
--- a/include/uapi/linux/bpf_perf_event.h
+++ b/include/uapi/linux/bpf_perf_event.h
@@ -10,10 +10,20 @@
 
 #include <asm/bpf_perf_event.h>
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((preserve_static_offset))
+#elif __has_attribute(btf_decl_tag) && !defined(__cplusplus)
+#define __bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
 	__u64 sample_period;
 	__u64 addr;
-};
+} __bpf_ctx;
+
+#undef __bpf_ctx
 
 #endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e0545201b55f..351c39842b7b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -69,6 +69,14 @@ enum {
 /* BPF has 10 general purpose 64-bit registers and stack frame. */
 #define MAX_BPF_REG	__MAX_BPF_REG
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((preserve_static_offset))
+#elif __has_attribute(btf_decl_tag) && !defined(__cplusplus)
+#define __bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_insn {
 	__u8	code;		/* opcode */
 	__u8	dst_reg:4;	/* dest register */
@@ -6190,7 +6198,7 @@ struct __sk_buff {
 	__u8  tstamp_type;
 	__u32 :24;		/* Padding, future use. */
 	__u64 hwtstamp;
-};
+} __bpf_ctx;
 
 struct bpf_tunnel_key {
 	__u32 tunnel_id;
@@ -6271,7 +6279,7 @@ struct bpf_sock {
 	__u32 dst_ip6[4];
 	__u32 state;
 	__s32 rx_queue_mapping;
-};
+} __bpf_ctx;
 
 struct bpf_tcp_sock {
 	__u32 snd_cwnd;		/* Sending congestion window		*/
@@ -6379,7 +6387,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
-};
+} __bpf_ctx;
 
 /* DEVMAP map-value layout
  *
@@ -6429,7 +6437,7 @@ struct sk_msg_md {
 	__u32 size;		/* Total size of sk_msg */
 
 	__bpf_md_ptr(struct bpf_sock *, sk); /* current socket */
-};
+} __bpf_ctx;
 
 struct sk_reuseport_md {
 	/*
@@ -6468,7 +6476,7 @@ struct sk_reuseport_md {
 	 */
 	__bpf_md_ptr(struct bpf_sock *, sk);
 	__bpf_md_ptr(struct bpf_sock *, migrating_sk);
-};
+} __bpf_ctx;
 
 #define BPF_TAG_SIZE	8
 
@@ -6678,7 +6686,7 @@ struct bpf_sock_addr {
 				 * Stored in network byte order.
 				 */
 	__bpf_md_ptr(struct bpf_sock *, sk);
-};
+} __bpf_ctx;
 
 /* User bpf_sock_ops struct to access socket values and specify request ops
  * and their replies.
@@ -6761,7 +6769,7 @@ struct bpf_sock_ops {
 				 * been written yet.
 				 */
 	__u64 skb_hwtstamp;
-};
+} __bpf_ctx;
 
 /* Definitions for bpf_sock_ops_cb_flags */
 enum {
@@ -7034,11 +7042,11 @@ struct bpf_cgroup_dev_ctx {
 	__u32 access_type;
 	__u32 major;
 	__u32 minor;
-};
+} __bpf_ctx;
 
 struct bpf_raw_tracepoint_args {
 	__u64 args[0];
-};
+} __bpf_ctx;
 
 /* DIRECT:  Skip the FIB rules and go to FIB table associated with device
  * OUTPUT:  Do lookup from egress perspective; default is ingress
@@ -7245,7 +7253,7 @@ struct bpf_sysctl {
 	__u32	file_pos;	/* Sysctl file position to read from, write to.
 				 * Allows 1,2,4-byte read an 4-byte write.
 				 */
-};
+} __bpf_ctx;
 
 struct bpf_sockopt {
 	__bpf_md_ptr(struct bpf_sock *, sk);
@@ -7256,7 +7264,7 @@ struct bpf_sockopt {
 	__s32	optname;
 	__s32	optlen;
 	__s32	retval;
-};
+} __bpf_ctx;
 
 struct bpf_pidns_info {
 	__u32 pid;
@@ -7280,7 +7288,7 @@ struct bpf_sk_lookup {
 	__u32 local_ip6[4];	/* Network byte order */
 	__u32 local_port;	/* Host byte order */
 	__u32 ingress_ifindex;		/* The arriving interface. Determined by inet_iif. */
-};
+} __bpf_ctx;
 
 /*
  * struct btf_ptr is used for typed pointer representation; the
@@ -7406,4 +7414,6 @@ struct bpf_iter_num {
 	__u64 __opaque[1];
 } __attribute__((aligned(8)));
 
+#undef __bpf_ctx
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/tools/include/uapi/linux/bpf_perf_event.h b/tools/include/uapi/linux/bpf_perf_event.h
index eb1b9d21250c..4f3f1e906f96 100644
--- a/tools/include/uapi/linux/bpf_perf_event.h
+++ b/tools/include/uapi/linux/bpf_perf_event.h
@@ -10,10 +10,20 @@
 
 #include <asm/bpf_perf_event.h>
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((preserve_static_offset))
+#elif __has_attribute(btf_decl_tag) && !defined(__cplusplus)
+#define __bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_perf_event_data {
 	bpf_user_pt_regs_t regs;
 	__u64 sample_period;
 	__u64 addr;
-};
+} __bpf_ctx;
+
+#undef __bpf_ctx
 
 #endif /* _UAPI__LINUX_BPF_PERF_EVENT_H__ */
-- 
2.42.1

[RFC v3 2/3] bpftool: add attribute preserve_static_offset for context types

[Eduard Zingerman]

When printing vmlinux.h, emit attribute preserve_static_offset [0]
for types that have associated BTF decl tag T with value
"preserve_static_offset".

To avoid hacking libbpf dump logic emit forward declarations annotated
with attribute. Such forward declarations have to come before
structure definitions.

Only emit such forward declarations when context types are present in
target BTF (identified by name).

C language standard wording in section "6.7.2.1 Structure and union
specifiers" [1] is vague, but example in 6.7.2.1.21 explicitly allows
such notation, and it matches clang behavior.

Here is how 'bpftool btf gen ... format c' looks after this change:

    #ifndef __VMLINUX_H__
    #define __VMLINUX_H__

    #if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && \
        __has_attribute(preserve_static_offset)
    #pragma clang attribute push \
              (__attribute__((preserve_static_offset)), apply_to = record)

    struct bpf_cgroup_dev_ctx;
    ...

    #pragma clang attribute pop
    #endif

    ... rest of the output unchanged ...

This is a follow up for discussion in thread [2].

[0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf
[2] https://lore.kernel.org/bpf/fce6188a-6ccc-4b92-9aa7-9ee18b2f2fa1@isovalent.com/

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/bpf/bpftool/btf.c | 135 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 91fcb75babe3..feded48ddd76 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -460,6 +460,136 @@ static void __printf(2, 0) btf_dump_printf(void *ctx,
 	vfprintf(stdout, fmt, args);
 }
 
+/* Recursively walk all dependencies of 'id' and mark those as true in
+ * array 'deps'. The goal is to find all types that would be printed by
+ * btf_dump if 'id' is dumped.
+ */
+static void mark_dependencies(const struct btf *btf, __u32 id, bool *deps)
+{
+	const struct btf_param *params;
+	const struct btf_array *arr;
+	const struct btf_type *t;
+	struct btf_member *m;
+	__u16 vlen, i;
+
+	if (id == 0 || deps[id])
+		return;
+
+	deps[id] = true;
+	t = btf__type_by_id(btf, id);
+	if (!t)
+		return;
+
+	switch (btf_kind(t)) {
+	case BTF_KIND_PTR:
+	case BTF_KIND_TYPEDEF:
+	case BTF_KIND_VOLATILE:
+	case BTF_KIND_CONST:
+	case BTF_KIND_RESTRICT:
+	case BTF_KIND_TYPE_TAG:
+	case BTF_KIND_DECL_TAG:
+	case BTF_KIND_FUNC:
+		mark_dependencies(btf, t->type, deps);
+		break;
+	case BTF_KIND_STRUCT:
+	case BTF_KIND_UNION:
+		vlen = btf_vlen(t);
+		m = btf_members(t);
+		for (i = 0; i < vlen; ++i)
+			mark_dependencies(btf, m[i].type, deps);
+		break;
+	case BTF_KIND_ARRAY:
+		arr = btf_array(t);
+		mark_dependencies(btf, arr->type, deps);
+		break;
+	case BTF_KIND_FUNC_PROTO:
+		vlen = btf_vlen(t);
+		params = btf_params(t);
+		mark_dependencies(btf, t->type, deps);
+		for (i = 0; i < vlen; ++i)
+			mark_dependencies(btf, params[i].type, deps);
+		break;
+	default:
+		/* ignore */
+		break;
+	}
+}
+
+/* Iterate all types in 'btf', if there are BTF_DECL_TAG records R
+ * with "preserve_static_offset" tag - emit a forward declaration
+ * for R->type annotated with preserve_static_offset attribute [0].
+ *
+ * If root_type_ids/root_type_cnt is specified, filter generated declarations
+ * to only include root_type_ids and corresponding dependencies.
+ *
+ * [0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
+ */
+static int emit_static_offset_protos(const struct btf *btf,
+				     __u32 *root_type_ids, int root_type_cnt)
+{
+	bool *root_type_deps = NULL;
+	bool first = true;
+	__u32 i, id, cnt;
+
+	cnt = btf__type_cnt(btf);
+	if (root_type_cnt) {
+		root_type_deps = calloc(cnt, sizeof(*root_type_deps));
+		if (!root_type_deps)
+			return -ENOMEM;
+
+		for (i = 0; i < (__u32)root_type_cnt; ++i)
+			mark_dependencies(btf, root_type_ids[i], root_type_deps);
+	}
+
+	for (id = 1; id < cnt; ++id) {
+		const struct btf_type *t, *s;
+		const char *name, *tag;
+
+		t = btf__type_by_id(btf, id);
+		if (!t)
+			continue;
+
+		if (!btf_is_decl_tag(t))
+			continue;
+
+		tag = btf__name_by_offset(btf, t->name_off);
+		if (strcmp(tag, "preserve_static_offset") != 0)
+			continue;
+
+		if (root_type_deps && !root_type_deps[t->type])
+			continue;
+
+		s = btf__type_by_id(btf, t->type);
+		if (!s)
+			continue;
+
+		if (!btf_is_struct(s) && !btf_is_union(s))
+			continue;
+
+		name = btf__name_by_offset(btf, s->name_off);
+		if (!name)
+			continue;
+
+		if (first) {
+			first = false;
+			printf("#if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && __has_attribute(preserve_static_offset)\n");
+			printf("#pragma clang attribute push (__attribute__((preserve_static_offset)), apply_to = record)\n");
+			printf("\n");
+		}
+
+		printf("struct %s;\n", name);
+	}
+
+	if (!first) {
+		printf("\n");
+		printf("#pragma clang attribute pop\n");
+		printf("#endif /* BPF_NO_PRESERVE_STATIC_OFFSET */\n\n");
+	}
+
+	free(root_type_deps);
+	return 0;
+}
+
 static int dump_btf_c(const struct btf *btf,
 		      __u32 *root_type_ids, int root_type_cnt)
 {
@@ -473,6 +603,11 @@ static int dump_btf_c(const struct btf *btf,
 	printf("#ifndef __VMLINUX_H__\n");
 	printf("#define __VMLINUX_H__\n");
 	printf("\n");
+
+	err = emit_static_offset_protos(btf, root_type_ids, root_type_cnt);
+	if (err)
+		goto done;
+
 	printf("#ifndef BPF_NO_PRESERVE_ACCESS_INDEX\n");
 	printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
 	printf("#endif\n\n");
-- 
2.42.1

[RFC v3 3/3] selftests/bpf: verify bpftool emits preserve_static_offset

[Eduard Zingerman]

Extend test_bpftool.py with following test cases:
- Load a small program that has some context types in it's BTF,
  verify that "bpftool btf dump file ... format c" emits
  preserve_static_offset attribute.
- Load a small program that has no context types in it's BTF,
  verify that "bpftool btf dump file ... format c" does not emit
  preserve_static_offset attribute.
- Load a small program that uses a map,
  verify that "bpftool btf dump map pinned ... value format c"
  emit preserve_static_offset for expected types.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/progs/dummy_no_context_btf.c          |  12 +++
 .../selftests/bpf/progs/dummy_prog_with_map.c |  65 ++++++++++++
 .../selftests/bpf/progs/dummy_sk_buff_user.c  |  29 +++++
 tools/testing/selftests/bpf/test_bpftool.py   | 100 ++++++++++++++++++
 4 files changed, 206 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_no_context_btf.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_prog_with_map.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c

diff --git a/tools/testing/selftests/bpf/progs/dummy_no_context_btf.c b/tools/testing/selftests/bpf/progs/dummy_no_context_btf.c
new file mode 100644
index 000000000000..5a1df4984dce
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_no_context_btf.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* A dummy program that does not reference context types in it's BTF */
+SEC("tc")
+__u32 dummy_prog(void *ctx)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/dummy_prog_with_map.c b/tools/testing/selftests/bpf/progs/dummy_prog_with_map.c
new file mode 100644
index 000000000000..0268317494ef
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_prog_with_map.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+#if __has_attribute(btf_decl_tag)
+#define __decl_tag_bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#endif
+
+struct test_struct_a {
+	int v;
+} __decl_tag_bpf_ctx;
+
+struct test_struct_b {
+	int v;
+} __decl_tag_bpf_ctx;
+
+struct test_struct_c {
+	int v;
+} __decl_tag_bpf_ctx;
+
+struct test_struct_d {
+	int v;
+} __decl_tag_bpf_ctx;
+
+struct test_struct_e {
+	int v;
+} __decl_tag_bpf_ctx;
+
+struct test_struct_f {
+	int v;
+} __decl_tag_bpf_ctx;
+
+typedef struct test_struct_c test_struct_c_td;
+
+struct map_value {
+	struct test_struct_a a;
+	struct test_struct_b b[2];
+	test_struct_c_td c;
+	const struct test_struct_d *(*d)(volatile struct test_struct_e *);
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 4);
+	__type(key, int);
+	__type(value, struct map_value);
+} test_map1 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 4);
+	__type(key, int);
+	__type(value, struct test_struct_f);
+} test_map2 SEC(".maps");
+
+/* A dummy program that references map 'test_map', used by test_bpftool.py */
+SEC("tc")
+int dummy_prog_with_map(void *ctx)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c b/tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c
new file mode 100644
index 000000000000..8c10aebe8689
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* In linux/bpf.h __bpf_ctx macro is defined differently for BPF and
+ * non-BPF targets:
+ * - for BPF it is __attribute__((preserve_static_offset))
+ * - for non-BPF it is __attribute__((btf_decl_tag("preserve_static_offset")))
+ *
+ * bpftool uses decl tag as a signal to emit preserve_static_offset,
+ * thus additional declaration is needed in this test.
+ */
+#if __has_attribute(btf_decl_tag)
+#define __decl_tag_bpf_ctx __attribute__((btf_decl_tag(("preserve_static_offset"))))
+#endif
+
+struct __decl_tag_bpf_ctx __sk_buff;
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+/* A dummy program that references __sk_buff type in it's BTF,
+ * used by test_bpftool.py.
+ */
+SEC("tc")
+int sk_buff_user(struct __sk_buff *skb)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_bpftool.py b/tools/testing/selftests/bpf/test_bpftool.py
index 1c2408ee1f5d..d3a8b71afcc1 100644
--- a/tools/testing/selftests/bpf/test_bpftool.py
+++ b/tools/testing/selftests/bpf/test_bpftool.py
@@ -3,10 +3,12 @@
 
 import collections
 import functools
+import io
 import json
 import os
 import socket
 import subprocess
+import tempfile
 import unittest
 
 
@@ -25,6 +27,10 @@ class UnprivilegedUserError(Exception):
     pass
 
 
+class MissingDependencyError(Exception):
+    pass
+
+
 def _bpftool(args, json=True):
     _args = ["bpftool"]
     if json:
@@ -63,12 +69,26 @@ DMESG_EMITTING_HELPERS = [
         "bpf_trace_vprintk",
     ]
 
+BPFFS_MOUNT = "/sys/fs/bpf/"
+
+DUMMY_SK_BUFF_USER_OBJ = cur_dir + "/dummy_sk_buff_user.bpf.o"
+DUMMY_NO_CONTEXT_BTF_OBJ = cur_dir + "/dummy_no_context_btf.bpf.o"
+DUMMY_PROG_WITH_MAP_OBJ = cur_dir + "/dummy_prog_with_map.bpf.o"
+
 class TestBpftool(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
         if os.getuid() != 0:
             raise UnprivilegedUserError(
                 "This test suite needs root privileges")
+        objs = [DUMMY_SK_BUFF_USER_OBJ,
+                DUMMY_NO_CONTEXT_BTF_OBJ,
+                DUMMY_PROG_WITH_MAP_OBJ]
+        for obj in objs:
+            if os.path.exists(obj):
+                continue
+            raise MissingDependencyError(
+                "File " + obj + " does not exist, make sure progs/*.c are compiled")
 
     @default_iface
     def test_feature_dev_json(self, iface):
@@ -172,3 +192,83 @@ class TestBpftool(unittest.TestCase):
         res = bpftool(["feature", "probe", "macros"])
         for pattern in expected_patterns:
             self.assertRegex(res, pattern)
+
+    def assertStringsPresent(self, text, patterns):
+        pos = 0
+        for i, pat in enumerate(patterns):
+            m = text.find(pat, pos)
+            if m == -1:
+                with io.StringIO() as msg:
+                    print("Can't find expected string:", file=msg)
+                    for s in patterns[0:i]:
+                        print("    MATCHED: " + s, file=msg)
+                    print("NOT MATCHED: " + pat, file=msg)
+                    print("", file=msg)
+                    print("Searching in:", file=msg)
+                    print(text, file=msg)
+                    self.fail(msg.getvalue())
+            pos += len(pat)
+
+    def assertPreserveStaticOffset(self, btf_dump, types):
+        self.assertStringsPresent(btf_dump, [
+            "#if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && " +
+              "__has_attribute(preserve_static_offset)",
+            "#pragma clang attribute push " +
+              "(__attribute__((preserve_static_offset)), apply_to = record)"
+        ] + ["struct " + t + ";" for t in types] + [
+            "#endif /* BPF_NO_PRESERVE_STATIC_OFFSET */"
+        ])
+
+    # Load a small program that has some context types in it's BTF,
+    # verify that "bpftool btf dump file ... format c" emits
+    # preserve_static_offset attribute.
+    def test_c_dump_preserve_static_offset_present(self):
+        res = bpftool(["btf", "dump", "file", DUMMY_SK_BUFF_USER_OBJ, "format", "c"])
+        self.assertPreserveStaticOffset(res, ["__sk_buff"])
+
+    # Load a small program that has no context types in it's BTF,
+    # verify that "bpftool btf dump file ... format c" does not emit
+    # preserve_static_offset attribute.
+    def test_c_dump_no_preserve_static_offset(self):
+        res = bpftool(["btf", "dump", "file", DUMMY_NO_CONTEXT_BTF_OBJ, "format", "c"])
+        self.assertNotRegex(res, "preserve_static_offset")
+        self.assertStringsPresent(res, [
+            "preserve_access_index",
+            "typedef unsigned int __u32;"
+        ])
+
+    # When BTF is dumped for maps bpftool follows a slightly different
+    # code path, that filters which BTF types would be printed.
+    # Test this code path here:
+    # - load a program that uses a map, value type of which references
+    #   a number of structs annotated with preserve_static_offset;
+    # - dump BTF for that map and check preserve_static_offset annotation
+    #   for expected structs.
+    def test_c_dump_preserve_static_offset_map(self):
+        prog_pin = tempfile.mktemp(prefix="dummy_prog_with_map", dir=BPFFS_MOUNT)
+        maps_dir = tempfile.mktemp(prefix="dummy_prog_with_map_maps", dir=BPFFS_MOUNT)
+        map_pin1 = maps_dir + "/test_map1"
+        map_pin2 = maps_dir + "/test_map2"
+
+        bpftool(["prog", "load", DUMMY_PROG_WITH_MAP_OBJ, prog_pin, "pinmaps", maps_dir])
+        try:
+            map1 = bpftool(["btf", "dump", "map", "pinned", map_pin1, "value", "format", "c"])
+            map2 = bpftool(["btf", "dump", "map", "pinned", map_pin2, "value", "format", "c"])
+        finally:
+            os.remove(prog_pin)
+            os.remove(map_pin1)
+            os.remove(map_pin2)
+            os.rmdir(maps_dir)
+
+        # test_map1 should have all types except struct test_struct_f
+        self.assertPreserveStaticOffset(map1, [
+            "test_struct_a", "test_struct_b", "test_struct_c",
+            "test_struct_d", "test_struct_e",
+        ])
+        self.assertNotRegex(map1, "test_struct_f")
+
+        # test_map2 should have only struct test_struct_f
+        self.assertPreserveStaticOffset(map2, [
+            "test_struct_f"
+        ])
+        self.assertNotRegex(map2, "struct test_struct_a")
-- 
2.42.1

Re: [RFC v3 0/3] use preserve_static_offset in bpf uapi headers

[Alexei Starovoitov]

On Wed, Dec 20, 2023 at 5:34 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
>
> Note:
> This RFC does not handle type pt_regs used for kprobes/
> This type is defined in architecture specific headers like
> arch/x86/include/asm/ptrace.h and is hidden behind typedef
> bpf_user_pt_regs_t in include/uapi/asm-generic/bpf_perf_event.h.
> There are two ways to handle struct pt_regs:
> 1. Modify all architecture specific ptrace.h files to use __bpf_ctx;
> 2. Add annotated forward declaration for pt_regs in
>    include/uapi/asm-generic/bpf_perf_event.h, e.g. as follows:
>
>     #if __has_attribute(preserve_static_offset) && defined(__bpf__)
>     #define __bpf_ctx __attribute__((preserve_static_offset))
>     #else
>     #define __bpf_ctx
>     #endif
>
>     struct __bpf_ctx pt_regs;
>
>     #undef __bpf_ctx
>
>     #include <linux/ptrace.h>
>
>     /* Export kernel pt_regs structure */
>     typedef struct pt_regs bpf_user_pt_regs_t;
>
> Unfortunately, it might be the case that option (2) is not sufficient,
> as at-least BPF selftests access pt_regs either via vmlinux.h or by
> directly including ptrace.h.
>
> If option (1) is to be implemented, it feels unreasonable to continue
> copying definition of __bpf_ctx macro from file to file.
> Given absence of common uapi exported headers between bpf.h and
> bpf_perf_event.h/ptrace.h, it looks like a new uapi header would have
> to be added, e.g. include/uapi/bpf_compiler.h.
> For the moment this header would contain only the definition for
> __bpf_ctx, and would be included in bpf.h, nf_bpf_link.h and
> architecture specific ptrace.h.
>
> Please advise.

I'm afraid option 1 is a non starter. bpf quirks cannot impose
such heavy tax on the kernel.

Option 2 is equally hacky.

I think we should do what v2 did and hard code pt_regs in bpftool.

Re: [RFC v3 0/3] use preserve_static_offset in bpf uapi headers

[Eduard Zingerman]

On Wed, 2023-12-20 at 11:20 -0800, Alexei Starovoitov wrote:
> On Wed, Dec 20, 2023 at 5:34 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > This RFC does not handle type pt_regs used for kprobes/
> > This type is defined in architecture specific headers like
> > arch/x86/include/asm/ptrace.h and is hidden behind typedef
> > bpf_user_pt_regs_t in include/uapi/asm-generic/bpf_perf_event.h.
> > There are two ways to handle struct pt_regs:
> > 1. Modify all architecture specific ptrace.h files to use __bpf_ctx;
> > 2. Add annotated forward declaration for pt_regs in
> >    include/uapi/asm-generic/bpf_perf_event.h, e.g. as follows:
> > 
> >     #if __has_attribute(preserve_static_offset) && defined(__bpf__)
> >     #define __bpf_ctx __attribute__((preserve_static_offset))
> >     #else
> >     #define __bpf_ctx
> >     #endif
> > 
> >     struct __bpf_ctx pt_regs;
> > 
> >     #undef __bpf_ctx
> > 
> >     #include <linux/ptrace.h>
> > 
> >     /* Export kernel pt_regs structure */
> >     typedef struct pt_regs bpf_user_pt_regs_t;
> > 
> > Unfortunately, it might be the case that option (2) is not sufficient,
> > as at-least BPF selftests access pt_regs either via vmlinux.h or by
> > directly including ptrace.h.
> > 
> > If option (1) is to be implemented, it feels unreasonable to continue
> > copying definition of __bpf_ctx macro from file to file.
> > Given absence of common uapi exported headers between bpf.h and
> > bpf_perf_event.h/ptrace.h, it looks like a new uapi header would have
> > to be added, e.g. include/uapi/bpf_compiler.h.
> > For the moment this header would contain only the definition for
> > __bpf_ctx, and would be included in bpf.h, nf_bpf_link.h and
> > architecture specific ptrace.h.
> > 
> > Please advise.
> 
> I'm afraid option 1 is a non starter. bpf quirks cannot impose
> such heavy tax on the kernel.
> 
> Option 2 is equally hacky.
> 
> I think we should do what v2 did and hard code pt_regs in bpftool.

I agree on (1).
As for (2), I use the same hack in current patch for bpftool to avoid
hacking main logic of BPF dump, it works and is allowed by C language
standard (albeit in vague terms, but example is present).
Unfortunately (2) does not propagate to vmlinux.h.

Quentin, Alan, what do you think about hard-coding only pt_regs?

Re: [RFC v3 0/3] use preserve_static_offset in bpf uapi headers

[Quentin Monnet]

2023-12-20 20:19 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> On Wed, 2023-12-20 at 11:20 -0800, Alexei Starovoitov wrote:
>> On Wed, Dec 20, 2023 at 5:34 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>> This RFC does not handle type pt_regs used for kprobes/
>>> This type is defined in architecture specific headers like
>>> arch/x86/include/asm/ptrace.h and is hidden behind typedef
>>> bpf_user_pt_regs_t in include/uapi/asm-generic/bpf_perf_event.h.
>>> There are two ways to handle struct pt_regs:
>>> 1. Modify all architecture specific ptrace.h files to use __bpf_ctx;
>>> 2. Add annotated forward declaration for pt_regs in
>>>    include/uapi/asm-generic/bpf_perf_event.h, e.g. as follows:
>>>
>>>     #if __has_attribute(preserve_static_offset) && defined(__bpf__)
>>>     #define __bpf_ctx __attribute__((preserve_static_offset))
>>>     #else
>>>     #define __bpf_ctx
>>>     #endif
>>>
>>>     struct __bpf_ctx pt_regs;
>>>
>>>     #undef __bpf_ctx
>>>
>>>     #include <linux/ptrace.h>
>>>
>>>     /* Export kernel pt_regs structure */
>>>     typedef struct pt_regs bpf_user_pt_regs_t;
>>>
>>> Unfortunately, it might be the case that option (2) is not sufficient,
>>> as at-least BPF selftests access pt_regs either via vmlinux.h or by
>>> directly including ptrace.h.
>>>
>>> If option (1) is to be implemented, it feels unreasonable to continue
>>> copying definition of __bpf_ctx macro from file to file.
>>> Given absence of common uapi exported headers between bpf.h and
>>> bpf_perf_event.h/ptrace.h, it looks like a new uapi header would have
>>> to be added, e.g. include/uapi/bpf_compiler.h.
>>> For the moment this header would contain only the definition for
>>> __bpf_ctx, and would be included in bpf.h, nf_bpf_link.h and
>>> architecture specific ptrace.h.
>>>
>>> Please advise.
>>
>> I'm afraid option 1 is a non starter. bpf quirks cannot impose
>> such heavy tax on the kernel.
>>
>> Option 2 is equally hacky.
>>
>> I think we should do what v2 did and hard code pt_regs in bpftool.
> 
> I agree on (1).
> As for (2), I use the same hack in current patch for bpftool to avoid
> hacking main logic of BPF dump, it works and is allowed by C language
> standard (albeit in vague terms, but example is present).
> Unfortunately (2) does not propagate to vmlinux.h.
> 
> Quentin, Alan, what do you think about hard-coding only pt_regs?


It sounds like an acceptable compromise.

Quentin