[RFC v2 0/2] use preserve_static_offset in bpf uapi headers

[RFC v2 0/2] 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:
- 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);

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           |  10 +-
 include/uapi/linux/bpf.h                      |  32 +++--
 include/uapi/linux/bpf_perf_event.h           |  10 +-
 tools/bpf/bpftool/btf.c                       | 131 ++++++++++++++++--
 tools/include/uapi/linux/bpf.h                |  32 +++--
 tools/include/uapi/linux/bpf_perf_event.h     |  10 +-
 .../bpf/progs/dummy_no_context_btf.c          |  12 ++
 .../selftests/bpf/progs/dummy_sk_buff_user.c  |  14 ++
 tools/testing/selftests/bpf/test_bpftool.py   |  61 ++++++++
 9 files changed, 270 insertions(+), 42 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_no_context_btf.c
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c

--
2.42.1

[RFC v2 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.

Type 'pt_regs' is not handled yet.

[0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset

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

diff --git a/include/net/netfilter/nf_bpf_link.h b/include/net/netfilter/nf_bpf_link.h
index 6c984b0ea838..e5555b1ac55d 100644
--- a/include/net/netfilter/nf_bpf_link.h
+++ b/include/net/netfilter/nf_bpf_link.h
@@ -1,9 +1,15 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((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 +19,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..f533301de5e4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -69,6 +69,12 @@ 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))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_insn {
 	__u8	code;		/* opcode */
 	__u8	dst_reg:4;	/* dest register */
@@ -6190,7 +6196,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 +6277,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 +6385,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 +6435,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 +6474,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 +6684,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 +6767,7 @@ struct bpf_sock_ops {
 				 * been written yet.
 				 */
 	__u64 skb_hwtstamp;
-};
+} __bpf_ctx;
 
 /* Definitions for bpf_sock_ops_cb_flags */
 enum {
@@ -7034,11 +7040,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 +7251,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 +7262,7 @@ struct bpf_sockopt {
 	__s32	optname;
 	__s32	optlen;
 	__s32	retval;
-};
+} __bpf_ctx;
 
 struct bpf_pidns_info {
 	__u32 pid;
@@ -7280,7 +7286,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 +7412,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..608e366877fc 100644
--- a/include/uapi/linux/bpf_perf_event.h
+++ b/include/uapi/linux/bpf_perf_event.h
@@ -10,10 +10,18 @@
 
 #include <asm/bpf_perf_event.h>
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((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..f533301de5e4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -69,6 +69,12 @@ 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))
+#else
+#define __bpf_ctx
+#endif
+
 struct bpf_insn {
 	__u8	code;		/* opcode */
 	__u8	dst_reg:4;	/* dest register */
@@ -6190,7 +6196,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 +6277,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 +6385,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 +6435,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 +6474,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 +6684,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 +6767,7 @@ struct bpf_sock_ops {
 				 * been written yet.
 				 */
 	__u64 skb_hwtstamp;
-};
+} __bpf_ctx;
 
 /* Definitions for bpf_sock_ops_cb_flags */
 enum {
@@ -7034,11 +7040,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 +7251,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 +7262,7 @@ struct bpf_sockopt {
 	__s32	optname;
 	__s32	optlen;
 	__s32	retval;
-};
+} __bpf_ctx;
 
 struct bpf_pidns_info {
 	__u32 pid;
@@ -7280,7 +7286,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 +7412,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..608e366877fc 100644
--- a/tools/include/uapi/linux/bpf_perf_event.h
+++ b/tools/include/uapi/linux/bpf_perf_event.h
@@ -10,10 +10,18 @@
 
 #include <asm/bpf_perf_event.h>
 
+#if __has_attribute(preserve_static_offset) && defined(__bpf__)
+#define __bpf_ctx __attribute__((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 v2 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 are used as context parameters for BPF programs. 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/20231208000531.19179-1-eddyz87@gmail.com/

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

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 91fcb75babe3..2abe71194afb 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -460,11 +460,118 @@ static void __printf(2, 0) btf_dump_printf(void *ctx,
 	vfprintf(stdout, fmt, args);
 }
 
+static const char * const context_types[] = {
+	"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_buff",
+	"sk_msg_md",
+	"sk_reuseport_md",
+	"xdp_md",
+	"pt_regs",
+};
+
+static bool is_context_type_name(const struct btf *btf, const char *name)
+{
+	__u32 i;
+
+	for (i = 0; i < ARRAY_SIZE(context_types); ++i)
+		if (strcmp(name, context_types[i]) == 0)
+			return true;
+
+	return false;
+}
+
+/* When root_type_ids == NULL represents an iterator
+ * over all type ids in BTF: [1 .. btf__type_cnt(btf)].
+ *
+ * When root_type_ids != NULL represents an iterator
+ * over all type ids in root_type_ids array.
+ */
+struct root_type_iter {
+	__u32 *root_type_ids;
+	__u32 cnt;
+	__u32 pos;
+};
+
+static struct root_type_iter make_root_type_iter(const struct btf *btf,
+						 __u32 *root_type_ids, int root_type_cnt)
+{
+	if (root_type_cnt)
+		return (struct root_type_iter) { root_type_ids, root_type_cnt, 0 };
+
+	return (struct root_type_iter) { NULL, btf__type_cnt(btf), 1 };
+}
+
+static __u32 root_type_iter_next(struct root_type_iter *iter)
+{
+	if (iter->pos >= iter->cnt)
+		return 0;
+
+	if (iter->root_type_ids)
+		return iter->root_type_ids[iter->pos++];
+
+	return iter->pos++;
+}
+
+/* Iterate all types in 'btf', if there are types with name matching
+ * name of a BPF program context parameter type - emit a forward
+ * declaration for this type annotated with preserve_static_offset
+ * attribute [0].
+ *
+ * [0] https://clang.llvm.org/docs/AttributeReference.html#preserve-static-offset
+ */
+static void emit_static_offset_protos(const struct btf *btf, struct root_type_iter iter)
+{
+	bool first = true;
+	__u32 id;
+
+	while ((id = root_type_iter_next(&iter))) {
+		const struct btf_type *t;
+		const char *name;
+
+		t = btf__type_by_id(btf, id);
+		if (!t)
+			continue;
+
+		name = btf__name_by_offset(btf, t->name_off);
+		if (!name)
+			continue;
+
+		if (!btf_is_struct(t) || !is_context_type_name(btf, 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");
+	}
+}
+
 static int dump_btf_c(const struct btf *btf,
 		      __u32 *root_type_ids, int root_type_cnt)
 {
+	struct root_type_iter iter;
 	struct btf_dump *d;
-	int err = 0, i;
+	int err = 0;
+	__u32 id;
 
 	d = btf_dump__new(btf, btf_dump_printf, NULL, NULL);
 	if (!d)
@@ -473,24 +580,18 @@ static int dump_btf_c(const struct btf *btf,
 	printf("#ifndef __VMLINUX_H__\n");
 	printf("#define __VMLINUX_H__\n");
 	printf("\n");
+
+	emit_static_offset_protos(btf, make_root_type_iter(btf, root_type_ids, root_type_cnt));
+
 	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");
 
-	if (root_type_cnt) {
-		for (i = 0; i < root_type_cnt; i++) {
-			err = btf_dump__dump_type(d, root_type_ids[i]);
-			if (err)
-				goto done;
-		}
-	} else {
-		int cnt = btf__type_cnt(btf);
-
-		for (i = 1; i < cnt; i++) {
-			err = btf_dump__dump_type(d, i);
-			if (err)
-				goto done;
-		}
+	iter = make_root_type_iter(btf, root_type_ids, root_type_cnt);
+	while ((id = root_type_iter_next(&iter))) {
+		err = btf_dump__dump_type(d, id);
+		if (err)
+			goto done;
 	}
 
 	printf("#ifndef BPF_NO_PRESERVE_ACCESS_INDEX\n");
-- 
2.42.1

[RFC v2 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.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../bpf/progs/dummy_no_context_btf.c          | 12 ++++
 .../selftests/bpf/progs/dummy_sk_buff_user.c  | 14 +++++
 tools/testing/selftests/bpf/test_bpftool.py   | 61 +++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/dummy_no_context_btf.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_sk_buff_user.c b/tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c
new file mode 100644
index 000000000000..f271881bcbd0
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/dummy_sk_buff_user.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#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..3117f431dd92 100644
--- a/tools/testing/selftests/bpf/test_bpftool.py
+++ b/tools/testing/selftests/bpf/test_bpftool.py
@@ -8,6 +8,7 @@ import os
 import socket
 import subprocess
 import unittest
+import io
 
 
 # Add the source tree of bpftool and /usr/local/sbin to PATH
@@ -25,6 +26,10 @@ class UnprivilegedUserError(Exception):
     pass
 
 
+class MissingDependencyError(Exception):
+    pass
+
+
 def _bpftool(args, json=True):
     _args = ["bpftool"]
     if json:
@@ -63,12 +68,22 @@ DMESG_EMITTING_HELPERS = [
         "bpf_trace_vprintk",
     ]
 
+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"
+
 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]
+        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 +187,49 @@ 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)
+
+    # 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.assertStringsPresent(res, [
+            "#if !defined(BPF_NO_PRESERVE_STATIC_OFFSET) && " +
+              "__has_attribute(preserve_static_offset)",
+            "#pragma clang attribute push " +
+              "(__attribute__((preserve_static_offset)), apply_to = record)",
+            "struct __sk_buff;",
+            "struct bpf_sock;",
+            "#pragma clang attribute pop",
+            "#endif /* BPF_NO_PRESERVE_STATIC_OFFSET */",
+            "#pragma clang attribute push " +
+              "(__attribute__((preserve_access_index)), apply_to = record)",
+            "struct __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;"
+        ])
-- 
2.42.1

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

[Quentin Monnet]

2023-12-12 02:32 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> When printing vmlinux.h emit attribute preserve_static_offset [0] for
> types that are used as context parameters for BPF programs. 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/20231208000531.19179-1-eddyz87@gmail.com/
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/bpf/bpftool/btf.c | 131 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 116 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 91fcb75babe3..2abe71194afb 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -460,11 +460,118 @@ static void __printf(2, 0) btf_dump_printf(void *ctx,
>  	vfprintf(stdout, fmt, args);
>  }
>  
> +static const char * const context_types[] = {
> +	"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_buff",
> +	"sk_msg_md",
> +	"sk_reuseport_md",
> +	"xdp_md",
> +	"pt_regs",
> +};

Hi, and thanks for this!

Apologies for missing the discussion on v1. Reading through the previous
thread I see that they were votes in favour of the hard-coded approach,
but I would ask folks to please reconsider.

I'm not keen on taking this list in bpftool, it doesn't seem to be the
right place for that. I understand there's no plan to add new mirror
context structs, but if we change policy for whatever reason, we're sure
to miss the update in this list and that's a bug in bpftool. If bpftool
ever gets ported to Windows and Windows needs support for new structs,
that's more juggling to do to support multiple platforms. And if any
tool other than bpftool needs to generate vmlinux.h headers in the
future, it's back to square one - although by then there might be extra
pushback for changing the BTF, if bpftool already does the work.

Like Alan, I rather share your own inclination towards the more generic
declaration tags approach, if you don't mind the additional work.

Quentin

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

[Eduard Zingerman]

On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
[...]
> Hi, and thanks for this!
> 
> Apologies for missing the discussion on v1. Reading through the previous
> thread I see that they were votes in favour of the hard-coded approach,
> but I would ask folks to please reconsider.
> 
> I'm not keen on taking this list in bpftool, it doesn't seem to be the
> right place for that. I understand there's no plan to add new mirror
> context structs, but if we change policy for whatever reason, we're sure
> to miss the update in this list and that's a bug in bpftool. If bpftool
> ever gets ported to Windows and Windows needs support for new structs,
> that's more juggling to do to support multiple platforms. And if any
> tool other than bpftool needs to generate vmlinux.h headers in the
> future, it's back to square one - although by then there might be extra
> pushback for changing the BTF, if bpftool already does the work.
> 
> Like Alan, I rather share your own inclination towards the more generic
> declaration tags approach, if you don't mind the additional work.

Understood, thank you for feedback.
The second option is to:

1. Define __bpf_ctx macro in linux headers as follows:

    #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

2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
    corresponding decl tag is present in the BTF.

2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
    types with corresponding decl tag. (Like in this patch but check
    for decl tag instead of name).

I think that 2b is better, because emitting
BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
libbpf does not emit any macro definitions at the moment.

wdyt?

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

[Quentin Monnet]

2023-12-12 15:58 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
> On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
> [...]
>> Hi, and thanks for this!
>>
>> Apologies for missing the discussion on v1. Reading through the previous
>> thread I see that they were votes in favour of the hard-coded approach,
>> but I would ask folks to please reconsider.
>>
>> I'm not keen on taking this list in bpftool, it doesn't seem to be the
>> right place for that. I understand there's no plan to add new mirror
>> context structs, but if we change policy for whatever reason, we're sure
>> to miss the update in this list and that's a bug in bpftool. If bpftool
>> ever gets ported to Windows and Windows needs support for new structs,
>> that's more juggling to do to support multiple platforms. And if any
>> tool other than bpftool needs to generate vmlinux.h headers in the
>> future, it's back to square one - although by then there might be extra
>> pushback for changing the BTF, if bpftool already does the work.
>>
>> Like Alan, I rather share your own inclination towards the more generic
>> declaration tags approach, if you don't mind the additional work.
> 
> Understood, thank you for feedback.
> The second option is to:
> 
> 1. Define __bpf_ctx macro in linux headers as follows:
> 
>     #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
> 
> 2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
>     corresponding decl tag is present in the BTF.
> 
> 2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
>     types with corresponding decl tag. (Like in this patch but check
>     for decl tag instead of name).

I don't have a strong opinion on that part, so...

> I think that 2b is better, because emitting
> BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
> BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
> libbpf does not emit any macro definitions at the moment.

... the above makes sense, I'd say let's go for this if nobody else
objects (or wants it in libbpf instead - but bpftool is fine as far as
I'm concerned).

Thanks,
Quentin

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

[Yonghong Song]

On 12/12/23 8:07 AM, Quentin Monnet wrote:
> 2023-12-12 15:58 UTC+0000 ~ Eduard Zingerman <eddyz87@gmail.com>
>> On Tue, 2023-12-12 at 11:39 +0000, Quentin Monnet wrote:
>> [...]
>>> Hi, and thanks for this!
>>>
>>> Apologies for missing the discussion on v1. Reading through the previous
>>> thread I see that they were votes in favour of the hard-coded approach,
>>> but I would ask folks to please reconsider.
>>>
>>> I'm not keen on taking this list in bpftool, it doesn't seem to be the
>>> right place for that. I understand there's no plan to add new mirror
>>> context structs, but if we change policy for whatever reason, we're sure
>>> to miss the update in this list and that's a bug in bpftool. If bpftool
>>> ever gets ported to Windows and Windows needs support for new structs,
>>> that's more juggling to do to support multiple platforms. And if any
>>> tool other than bpftool needs to generate vmlinux.h headers in the
>>> future, it's back to square one - although by then there might be extra
>>> pushback for changing the BTF, if bpftool already does the work.
>>>
>>> Like Alan, I rather share your own inclination towards the more generic
>>> declaration tags approach, if you don't mind the additional work.
>> Understood, thank you for feedback.
>> The second option is to:
>>
>> 1. Define __bpf_ctx macro in linux headers as follows:
>>
>>      #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
>>
>> 2a. Update libbpf to emit __attribute__((preserve_static_offset)) when
>>      corresponding decl tag is present in the BTF.
>>
>> 2b. Update bpftool to emit __attribute__((preserve_static_offset)) for
>>      types with corresponding decl tag. (Like in this patch but check
>>      for decl tag instead of name).
> I don't have a strong opinion on that part, so...
>
>> I think that 2b is better, because emitting
>> BPF_NO_PRESERVE_STATIC_OFFSET from the same place where
>> BPF_NO_PRESERVE_ACCESS_INDEX makes more sense,
>> libbpf does not emit any macro definitions at the moment.
> ... the above makes sense, I'd say let's go for this if nobody else
> objects (or wants it in libbpf instead - but bpftool is fine as far as
> I'm concerned).

Thanks, Eduard and Quentin, my original proposal for hardcoded list is
for simplicity. Eduard, could you help also do an implementation like
you proposed in the above so we can then compare each other?

[...]