[PATCH v3 bpf-next 0/3] Convert BPF UAPI constants into enum values

[PATCH v3 bpf-next 0/3] Convert BPF UAPI constants into enum values

[Andrii Nakryiko]

Convert BPF-related UAPI constants, currently defined as #define macro, into
anonymous enums. This has no difference in terms of usage of such constants in
C code (they are still could be used in all the compile-time contexts that
`#define`s can), but they are recorded as part of DWARF type info, and
subsequently get recorded as part of kernel's BTF type info. This allows those
constants to be emitted as part of vmlinux.h auto-generated header file and be
used from BPF programs. Which is especially convenient for all kinds of BPF
helper flags and makes CO-RE BPF programs nicer to write.

libbpf's btf_dump logic currently assumes enum values are signed 32-bit
values, but that doesn't match a typical case, so switch it to emit unsigned
values. Once BTF encoding of BTF_KIND_ENUM is extended to capture signedness
properly, this will be made more flexible.

As an immediate validation of the approach, runqslower's copy of
BPF_F_CURRENT_CPU #define is dropped in favor of its enum variant from
vmlinux.h.

v2->v3:
- convert only constants usable from BPF programs (BPF helper flags, map
  create flags, etc) (Alexei);

v1->v2:
- fix up btf_dump test to use max 32-bit unsigned value instead of negative one.


Andrii Nakryiko (3):
  bpf: switch BPF UAPI #define constants used from BPF program side to
    enums
  libbpf: assume unsigned values for BTF_KIND_ENUM
  tools/runqslower: drop copy/pasted BPF_F_CURRENT_CPU definiton

 include/uapi/linux/bpf.h                      | 175 ++++++++++-------
 tools/bpf/runqslower/runqslower.bpf.c         |   3 -
 tools/include/uapi/linux/bpf.h                | 177 +++++++++++-------
 tools/lib/bpf/btf_dump.c                      |   8 +-
 .../bpf/progs/btf_dump_test_case_syntax.c     |   2 +-
 5 files changed, 224 insertions(+), 141 deletions(-)

-- 
2.17.1

[PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Andrii Nakryiko]

Switch BPF UAPI constants, previously defined as #define macro, to anonymous
enum values. This preserves constants values and behavior in expressions, but
has added advantaged of being captured as part of DWARF and, subsequently, BTF
type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
for BPF applications, as it will not require BPF users to copy/paste various
flags and constants, which are frequently used with BPF helpers. Only those
constants that are used/useful from BPF program side are converted.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/uapi/linux/bpf.h       | 175 ++++++++++++++++++++------------
 tools/include/uapi/linux/bpf.h | 177 ++++++++++++++++++++-------------
 2 files changed, 219 insertions(+), 133 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8e98ced0963b..3ce4e8759661 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -325,44 +325,46 @@ enum bpf_attach_type {
 #define BPF_PSEUDO_CALL		1
 
 /* flags for BPF_MAP_UPDATE_ELEM command */
-#define BPF_ANY		0 /* create new element or update existing */
-#define BPF_NOEXIST	1 /* create new element if it didn't exist */
-#define BPF_EXIST	2 /* update existing element */
-#define BPF_F_LOCK	4 /* spin_lock-ed map_lookup/map_update */
+enum {
+	BPF_ANY		= 0, /* create new element or update existing */
+	BPF_NOEXIST	= 1, /* create new element if it didn't exist */
+	BPF_EXIST	= 2, /* update existing element */
+	BPF_F_LOCK	= 4, /* spin_lock-ed map_lookup/map_update */
+};
 
 /* flags for BPF_MAP_CREATE command */
-#define BPF_F_NO_PREALLOC	(1U << 0)
+enum {
+	BPF_F_NO_PREALLOC	= (1U << 0),
 /* Instead of having one common LRU list in the
  * BPF_MAP_TYPE_LRU_[PERCPU_]HASH map, use a percpu LRU list
  * which can scale and perform better.
  * Note, the LRU nodes (including free nodes) cannot be moved
  * across different LRU lists.
  */
-#define BPF_F_NO_COMMON_LRU	(1U << 1)
+	BPF_F_NO_COMMON_LRU	= (1U << 1),
 /* Specify numa node during map creation */
-#define BPF_F_NUMA_NODE		(1U << 2)
-
-#define BPF_OBJ_NAME_LEN 16U
+	BPF_F_NUMA_NODE		= (1U << 2),
 
 /* Flags for accessing BPF object from syscall side. */
-#define BPF_F_RDONLY		(1U << 3)
-#define BPF_F_WRONLY		(1U << 4)
+	BPF_F_RDONLY		= (1U << 3),
+	BPF_F_WRONLY		= (1U << 4),
 
 /* Flag for stack_map, store build_id+offset instead of pointer */
-#define BPF_F_STACK_BUILD_ID	(1U << 5)
+	BPF_F_STACK_BUILD_ID	= (1U << 5),
 
 /* Zero-initialize hash function seed. This should only be used for testing. */
-#define BPF_F_ZERO_SEED		(1U << 6)
+	BPF_F_ZERO_SEED		= (1U << 6),
 
 /* Flags for accessing BPF object from program side. */
-#define BPF_F_RDONLY_PROG	(1U << 7)
-#define BPF_F_WRONLY_PROG	(1U << 8)
+	BPF_F_RDONLY_PROG	= (1U << 7),
+	BPF_F_WRONLY_PROG	= (1U << 8),
 
 /* Clone map from listener for newly accepted socket */
-#define BPF_F_CLONE		(1U << 9)
+	BPF_F_CLONE		= (1U << 9),
 
 /* Enable memory-mapping BPF map */
-#define BPF_F_MMAPABLE		(1U << 10)
+	BPF_F_MMAPABLE		= (1U << 10),
+};
 
 /* Flags for BPF_PROG_QUERY. */
 
@@ -391,6 +393,8 @@ struct bpf_stack_build_id {
 	};
 };
 
+#define BPF_OBJ_NAME_LEN 16U
+
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
 		__u32	map_type;	/* one of enum bpf_map_type */
@@ -3045,72 +3049,100 @@ enum bpf_func_id {
 /* All flags used by eBPF helper functions, placed here. */
 
 /* BPF_FUNC_skb_store_bytes flags. */
-#define BPF_F_RECOMPUTE_CSUM		(1ULL << 0)
-#define BPF_F_INVALIDATE_HASH		(1ULL << 1)
+enum {
+	BPF_F_RECOMPUTE_CSUM		= (1ULL << 0),
+	BPF_F_INVALIDATE_HASH		= (1ULL << 1),
+};
 
 /* BPF_FUNC_l3_csum_replace and BPF_FUNC_l4_csum_replace flags.
  * First 4 bits are for passing the header field size.
  */
-#define BPF_F_HDR_FIELD_MASK		0xfULL
+enum {
+	BPF_F_HDR_FIELD_MASK		= 0xfULL,
+};
 
 /* BPF_FUNC_l4_csum_replace flags. */
-#define BPF_F_PSEUDO_HDR		(1ULL << 4)
-#define BPF_F_MARK_MANGLED_0		(1ULL << 5)
-#define BPF_F_MARK_ENFORCE		(1ULL << 6)
+enum {
+	BPF_F_PSEUDO_HDR		= (1ULL << 4),
+	BPF_F_MARK_MANGLED_0		= (1ULL << 5),
+	BPF_F_MARK_ENFORCE		= (1ULL << 6),
+};
 
 /* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
-#define BPF_F_INGRESS			(1ULL << 0)
+enum {
+	BPF_F_INGRESS			= (1ULL << 0),
+};
 
 /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key flags. */
-#define BPF_F_TUNINFO_IPV6		(1ULL << 0)
+enum {
+	BPF_F_TUNINFO_IPV6		= (1ULL << 0),
+};
 
 /* flags for both BPF_FUNC_get_stackid and BPF_FUNC_get_stack. */
-#define BPF_F_SKIP_FIELD_MASK		0xffULL
-#define BPF_F_USER_STACK		(1ULL << 8)
+enum {
+	BPF_F_SKIP_FIELD_MASK		= 0xffULL,
+	BPF_F_USER_STACK		= (1ULL << 8),
 /* flags used by BPF_FUNC_get_stackid only. */
-#define BPF_F_FAST_STACK_CMP		(1ULL << 9)
-#define BPF_F_REUSE_STACKID		(1ULL << 10)
+	BPF_F_FAST_STACK_CMP		= (1ULL << 9),
+	BPF_F_REUSE_STACKID		= (1ULL << 10),
 /* flags used by BPF_FUNC_get_stack only. */
-#define BPF_F_USER_BUILD_ID		(1ULL << 11)
+	BPF_F_USER_BUILD_ID		= (1ULL << 11),
+};
 
 /* BPF_FUNC_skb_set_tunnel_key flags. */
-#define BPF_F_ZERO_CSUM_TX		(1ULL << 1)
-#define BPF_F_DONT_FRAGMENT		(1ULL << 2)
-#define BPF_F_SEQ_NUMBER		(1ULL << 3)
+enum {
+	BPF_F_ZERO_CSUM_TX		= (1ULL << 1),
+	BPF_F_DONT_FRAGMENT		= (1ULL << 2),
+	BPF_F_SEQ_NUMBER		= (1ULL << 3),
+};
 
 /* BPF_FUNC_perf_event_output, BPF_FUNC_perf_event_read and
  * BPF_FUNC_perf_event_read_value flags.
  */
-#define BPF_F_INDEX_MASK		0xffffffffULL
-#define BPF_F_CURRENT_CPU		BPF_F_INDEX_MASK
+enum {
+	BPF_F_INDEX_MASK		= 0xffffffffULL,
+	BPF_F_CURRENT_CPU		= BPF_F_INDEX_MASK,
 /* BPF_FUNC_perf_event_output for sk_buff input context. */
-#define BPF_F_CTXLEN_MASK		(0xfffffULL << 32)
+	BPF_F_CTXLEN_MASK		= (0xfffffULL << 32),
+};
 
 /* Current network namespace */
-#define BPF_F_CURRENT_NETNS		(-1L)
+enum {
+	BPF_F_CURRENT_NETNS		= (-1L),
+};
 
 /* BPF_FUNC_skb_adjust_room flags. */
-#define BPF_F_ADJ_ROOM_FIXED_GSO	(1ULL << 0)
+enum {
+	BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
+	BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	= (1ULL << 1),
+	BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
+	BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
+	BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
+};
 
-#define BPF_ADJ_ROOM_ENCAP_L2_MASK	0xff
-#define BPF_ADJ_ROOM_ENCAP_L2_SHIFT	56
+enum {
+	BPF_ADJ_ROOM_ENCAP_L2_MASK	= 0xff,
+	BPF_ADJ_ROOM_ENCAP_L2_SHIFT	= 56,
+};
 
-#define BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	(1ULL << 1)
-#define BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	(1ULL << 2)
-#define BPF_F_ADJ_ROOM_ENCAP_L4_GRE	(1ULL << 3)
-#define BPF_F_ADJ_ROOM_ENCAP_L4_UDP	(1ULL << 4)
 #define BPF_F_ADJ_ROOM_ENCAP_L2(len)	(((__u64)len & \
 					  BPF_ADJ_ROOM_ENCAP_L2_MASK) \
 					 << BPF_ADJ_ROOM_ENCAP_L2_SHIFT)
 
 /* BPF_FUNC_sysctl_get_name flags. */
