[RFC 0/9] uapi/bpf.h for robots

[RFC 0/9] uapi/bpf.h for robots

[Lorenz Bauer]

Hi,

I recently presented at LPC 2021 [1] on problems I encountered when
generating bindings from bpf.h. I proposed the following changes:

* Use enums instead of macros
* Use __bpf_md_ptr throughout
* Give types / fields in bpf_attr a name
* Have one bpf_attr field per bpf_cmd

As David Miller pointed out, changing a macro definition to an enum
is a breaking change if users use the C preprocessor for conditional
compilation. Andrii has made a similar change in commit 1aae4bdd7879
("bpf: Switch BPF UAPI #define constants used from BPF program side to enums")
where he simply converted to enum. I'm doing the same here.

Next is using __bpf_md_ptr, which seems to work quite well. We can even add
const qualifiers. A minor nit is that we have to give the unnamed field
generated by __bpf_md_ptr a name, otherwise we can't refer to it on the
kernel side to get a __user pointer. Is there a better way? If not, is
there a better naming scheme?

Giving types and fields a name in bpf_attr goes hand in hand with having
one field per bpf_cmd in bpf_attr. This means that kernel-side code would
become more verbose:

    int ufd = attr->map_fd;

    vs.

    int ufd = attr->map_create.map_fd;

Not great. The solution I've prototyped is that we convert from bpf_attr
to e.g. struct bpf_map_create_attr as early as possible. This is made
easier by a new macro CHECK_ATTR_TAIL which allows us to remove the
XXX_LAST_FIELD macros. There are a couple places where I cheat and
cast back to union bpf_attr: in the real series more function signatures
would need changing, which will cause a ripple effect.

Finally, I convert one function in libbpf to use one of the new types
to show what the changes look like from the libbpf side. It also ensures that
all other syscall wrappers continue to compile unchanged.

So, what does everyone think?

1: https://linuxplumbersconf.org/event/11/contributions/937/

Lorenz Bauer (9):
  bpf: name enums used from userspace
  bpf: various constants
  bpf: move up __bpf_md_ptr
  bpf: name __u64 member of __bpf_md_ptr
  bpf: introduce CHECK_ATTR_TAIL
  bpf: struct bpf_map_create_attr
  bpf: split map modification structs
  selftests: sync bpf.h
  libbpf: use new-style syscall args

 include/linux/bpf.h            |   4 +-
 include/uapi/linux/bpf.h       | 200 ++++++++++++++++++++++-----------
 kernel/bpf/syscall.c           |  84 ++++++--------
 tools/include/uapi/linux/bpf.h | 200 ++++++++++++++++++++++-----------
 tools/lib/bpf/bpf.c            |  13 +--
 5 files changed, 309 insertions(+), 192 deletions(-)

-- 
2.30.2

[RFC 1/9] bpf: name enums used from userspace

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 58 +++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6fc59d61937a..78b532d28761 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -50,7 +50,7 @@
 #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
 
 /* Register numbers */
