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

* [RFC 0/9] uapi/bpf.h for robots
@ 2021-10-14 14:34 Lorenz Bauer
  2021-10-14 14:34 ` [RFC 1/9] bpf: name enums used from userspace Lorenz Bauer
                   ` (11 more replies)
  0 siblings, 12 replies; 25+ messages in thread
From: Lorenz Bauer @ 2021-10-14 14:34 UTC (permalink / raw)
  To: andrii, ast, daniel; +Cc: bpf, kernel-team, 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

^ permalink raw reply	[flat|nested] 25+ messages in thread