-#define BPF_F_SYSCTL_BASE_NAME		(1ULL << 0)
+enum {
+	BPF_F_SYSCTL_BASE_NAME		= (1ULL << 0),
+};
 
 /* BPF_FUNC_sk_storage_get flags */
-#define BPF_SK_STORAGE_GET_F_CREATE	(1ULL << 0)
+enum {
+	BPF_SK_STORAGE_GET_F_CREATE	= (1ULL << 0),
+};
 
 /* BPF_FUNC_read_branch_records flags. */
-#define BPF_F_GET_BRANCH_RECORDS_SIZE	(1ULL << 0)
+enum {
+	BPF_F_GET_BRANCH_RECORDS_SIZE	= (1ULL << 0),
+};
 
 /* Mode for BPF_FUNC_skb_adjust_room helper. */
 enum bpf_adj_room_mode {
@@ -3528,13 +3560,14 @@ struct bpf_sock_ops {
 };
 
 /* Definitions for bpf_sock_ops_cb_flags */
-#define BPF_SOCK_OPS_RTO_CB_FLAG	(1<<0)
-#define BPF_SOCK_OPS_RETRANS_CB_FLAG	(1<<1)
-#define BPF_SOCK_OPS_STATE_CB_FLAG	(1<<2)
-#define BPF_SOCK_OPS_RTT_CB_FLAG	(1<<3)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS       0xF		/* Mask of all currently
-							 * supported cb flags
-							 */
+enum {
+	BPF_SOCK_OPS_RTO_CB_FLAG	= (1<<0),
+	BPF_SOCK_OPS_RETRANS_CB_FLAG	= (1<<1),
+	BPF_SOCK_OPS_STATE_CB_FLAG	= (1<<2),
+	BPF_SOCK_OPS_RTT_CB_FLAG	= (1<<3),
+/* Mask of all currently supported cb flags */
+	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0xF,
+};
 
 /* List of known BPF sock_ops operators.
  * New entries can only be added at the end
@@ -3613,8 +3646,10 @@ enum {
 	BPF_TCP_MAX_STATES	/* Leave at the end! */
 };
 
