Re: [PATCH bpf-next 1/1] bpf: Mark virtual BPF context structures as preserve_static_offset

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

On 12/7/23 4:05 PM, Eduard Zingerman wrote:
> Add __attribute__((preserve_static_offset)) for the following BPF
> related structures:
> - __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
>
> Access to these structures 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. __attribute__((preserve_static_offset))
> is a hint to clang that ensures that constant offsets are used.
> (See https://reviews.llvm.org/D133361 for details).
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   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(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index e0545201b55f..75eee56ed732 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,7 +7040,7 @@ struct bpf_cgroup_dev_ctx {
>   	__u32 access_type;
>   	__u32 major;
>   	__u32 minor;
> -};
> +} __bpf_ctx;
>   
>   struct bpf_raw_tracepoint_args {
>   	__u64 args[0];
> @@ -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;

should we undef __bpf_ctx at the end of the file?
The same for below bpf_perf_event.h file.

>   
>   /*
>    * struct btf_ptr is used for typed pointer representation; the
> diff --git a/include/uapi/linux/bpf_perf_event.h b/include/uapi/linux/bpf_perf_event.h
> index eb1b9d21250c..cf614ddf0381 100644
> --- a/include/uapi/linux/bpf_perf_event.h
> +++ b/include/uapi/linux/bpf_perf_event.h
> @@ -10,10 +10,16 @@
>   
>   #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;
>   
>   #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..75eee56ed732 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h

[...]