-enum {
+enum bpf_reg {
 	BPF_REG_0 = 0,
 	BPF_REG_1,
 	BPF_REG_2,
@@ -1056,16 +1056,19 @@ enum bpf_link_type {
  * All eligible programs are executed regardless of return code from
  * earlier programs.
  */
-#define BPF_F_ALLOW_OVERRIDE	(1U << 0)
-#define BPF_F_ALLOW_MULTI	(1U << 1)
-#define BPF_F_REPLACE		(1U << 2)
+enum bpf_prog_attach_flag {
+	BPF_F_ALLOW_OVERRIDE	= (1U << 0),
+	BPF_F_ALLOW_MULTI	= (1U << 1),
+	BPF_F_REPLACE		= (1U << 2),
+};
 
+enum bpf_prog_load_flag {
 /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
  * verifier will perform strict alignment checking as if the kernel
  * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
  * and NET_IP_ALIGN defined to 2.
  */
-#define BPF_F_STRICT_ALIGNMENT	(1U << 0)
+	BPF_F_STRICT_ALIGNMENT = (1U << 0),
 
 /* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the
  * verifier will allow any alignment whatsoever.  On platforms
@@ -1079,7 +1082,7 @@ enum bpf_link_type {
  * of an unaligned access the alignment check would trigger before
  * the one we are interested in.
  */
-#define BPF_F_ANY_ALIGNMENT	(1U << 1)
+	BPF_F_ANY_ALIGNMENT = (1U << 1),
 
 /* BPF_F_TEST_RND_HI32 is used in BPF_PROG_LOAD command for testing purpose.
  * Verifier does sub-register def/use analysis and identifies instructions whose
@@ -1097,10 +1100,10 @@ enum bpf_link_type {
  * Then, if verifier is not doing correct analysis, such randomization will
  * regress tests to expose bugs.
  */
-#define BPF_F_TEST_RND_HI32	(1U << 2)
+	BPF_F_TEST_RND_HI32 = (1U << 2),
 
 /* The verifier internal test flag. Behavior is undefined */
-#define BPF_F_TEST_STATE_FREQ	(1U << 3)
+	BPF_F_TEST_STATE_FREQ = (1U << 3),
 
 /* If BPF_F_SLEEPABLE is used in BPF_PROG_LOAD command, the verifier will
  * restrict map and helper usage for such programs. Sleepable BPF programs can
@@ -1108,8 +1111,10 @@ enum bpf_link_type {
  * Such programs are allowed to use helpers that may sleep like
  * bpf_copy_from_user().
  */
-#define BPF_F_SLEEPABLE		(1U << 4)
+	BPF_F_SLEEPABLE = (1U << 4),
+};
 
+enum bpf_pseudo_src_reg {
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
@@ -1121,8 +1126,8 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of map
  * verifier type:    CONST_PTR_TO_MAP
  */
-#define BPF_PSEUDO_MAP_FD	1
-#define BPF_PSEUDO_MAP_IDX	5
+	BPF_PSEUDO_MAP_FD  = 1,
+	BPF_PSEUDO_MAP_IDX = 5,
 
 /* insn[0].src_reg:  BPF_PSEUDO_MAP_[IDX_]VALUE
  * insn[0].imm:      map fd or fd_idx
@@ -1132,8 +1137,8 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of map[0]+offset
  * verifier type:    PTR_TO_MAP_VALUE
  */
-#define BPF_PSEUDO_MAP_VALUE		2
-#define BPF_PSEUDO_MAP_IDX_VALUE	6
+	BPF_PSEUDO_MAP_VALUE     = 2,
+	BPF_PSEUDO_MAP_IDX_VALUE = 6,
 
 /* insn[0].src_reg:  BPF_PSEUDO_BTF_ID
  * insn[0].imm:      kernel btd id of VAR
@@ -1144,7 +1149,7 @@ enum bpf_link_type {
  * verifier type:    PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var
  *                   is struct/union.
  */
-#define BPF_PSEUDO_BTF_ID	3
+	BPF_PSEUDO_BTF_ID = 3,
 /* insn[0].src_reg:  BPF_PSEUDO_FUNC
  * insn[0].imm:      insn offset to the func
  * insn[1].imm:      0
@@ -1153,19 +1158,20 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of the function
  * verifier type:    PTR_TO_FUNC.
  */
-#define BPF_PSEUDO_FUNC		4
+	BPF_PSEUDO_FUNC = 4,
 
 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative
  * offset to another bpf function
  */
-#define BPF_PSEUDO_CALL		1
+	BPF_PSEUDO_CALL = 1,
 /* when bpf_call->src_reg == BPF_PSEUDO_KFUNC_CALL,
  * bpf_call->imm == btf_id of a BTF_KIND_FUNC in the running kernel
  */
-#define BPF_PSEUDO_KFUNC_CALL	2
+	BPF_PSEUDO_KFUNC_CALL = 2,
+};
 
 /* flags for BPF_MAP_UPDATE_ELEM command */
-enum {
+enum bpf_map_update_elem_flag {
 	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 */
@@ -1173,7 +1179,7 @@ enum {
 };
 
 /* flags for BPF_MAP_CREATE command */
-enum {
+enum bpf_map_create_flag {
 	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
@@ -1213,17 +1219,19 @@ enum {
 };
 
 /* Flags for BPF_PROG_QUERY. */
-
+enum bpf_prog_query_flag {
 /* Query effective (directly attached + inherited from ancestor cgroups)
  * programs that will be executed for events within a cgroup.
  * attach_flags with this flag are returned only for directly attached programs.
  */
-#define BPF_F_QUERY_EFFECTIVE	(1U << 0)
-
-/* Flags for BPF_PROG_TEST_RUN */
+	BPF_F_QUERY_EFFECTIVE = (1U << 0),
+};
 
+/* Flags for BPF_PROG_RUN */
+enum bpf_prog_run_flag {
 /* If set, run the test on the cpu specified by bpf_attr.test.cpu */
-#define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
+	BPF_F_TEST_RUN_ON_CPU = (1U << 0),
+};
 
 /* type for BPF_ENABLE_STATS */
 enum bpf_stats_type {
@@ -5230,7 +5238,7 @@ enum {
 };
 
 /* BPF ring buffer constants */
-enum {
+enum bpf_ringbuf_const {
 	BPF_RINGBUF_BUSY_BIT		= (1U << 31),
 	BPF_RINGBUF_DISCARD_BIT		= (1U << 30),
 	BPF_RINGBUF_HDR_SZ		= 8,
-- 
2.30.2

[RFC 2/9] bpf: various constants

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 78b532d28761..211b9d902006 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1248,7 +1248,10 @@ enum bpf_stack_build_id_status {
 	BPF_STACK_BUILD_ID_IP = 2,
 };
 
-#define BPF_BUILD_ID_SIZE 20
+enum {
+	BPF_BUILD_ID_SIZE = 20,
+};
+
 struct bpf_stack_build_id {
 	__s32		status;
 	unsigned char	build_id[BPF_BUILD_ID_SIZE];
@@ -1258,7 +1261,9 @@ struct bpf_stack_build_id {
 	};
 };
 
-#define BPF_OBJ_NAME_LEN 16U
+enum {
+	BPF_OBJ_NAME_LEN = 16U,
+};
 
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
@@ -5464,7 +5469,9 @@ struct bpf_xdp_sock {
 	__u32 queue_id;
 };
 
-#define XDP_PACKET_HEADROOM 256
+enum {
+	XDP_PACKET_HEADROOM = 256,
+};
 
 /* User return codes for XDP prog type.
  * A valid XDP program must return one of these defined values. All other
@@ -5582,7 +5589,9 @@ struct sk_reuseport_md {
 	__bpf_md_ptr(struct bpf_sock *, migrating_sk);
 };
 
-#define BPF_TAG_SIZE	8
+enum {
+	BPF_TAG_SIZE = 8,
+};
 
 struct bpf_prog_info {
 	__u32 type;
-- 
2.30.2

Re: [RFC 2/9] bpf: various constants

[Greg KH]

On Thu, Oct 14, 2021 at 03:34:26PM +0100, Lorenz Bauer wrote:
> ---
>  include/uapi/linux/bpf.h | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)

I know I don't take matches without any changelog text, maybe other
maintainers are more lax?

Also, no signed-off-by:?

:(



> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 78b532d28761..211b9d902006 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1248,7 +1248,10 @@ enum bpf_stack_build_id_status {
>  	BPF_STACK_BUILD_ID_IP = 2,
>  };
>  
> -#define BPF_BUILD_ID_SIZE 20
> +enum {
> +	BPF_BUILD_ID_SIZE = 20,
> +};
> +
>  struct bpf_stack_build_id {
>  	__s32		status;
>  	unsigned char	build_id[BPF_BUILD_ID_SIZE];
> @@ -1258,7 +1261,9 @@ struct bpf_stack_build_id {
>  	};
>  };
>  
> -#define BPF_OBJ_NAME_LEN 16U
> +enum {
> +	BPF_OBJ_NAME_LEN = 16U,
> +};
>  
>  union bpf_attr {
>  	struct { /* anonymous struct used by BPF_MAP_CREATE command */
> @@ -5464,7 +5469,9 @@ struct bpf_xdp_sock {
>  	__u32 queue_id;
>  };
>  
> -#define XDP_PACKET_HEADROOM 256
> +enum {
> +	XDP_PACKET_HEADROOM = 256,
> +};
>  
>  /* User return codes for XDP prog type.
>   * A valid XDP program must return one of these defined values. All other
> @@ -5582,7 +5589,9 @@ struct sk_reuseport_md {
>  	__bpf_md_ptr(struct bpf_sock *, migrating_sk);
>  };
>  
> -#define BPF_TAG_SIZE	8
> +enum {
> +	BPF_TAG_SIZE = 8,
> +};
>  
>  struct bpf_prog_info {
>  	__u32 type;
> -- 
> 2.30.2
> 

Re: [RFC 2/9] bpf: various constants

[Lorenz Bauer]

On Thu, 14 Oct 2021 at 15:43, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 14, 2021 at 03:34:26PM +0100, Lorenz Bauer wrote:
> > ---
> >  include/uapi/linux/bpf.h | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
>
> I know I don't take matches without any changelog text, maybe other
> maintainers are more lax?

Hi Greg,

The patches aren't ready to go in, I'm looking for feedback. The
rationale in the cover letter for the series, I thought the RFC tag
would be enough, sorry about that. I expect that there will be a lot
of changes (if it lands at all) so I didn't invest the time to write
commit descriptions.

Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [RFC 2/9] bpf: various constants

[Greg KH]

On Thu, Oct 14, 2021 at 03:47:19PM +0100, Lorenz Bauer wrote:
> On Thu, 14 Oct 2021 at 15:43, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Oct 14, 2021 at 03:34:26PM +0100, Lorenz Bauer wrote:
> > > ---
> > >  include/uapi/linux/bpf.h | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > I know I don't take matches without any changelog text, maybe other
> > maintainers are more lax?
> 
> Hi Greg,
> 
> The patches aren't ready to go in, I'm looking for feedback. The
> rationale in the cover letter for the series, I thought the RFC tag
> would be enough, sorry about that. I expect that there will be a lot
> of changes (if it lands at all) so I didn't invest the time to write
> commit descriptions.

commit descriptions are usually the hardest part of the patch to write,
but the most important for the reviewers.  We optimize for reviewers,
not submitters :)

thanks,

greg k-h

[RFC 3/9] bpf: move up __bpf_md_ptr

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 211b9d902006..dec062fa0604 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -49,6 +49,12 @@
 #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
 #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
 
+#define __bpf_md_ptr(type, name)	\
+union {					\
+	type name;			\
+	__u64 :64;			\
+} __attribute__((aligned(8)))
+
 /* Register numbers */
 enum bpf_reg {
 	BPF_REG_0 = 0,
@@ -5285,12 +5291,6 @@ enum {
 	BPF_F_EXCLUDE_INGRESS	= (1ULL << 4),
 };
 
-#define __bpf_md_ptr(type, name)	\
-union {					\
-	type name;			\
-	__u64 :64;			\
-} __attribute__((aligned(8)))
-
 /* user accessible mirror of in-kernel sk_buff.
  * new fields can only be added to the end of this structure
  */
-- 
2.30.2

[RFC 4/9] bpf: name __u64 member of __bpf_md_ptr

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dec062fa0604..c1b1ce0e26a6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -52,7 +52,7 @@
 #define __bpf_md_ptr(type, name)	\
 union {					\
 	type name;			\
-	__u64 :64;			\
+	__u64 name##_u64;		\
 } __attribute__((aligned(8)))
 
 /* Register numbers */
-- 
2.30.2

[RFC 5/9] bpf: enum bpf_map_create_attr

[Lorenz Bauer]

---
 include/linux/bpf.h      |  4 +--
 include/uapi/linux/bpf.h | 62 ++++++++++++++++++++++++++--------------
 kernel/bpf/syscall.c     | 27 +++++++++++------
 3 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b6c45a6cbbba..80791db7945a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1613,8 +1613,8 @@ int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu,
 /* Return map's numa specified by userspace */
 static inline int bpf_map_attr_numa_node(const union bpf_attr *attr)
 {
-	return (attr->map_flags & BPF_F_NUMA_NODE) ?
-		attr->numa_node : NUMA_NO_NODE;
+	return (attr->map_create.map_flags & BPF_F_NUMA_NODE) ?
+		attr->map_create.numa_node : NUMA_NO_NODE;
 }
 
 struct bpf_prog *bpf_prog_get_type_path(const char *name, enum bpf_prog_type type);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c1b1ce0e26a6..f1c163778d7a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1271,29 +1271,31 @@ enum {
 	BPF_OBJ_NAME_LEN = 16U,
 };
 
+struct bpf_map_create_attr {
+	__u32	map_type;	/* one of enum bpf_map_type */
+	__u32	key_size;	/* size of key in bytes */
+	__u32	value_size;	/* size of value in bytes */
+	__u32	max_entries;	/* max number of entries in a map */
+	__u32	map_flags;	/* BPF_MAP_CREATE related
+				 * flags defined above.
+				 */
+	__u32	inner_map_fd;	/* fd pointing to the inner map */
+	__u32	numa_node;	/* numa node (effective only if
+				 * BPF_F_NUMA_NODE is set).
+				 */
+	char	map_name[BPF_OBJ_NAME_LEN];
+	__u32	map_ifindex;	/* ifindex of netdev to create on */
+	__u32	btf_fd;		/* fd pointing to a BTF type data */
+	__u32	btf_key_type_id;	/* BTF type_id of the key */
+	__u32	btf_value_type_id;	/* BTF type_id of the value */
+	__u32	btf_vmlinux_value_type_id;	/* BTF type_id of a kernel-
+						 * struct stored as the
+						 * map value
+						 */
+};
+
 union bpf_attr {
-	struct { /* anonymous struct used by BPF_MAP_CREATE command */
-		__u32	map_type;	/* one of enum bpf_map_type */
-		__u32	key_size;	/* size of key in bytes */
-		__u32	value_size;	/* size of value in bytes */
-		__u32	max_entries;	/* max number of entries in a map */
-		__u32	map_flags;	/* BPF_MAP_CREATE related
-					 * flags defined above.
-					 */
-		__u32	inner_map_fd;	/* fd pointing to the inner map */
-		__u32	numa_node;	/* numa node (effective only if
-					 * BPF_F_NUMA_NODE is set).
-					 */
-		char	map_name[BPF_OBJ_NAME_LEN];
-		__u32	map_ifindex;	/* ifindex of netdev to create on */
-		__u32	btf_fd;		/* fd pointing to a BTF type data */
-		__u32	btf_key_type_id;	/* BTF type_id of the key */
-		__u32	btf_value_type_id;	/* BTF type_id of the value */
-		__u32	btf_vmlinux_value_type_id;/* BTF type_id of a kernel-
-						   * struct stored as the
-						   * map value
-						   */
-	};
+	struct bpf_map_create_attr map_create;
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
 		__u32		map_fd;
@@ -1506,6 +1508,22 @@ union bpf_attr {
 		__u32		flags;		/* extra flags */
 	} prog_bind_map;
 
+	/* DEPRECATED: these are kept for compatibility purposes. */
+	struct { /* anonymous struct used by BPF_MAP_CREATE command */
+		__u32	map_type;
+		__u32	key_size;
+		__u32	value_size;
+		__u32	max_entries;
+		__u32	map_flags;
+		__u32	inner_map_fd;
+		__u32	numa_node;
+		char	map_name[BPF_OBJ_NAME_LEN];
+		__u32	map_ifindex;
+		__u32	btf_fd;
+		__u32	btf_key_type_id;
+		__u32	btf_value_type_id;
+		__u32	btf_vmlinux_value_type_id;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..f7b57877acd2 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -103,7 +103,7 @@ const struct bpf_map_ops bpf_map_offload_ops = {
 	.map_check_btf = map_check_no_btf,
 };
 
-static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
+static struct bpf_map *find_and_alloc_map(struct bpf_map_create_attr *attr)
 {
 	const struct bpf_map_ops *ops;
 	u32 type = attr->map_type;
@@ -118,13 +118,13 @@ static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
 		return ERR_PTR(-EINVAL);
 
 	if (ops->map_alloc_check) {
-		err = ops->map_alloc_check(attr);
+		err = ops->map_alloc_check((union bpf_attr *)attr); /* XXX: Dodgy cast */
 		if (err)
 			return ERR_PTR(err);
 	}
 	if (attr->map_ifindex)
 		ops = &bpf_map_offload_ops;
-	map = ops->map_alloc(attr);
+	map = ops->map_alloc((union bpf_attr *)attr); /* XXX: Dodgy cast */
 	if (IS_ERR(map))
 		return map;
 	map->ops = ops;
@@ -719,6 +719,15 @@ int bpf_get_file_flag(int flags)
 		   offsetof(union bpf_attr, CMD##_LAST_FIELD) - \
 		   sizeof(attr->CMD##_LAST_FIELD)) != NULL
 
+/* helper macro to extract a field from union bpf_attr while checking that the tail is zero. */
+#define ATTR_FIELD(attr, field) ({ \
+		typeof(&((attr)->field)) __tmp = &((attr)->field); \
+		if (memchr_inv((void *)__tmp + sizeof((attr)->field), 0, sizeof(*(attr)) - sizeof((attr)->field))) { \
+			__tmp = NULL; \
+		} \
+		__tmp; \
+	})
+
 /* dst and src must have at least "size" number of bytes.
  * Return strlen on success and < 0 on error.
  */
@@ -810,19 +819,19 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
 	return ret;
 }
 
-#define BPF_MAP_CREATE_LAST_FIELD btf_vmlinux_value_type_id
 /* called via syscall */
-static int map_create(union bpf_attr *attr)
+static int map_create(struct bpf_map_create_attr *attr)
 {
-	int numa_node = bpf_map_attr_numa_node(attr);
+	int numa_node;
 	struct bpf_map *map;
 	int f_flags;
 	int err;
 
-	err = CHECK_ATTR(BPF_MAP_CREATE);
-	if (err)
+	if (!attr)
 		return -EINVAL;
 
+	numa_node = bpf_map_attr_numa_node((union bpf_attr *)attr); /* Dodgy cast */
+
 	if (attr->btf_vmlinux_value_type_id) {
 		if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
 		    attr->btf_key_type_id || attr->btf_value_type_id)
@@ -4566,7 +4575,7 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 
 	switch (cmd) {
 	case BPF_MAP_CREATE:
-		err = map_create(&attr);
+		err = map_create(ATTR_FIELD(&attr, map_create));
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
 		err = map_lookup_elem(&attr);
-- 
2.30.2

[RFC 5/9] bpf: introduce CHECK_ATTR_TAIL

[Lorenz Bauer]

---
 kernel/bpf/syscall.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..14c2cfe6ef38 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -719,6 +719,10 @@ int bpf_get_file_flag(int flags)
 		   offsetof(union bpf_attr, CMD##_LAST_FIELD) - \
 		   sizeof(attr->CMD##_LAST_FIELD)) != NULL
 
+#define CHECK_ATTR_TAIL(attr, field) \
+	(memchr_inv((void *)(attr) + offsetofend(typeof(*attr), field), 0, \
+		    sizeof(*(attr)) - offsetofend(typeof(*attr), field)) != NULL ? -EINVAL : 0)
+
 /* dst and src must have at least "size" number of bytes.
  * Return strlen on success and < 0 on error.
  */
-- 
2.30.2

[RFC 6/9] bpf: split map modification structs

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 51 +++++++++++++++++++++++------
 kernel/bpf/syscall.c     | 70 ++++++++++++++++------------------------
 2 files changed, 70 insertions(+), 51 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f1c163778d7a..d3acd12d98c1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1294,18 +1294,41 @@ struct bpf_map_create_attr {
 						 */
 };
 
+struct bpf_map_lookup_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_update_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_delete_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+};
+
+struct bpf_map_get_next_key_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, next_key);
+};
+
 union bpf_attr {
 	struct bpf_map_create_attr map_create;
 
-	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
-		__u32		map_fd;
-		__aligned_u64	key;
-		union {
-			__aligned_u64 value;
-			__aligned_u64 next_key;
-		};
-		__u64		flags;
-	};
+	struct bpf_map_lookup_elem_attr map_lookup_elem;
+
+	struct bpf_map_update_elem_attr map_update_elem;
+
+	struct bpf_map_delete_elem_attr map_delete_elem;
+
+	struct bpf_map_get_next_key_attr map_get_next_key;
 
 	struct { /* struct used by BPF_MAP_*_BATCH commands */
 		__aligned_u64	in_batch;	/* start batch,
@@ -1524,6 +1547,16 @@ union bpf_attr {
 		__u32	btf_value_type_id;
 		__u32	btf_vmlinux_value_type_id;
 	};
+
+	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+		__u32		map_fd;
+		__aligned_u64	key;
+		union {
+			__aligned_u64 value;
+			__aligned_u64 next_key;
+		};
+		__u64		flags;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f7b57877acd2..c4aecdbb390e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -728,6 +728,11 @@ int bpf_get_file_flag(int flags)
 		__tmp; \
 	})
 
+#define CHECK_ATTR_TAIL(attr, field) \
+	(memchr_inv((void *)(attr) + offsetofend(typeof(*attr), field), 0, \
+		    sizeof(*(attr)) - offsetofend(typeof(*attr), field)) != NULL ? -EINVAL : 0)
+
+
 /* dst and src must have at least "size" number of bytes.
  * Return strlen on success and < 0 on error.
  */
@@ -1041,23 +1046,17 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
 	return NULL;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD flags
-
-static int map_lookup_elem(union bpf_attr *attr)
+static int map_lookup_elem(struct bpf_map_lookup_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *uvalue = u64_to_user_ptr(attr->value);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *uvalue = u64_to_user_ptr(attr->value_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_LOOKUP_ELEM))
-		return -EINVAL;
-
 	if (attr->flags & ~BPF_F_LOCK)
 		return -EINVAL;
 
@@ -1108,23 +1107,17 @@ static int map_lookup_elem(union bpf_attr *attr)
 	return err;
 }
 
-
-#define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
-
-static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
+static int map_update_elem(struct bpf_map_update_elem_attr *attr, bpfptr_t uattr)
 {
-	bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
-	bpfptr_t uvalue = make_bpfptr(attr->value, uattr.is_kernel);
-	int ufd = attr->map_fd;
+	bpfptr_t ukey = make_bpfptr(attr->key_u64, uattr.is_kernel);
+	bpfptr_t uvalue = make_bpfptr(attr->value_u64, uattr.is_kernel);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_UPDATE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1168,20 +1161,15 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
 	return err;
 }
 
-#define BPF_MAP_DELETE_ELEM_LAST_FIELD key
-
-static int map_delete_elem(union bpf_attr *attr)
+static int map_delete_elem(struct bpf_map_delete_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	struct fd f;
 	void *key;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_DELETE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1220,22 +1208,16 @@ static int map_delete_elem(union bpf_attr *attr)
 	return err;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD next_key
-
-static int map_get_next_key(union bpf_attr *attr)
+static int map_get_next_key(struct bpf_map_get_next_key_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *unext_key = u64_to_user_ptr(attr->next_key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *unext_key = u64_to_user_ptr(attr->next_key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *next_key;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_GET_NEXT_KEY))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -4578,16 +4560,20 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 		err = map_create(ATTR_FIELD(&attr, map_create));
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
-		err = map_lookup_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_lookup_elem);
+		err = err ?: map_lookup_elem(&attr.map_lookup_elem);
 		break;
 	case BPF_MAP_UPDATE_ELEM:
-		err = map_update_elem(&attr, uattr);
+		err = CHECK_ATTR_TAIL(&attr, map_update_elem);
+		err = err ?: map_update_elem(&attr.map_update_elem, uattr);
 		break;
 	case BPF_MAP_DELETE_ELEM:
-		err = map_delete_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_delete_elem);
+		err = err ?: map_delete_elem(&attr.map_delete_elem);
 		break;
 	case BPF_MAP_GET_NEXT_KEY:
-		err = map_get_next_key(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_get_next_key);
+		err = err ?: map_get_next_key(&attr.map_get_next_key);
 		break;
 	case BPF_MAP_FREEZE:
 		err = map_freeze(&attr);
-- 
2.30.2

Re: [RFC 6/9] bpf: split map modification structs

[Alexei Starovoitov]

On Thu, Oct 14, 2021 at 03:34:31PM +0100, Lorenz Bauer wrote:
> ---
>  include/uapi/linux/bpf.h | 51 +++++++++++++++++++++++------
>  kernel/bpf/syscall.c     | 70 ++++++++++++++++------------------------
>  2 files changed, 70 insertions(+), 51 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f1c163778d7a..d3acd12d98c1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1294,18 +1294,41 @@ struct bpf_map_create_attr {
>  						 */
>  };
>  
> +struct bpf_map_lookup_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, value);
> +	__u64 flags;
> +};
> +
> +struct bpf_map_update_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, value);
> +	__u64 flags;
> +};
> +
> +struct bpf_map_delete_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +};
> +
> +struct bpf_map_get_next_key_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, next_key);
> +};
> +
>  union bpf_attr {
>  	struct bpf_map_create_attr map_create;
>  
> -	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
> -		__u32		map_fd;
> -		__aligned_u64	key;
> -		union {
> -			__aligned_u64 value;
> -			__aligned_u64 next_key;
> -		};
> -		__u64		flags;
> -	};
> +	struct bpf_map_lookup_elem_attr map_lookup_elem;
> +
> +	struct bpf_map_update_elem_attr map_update_elem;
> +
> +	struct bpf_map_delete_elem_attr map_delete_elem;
> +
> +	struct bpf_map_get_next_key_attr map_get_next_key;

I think such additions to bpf_attr are good. They do make it
the interface more obvious.
But please don't move or delete the existing anon struct.
Adding 'deprecated' comment won't make it deprecated.
It will be used regardless.
If we ever extend 'map_lookup' command we might even add
fields to both. tbd.

[RFC 6/9] bpf: struct bpf_map_create_attr

[Lorenz Bauer]

---
 include/linux/bpf.h      |  4 +--
 include/uapi/linux/bpf.h | 62 ++++++++++++++++++++++++++--------------
 kernel/bpf/syscall.c     | 15 ++++------
 3 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..80c6dfdbd9d9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1617,8 +1617,8 @@ int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu,
 /* Return map's numa specified by userspace */
 static inline int bpf_map_attr_numa_node(const union bpf_attr *attr)
 {
-	return (attr->map_flags & BPF_F_NUMA_NODE) ?
-		attr->numa_node : NUMA_NO_NODE;
+	return (attr->map_create.map_flags & BPF_F_NUMA_NODE) ?
+		attr->map_create.numa_node : NUMA_NO_NODE;
 }
 
 struct bpf_prog *bpf_prog_get_type_path(const char *name, enum bpf_prog_type type);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c1b1ce0e26a6..f1c163778d7a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1271,29 +1271,31 @@ enum {
 	BPF_OBJ_NAME_LEN = 16U,
 };
 
+struct bpf_map_create_attr {
+	__u32	map_type;	/* one of enum bpf_map_type */
+	__u32	key_size;	/* size of key in bytes */
+	__u32	value_size;	/* size of value in bytes */
+	__u32	max_entries;	/* max number of entries in a map */
+	__u32	map_flags;	/* BPF_MAP_CREATE related
+				 * flags defined above.
+				 */
+	__u32	inner_map_fd;	/* fd pointing to the inner map */
+	__u32	numa_node;	/* numa node (effective only if
+				 * BPF_F_NUMA_NODE is set).
+				 */
+	char	map_name[BPF_OBJ_NAME_LEN];
+	__u32	map_ifindex;	/* ifindex of netdev to create on */
+	__u32	btf_fd;		/* fd pointing to a BTF type data */
+	__u32	btf_key_type_id;	/* BTF type_id of the key */
+	__u32	btf_value_type_id;	/* BTF type_id of the value */
+	__u32	btf_vmlinux_value_type_id;	/* BTF type_id of a kernel-
+						 * struct stored as the
+						 * map value
+						 */
+};
+
 union bpf_attr {
-	struct { /* anonymous struct used by BPF_MAP_CREATE command */
-		__u32	map_type;	/* one of enum bpf_map_type */
-		__u32	key_size;	/* size of key in bytes */
-		__u32	value_size;	/* size of value in bytes */
-		__u32	max_entries;	/* max number of entries in a map */
-		__u32	map_flags;	/* BPF_MAP_CREATE related
-					 * flags defined above.
-					 */
-		__u32	inner_map_fd;	/* fd pointing to the inner map */
-		__u32	numa_node;	/* numa node (effective only if
-					 * BPF_F_NUMA_NODE is set).
-					 */
-		char	map_name[BPF_OBJ_NAME_LEN];
-		__u32	map_ifindex;	/* ifindex of netdev to create on */
-		__u32	btf_fd;		/* fd pointing to a BTF type data */
-		__u32	btf_key_type_id;	/* BTF type_id of the key */
-		__u32	btf_value_type_id;	/* BTF type_id of the value */
-		__u32	btf_vmlinux_value_type_id;/* BTF type_id of a kernel-
-						   * struct stored as the
-						   * map value
-						   */
-	};
+	struct bpf_map_create_attr map_create;
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
 		__u32		map_fd;
@@ -1506,6 +1508,22 @@ union bpf_attr {
 		__u32		flags;		/* extra flags */
 	} prog_bind_map;
 
+	/* DEPRECATED: these are kept for compatibility purposes. */
+	struct { /* anonymous struct used by BPF_MAP_CREATE command */
+		__u32	map_type;
+		__u32	key_size;
+		__u32	value_size;
+		__u32	max_entries;
+		__u32	map_flags;
+		__u32	inner_map_fd;
+		__u32	numa_node;
+		char	map_name[BPF_OBJ_NAME_LEN];
+		__u32	map_ifindex;
+		__u32	btf_fd;
+		__u32	btf_key_type_id;
+		__u32	btf_value_type_id;
+		__u32	btf_vmlinux_value_type_id;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 14c2cfe6ef38..337fbd2f1874 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -103,7 +103,7 @@ const struct bpf_map_ops bpf_map_offload_ops = {
 	.map_check_btf = map_check_no_btf,
 };
 
-static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
+static struct bpf_map *find_and_alloc_map(struct bpf_map_create_attr *attr)
 {
 	const struct bpf_map_ops *ops;
 	u32 type = attr->map_type;
@@ -118,13 +118,13 @@ static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
 		return ERR_PTR(-EINVAL);
 
 	if (ops->map_alloc_check) {
-		err = ops->map_alloc_check(attr);
+		err = ops->map_alloc_check((union bpf_attr *)attr); /* XXX: Dodgy cast */
 		if (err)
 			return ERR_PTR(err);
 	}
 	if (attr->map_ifindex)
 		ops = &bpf_map_offload_ops;
-	map = ops->map_alloc(attr);
+	map = ops->map_alloc((union bpf_attr *)attr); /* XXX: Dodgy cast */
 	if (IS_ERR(map))
 		return map;
 	map->ops = ops;
@@ -814,18 +814,14 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
 	return ret;
 }
 
-#define BPF_MAP_CREATE_LAST_FIELD btf_vmlinux_value_type_id
 /* called via syscall */
-static int map_create(union bpf_attr *attr)
+static int map_create(struct bpf_map_create_attr *attr)
 {
 	int numa_node = bpf_map_attr_numa_node(attr);
 	struct bpf_map *map;
 	int f_flags;
 	int err;
 
-	err = CHECK_ATTR(BPF_MAP_CREATE);
-	if (err)
-		return -EINVAL;
 
 	if (attr->btf_vmlinux_value_type_id) {
 		if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS ||
@@ -4570,7 +4566,8 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 
 	switch (cmd) {
 	case BPF_MAP_CREATE:
-		err = map_create(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_create);
+		err = err ?: map_create(&attr.map_create);
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
 		err = map_lookup_elem(&attr);
-- 
2.30.2

[RFC 7/9] bpf: split get_id and fd_by_id in bpf_attr

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 60 ++++++++++++++++++++++++++++++++--------
 kernel/bpf/syscall.c     | 18 ++++++------
 2 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d3acd12d98c1..13d126c201ce 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1422,17 +1422,43 @@ union bpf_attr {
 		__u32		cpu;
 	} test;
 
-	struct { /* anonymous struct used by BPF_*_GET_*_ID */
-		union {
-			__u32		start_id;
-			__u32		prog_id;
-			__u32		map_id;
-			__u32		btf_id;
-			__u32		link_id;
-		};
-		__u32		next_id;
-		__u32		open_flags;
-	};
+	struct { /* used by BPF_PROG_GET_FD_BY_ID command */
+		__u32 id;
+	} prog_get_fd_by_id;
+
+	struct { /* used by BPF_MAP_GET_FD_BY_ID command */
+		__u32 id;
+		__u32 ingnored;
+		__u32 open_flags;
+	} map_get_fd_by_id;
+
+	struct { /* used by BPF_BTF_GET_FD_BY_ID command */
+		__u32 id;
+	} btf_get_fd_by_id;
+
+	struct { /* used by BPF_LINK_GET_FD_BY_ID command */
+		__u32 id;
+	} link_get_fd_by_id;
+
+	struct { /* used by BPF_PROG_GET_NEXT_ID command */
+		__u32 start_id;
+		__u32 next_id;
+	} prog_get_next_id;
+
+	struct { /* used by BPF_MAP_GET_NEXT_ID command */
+		__u32 start_id;
+		__u32 next_id;
+	} map_get_next_id;
+
+	struct { /* used by BPF_BTF_GET_NEXT_ID command */
+		__u32 start_id;
+		__u32 next_id;
+	} btf_get_next_id;
+
+	struct { /* used by BPF_LINK_GET_NEXT_ID command */
+		__u32 start_id;
+		__u32 next_id;
+	} link_get_next_id;
 
 	struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
 		__u32		bpf_fd;
@@ -1557,6 +1583,18 @@ union bpf_attr {
 		};
 		__u64		flags;
 	};
+
+	struct { /* anonymous struct used by BPF_*_GET_*_ID */
+		union {
+			__u32		start_id;
+			__u32		prog_id;
+			__u32		map_id;
+			__u32		btf_id;
+			__u32		link_id;
+		};
+		__u32		next_id;
+		__u32		open_flags;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c4aecdbb390e..234860bd05bf 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3367,7 +3367,7 @@ struct bpf_prog *bpf_prog_get_curr_or_next(u32 *id)
 	return prog;
 }
 
-#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD prog_id
+#define BPF_PROG_GET_FD_BY_ID_LAST_FIELD prog_get_fd_by_id.id
 
 struct bpf_prog *bpf_prog_by_id(u32 id)
 {
@@ -3389,7 +3389,7 @@ struct bpf_prog *bpf_prog_by_id(u32 id)
 static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
 {
 	struct bpf_prog *prog;
-	u32 id = attr->prog_id;
+	u32 id = attr->prog_get_fd_by_id.id;
 	int fd;
 
 	if (CHECK_ATTR(BPF_PROG_GET_FD_BY_ID))
@@ -3409,12 +3409,12 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
 	return fd;
 }
 
-#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD open_flags
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD map_get_fd_by_id.open_flags
 
 static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
 {
 	struct bpf_map *map;
-	u32 id = attr->map_id;
+	u32 id = attr->map_get_fd_by_id.id;
 	int f_flags;
 	int fd;
 
@@ -3425,7 +3425,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	f_flags = bpf_get_file_flag(attr->open_flags);
+	f_flags = bpf_get_file_flag(attr->map_get_fd_by_id.open_flags);
 	if (f_flags < 0)
 		return f_flags;
 
@@ -3984,7 +3984,7 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr)
 	return btf_new_fd(attr, uattr);
 }
 
-#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_id
+#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_get_fd_by_id.id
 
 static int bpf_btf_get_fd_by_id(const union bpf_attr *attr)
 {
@@ -3994,7 +3994,7 @@ static int bpf_btf_get_fd_by_id(const union bpf_attr *attr)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	return btf_get_fd_by_id(attr->btf_id);
+	return btf_get_fd_by_id(attr->btf_get_fd_by_id.id);
 }
 
 static int bpf_task_fd_query_copy(const union bpf_attr *attr,
@@ -4369,12 +4369,12 @@ struct bpf_link *bpf_link_by_id(u32 id)
 	return link;
 }
 
-#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD link_id
+#define BPF_LINK_GET_FD_BY_ID_LAST_FIELD link_get_fd_by_id.id
 
 static int bpf_link_get_fd_by_id(const union bpf_attr *attr)
 {
 	struct bpf_link *link;
-	u32 id = attr->link_id;
+	u32 id = attr->link_get_fd_by_id.id;
 	int fd;
 
 	if (CHECK_ATTR(BPF_LINK_GET_FD_BY_ID))
-- 
2.30.2

Re: [RFC 7/9] bpf: split get_id and fd_by_id in bpf_attr

[Alexei Starovoitov]

On Thu, Oct 14, 2021 at 03:34:33PM +0100, Lorenz Bauer wrote:
> ---
>  include/uapi/linux/bpf.h | 60 ++++++++++++++++++++++++++++++++--------
>  kernel/bpf/syscall.c     | 18 ++++++------
>  2 files changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d3acd12d98c1..13d126c201ce 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1422,17 +1422,43 @@ union bpf_attr {
>  		__u32		cpu;
>  	} test;
>  
> -	struct { /* anonymous struct used by BPF_*_GET_*_ID */
> -		union {
> -			__u32		start_id;
> -			__u32		prog_id;
> -			__u32		map_id;
> -			__u32		btf_id;
> -			__u32		link_id;
> -		};
> -		__u32		next_id;
> -		__u32		open_flags;
> -	};
> +	struct { /* used by BPF_PROG_GET_FD_BY_ID command */
> +		__u32 id;
> +	} prog_get_fd_by_id;
> +
> +	struct { /* used by BPF_MAP_GET_FD_BY_ID command */
> +		__u32 id;
> +		__u32 ingnored;
> +		__u32 open_flags;
> +	} map_get_fd_by_id;
> +
> +	struct { /* used by BPF_BTF_GET_FD_BY_ID command */
> +		__u32 id;
> +	} btf_get_fd_by_id;
> +
> +	struct { /* used by BPF_LINK_GET_FD_BY_ID command */
> +		__u32 id;
> +	} link_get_fd_by_id;
> +
> +	struct { /* used by BPF_PROG_GET_NEXT_ID command */
> +		__u32 start_id;
> +		__u32 next_id;
> +	} prog_get_next_id;
> +
> +	struct { /* used by BPF_MAP_GET_NEXT_ID command */
> +		__u32 start_id;
> +		__u32 next_id;
> +	} map_get_next_id;
> +
> +	struct { /* used by BPF_BTF_GET_NEXT_ID command */
> +		__u32 start_id;
> +		__u32 next_id;
> +	} btf_get_next_id;
> +
> +	struct { /* used by BPF_LINK_GET_NEXT_ID command */
> +		__u32 start_id;
> +		__u32 next_id;
> +	} link_get_next_id;

This one looks like churn though.

Re: [RFC 7/9] bpf: split get_id and fd_by_id in bpf_attr

[Lorenz Bauer]

On Wed, 20 Oct 2021 at 18:15, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> > +     struct { /* used by BPF_PROG_GET_FD_BY_ID command */
> > +             __u32 id;
> > +     } prog_get_fd_by_id;
> > +
> > +     struct { /* used by BPF_MAP_GET_FD_BY_ID command */
> > +             __u32 id;
> > +             __u32 ingnored;
> > +             __u32 open_flags;
> > +     } map_get_fd_by_id;
> > +

> > +     struct { /* used by BPF_PROG_GET_NEXT_ID command */
> > +             __u32 start_id;
> > +             __u32 next_id;
> > +     } prog_get_next_id;
> > +

> This one looks like churn though.

Yes, but it's still better than what we have now. There are three
distinct syscall signatures that a user needs to understand, which is
impossible right now without looking at the source. map_get_fd_by_id
is arguably dodgy with one field completely ignored. Having one struct
for each bpf_cmd makes code generation easier as well.

I could reduce this to just the three different variants, it opens us
up to another map_get_fd_by_id.

Lorenz
-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [RFC 7/9] bpf: split get_id and fd_by_id in bpf_attr

[Alexei Starovoitov]

On Thu, Oct 21, 2021 at 8:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Wed, 20 Oct 2021 at 18:15, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > > +     struct { /* used by BPF_PROG_GET_FD_BY_ID command */
> > > +             __u32 id;
> > > +     } prog_get_fd_by_id;
> > > +
> > > +     struct { /* used by BPF_MAP_GET_FD_BY_ID command */
> > > +             __u32 id;
> > > +             __u32 ingnored;
> > > +             __u32 open_flags;
> > > +     } map_get_fd_by_id;
> > > +
>
> > > +     struct { /* used by BPF_PROG_GET_NEXT_ID command */
> > > +             __u32 start_id;
> > > +             __u32 next_id;
> > > +     } prog_get_next_id;
> > > +
>
> > This one looks like churn though.
>
> Yes, but it's still better than what we have now. There are three
> distinct syscall signatures that a user needs to understand, which is
> impossible right now without looking at the source. map_get_fd_by_id
> is arguably dodgy with one field completely ignored. Having one struct
> for each bpf_cmd makes code generation easier as well.
>
> I could reduce this to just the three different variants, it opens us
> up to another map_get_fd_by_id.

yes, but even with all of them there is still a risk of repeating
map_get_fd_by_id mistake.
To make progress maybe let's land the bits that we agree on
and keep brainstorming on the rest?
Or that's too little to be useful for automatic golang conversion?
If the whole thing is needed then golang converting script
should probably be part of the series otherwise things will bit rot.

Re: [RFC 7/9] bpf: split get_id and fd_by_id in bpf_attr

[Lorenz Bauer]

On Wed, 27 Oct 2021 at 19:21, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> >
> > I could reduce this to just the three different variants, it opens us
> > up to another map_get_fd_by_id.
>
> yes, but even with all of them there is still a risk of repeating
> map_get_fd_by_id mistake.
> To make progress maybe let's land the bits that we agree on
> and keep brainstorming on the rest?

Sounds good to me, which parts are good to go from your side? I wanted
to join BPF office hours yesterday to discuss, but I got sidetracked.
I'll join next week instead.

> Or that's too little to be useful for automatic golang conversion?
> If the whole thing is needed then golang converting script
> should probably be part of the series otherwise things will bit rot.

Every little helps I would say. Good point on bit rot, does it make
sense to e.g. not allow new defines by default, etc.? We could hook it
into the Makefile of selftests/bpf or some such?

Lorenz
--
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

[RFC 7/9] bpf: split map modification structs

[Lorenz Bauer]

---
 include/uapi/linux/bpf.h | 51 +++++++++++++++++++++++++------
 kernel/bpf/syscall.c     | 65 ++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 51 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f1c163778d7a..d3acd12d98c1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1294,18 +1294,41 @@ struct bpf_map_create_attr {
 						 */
 };
 
+struct bpf_map_lookup_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_update_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_delete_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+};
+
+struct bpf_map_get_next_key_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, next_key);
+};
+
 union bpf_attr {
 	struct bpf_map_create_attr map_create;
 
-	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
-		__u32		map_fd;
-		__aligned_u64	key;
-		union {
-			__aligned_u64 value;
-			__aligned_u64 next_key;
-		};
-		__u64		flags;
-	};
+	struct bpf_map_lookup_elem_attr map_lookup_elem;
+
+	struct bpf_map_update_elem_attr map_update_elem;
+
+	struct bpf_map_delete_elem_attr map_delete_elem;
+
+	struct bpf_map_get_next_key_attr map_get_next_key;
 
 	struct { /* struct used by BPF_MAP_*_BATCH commands */
 		__aligned_u64	in_batch;	/* start batch,
@@ -1524,6 +1547,16 @@ union bpf_attr {
 		__u32	btf_value_type_id;
 		__u32	btf_vmlinux_value_type_id;
 	};
+
+	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+		__u32		map_fd;
+		__aligned_u64	key;
+		union {
+			__aligned_u64 value;
+			__aligned_u64 next_key;
+		};
+		__u64		flags;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 337fbd2f1874..f44d888bebb9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1032,23 +1032,17 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
 	return NULL;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD flags
-
-static int map_lookup_elem(union bpf_attr *attr)
+static int map_lookup_elem(struct bpf_map_lookup_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *uvalue = u64_to_user_ptr(attr->value);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *uvalue = u64_to_user_ptr(attr->value_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_LOOKUP_ELEM))
-		return -EINVAL;
-
 	if (attr->flags & ~BPF_F_LOCK)
 		return -EINVAL;
 
@@ -1099,23 +1093,17 @@ static int map_lookup_elem(union bpf_attr *attr)
 	return err;
 }
 
-
-#define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
-
-static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
+static int map_update_elem(struct bpf_map_update_elem_attr *attr, bpfptr_t uattr)
 {
-	bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
-	bpfptr_t uvalue = make_bpfptr(attr->value, uattr.is_kernel);
-	int ufd = attr->map_fd;
+	bpfptr_t ukey = make_bpfptr(attr->key_u64, uattr.is_kernel);
+	bpfptr_t uvalue = make_bpfptr(attr->value_u64, uattr.is_kernel);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_UPDATE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1159,20 +1147,15 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
 	return err;
 }
 
-#define BPF_MAP_DELETE_ELEM_LAST_FIELD key
-
-static int map_delete_elem(union bpf_attr *attr)
+static int map_delete_elem(struct bpf_map_delete_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	struct fd f;
 	void *key;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_DELETE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1211,22 +1194,16 @@ static int map_delete_elem(union bpf_attr *attr)
 	return err;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD next_key
-
-static int map_get_next_key(union bpf_attr *attr)
+static int map_get_next_key(struct bpf_map_get_next_key_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *unext_key = u64_to_user_ptr(attr->next_key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *unext_key = u64_to_user_ptr(attr->next_key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *next_key;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_GET_NEXT_KEY))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -4570,16 +4547,20 @@ static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 		err = err ?: map_create(&attr.map_create);
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
-		err = map_lookup_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_lookup_elem);
+		err = err ?: map_lookup_elem(&attr.map_lookup_elem);
 		break;
 	case BPF_MAP_UPDATE_ELEM:
-		err = map_update_elem(&attr, uattr);
+		err = CHECK_ATTR_TAIL(&attr, map_update_elem);
+		err = err ?: map_update_elem(&attr.map_update_elem, uattr);
 		break;
 	case BPF_MAP_DELETE_ELEM:
-		err = map_delete_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_delete_elem);
+		err = err ?: map_delete_elem(&attr.map_delete_elem);
 		break;
 	case BPF_MAP_GET_NEXT_KEY:
-		err = map_get_next_key(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_get_next_key);
+		err = err ?: map_get_next_key(&attr.map_get_next_key);
 		break;
 	case BPF_MAP_FREEZE:
 		err = map_freeze(&attr);
-- 
2.30.2

[RFC 8/9] selftests: sync bpf.h

[Lorenz Bauer]

---
 tools/include/uapi/linux/bpf.h | 200 ++++++++++++++++++++++-----------
 1 file changed, 134 insertions(+), 66 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6fc59d61937a..d3acd12d98c1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -49,8 +49,14 @@
 #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
 #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
 
+#define __bpf_md_ptr(type, name)	\
+union {					\
+	type name;			\
+	__u64 name##_u64;		\
+} __attribute__((aligned(8)))
+
 /* Register numbers */
-enum {
+enum bpf_reg {
 	BPF_REG_0 = 0,
 	BPF_REG_1,
 	BPF_REG_2,
@@ -1056,16 +1062,19 @@ enum bpf_link_type {
  * All eligible programs are executed regardless of return code from
  * earlier programs.
  */
-#define BPF_F_ALLOW_OVERRIDE	(1U << 0)
-#define BPF_F_ALLOW_MULTI	(1U << 1)
-#define BPF_F_REPLACE		(1U << 2)
+enum bpf_prog_attach_flag {
+	BPF_F_ALLOW_OVERRIDE	= (1U << 0),
+	BPF_F_ALLOW_MULTI	= (1U << 1),
+	BPF_F_REPLACE		= (1U << 2),
+};
 
+enum bpf_prog_load_flag {
 /* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
  * verifier will perform strict alignment checking as if the kernel
  * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
  * and NET_IP_ALIGN defined to 2.
  */
-#define BPF_F_STRICT_ALIGNMENT	(1U << 0)
+	BPF_F_STRICT_ALIGNMENT = (1U << 0),
 
 /* If BPF_F_ANY_ALIGNMENT is used in BPF_PROF_LOAD command, the
  * verifier will allow any alignment whatsoever.  On platforms
@@ -1079,7 +1088,7 @@ enum bpf_link_type {
  * of an unaligned access the alignment check would trigger before
  * the one we are interested in.
  */
-#define BPF_F_ANY_ALIGNMENT	(1U << 1)
+	BPF_F_ANY_ALIGNMENT = (1U << 1),
 
 /* BPF_F_TEST_RND_HI32 is used in BPF_PROG_LOAD command for testing purpose.
  * Verifier does sub-register def/use analysis and identifies instructions whose
@@ -1097,10 +1106,10 @@ enum bpf_link_type {
  * Then, if verifier is not doing correct analysis, such randomization will
  * regress tests to expose bugs.
  */
-#define BPF_F_TEST_RND_HI32	(1U << 2)
+	BPF_F_TEST_RND_HI32 = (1U << 2),
 
 /* The verifier internal test flag. Behavior is undefined */
-#define BPF_F_TEST_STATE_FREQ	(1U << 3)
+	BPF_F_TEST_STATE_FREQ = (1U << 3),
 
 /* If BPF_F_SLEEPABLE is used in BPF_PROG_LOAD command, the verifier will
  * restrict map and helper usage for such programs. Sleepable BPF programs can
@@ -1108,8 +1117,10 @@ enum bpf_link_type {
  * Such programs are allowed to use helpers that may sleep like
  * bpf_copy_from_user().
  */
-#define BPF_F_SLEEPABLE		(1U << 4)
+	BPF_F_SLEEPABLE = (1U << 4),
+};
 
+enum bpf_pseudo_src_reg {
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *
@@ -1121,8 +1132,8 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of map
  * verifier type:    CONST_PTR_TO_MAP
  */
-#define BPF_PSEUDO_MAP_FD	1
-#define BPF_PSEUDO_MAP_IDX	5
+	BPF_PSEUDO_MAP_FD  = 1,
+	BPF_PSEUDO_MAP_IDX = 5,
 
 /* insn[0].src_reg:  BPF_PSEUDO_MAP_[IDX_]VALUE
  * insn[0].imm:      map fd or fd_idx
@@ -1132,8 +1143,8 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of map[0]+offset
  * verifier type:    PTR_TO_MAP_VALUE
  */
-#define BPF_PSEUDO_MAP_VALUE		2
-#define BPF_PSEUDO_MAP_IDX_VALUE	6
+	BPF_PSEUDO_MAP_VALUE     = 2,
+	BPF_PSEUDO_MAP_IDX_VALUE = 6,
 
 /* insn[0].src_reg:  BPF_PSEUDO_BTF_ID
  * insn[0].imm:      kernel btd id of VAR
@@ -1144,7 +1155,7 @@ enum bpf_link_type {
  * verifier type:    PTR_TO_BTF_ID or PTR_TO_MEM, depending on whether the var
  *                   is struct/union.
  */
-#define BPF_PSEUDO_BTF_ID	3
+	BPF_PSEUDO_BTF_ID = 3,
 /* insn[0].src_reg:  BPF_PSEUDO_FUNC
  * insn[0].imm:      insn offset to the func
  * insn[1].imm:      0
@@ -1153,19 +1164,20 @@ enum bpf_link_type {
  * ldimm64 rewrite:  address of the function
  * verifier type:    PTR_TO_FUNC.
  */
-#define BPF_PSEUDO_FUNC		4
+	BPF_PSEUDO_FUNC = 4,
 
 /* when bpf_call->src_reg == BPF_PSEUDO_CALL, bpf_call->imm == pc-relative
  * offset to another bpf function
  */
-#define BPF_PSEUDO_CALL		1
+	BPF_PSEUDO_CALL = 1,
 /* when bpf_call->src_reg == BPF_PSEUDO_KFUNC_CALL,
  * bpf_call->imm == btf_id of a BTF_KIND_FUNC in the running kernel
  */
-#define BPF_PSEUDO_KFUNC_CALL	2
+	BPF_PSEUDO_KFUNC_CALL = 2,
+};
 
 /* flags for BPF_MAP_UPDATE_ELEM command */
-enum {
+enum bpf_map_update_elem_flag {
 	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 */
@@ -1173,7 +1185,7 @@ enum {
 };
 
 /* flags for BPF_MAP_CREATE command */
-enum {
+enum bpf_map_create_flag {
 	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
@@ -1213,17 +1225,19 @@ enum {
 };
 
 /* Flags for BPF_PROG_QUERY. */
-
+enum bpf_prog_query_flag {
 /* Query effective (directly attached + inherited from ancestor cgroups)
  * programs that will be executed for events within a cgroup.
  * attach_flags with this flag are returned only for directly attached programs.
  */
-#define BPF_F_QUERY_EFFECTIVE	(1U << 0)
-
-/* Flags for BPF_PROG_TEST_RUN */
+	BPF_F_QUERY_EFFECTIVE = (1U << 0),
+};
 
+/* Flags for BPF_PROG_RUN */
+enum bpf_prog_run_flag {
 /* If set, run the test on the cpu specified by bpf_attr.test.cpu */
-#define BPF_F_TEST_RUN_ON_CPU	(1U << 0)
+	BPF_F_TEST_RUN_ON_CPU = (1U << 0),
+};
 
 /* type for BPF_ENABLE_STATS */
 enum bpf_stats_type {
@@ -1240,7 +1254,10 @@ enum bpf_stack_build_id_status {
 	BPF_STACK_BUILD_ID_IP = 2,
 };
 
-#define BPF_BUILD_ID_SIZE 20
+enum {
+	BPF_BUILD_ID_SIZE = 20,
+};
+
 struct bpf_stack_build_id {
 	__s32		status;
 	unsigned char	build_id[BPF_BUILD_ID_SIZE];
@@ -1250,41 +1267,68 @@ struct bpf_stack_build_id {
 	};
 };
 
-#define BPF_OBJ_NAME_LEN 16U
+enum {
+	BPF_OBJ_NAME_LEN = 16U,
+};
+
+struct bpf_map_create_attr {
+	__u32	map_type;	/* one of enum bpf_map_type */
+	__u32	key_size;	/* size of key in bytes */
+	__u32	value_size;	/* size of value in bytes */
+	__u32	max_entries;	/* max number of entries in a map */
+	__u32	map_flags;	/* BPF_MAP_CREATE related
+				 * flags defined above.
+				 */
+	__u32	inner_map_fd;	/* fd pointing to the inner map */
+	__u32	numa_node;	/* numa node (effective only if
+				 * BPF_F_NUMA_NODE is set).
+				 */
+	char	map_name[BPF_OBJ_NAME_LEN];
+	__u32	map_ifindex;	/* ifindex of netdev to create on */
+	__u32	btf_fd;		/* fd pointing to a BTF type data */
+	__u32	btf_key_type_id;	/* BTF type_id of the key */
+	__u32	btf_value_type_id;	/* BTF type_id of the value */
+	__u32	btf_vmlinux_value_type_id;	/* BTF type_id of a kernel-
+						 * struct stored as the
+						 * map value
+						 */
+};
+
+struct bpf_map_lookup_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_update_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_delete_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+};
+
+struct bpf_map_get_next_key_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, next_key);
+};
 
 union bpf_attr {
-	struct { /* anonymous struct used by BPF_MAP_CREATE command */
-		__u32	map_type;	/* one of enum bpf_map_type */
-		__u32	key_size;	/* size of key in bytes */
-		__u32	value_size;	/* size of value in bytes */
-		__u32	max_entries;	/* max number of entries in a map */
-		__u32	map_flags;	/* BPF_MAP_CREATE related
-					 * flags defined above.
-					 */
-		__u32	inner_map_fd;	/* fd pointing to the inner map */
-		__u32	numa_node;	/* numa node (effective only if
-					 * BPF_F_NUMA_NODE is set).
-					 */
-		char	map_name[BPF_OBJ_NAME_LEN];
-		__u32	map_ifindex;	/* ifindex of netdev to create on */
-		__u32	btf_fd;		/* fd pointing to a BTF type data */
-		__u32	btf_key_type_id;	/* BTF type_id of the key */
-		__u32	btf_value_type_id;	/* BTF type_id of the value */
-		__u32	btf_vmlinux_value_type_id;/* BTF type_id of a kernel-
-						   * struct stored as the
-						   * map value
-						   */
-	};
+	struct bpf_map_create_attr map_create;
 
-	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
-		__u32		map_fd;
-		__aligned_u64	key;
-		union {
-			__aligned_u64 value;
-			__aligned_u64 next_key;
-		};
-		__u64		flags;
-	};
+	struct bpf_map_lookup_elem_attr map_lookup_elem;
+
+	struct bpf_map_update_elem_attr map_update_elem;
+
+	struct bpf_map_delete_elem_attr map_delete_elem;
+
+	struct bpf_map_get_next_key_attr map_get_next_key;
 
 	struct { /* struct used by BPF_MAP_*_BATCH commands */
 		__aligned_u64	in_batch;	/* start batch,
@@ -1487,6 +1531,32 @@ union bpf_attr {
 		__u32		flags;		/* extra flags */
 	} prog_bind_map;
 
+	/* DEPRECATED: these are kept for compatibility purposes. */
+	struct { /* anonymous struct used by BPF_MAP_CREATE command */
+		__u32	map_type;
+		__u32	key_size;
+		__u32	value_size;
+		__u32	max_entries;
+		__u32	map_flags;
+		__u32	inner_map_fd;
+		__u32	numa_node;
+		char	map_name[BPF_OBJ_NAME_LEN];
+		__u32	map_ifindex;
+		__u32	btf_fd;
+		__u32	btf_key_type_id;
+		__u32	btf_value_type_id;
+		__u32	btf_vmlinux_value_type_id;
+	};
+
+	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+		__u32		map_fd;
+		__aligned_u64	key;
+		union {
+			__aligned_u64 value;
+			__aligned_u64 next_key;
+		};
+		__u64		flags;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
@@ -5230,7 +5300,7 @@ enum {
 };
 
 /* BPF ring buffer constants */
-enum {
+enum bpf_ringbuf_const {
 	BPF_RINGBUF_BUSY_BIT		= (1U << 31),
 	BPF_RINGBUF_DISCARD_BIT		= (1U << 30),
 	BPF_RINGBUF_HDR_SZ		= 8,
@@ -5272,12 +5342,6 @@ enum {
 	BPF_F_EXCLUDE_INGRESS	= (1ULL << 4),
 };
 
-#define __bpf_md_ptr(type, name)	\
-union {					\
-	type name;			\
-	__u64 :64;			\
-} __attribute__((aligned(8)))
-
 /* user accessible mirror of in-kernel sk_buff.
  * new fields can only be added to the end of this structure
  */
@@ -5456,7 +5520,9 @@ struct bpf_xdp_sock {
 	__u32 queue_id;
 };
 
-#define XDP_PACKET_HEADROOM 256
+enum {
+	XDP_PACKET_HEADROOM = 256,
+};
 
 /* User return codes for XDP prog type.
  * A valid XDP program must return one of these defined values. All other
@@ -5574,7 +5640,9 @@ struct sk_reuseport_md {
 	__bpf_md_ptr(struct bpf_sock *, migrating_sk);
 };
 
-#define BPF_TAG_SIZE	8
+enum {
+	BPF_TAG_SIZE = 8,
+};
 
 struct bpf_prog_info {
 	__u32 type;
-- 
2.30.2

[RFC 9/9] libbpf: use new-style syscall args

[Lorenz Bauer]

---
 tools/lib/bpf/bpf.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 7d1741ceaa32..79a9bfe214b0 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -506,15 +506,14 @@ int bpf_map_delete_elem(int fd, const void *key)
 
 int bpf_map_get_next_key(int fd, const void *key, void *next_key)
 {
-	union bpf_attr attr;
-	int ret;
+	struct bpf_map_get_next_key_attr attr = {
+		.map_fd		= fd,
+		.key		= key,
+		.next_key	= next_key,
+	};
 
-	memset(&attr, 0, sizeof(attr));
-	attr.map_fd = fd;
-	attr.key = ptr_to_u64(key);
-	attr.next_key = ptr_to_u64(next_key);
+	int ret = sys_bpf(BPF_MAP_GET_NEXT_KEY, (union bpf_attr *)&attr, sizeof(attr));
 
-	ret = sys_bpf(BPF_MAP_GET_NEXT_KEY, &attr, sizeof(attr));
 	return libbpf_err_errno(ret);
 }
 
-- 
2.30.2

Re: [RFC 9/9] libbpf: use new-style syscall args

[Alexei Starovoitov]

On Thu, Oct 14, 2021 at 03:34:36PM +0100, Lorenz Bauer wrote:
> ---
>  tools/lib/bpf/bpf.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 7d1741ceaa32..79a9bfe214b0 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -506,15 +506,14 @@ int bpf_map_delete_elem(int fd, const void *key)
>  
>  int bpf_map_get_next_key(int fd, const void *key, void *next_key)
>  {
> -	union bpf_attr attr;
> -	int ret;
> +	struct bpf_map_get_next_key_attr attr = {
> +		.map_fd		= fd,
> +		.key		= key,
> +		.next_key	= next_key,
> +	};

I see this change as the main advantage of additional uapi structs.
Note, though, that such stack savings don't strictly need uapi extensions.
Light skeleton already doing that.
Every command is using exactly the right amount of stack/bytes.
The full 'union bpf_attr' is never allocated.
Take a look at bpf_gen__map_freeze() in libbpf gen_loader.c.
In case of light skeleton it's not only the stack. Saving space
in loader's map was important.