-#define TCP_BPF_IW		1001	/* Set TCP initial congestion window */
-#define TCP_BPF_SNDCWND_CLAMP	1002	/* Set sndcwnd_clamp */
+enum {
+	TCP_BPF_IW		= 1001,	/* Set TCP initial congestion window */
+	TCP_BPF_SNDCWND_CLAMP	= 1002,	/* Set sndcwnd_clamp */
+};
 
 struct bpf_perf_event_value {
 	__u64 counter;
@@ -3622,12 +3657,16 @@ struct bpf_perf_event_value {
 	__u64 running;
 };
 
-#define BPF_DEVCG_ACC_MKNOD	(1ULL << 0)
-#define BPF_DEVCG_ACC_READ	(1ULL << 1)
-#define BPF_DEVCG_ACC_WRITE	(1ULL << 2)
+enum {
+	BPF_DEVCG_ACC_MKNOD	= (1ULL << 0),
+	BPF_DEVCG_ACC_READ	= (1ULL << 1),
+	BPF_DEVCG_ACC_WRITE	= (1ULL << 2),
+};
 
-#define BPF_DEVCG_DEV_BLOCK	(1ULL << 0)
-#define BPF_DEVCG_DEV_CHAR	(1ULL << 1)
+enum {
+	BPF_DEVCG_DEV_BLOCK	= (1ULL << 0),
+	BPF_DEVCG_DEV_CHAR	= (1ULL << 1),
+};
 
 struct bpf_cgroup_dev_ctx {
 	/* access_type encoded as (BPF_DEVCG_ACC_* << 16) | BPF_DEVCG_DEV_* */
@@ -3643,8 +3682,10 @@ struct bpf_raw_tracepoint_args {
 /* DIRECT:  Skip the FIB rules and go to FIB table associated with device
  * OUTPUT:  Do lookup from egress perspective; default is ingress
  */
-#define BPF_FIB_LOOKUP_DIRECT  (1U << 0)
-#define BPF_FIB_LOOKUP_OUTPUT  (1U << 1)
+enum {
+	BPF_FIB_LOOKUP_DIRECT  = (1U << 0),
+	BPF_FIB_LOOKUP_OUTPUT  = (1U << 1),
+};
 
 enum {
 	BPF_FIB_LKUP_RET_SUCCESS,      /* lookup successful */
@@ -3716,9 +3757,11 @@ enum bpf_task_fd_type {
 	BPF_FD_TYPE_URETPROBE,		/* filename + offset */
 };
 
-#define BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG		(1U << 0)
-#define BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL		(1U << 1)
-#define BPF_FLOW_DISSECTOR_F_STOP_AT_ENCAP		(1U << 2)
+enum {
+	BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG		= (1U << 0),
+	BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL		= (1U << 1),
+	BPF_FLOW_DISSECTOR_F_STOP_AT_ENCAP		= (1U << 2),
+};
 
 struct bpf_flow_keys {
 	__u16	nhoff;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 906e9f2752db..3ce4e8759661 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -73,7 +73,7 @@ struct bpf_insn {
 /* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
 struct bpf_lpm_trie_key {
 	__u32	prefixlen;	/* up to 32 for AF_INET, 128 for AF_INET6 */
-	__u8	data[0];	/* Arbitrary size */
+	__u8	data[];	/* Arbitrary size */
 };
 
 struct bpf_cgroup_storage_key {
@@ -325,44 +325,46 @@ enum bpf_attach_type {
 #define BPF_PSEUDO_CALL		1
 
 /* flags for BPF_MAP_UPDATE_ELEM command */
-#define BPF_ANY		0 /* create new element or update existing */
-#define BPF_NOEXIST	1 /* create new element if it didn't exist */
-#define BPF_EXIST	2 /* update existing element */
-#define BPF_F_LOCK	4 /* spin_lock-ed map_lookup/map_update */
+enum {
+	BPF_ANY		= 0, /* create new element or update existing */
+	BPF_NOEXIST	= 1, /* create new element if it didn't exist */
+	BPF_EXIST	= 2, /* update existing element */
+	BPF_F_LOCK	= 4, /* spin_lock-ed map_lookup/map_update */
+};
 
 /* flags for BPF_MAP_CREATE command */
-#define BPF_F_NO_PREALLOC	(1U << 0)
+enum {
+	BPF_F_NO_PREALLOC	= (1U << 0),
 /* Instead of having one common LRU list in the
  * BPF_MAP_TYPE_LRU_[PERCPU_]HASH map, use a percpu LRU list
  * which can scale and perform better.
  * Note, the LRU nodes (including free nodes) cannot be moved
  * across different LRU lists.
  */
-#define BPF_F_NO_COMMON_LRU	(1U << 1)
+	BPF_F_NO_COMMON_LRU	= (1U << 1),
 /* Specify numa node during map creation */
-#define BPF_F_NUMA_NODE		(1U << 2)
-
-#define BPF_OBJ_NAME_LEN 16U
+	BPF_F_NUMA_NODE		= (1U << 2),
 
 /* Flags for accessing BPF object from syscall side. */
-#define BPF_F_RDONLY		(1U << 3)
-#define BPF_F_WRONLY		(1U << 4)
+	BPF_F_RDONLY		= (1U << 3),
+	BPF_F_WRONLY		= (1U << 4),
 
 /* Flag for stack_map, store build_id+offset instead of pointer */
-#define BPF_F_STACK_BUILD_ID	(1U << 5)
+	BPF_F_STACK_BUILD_ID	= (1U << 5),
 
 /* Zero-initialize hash function seed. This should only be used for testing. */
-#define BPF_F_ZERO_SEED		(1U << 6)
+	BPF_F_ZERO_SEED		= (1U << 6),
 
 /* Flags for accessing BPF object from program side. */
-#define BPF_F_RDONLY_PROG	(1U << 7)
-#define BPF_F_WRONLY_PROG	(1U << 8)
+	BPF_F_RDONLY_PROG	= (1U << 7),
+	BPF_F_WRONLY_PROG	= (1U << 8),
 
 /* Clone map from listener for newly accepted socket */
-#define BPF_F_CLONE		(1U << 9)
+	BPF_F_CLONE		= (1U << 9),
 
 /* Enable memory-mapping BPF map */
-#define BPF_F_MMAPABLE		(1U << 10)
+	BPF_F_MMAPABLE		= (1U << 10),
+};
 
 /* Flags for BPF_PROG_QUERY. */
 
@@ -391,6 +393,8 @@ struct bpf_stack_build_id {
 	};
 };
 
+#define BPF_OBJ_NAME_LEN 16U
+
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
 		__u32	map_type;	/* one of enum bpf_map_type */
@@ -3045,72 +3049,100 @@ enum bpf_func_id {
 /* All flags used by eBPF helper functions, placed here. */
 
 /* BPF_FUNC_skb_store_bytes flags. */
-#define BPF_F_RECOMPUTE_CSUM		(1ULL << 0)
-#define BPF_F_INVALIDATE_HASH		(1ULL << 1)
+enum {
+	BPF_F_RECOMPUTE_CSUM		= (1ULL << 0),
+	BPF_F_INVALIDATE_HASH		= (1ULL << 1),
+};
 
 /* BPF_FUNC_l3_csum_replace and BPF_FUNC_l4_csum_replace flags.
  * First 4 bits are for passing the header field size.
  */
-#define BPF_F_HDR_FIELD_MASK		0xfULL
+enum {
+	BPF_F_HDR_FIELD_MASK		= 0xfULL,
+};
 
 /* BPF_FUNC_l4_csum_replace flags. */
-#define BPF_F_PSEUDO_HDR		(1ULL << 4)
-#define BPF_F_MARK_MANGLED_0		(1ULL << 5)
-#define BPF_F_MARK_ENFORCE		(1ULL << 6)
+enum {
+	BPF_F_PSEUDO_HDR		= (1ULL << 4),
+	BPF_F_MARK_MANGLED_0		= (1ULL << 5),
+	BPF_F_MARK_ENFORCE		= (1ULL << 6),
+};
 
 /* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
-#define BPF_F_INGRESS			(1ULL << 0)
+enum {
+	BPF_F_INGRESS			= (1ULL << 0),
+};
 
 /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key flags. */
-#define BPF_F_TUNINFO_IPV6		(1ULL << 0)
+enum {
+	BPF_F_TUNINFO_IPV6		= (1ULL << 0),
+};
 
 /* flags for both BPF_FUNC_get_stackid and BPF_FUNC_get_stack. */
-#define BPF_F_SKIP_FIELD_MASK		0xffULL
-#define BPF_F_USER_STACK		(1ULL << 8)
+enum {
+	BPF_F_SKIP_FIELD_MASK		= 0xffULL,
+	BPF_F_USER_STACK		= (1ULL << 8),
 /* flags used by BPF_FUNC_get_stackid only. */
-#define BPF_F_FAST_STACK_CMP		(1ULL << 9)
-#define BPF_F_REUSE_STACKID		(1ULL << 10)
+	BPF_F_FAST_STACK_CMP		= (1ULL << 9),
+	BPF_F_REUSE_STACKID		= (1ULL << 10),
 /* flags used by BPF_FUNC_get_stack only. */
-#define BPF_F_USER_BUILD_ID		(1ULL << 11)
+	BPF_F_USER_BUILD_ID		= (1ULL << 11),
+};
 
 /* BPF_FUNC_skb_set_tunnel_key flags. */
-#define BPF_F_ZERO_CSUM_TX		(1ULL << 1)
-#define BPF_F_DONT_FRAGMENT		(1ULL << 2)
-#define BPF_F_SEQ_NUMBER		(1ULL << 3)
+enum {
+	BPF_F_ZERO_CSUM_TX		= (1ULL << 1),
+	BPF_F_DONT_FRAGMENT		= (1ULL << 2),
+	BPF_F_SEQ_NUMBER		= (1ULL << 3),
+};
 
 /* BPF_FUNC_perf_event_output, BPF_FUNC_perf_event_read and
  * BPF_FUNC_perf_event_read_value flags.
  */
-#define BPF_F_INDEX_MASK		0xffffffffULL
-#define BPF_F_CURRENT_CPU		BPF_F_INDEX_MASK
+enum {
+	BPF_F_INDEX_MASK		= 0xffffffffULL,
+	BPF_F_CURRENT_CPU		= BPF_F_INDEX_MASK,
 /* BPF_FUNC_perf_event_output for sk_buff input context. */
-#define BPF_F_CTXLEN_MASK		(0xfffffULL << 32)
+	BPF_F_CTXLEN_MASK		= (0xfffffULL << 32),
+};
 
 /* Current network namespace */
-#define BPF_F_CURRENT_NETNS		(-1L)
+enum {
+	BPF_F_CURRENT_NETNS		= (-1L),
+};
 
 /* BPF_FUNC_skb_adjust_room flags. */
-#define BPF_F_ADJ_ROOM_FIXED_GSO	(1ULL << 0)
+enum {
+	BPF_F_ADJ_ROOM_FIXED_GSO	= (1ULL << 0),
+	BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	= (1ULL << 1),
+	BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	= (1ULL << 2),
+	BPF_F_ADJ_ROOM_ENCAP_L4_GRE	= (1ULL << 3),
+	BPF_F_ADJ_ROOM_ENCAP_L4_UDP	= (1ULL << 4),
+};
 
-#define BPF_ADJ_ROOM_ENCAP_L2_MASK	0xff
-#define BPF_ADJ_ROOM_ENCAP_L2_SHIFT	56
+enum {
+	BPF_ADJ_ROOM_ENCAP_L2_MASK	= 0xff,
+	BPF_ADJ_ROOM_ENCAP_L2_SHIFT	= 56,
+};
 
-#define BPF_F_ADJ_ROOM_ENCAP_L3_IPV4	(1ULL << 1)
-#define BPF_F_ADJ_ROOM_ENCAP_L3_IPV6	(1ULL << 2)
-#define BPF_F_ADJ_ROOM_ENCAP_L4_GRE	(1ULL << 3)
-#define BPF_F_ADJ_ROOM_ENCAP_L4_UDP	(1ULL << 4)
 #define BPF_F_ADJ_ROOM_ENCAP_L2(len)	(((__u64)len & \
 					  BPF_ADJ_ROOM_ENCAP_L2_MASK) \
 					 << BPF_ADJ_ROOM_ENCAP_L2_SHIFT)
 
 /* BPF_FUNC_sysctl_get_name flags. */
-#define BPF_F_SYSCTL_BASE_NAME		(1ULL << 0)
+enum {
+	BPF_F_SYSCTL_BASE_NAME		= (1ULL << 0),
+};
 
 /* BPF_FUNC_sk_storage_get flags */
-#define BPF_SK_STORAGE_GET_F_CREATE	(1ULL << 0)
+enum {
+	BPF_SK_STORAGE_GET_F_CREATE	= (1ULL << 0),
+};
 
 /* BPF_FUNC_read_branch_records flags. */
-#define BPF_F_GET_BRANCH_RECORDS_SIZE	(1ULL << 0)
+enum {
+	BPF_F_GET_BRANCH_RECORDS_SIZE	= (1ULL << 0),
+};
 
 /* Mode for BPF_FUNC_skb_adjust_room helper. */
 enum bpf_adj_room_mode {
@@ -3528,13 +3560,14 @@ struct bpf_sock_ops {
 };
 
 /* Definitions for bpf_sock_ops_cb_flags */
-#define BPF_SOCK_OPS_RTO_CB_FLAG	(1<<0)
-#define BPF_SOCK_OPS_RETRANS_CB_FLAG	(1<<1)
-#define BPF_SOCK_OPS_STATE_CB_FLAG	(1<<2)
-#define BPF_SOCK_OPS_RTT_CB_FLAG	(1<<3)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS       0xF		/* Mask of all currently
-							 * supported cb flags
-							 */
+enum {
+	BPF_SOCK_OPS_RTO_CB_FLAG	= (1<<0),
+	BPF_SOCK_OPS_RETRANS_CB_FLAG	= (1<<1),
+	BPF_SOCK_OPS_STATE_CB_FLAG	= (1<<2),
+	BPF_SOCK_OPS_RTT_CB_FLAG	= (1<<3),
+/* Mask of all currently supported cb flags */
+	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0xF,
+};
 
 /* List of known BPF sock_ops operators.
  * New entries can only be added at the end
@@ -3613,8 +3646,10 @@ enum {
 	BPF_TCP_MAX_STATES	/* Leave at the end! */
 };
 
-#define TCP_BPF_IW		1001	/* Set TCP initial congestion window */
-#define TCP_BPF_SNDCWND_CLAMP	1002	/* Set sndcwnd_clamp */
+enum {
+	TCP_BPF_IW		= 1001,	/* Set TCP initial congestion window */
+	TCP_BPF_SNDCWND_CLAMP	= 1002,	/* Set sndcwnd_clamp */
+};
 
 struct bpf_perf_event_value {
 	__u64 counter;
@@ -3622,12 +3657,16 @@ struct bpf_perf_event_value {
 	__u64 running;
 };
 
-#define BPF_DEVCG_ACC_MKNOD	(1ULL << 0)
-#define BPF_DEVCG_ACC_READ	(1ULL << 1)
-#define BPF_DEVCG_ACC_WRITE	(1ULL << 2)
+enum {
+	BPF_DEVCG_ACC_MKNOD	= (1ULL << 0),
+	BPF_DEVCG_ACC_READ	= (1ULL << 1),
+	BPF_DEVCG_ACC_WRITE	= (1ULL << 2),
+};
 
-#define BPF_DEVCG_DEV_BLOCK	(1ULL << 0)
-#define BPF_DEVCG_DEV_CHAR	(1ULL << 1)
+enum {
+	BPF_DEVCG_DEV_BLOCK	= (1ULL << 0),
+	BPF_DEVCG_DEV_CHAR	= (1ULL << 1),
+};
 
 struct bpf_cgroup_dev_ctx {
 	/* access_type encoded as (BPF_DEVCG_ACC_* << 16) | BPF_DEVCG_DEV_* */
@@ -3643,8 +3682,10 @@ struct bpf_raw_tracepoint_args {
 /* DIRECT:  Skip the FIB rules and go to FIB table associated with device
  * OUTPUT:  Do lookup from egress perspective; default is ingress
  */
-#define BPF_FIB_LOOKUP_DIRECT  (1U << 0)
-#define BPF_FIB_LOOKUP_OUTPUT  (1U << 1)
+enum {
+	BPF_FIB_LOOKUP_DIRECT  = (1U << 0),
+	BPF_FIB_LOOKUP_OUTPUT  = (1U << 1),
+};
 
 enum {
 	BPF_FIB_LKUP_RET_SUCCESS,      /* lookup successful */
@@ -3716,9 +3757,11 @@ enum bpf_task_fd_type {
 	BPF_FD_TYPE_URETPROBE,		/* filename + offset */
 };
 
-#define BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG		(1U << 0)
-#define BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL		(1U << 1)
-#define BPF_FLOW_DISSECTOR_F_STOP_AT_ENCAP		(1U << 2)
+enum {
+	BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG		= (1U << 0),
+	BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL		= (1U << 1),
+	BPF_FLOW_DISSECTOR_F_STOP_AT_ENCAP		= (1U << 2),
+};
 
 struct bpf_flow_keys {
 	__u16	nhoff;
-- 
2.17.1

[PATCH v3 bpf-next 2/3] libbpf: assume unsigned values for BTF_KIND_ENUM

[Andrii Nakryiko]

Currently, BTF_KIND_ENUM type doesn't record whether enum values should be
interpreted as signed or unsigned. In Linux, most enums are unsigned, though,
so interpreting them as unsigned matches real world better.

Change btf_dump test case to test maximum 32-bit value, instead of negative
value.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/btf_dump.c                                  | 8 ++++----
 .../selftests/bpf/progs/btf_dump_test_case_syntax.c       | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index bd09ed1710f1..0d43d9bb821b 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -916,13 +916,13 @@ static void btf_dump_emit_enum_def(struct btf_dump *d, __u32 id,
 			/* enumerators share namespace with typedef idents */
 			dup_cnt = btf_dump_name_dups(d, d->ident_names, name);
 			if (dup_cnt > 1) {
-				btf_dump_printf(d, "\n%s%s___%zu = %d,",
+				btf_dump_printf(d, "\n%s%s___%zu = %u,",
 						pfx(lvl + 1), name, dup_cnt,
-						(__s32)v->val);
+						(__u32)v->val);
 			} else {
-				btf_dump_printf(d, "\n%s%s = %d,",
+				btf_dump_printf(d, "\n%s%s = %u,",
 						pfx(lvl + 1), name,
-						(__s32)v->val);
+						(__u32)v->val);
 			}
 		}
 		btf_dump_printf(d, "\n%s}", pfx(lvl));
diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
index d4a02fe44a12..31975c96e2c9 100644
--- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
+++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c
@@ -13,7 +13,7 @@ enum e1 {
 
 enum e2 {
 	C = 100,
-	D = -100,
+	D = 4294967295,
 	E = 0,
 };
 
-- 
2.17.1

[PATCH v3 bpf-next 3/3] tools/runqslower: drop copy/pasted BPF_F_CURRENT_CPU definiton

[Andrii Nakryiko]

With BPF_F_CURRENT_CPU being an enum, it is now captured in vmlinux.h and is
readily usable by runqslower. So drop local copy/pasted definition in favor of
the one coming from vmlinux.h.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/bpf/runqslower/runqslower.bpf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/bpf/runqslower/runqslower.bpf.c b/tools/bpf/runqslower/runqslower.bpf.c
index 48a39f72fadf..0ba501305bad 100644
--- a/tools/bpf/runqslower/runqslower.bpf.c
+++ b/tools/bpf/runqslower/runqslower.bpf.c
@@ -6,9 +6,6 @@
 
 #define TASK_RUNNING 0
 
-#define BPF_F_INDEX_MASK		0xffffffffULL
-#define BPF_F_CURRENT_CPU		BPF_F_INDEX_MASK
-
 const volatile __u64 min_us = 0;
 const volatile pid_t targ_pid = 0;
 
-- 
2.17.1

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Daniel Borkmann]

On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> Switch BPF UAPI constants, previously defined as #define macro, to anonymous
> enum values. This preserves constants values and behavior in expressions, but
> has added advantaged of being captured as part of DWARF and, subsequently, BTF
> type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
> for BPF applications, as it will not require BPF users to copy/paste various
> flags and constants, which are frequently used with BPF helpers. Only those
> constants that are used/useful from BPF program side are converted.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Just thinking out loud, is there some way this could be resolved generically
either from compiler side or via additional tooling where this ends up as BTF
data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
header and worst case libbpf could also ship a copy of it (?), but what about
all the other things one would need to redefine e.g. for tracing? Small example
that comes to mind are all these TASK_* defines in sched.h etc, and there's
probably dozens of other similar stuff needed too depending on the particular
case; would be nice to have some generic catch-all, hmm.

> ---
>   include/uapi/linux/bpf.h       | 175 ++++++++++++++++++++------------
>   tools/include/uapi/linux/bpf.h | 177 ++++++++++++++++++++-------------
>   2 files changed, 219 insertions(+), 133 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 8e98ced0963b..3ce4e8759661 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -325,44 +325,46 @@ enum bpf_attach_type {
>   #define BPF_PSEUDO_CALL		1
>   
>   /* flags for BPF_MAP_UPDATE_ELEM command */
> -#define BPF_ANY		0 /* create new element or update existing */
> -#define BPF_NOEXIST	1 /* create new element if it didn't exist */
> -#define BPF_EXIST	2 /* update existing element */
> -#define BPF_F_LOCK	4 /* spin_lock-ed map_lookup/map_update */
> +enum {
> +	BPF_ANY		= 0, /* create new element or update existing */
> +	BPF_NOEXIST	= 1, /* create new element if it didn't exist */
> +	BPF_EXIST	= 2, /* update existing element */
> +	BPF_F_LOCK	= 4, /* spin_lock-ed map_lookup/map_update */
> +};
[...]

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Andrii Nakryiko]

On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> > Switch BPF UAPI constants, previously defined as #define macro, to anonymous
> > enum values. This preserves constants values and behavior in expressions, but
> > has added advantaged of being captured as part of DWARF and, subsequently, BTF
> > type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
> > for BPF applications, as it will not require BPF users to copy/paste various
> > flags and constants, which are frequently used with BPF helpers. Only those
> > constants that are used/useful from BPF program side are converted.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Just thinking out loud, is there some way this could be resolved generically
> either from compiler side or via additional tooling where this ends up as BTF
> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
> header and worst case libbpf could also ship a copy of it (?), but what about
> all the other things one would need to redefine e.g. for tracing? Small example
> that comes to mind are all these TASK_* defines in sched.h etc, and there's
> probably dozens of other similar stuff needed too depending on the particular
> case; would be nice to have some generic catch-all, hmm.

Enum convertion seems to be the simplest and cleanest way,
unfortunately (as far as I know). DWARF has some extensions capturing
#defines, but values are strings (and need to be parsed, which is pain
already for "1 << 1ULL"), and it's some obscure extension, not a
standard thing. I agree would be nice not to have and change all UAPI
headers for this, but I'm not aware of the solution like that.

>
> > ---
> >   include/uapi/linux/bpf.h       | 175 ++++++++++++++++++++------------
> >   tools/include/uapi/linux/bpf.h | 177 ++++++++++++++++++++-------------
> >   2 files changed, 219 insertions(+), 133 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 8e98ced0963b..3ce4e8759661 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -325,44 +325,46 @@ enum bpf_attach_type {
> >   #define BPF_PSEUDO_CALL             1
> >
> >   /* flags for BPF_MAP_UPDATE_ELEM command */
> > -#define BPF_ANY              0 /* create new element or update existing */
> > -#define BPF_NOEXIST  1 /* create new element if it didn't exist */
> > -#define BPF_EXIST    2 /* update existing element */
> > -#define BPF_F_LOCK   4 /* spin_lock-ed map_lookup/map_update */
> > +enum {
> > +     BPF_ANY         = 0, /* create new element or update existing */
> > +     BPF_NOEXIST     = 1, /* create new element if it didn't exist */
> > +     BPF_EXIST       = 2, /* update existing element */
> > +     BPF_F_LOCK      = 4, /* spin_lock-ed map_lookup/map_update */
> > +};
> [...]

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
>> > Switch BPF UAPI constants, previously defined as #define macro, to anonymous
>> > enum values. This preserves constants values and behavior in expressions, but
>> > has added advantaged of being captured as part of DWARF and, subsequently, BTF
>> > type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
>> > for BPF applications, as it will not require BPF users to copy/paste various
>> > flags and constants, which are frequently used with BPF helpers. Only those
>> > constants that are used/useful from BPF program side are converted.
>> >
>> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>
>> Just thinking out loud, is there some way this could be resolved generically
>> either from compiler side or via additional tooling where this ends up as BTF
>> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
>> header and worst case libbpf could also ship a copy of it (?), but what about
>> all the other things one would need to redefine e.g. for tracing? Small example
>> that comes to mind are all these TASK_* defines in sched.h etc, and there's
>> probably dozens of other similar stuff needed too depending on the particular
>> case; would be nice to have some generic catch-all, hmm.
>
> Enum convertion seems to be the simplest and cleanest way,
> unfortunately (as far as I know). DWARF has some extensions capturing
> #defines, but values are strings (and need to be parsed, which is pain
> already for "1 << 1ULL"), and it's some obscure extension, not a
> standard thing. I agree would be nice not to have and change all UAPI
> headers for this, but I'm not aware of the solution like that.

Since this is a UAPI header, are we sure that no userspace programs are
using these defines in #ifdefs or something like that?

-Toke

Re: [PATCH v3 bpf-next 0/3] Convert BPF UAPI constants into enum values

[Daniel Borkmann]

On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> Convert BPF-related UAPI constants, currently defined as #define macro, into
> anonymous enums. This has no difference in terms of usage of such constants in
> C code (they are still could be used in all the compile-time contexts that
> `#define`s can), but they are recorded as part of DWARF type info, and
> subsequently get recorded as part of kernel's BTF type info. This allows those
> constants to be emitted as part of vmlinux.h auto-generated header file and be
> used from BPF programs. Which is especially convenient for all kinds of BPF
> helper flags and makes CO-RE BPF programs nicer to write.
> 
> libbpf's btf_dump logic currently assumes enum values are signed 32-bit
> values, but that doesn't match a typical case, so switch it to emit unsigned
> values. Once BTF encoding of BTF_KIND_ENUM is extended to capture signedness
> properly, this will be made more flexible.
> 
> As an immediate validation of the approach, runqslower's copy of
> BPF_F_CURRENT_CPU #define is dropped in favor of its enum variant from
> vmlinux.h.
> 
> v2->v3:
> - convert only constants usable from BPF programs (BPF helper flags, map
>    create flags, etc) (Alexei);
> 
> v1->v2:
> - fix up btf_dump test to use max 32-bit unsigned value instead of negative one.
> 
> 
> Andrii Nakryiko (3):
>    bpf: switch BPF UAPI #define constants used from BPF program side to
>      enums
>    libbpf: assume unsigned values for BTF_KIND_ENUM
>    tools/runqslower: drop copy/pasted BPF_F_CURRENT_CPU definiton
> 
>   include/uapi/linux/bpf.h                      | 175 ++++++++++-------
>   tools/bpf/runqslower/runqslower.bpf.c         |   3 -
>   tools/include/uapi/linux/bpf.h                | 177 +++++++++++-------
>   tools/lib/bpf/btf_dump.c                      |   8 +-
>   .../bpf/progs/btf_dump_test_case_syntax.c     |   2 +-
>   5 files changed, 224 insertions(+), 141 deletions(-)
> 

Applied, thanks!

Re: [PATCH v3 bpf-next 0/3] Convert BPF UAPI constants into enum values

[Andrii Nakryiko]

On Wed, Mar 4, 2020 at 7:21 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> > Convert BPF-related UAPI constants, currently defined as #define macro, into
> > anonymous enums. This has no difference in terms of usage of such constants in
> > C code (they are still could be used in all the compile-time contexts that
> > `#define`s can), but they are recorded as part of DWARF type info, and
> > subsequently get recorded as part of kernel's BTF type info. This allows those
> > constants to be emitted as part of vmlinux.h auto-generated header file and be
> > used from BPF programs. Which is especially convenient for all kinds of BPF
> > helper flags and makes CO-RE BPF programs nicer to write.
> >
> > libbpf's btf_dump logic currently assumes enum values are signed 32-bit
> > values, but that doesn't match a typical case, so switch it to emit unsigned
> > values. Once BTF encoding of BTF_KIND_ENUM is extended to capture signedness
> > properly, this will be made more flexible.
> >
> > As an immediate validation of the approach, runqslower's copy of
> > BPF_F_CURRENT_CPU #define is dropped in favor of its enum variant from
> > vmlinux.h.
> >
> > v2->v3:
> > - convert only constants usable from BPF programs (BPF helper flags, map
> >    create flags, etc) (Alexei);
> >
> > v1->v2:
> > - fix up btf_dump test to use max 32-bit unsigned value instead of negative one.
> >
> >
> > Andrii Nakryiko (3):
> >    bpf: switch BPF UAPI #define constants used from BPF program side to
> >      enums
> >    libbpf: assume unsigned values for BTF_KIND_ENUM
> >    tools/runqslower: drop copy/pasted BPF_F_CURRENT_CPU definiton
> >
> >   include/uapi/linux/bpf.h                      | 175 ++++++++++-------
> >   tools/bpf/runqslower/runqslower.bpf.c         |   3 -
> >   tools/include/uapi/linux/bpf.h                | 177 +++++++++++-------
> >   tools/lib/bpf/btf_dump.c                      |   8 +-
> >   .../bpf/progs/btf_dump_test_case_syntax.c     |   2 +-
> >   5 files changed, 224 insertions(+), 141 deletions(-)
> >
>
> Applied, thanks!

Great, thanks! This is a huge usability boost for BPF programs relying
on vmlinux.h!

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Daniel Borkmann]

On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
>>>> Switch BPF UAPI constants, previously defined as #define macro, to anonymous
>>>> enum values. This preserves constants values and behavior in expressions, but
>>>> has added advantaged of being captured as part of DWARF and, subsequently, BTF
>>>> type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
>>>> for BPF applications, as it will not require BPF users to copy/paste various
>>>> flags and constants, which are frequently used with BPF helpers. Only those
>>>> constants that are used/useful from BPF program side are converted.
>>>>
>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>
>>> Just thinking out loud, is there some way this could be resolved generically
>>> either from compiler side or via additional tooling where this ends up as BTF
>>> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
>>> header and worst case libbpf could also ship a copy of it (?), but what about
>>> all the other things one would need to redefine e.g. for tracing? Small example
>>> that comes to mind are all these TASK_* defines in sched.h etc, and there's
>>> probably dozens of other similar stuff needed too depending on the particular
>>> case; would be nice to have some generic catch-all, hmm.
>>
>> Enum convertion seems to be the simplest and cleanest way,
>> unfortunately (as far as I know). DWARF has some extensions capturing
>> #defines, but values are strings (and need to be parsed, which is pain
>> already for "1 << 1ULL"), and it's some obscure extension, not a
>> standard thing. I agree would be nice not to have and change all UAPI
>> headers for this, but I'm not aware of the solution like that.
> 
> Since this is a UAPI header, are we sure that no userspace programs are
> using these defines in #ifdefs or something like that?

Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:

enum {
         FOO = 42,
//#define FOO   FOO
};

#ifndef FOO
# warning "bar"
#endif

int main(int argc, char **argv)
{
         return FOO;
}

$ gcc -Wall -O2 foo.c
foo.c:7:3: warning: #warning "bar" [-Wcpp]
     7 | # warning "bar"
       |   ^~~~~~~

Commenting #define FOO FOO back in fixes it as we discussed in v2:

$ gcc -Wall -O2 foo.c
$

There's also a flag_enum attribute, but with the experiments I tried yesterday
night I couldn't get a warning to trigger for anonymous enums at least, so that
part should be ok.

I was about to push the series out, but agree that there may be a risk for #ifndefs
in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.

Thanks,
Daniel

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Alexei Starovoitov]

On Wed, Mar 4, 2020 at 7:39 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> I was about to push the series out, but agree that there may be a risk for #ifndefs
> in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.

There is really no risk.
Let's not be paranoid about it and uglify bpf.h for no reason.

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Daniel Borkmann]

On 3/4/20 4:38 PM, Daniel Borkmann wrote:
> On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>> On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>
>>>> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
>>>>> Switch BPF UAPI constants, previously defined as #define macro, to anonymous
>>>>> enum values. This preserves constants values and behavior in expressions, but
>>>>> has added advantaged of being captured as part of DWARF and, subsequently, BTF
>>>>> type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
>>>>> for BPF applications, as it will not require BPF users to copy/paste various
>>>>> flags and constants, which are frequently used with BPF helpers. Only those
>>>>> constants that are used/useful from BPF program side are converted.
>>>>>
>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>>
>>>> Just thinking out loud, is there some way this could be resolved generically
>>>> either from compiler side or via additional tooling where this ends up as BTF
>>>> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
>>>> header and worst case libbpf could also ship a copy of it (?), but what about
>>>> all the other things one would need to redefine e.g. for tracing? Small example
>>>> that comes to mind are all these TASK_* defines in sched.h etc, and there's
>>>> probably dozens of other similar stuff needed too depending on the particular
>>>> case; would be nice to have some generic catch-all, hmm.
>>>
>>> Enum convertion seems to be the simplest and cleanest way,
>>> unfortunately (as far as I know). DWARF has some extensions capturing
>>> #defines, but values are strings (and need to be parsed, which is pain
>>> already for "1 << 1ULL"), and it's some obscure extension, not a
>>> standard thing. I agree would be nice not to have and change all UAPI
>>> headers for this, but I'm not aware of the solution like that.
>>
>> Since this is a UAPI header, are we sure that no userspace programs are
>> using these defines in #ifdefs or something like that?
> 
> Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:
> 
> enum {
>          FOO = 42,
> //#define FOO   FOO
> };
> 
> #ifndef FOO
> # warning "bar"
> #endif
> 
> int main(int argc, char **argv)
> {
>          return FOO;
> }
> 
> $ gcc -Wall -O2 foo.c
> foo.c:7:3: warning: #warning "bar" [-Wcpp]
>      7 | # warning "bar"
>        |   ^~~~~~~
> 
> Commenting #define FOO FOO back in fixes it as we discussed in v2:
> 
> $ gcc -Wall -O2 foo.c
> $
> 
> There's also a flag_enum attribute, but with the experiments I tried yesterday
> night I couldn't get a warning to trigger for anonymous enums at least, so that
> part should be ok.
> 
> I was about to push the series out, but agree that there may be a risk for #ifndefs
> in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.

I checked Cilium, LLVM, bcc, bpftrace code, and various others at least there it
seems okay with the current approach, meaning no such if{,n}def seen that would
cause a build warning. Also suricata seems to ship the BPF header itself. But
iproute2 had the following in include/bpf_util.h:

#ifndef BPF_PSEUDO_MAP_FD
# define BPF_PSEUDO_MAP_FD      1
#endif

It's still not what was converted though. I would expect risk might be rather low.
Toke, is there anything on your side affected?

Thanks,
Daniel

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Andrii Nakryiko]

On Wed, Mar 4, 2020 at 7:57 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 3/4/20 4:38 PM, Daniel Borkmann wrote:
> > On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>> On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>>
> >>>> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> >>>>> Switch BPF UAPI constants, previously defined as #define macro, to anonymous
> >>>>> enum values. This preserves constants values and behavior in expressions, but
> >>>>> has added advantaged of being captured as part of DWARF and, subsequently, BTF
> >>>>> type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
> >>>>> for BPF applications, as it will not require BPF users to copy/paste various
> >>>>> flags and constants, which are frequently used with BPF helpers. Only those
> >>>>> constants that are used/useful from BPF program side are converted.
> >>>>>
> >>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> >>>>
> >>>> Just thinking out loud, is there some way this could be resolved generically
> >>>> either from compiler side or via additional tooling where this ends up as BTF
> >>>> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
> >>>> header and worst case libbpf could also ship a copy of it (?), but what about
> >>>> all the other things one would need to redefine e.g. for tracing? Small example
> >>>> that comes to mind are all these TASK_* defines in sched.h etc, and there's
> >>>> probably dozens of other similar stuff needed too depending on the particular
> >>>> case; would be nice to have some generic catch-all, hmm.
> >>>
> >>> Enum convertion seems to be the simplest and cleanest way,
> >>> unfortunately (as far as I know). DWARF has some extensions capturing
> >>> #defines, but values are strings (and need to be parsed, which is pain
> >>> already for "1 << 1ULL"), and it's some obscure extension, not a
> >>> standard thing. I agree would be nice not to have and change all UAPI
> >>> headers for this, but I'm not aware of the solution like that.
> >>
> >> Since this is a UAPI header, are we sure that no userspace programs are
> >> using these defines in #ifdefs or something like that?
> >
> > Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:
> >
> > enum {
> >          FOO = 42,
> > //#define FOO   FOO
> > };
> >
> > #ifndef FOO
> > # warning "bar"
> > #endif
> >
> > int main(int argc, char **argv)
> > {
> >          return FOO;
> > }
> >
> > $ gcc -Wall -O2 foo.c
> > foo.c:7:3: warning: #warning "bar" [-Wcpp]
> >      7 | # warning "bar"
> >        |   ^~~~~~~
> >
> > Commenting #define FOO FOO back in fixes it as we discussed in v2:
> >
> > $ gcc -Wall -O2 foo.c
> > $
> >
> > There's also a flag_enum attribute, but with the experiments I tried yesterday
> > night I couldn't get a warning to trigger for anonymous enums at least, so that
> > part should be ok.
> >
> > I was about to push the series out, but agree that there may be a risk for #ifndefs
> > in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.
>
> I checked Cilium, LLVM, bcc, bpftrace code, and various others at least there it
> seems okay with the current approach, meaning no such if{,n}def seen that would
> cause a build warning. Also suricata seems to ship the BPF header itself. But
> iproute2 had the following in include/bpf_util.h:
>
> #ifndef BPF_PSEUDO_MAP_FD
> # define BPF_PSEUDO_MAP_FD      1
> #endif

This actually would still work, even if BPF_PSEUDO_MAP_FD was
converted to enum (it would just ignore enum value and hard-code it to
1).

>
> It's still not what was converted though. I would expect risk might be rather low.
> Toke, is there anything on your side affected?
>
> Thanks,
> Daniel

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Daniel Borkmann]

On 3/4/20 4:50 PM, Alexei Starovoitov wrote:
> On Wed, Mar 4, 2020 at 7:39 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> I was about to push the series out, but agree that there may be a risk for #ifndefs
>> in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.
> 
> There is really no risk.
> Let's not be paranoid about it and uglify bpf.h for no reason.

 From what I've seen it seems so, yes. I've pushed the series now. Worst case we can
always add the define FOO FOO before it's exposed.

Thanks,
Daniel

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Alexei Starovoitov]

