diff mbox series

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

Message ID 20211014143436.54470-10-lmb@cloudflare.com (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series uapi/bpf.h for robots | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf fail VM_Test
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-next fail VM_Test
bpf/vmtest-bpf-next-PR fail PR summary

Commit Message

Lorenz Bauer Oct. 14, 2021, 2:34 p.m. UTC 
---
 include/uapi/linux/bpf.h | 60 ++++++++++++++++++++++++++++++++--------
 kernel/bpf/syscall.c     | 18 ++++++------
 2 files changed, 58 insertions(+), 20 deletions(-)

Comments

Alexei Starovoitov Oct. 20, 2021, 5:15 p.m. UTC | #1 
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.
Lorenz Bauer Oct. 21, 2021, 3:59 p.m. UTC | #2 
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
Alexei Starovoitov Oct. 27, 2021, 6:20 p.m. UTC | #3 
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.
Lorenz Bauer Oct. 29, 2021, 2:01 p.m. UTC | #4 
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
diff mbox series

Patch

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))