On Wed, Mar 04, 2020 at 04:57:46PM +0100, Daniel Borkmann wrote:
> On 3/4/20 4:38 PM, Daniel Borkmann wrote:
> > On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
> > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> > > > On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > > 
> > > > > On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
> > > > > > Switch BPF UAPI constants, previously defined as #define macro, to anonymous
> > > > > > enum values. This preserves constants values and behavior in expressions, but
> > > > > > has added advantaged of being captured as part of DWARF and, subsequently, BTF
> > > > > > type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
> > > > > > for BPF applications, as it will not require BPF users to copy/paste various
> > > > > > flags and constants, which are frequently used with BPF helpers. Only those
> > > > > > constants that are used/useful from BPF program side are converted.
> > > > > > 
> > > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > > 
> > > > > Just thinking out loud, is there some way this could be resolved generically
> > > > > either from compiler side or via additional tooling where this ends up as BTF
> > > > > data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
> > > > > header and worst case libbpf could also ship a copy of it (?), but what about
> > > > > all the other things one would need to redefine e.g. for tracing? Small example
> > > > > that comes to mind are all these TASK_* defines in sched.h etc, and there's
> > > > > probably dozens of other similar stuff needed too depending on the particular
> > > > > case; would be nice to have some generic catch-all, hmm.
> > > > 
> > > > Enum convertion seems to be the simplest and cleanest way,
> > > > unfortunately (as far as I know). DWARF has some extensions capturing
> > > > #defines, but values are strings (and need to be parsed, which is pain
> > > > already for "1 << 1ULL"), and it's some obscure extension, not a
> > > > standard thing. I agree would be nice not to have and change all UAPI
> > > > headers for this, but I'm not aware of the solution like that.
> > > 
> > > Since this is a UAPI header, are we sure that no userspace programs are
> > > using these defines in #ifdefs or something like that?
> > 
> > Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:
> > 
> > enum {
> >          FOO = 42,
> > //#define FOO   FOO
> > };
> > 
> > #ifndef FOO
> > # warning "bar"
> > #endif
> > 
> > int main(int argc, char **argv)
> > {
> >          return FOO;
> > }
> > 
> > $ gcc -Wall -O2 foo.c
> > foo.c:7:3: warning: #warning "bar" [-Wcpp]
> >      7 | # warning "bar"
> >        |   ^~~~~~~
> > 
> > Commenting #define FOO FOO back in fixes it as we discussed in v2:
> > 
> > $ gcc -Wall -O2 foo.c
> > $
> > 
> > There's also a flag_enum attribute, but with the experiments I tried yesterday
> > night I couldn't get a warning to trigger for anonymous enums at least, so that
> > part should be ok.
> > 
> > I was about to push the series out, but agree that there may be a risk for #ifndefs
> > in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.
> 
> I checked Cilium, LLVM, bcc, bpftrace code, and various others at least there it
> seems okay with the current approach, meaning no such if{,n}def seen that would
> cause a build warning. Also suricata seems to ship the BPF header itself. But
> iproute2 had the following in include/bpf_util.h:
> 
> #ifndef BPF_PSEUDO_MAP_FD
> # define BPF_PSEUDO_MAP_FD      1
> #endif

Consider that users can do all sorts of stupid things with uapi headers like:
#if BPF_OBJ_NAME_LEN == 16
int foo;
#else
int bar;
#endif

Does that mean that we cannnot change any #define ever?
Of course not.

Consider that #define A A
is also broken in such cases:

For example:
enum {
        A = 1
#define A A
};
#if A == 1
int foo;
#else
int bar;
#endif

Will give different 'int' variable vs:

#define A 1
#if A == 1
int foo;
#else
int bar;
#endif

So ? Let's paralyze the development because of crazy users? No.

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 3/4/20 4:38 PM, Daniel Borkmann wrote:
>> On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>>> On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>
>>>>> On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
>>>>>> Switch BPF UAPI constants, previously defined as #define macro, to anonymous
>>>>>> enum values. This preserves constants values and behavior in expressions, but
>>>>>> has added advantaged of being captured as part of DWARF and, subsequently, BTF
>>>>>> type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
>>>>>> for BPF applications, as it will not require BPF users to copy/paste various
>>>>>> flags and constants, which are frequently used with BPF helpers. Only those
>>>>>> constants that are used/useful from BPF program side are converted.
>>>>>>
>>>>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>>>>
>>>>> Just thinking out loud, is there some way this could be resolved generically
>>>>> either from compiler side or via additional tooling where this ends up as BTF
>>>>> data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
>>>>> header and worst case libbpf could also ship a copy of it (?), but what about
>>>>> all the other things one would need to redefine e.g. for tracing? Small example
>>>>> that comes to mind are all these TASK_* defines in sched.h etc, and there's
>>>>> probably dozens of other similar stuff needed too depending on the particular
>>>>> case; would be nice to have some generic catch-all, hmm.
>>>>
>>>> Enum convertion seems to be the simplest and cleanest way,
>>>> unfortunately (as far as I know). DWARF has some extensions capturing
>>>> #defines, but values are strings (and need to be parsed, which is pain
>>>> already for "1 << 1ULL"), and it's some obscure extension, not a
>>>> standard thing. I agree would be nice not to have and change all UAPI
>>>> headers for this, but I'm not aware of the solution like that.
>>>
>>> Since this is a UAPI header, are we sure that no userspace programs are
>>> using these defines in #ifdefs or something like that?
>> 
>> Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:
>> 
>> enum {
>>          FOO = 42,
>> //#define FOO   FOO
>> };
>> 
>> #ifndef FOO
>> # warning "bar"
>> #endif
>> 
>> int main(int argc, char **argv)
>> {
>>          return FOO;
>> }
>> 
>> $ gcc -Wall -O2 foo.c
>> foo.c:7:3: warning: #warning "bar" [-Wcpp]
>>      7 | # warning "bar"
>>        |   ^~~~~~~
>> 
>> Commenting #define FOO FOO back in fixes it as we discussed in v2:
>> 
>> $ gcc -Wall -O2 foo.c
>> $
>> 
>> There's also a flag_enum attribute, but with the experiments I tried yesterday
>> night I couldn't get a warning to trigger for anonymous enums at least, so that
>> part should be ok.
>> 
>> I was about to push the series out, but agree that there may be a risk for #ifndefs
>> in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.
>
> I checked Cilium, LLVM, bcc, bpftrace code, and various others at least there it
> seems okay with the current approach, meaning no such if{,n}def seen that would
> cause a build warning. Also suricata seems to ship the BPF header itself. But
> iproute2 had the following in include/bpf_util.h:
>
> #ifndef BPF_PSEUDO_MAP_FD
> # define BPF_PSEUDO_MAP_FD      1
> #endif
>
> It's still not what was converted though. I would expect risk might be
> rather low.

OK, fair enough; thank you for checking :)

> Toke, is there anything on your side affected?

Nope, we're not #if-testing for any of the variables changed in this
patch in either xdp-tutorial or xdp-tools.

-Toke

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Michael Forney]

Hi,

On 2020-03-04, Daniel Borkmann <daniel@iogearbox.net> wrote:
> I was about to push the series out, but agree that there may be a risk for
> #ifndefs
> in the BPF C code. If we want to be on safe side, #define FOO FOO would be
> needed.

I did indeed hit some breakage due to this change, but not for the
anticipated reason.

The C standard requires that enumeration constants be representable as
an int, and have type int. While it is a common extension to allow
constants that exceed the limits of int, and this is required
elsewhere in Linux UAPI headers, this is the first case I've
encountered where the constant is not representable as unsigned int
either:

	enum {
		BPF_F_CTXLEN_MASK		= (0xfffffULL << 32),
	};

To see why this can be problematic, consider the following program:

	#include <stdio.h>
	
	enum {
		A = 1,
		B = 0x80000000,
		C = 1ULL << 32,
	
		A1 = sizeof(A),
		B1 = sizeof(B),
	};
	
	enum {
		A2 = sizeof(A),
		B2 = sizeof(B),
	};
	
	int main(void) {
		printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
		printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
	}

You might be surprised by the output:

	sizeof(A) = 4, 4
	sizeof(B) = 4, 8

This is because the type of B is different inside and outside the
enum. In my C compiler, I have implemented the extension only for
constants that fit in unsigned int to avoid these confusing semantics.

Since BPF_F_CTXLEN_MASK is the only offending constant, is it possible
to restore its definition as a macro?

Also, I'm not sure if it was considered, but using enums also changes
the signedness of these constants. Many of the previous macro
expressions had type unsigned long long, and now they have type int
(the type of the expression specifying the constant value does not
matter). I could see this causing problems if these constants are used
in expressions involving shifts or implicit conversions.

Thanks for your time,

-Michael

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Alexei Starovoitov]

On Mon, Jun 01, 2020 at 10:31:34PM -0700, Michael Forney wrote:
> Hi,
> 
> On 2020-03-04, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > I was about to push the series out, but agree that there may be a risk for
> > #ifndefs
> > in the BPF C code. If we want to be on safe side, #define FOO FOO would be
> > needed.
> 
> I did indeed hit some breakage due to this change, but not for the
> anticipated reason.
> 
> The C standard requires that enumeration constants be representable as
> an int, and have type int. While it is a common extension to allow
> constants that exceed the limits of int, and this is required
> elsewhere in Linux UAPI headers, this is the first case I've
> encountered where the constant is not representable as unsigned int
> either:
> 
> 	enum {
> 		BPF_F_CTXLEN_MASK		= (0xfffffULL << 32),
> 	};
> 
> To see why this can be problematic, consider the following program:
> 
> 	#include <stdio.h>
> 	
> 	enum {
> 		A = 1,
> 		B = 0x80000000,
> 		C = 1ULL << 32,
> 	
> 		A1 = sizeof(A),
> 		B1 = sizeof(B),
> 	};
> 	
> 	enum {
> 		A2 = sizeof(A),
> 		B2 = sizeof(B),
> 	};
> 	
> 	int main(void) {
> 		printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
> 		printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
> 	}
> 
> You might be surprised by the output:
> 
> 	sizeof(A) = 4, 4
> 	sizeof(B) = 4, 8
> 
> This is because the type of B is different inside and outside the
> enum. In my C compiler, I have implemented the extension only for
> constants that fit in unsigned int to avoid these confusing semantics.
> 
> Since BPF_F_CTXLEN_MASK is the only offending constant, is it possible
> to restore its definition as a macro?

It's possible, but I'm not sure what it will fix.
Your example is a bit misleading, since it's talking about B
which doesn't have type specifier, whereas enums in bpf.h have ULL
suffix where necessary.
And the one you pointed out BPF_F_CTXLEN_MASK has sizeof == 8 in all cases.

Also when B is properly annotated like 0x80000000ULL it will have size 8
as well.

#include <stdio.h>

enum {
        A = 1,
        B = 0x80000000,
        C = 1ULL << 32,
        D = 0x80000000ULL,

        A1 = sizeof(A),
        B1 = sizeof(B),
        C1 = sizeof(C),
        D1 = sizeof(D),
};

enum {
        A2 = sizeof(A),
        B2 = sizeof(B),
        C2 = sizeof(C),
        D2 = sizeof(D),
};

int main(void) {
        printf("sizeof(A) = %d, %d\n", (int)A1, (int)A2);
        printf("sizeof(B) = %d, %d\n", (int)B1, (int)B2);
        printf("sizeof(C) = %d, %d\n", (int)C1, (int)C2);
        printf("sizeof(D) = %d, %d\n", (int)D1, (int)D2);
}

sizeof(A) = 4, 4
sizeof(B) = 4, 8
sizeof(C) = 8, 8
sizeof(D) = 8, 8

So the problem is only with non-annotated enums that are mixed
in a enum with some values <32bit and others >32 bit.
bpf.h has only one such enum:
enum {
        BPF_F_INDEX_MASK                = 0xffffffffULL,
        BPF_F_CURRENT_CPU               = BPF_F_INDEX_MASK,
        BPF_F_CTXLEN_MASK               = (0xfffffULL << 32),
};

and all values are annotated with ULL.
So I really don't see a problem.

> Also, I'm not sure if it was considered, but using enums also changes
> the signedness of these constants. Many of the previous macro
> expressions had type unsigned long long, and now they have type int
> (the type of the expression specifying the constant value does not
> matter). I could see this causing problems if these constants are used
> in expressions involving shifts or implicit conversions.

It would have been if the enums were not annotated. But that's not the case. 

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Michael Forney]

On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> It's possible, but I'm not sure what it will fix.
> Your example is a bit misleading, since it's talking about B
> which doesn't have type specifier, whereas enums in bpf.h have ULL
> suffix where necessary.
> And the one you pointed out BPF_F_CTXLEN_MASK has sizeof == 8 in all cases.

Apologies if I wasn't clear, I was just trying to explain why this C
extension can have confusing semantics where the type of an enum
constant depends on where it is used. You're right that it doesn't
happen in this particular case.

The breakage appears with my C compiler, which as I mentioned, only
implements the extension when the enum constants fit into unsigned int
to avoid these problems.

$ cproc -x c -c - -o /dev/null <<EOF
> #include <linux/bpf.h>
> EOF
<stdin>:420:41: error: enumerator 'BPF_F_CTXLEN_MASK' value cannot be
represented as 'int' or 'unsigned int'
cproc: compile: process 3772 exited with status 1
cproc: preprocess: process signaled: Terminated
cproc: assemble: process signaled: Terminated
$

Since the Linux UAPI headers may be used with a variety of compilers,
I think it's important to stick to the standard as much as possible.
BPF_F_CTXLEN_MASK is the only enum constant I've encountered in the
Linux UAPI that has a value outside the range of unsigned int.

> Also when B is properly annotated like 0x80000000ULL it will have size 8
> as well.

Even with a suffixed integer literal, it still may be the case that an
annotated constant has a different type inside and outside the enum.

For example, in

	enum {
		A = 0x80000000ULL,
		S1 = sizeof(A),
	};
	enum {
		S2 = sizeof(A),
	};

we have S1 == 8 and S2 == 4.

>> Also, I'm not sure if it was considered, but using enums also changes
>> the signedness of these constants. Many of the previous macro
>> expressions had type unsigned long long, and now they have type int
>> (the type of the expression specifying the constant value does not
>> matter). I could see this causing problems if these constants are used
>> in expressions involving shifts or implicit conversions.
>
> It would have been if the enums were not annotated. But that's not the case.

The type of the expression has no relation to the type of the constant
outside the enum. Take this example from bpf.h:

	enum {
		BPF_DEVCG_DEV_BLOCK     = (1ULL << 0),
	 	BPF_DEVCG_DEV_CHAR      = (1ULL << 1),
	};

Previously, with the defines, they had type unsigned long long. Now,
they have type int. sizeof(BPF_DEVCG_DEV_BLOCK) == 4 and
-BPF_DEVCG_DEV_BLOCK < 0 == 1.

-Michael

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Alexei Starovoitov]

On Tue, Jun 02, 2020 at 02:40:52PM -0700, Michael Forney wrote:
> On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > It's possible, but I'm not sure what it will fix.
> > Your example is a bit misleading, since it's talking about B
> > which doesn't have type specifier, whereas enums in bpf.h have ULL
> > suffix where necessary.
> > And the one you pointed out BPF_F_CTXLEN_MASK has sizeof == 8 in all cases.
> 
> Apologies if I wasn't clear, I was just trying to explain why this C
> extension can have confusing semantics where the type of an enum
> constant depends on where it is used. You're right that it doesn't
> happen in this particular case.
> 
> The breakage appears with my C compiler, which as I mentioned, only
> implements the extension when the enum constants fit into unsigned int
> to avoid these problems.
> 
> $ cproc -x c -c - -o /dev/null <<EOF
> > #include <linux/bpf.h>
> > EOF
> <stdin>:420:41: error: enumerator 'BPF_F_CTXLEN_MASK' value cannot be
> represented as 'int' or 'unsigned int'
> cproc: compile: process 3772 exited with status 1
> cproc: preprocess: process signaled: Terminated
> cproc: assemble: process signaled: Terminated
> $
> 
> Since the Linux UAPI headers may be used with a variety of compilers,
> I think it's important to stick to the standard as much as possible.
> BPF_F_CTXLEN_MASK is the only enum constant I've encountered in the
> Linux UAPI that has a value outside the range of unsigned int.

the enum definition of BPF_F_CTXLEN_MASK is certainly within standard.
I don't think kernel should adjust its headers because some compiler
is failing to understand C standard.

> > Also when B is properly annotated like 0x80000000ULL it will have size 8
> > as well.
> 
> Even with a suffixed integer literal, it still may be the case that an
> annotated constant has a different type inside and outside the enum.
> 
> For example, in
> 
> 	enum {
> 		A = 0x80000000ULL,
> 		S1 = sizeof(A),
> 	};
> 	enum {
> 		S2 = sizeof(A),
> 	};
> 
> we have S1 == 8 and S2 == 4.

correct, because enum needs to fit all of its constants.
It's not at all related to size.
sizeof() is an expression that is being evulated in the context and
affects the size of enum.
It could have been any other math operation on constants known
at compile time.

> 
> >> Also, I'm not sure if it was considered, but using enums also changes
> >> the signedness of these constants. Many of the previous macro
> >> expressions had type unsigned long long, and now they have type int
> >> (the type of the expression specifying the constant value does not
> >> matter). I could see this causing problems if these constants are used
> >> in expressions involving shifts or implicit conversions.
> >
> > It would have been if the enums were not annotated. But that's not the case.
> 
> The type of the expression has no relation to the type of the constant
> outside the enum. Take this example from bpf.h:
> 
> 	enum {
> 		BPF_DEVCG_DEV_BLOCK     = (1ULL << 0),
> 	 	BPF_DEVCG_DEV_CHAR      = (1ULL << 1),
> 	};
> 
> Previously, with the defines, they had type unsigned long long. Now,
> they have type int. sizeof(BPF_DEVCG_DEV_BLOCK) == 4 and
> -BPF_DEVCG_DEV_BLOCK < 0 == 1.

and I still don't see how it breaks anything.
If it was #define and .h switched its definition from 1 to 1ULL
it would have had the same effect. That is the point of the constant in .h.
Same effect regardless whether it was done via #define and via enum.
The size and type of the constant may change. It's not uapi breakage.

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Michael Forney]

On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> the enum definition of BPF_F_CTXLEN_MASK is certainly within standard.
> I don't think kernel should adjust its headers because some compiler
> is failing to understand C standard.

This is not true. See C11 6.7.2.2p2: "The expression that defines the
value of an enumeration constant shall be an integer constant
expression that has a value representable as an int."

You can also see this with gcc if you turn on -Wpedantic and include
it in a way such that warnings are not silenced:

$ gcc -Wpedantic -x c -c -o /dev/null /usr/include/linux/bpf.h
/usr/include/linux/bpf.h:76:7: warning: ISO C forbids zero-size array
'data' [-Wpedantic]
   76 |  __u8 data[0]; /* Arbitrary size */
      |       ^~~~
/usr/include/linux/bpf.h:3220:22: warning: ISO C restricts enumerator
values to range of 'int' [-Wpedantic]
 3220 |  BPF_F_INDEX_MASK  = 0xffffffffULL,
      |                      ^~~~~~~~~~~~~
/usr/include/linux/bpf.h:3221:23: warning: ISO C restricts enumerator
values to range of 'int' [-Wpedantic]
 3221 |  BPF_F_CURRENT_CPU  = BPF_F_INDEX_MASK,
      |                       ^~~~~~~~~~~~~~~~
/usr/include/linux/bpf.h:3223:23: warning: ISO C restricts enumerator
values to range of 'int' [-Wpedantic]
 3223 |  BPF_F_CTXLEN_MASK  = (0xfffffULL << 32),
      |                       ^
/usr/include/linux/bpf.h:3797:8: warning: ISO C forbids zero-size
array 'args' [-Wpedantic]
 3797 |  __u64 args[0];
      |        ^~~~
$

-Michael

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Alexei Starovoitov]

On Tue, Jun 2, 2020 at 4:21 PM Michael Forney <mforney@mforney.org> wrote:
>
> On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > the enum definition of BPF_F_CTXLEN_MASK is certainly within standard.
> > I don't think kernel should adjust its headers because some compiler
> > is failing to understand C standard.
>
> This is not true. See C11 6.7.2.2p2: "The expression that defines the
> value of an enumeration constant shall be an integer constant
> expression that has a value representable as an int."
>
> You can also see this with gcc if you turn on -Wpedantic and include
> it in a way such that warnings are not silenced:
>
> $ gcc -Wpedantic -x c -c -o /dev/null /usr/include/linux/bpf.h

ISO C forbids zero-size arrays, unnamed struct/union, gcc extensions,
empty unions, etc
So ?

warning: ISO C forbids zero-size array ‘args’ [-Wpedantic]
 4095 |  __u64 args[0];
 warning: ISO C90 doesn’t support unnamed structs/unions [-Wpedantic]
 3795 |  __bpf_md_ptr(void *, data_end);

#define BPF_F_CTXLEN_MASK BPF_F_CTXLEN_MASK
will only work as workaround for _your_ compiler.
We are not gonna add hacks like this for every compiler.

Re: [PATCH v3 bpf-next 1/3] bpf: switch BPF UAPI #define constants used from BPF program side to enums

[Michael Forney]

On 2020-06-02, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> ISO C forbids zero-size arrays, unnamed struct/union, gcc extensions,
> empty unions, etc
> So ?

So their use should be avoided in UAPI headers whenever possible.
While the other extensions are simple and have clear semantics, enums
with out-of-range values do not. Even gcc and clang don't agree on
what the types of out-of-range constants are within the enum
specifier:

	enum { A = 0x80000000ULL, is_clang =
__builtin_types_compatible_p(__typeof__(A), unsigned long long) };

On gcc x86_64, is_clang == 0. On clang x86_64, is_clang == 1.

> #define BPF_F_CTXLEN_MASK BPF_F_CTXLEN_MASK
> will only work as workaround for _your_ compiler.
> We are not gonna add hacks like this for every compiler.

This doesn't solve the problem, which is that after preprocessing,
BPF_F_INDEX_MASK and BPF_F_CURRENT_CPU, and BPF_F_CTXLEN_MASK are enum
constants with values that can't be represented as int. Changing them
back to defines will. This is not a hack, it is following the C
standard.

> and I still don't see how it breaks anything.
> If it was #define and .h switched its definition from 1 to 1ULL
> it would have had the same effect. That is the point of the constant in .h.
> Same effect regardless whether it was done via #define and via enum.
> The size and type of the constant may change. It's not uapi breakage.

My point is that something like

	unsigned long long x = BPF_DEVCG_DEV_BLOCK << 32;

used to be perfectly fine in Linux 5.6. Now, in 5.7 with the enum
definition, it is undefined behavior.

-Michael