Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, May 25, 2023 at 2:51 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
> > On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> >>>
> >>> And there were a bunch of other similar changes. Please take a thorough
> >>> look and suggest more changes or which changes to drop. I'm not married
> >>> to any of them, it just felt like a good improvement.
> >>
> >> Agree that current layout sucks, but ...
> >>
> >>>   include/uapi/linux/bpf.h       | 235 +++++++++++++++++++++++++++------
> >>>   kernel/bpf/syscall.c           |  40 +++---
> >>>   tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> >>>   3 files changed, 405 insertions(+), 105 deletions(-)
> >>
> >> ... the diff makes it worse. The diffstat for "nop" change is a red flag.
> >
> > Only 100 lines are a real "nop" change to copy/paste existing fields
> > that are in unnamed fields. The rest is a value add.
> >
> > I don't think the deal is in stats, though, right?
> >
> >>> +     /*
> >>> +      * LEGACY anonymous substructs, for backwards compatibility.
> >>> +      * Each of the below anonymous substructs are ABI compatible with one
> >>> +      * of the above named substructs. Please use named substructs.
> >>> +      */
> >>
> >> All of them cannot be removed. This bagage will be a forever eyesore.
> >> Currently it's not pretty. The diffs make uapi file just ugly.
> >> Especially considering how 'named' and 'legacy' will start diverging.
> >
> > We have to allow "divergence" (only in the sense that new fields only
> > go into named substructs, but the existing fields stay fixed, of
> > course), to avoid more naming conflicts. If that wasn't the case,
> > using struct_group() macro could have been used to avoid a copy/paste
> > of those anonymous field/struct copies.
> >
> > So I'm not happy about those 100 lines copy paste of fixed fields
> > either, but at least that would get us out of the current global
> > naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.
> >
> >> New commands are thankfully named. We've learned the lesson,
> >
> > Unfortunately, the problem is that unnamed commands are the ones that
> > are most likely to keep evolving.
> >
> >> but prior mistake is unfixable. We have to live with it.
> >
> > Ok, too bad, but it's fine. It was worth a try.
> >
> > I tried to come up with something like struct_group() approach to
> > minimize code changes in UAPI header, but we have a more complicated
> > situation where part of struct has to be both anonymous and named,
> > while another part (newly added fields) should go only to named parts.
> > And that doesn't seem to be possible to support with a macro,
> > unfortunately.
>
> Nice idea on the struct_group()-like approach, but agree that this is
> going to be tough given we need to divert anonymous and named parts as
> you mention. One other wild thought ... we remove the bpf_attr entirely
> from the uapi header, and have a kernel/bpf/bpf.cmd description and
> generate the bpf_attr into a uapi header via script which the main header
> can include. Kind of similar to the suggestion, but more flexible than
> macro magic. We also have things like syscall table header generated via
> script.. so it wouldn't be first. Still doesn't remove the eyesore, just
> packages it differently. ;/

There are two more ways, neither is that pretty. But I'll just outline
them here for completeness.

First, we can define about 6 variants (one for each command with anon
field) of macro with different numbers of arguments, one for each
existing field. Replace all semicolons with commas and do something
like this (we can prettify the below some more, I didn't want to waste
too much time on this demo):

#define __bpf_cmd4(type, f1, f2, f3, f4, new_fields...)        \
       struct {                                                \
               f1; f2; f3; f4;                                 \
       };                                                      \
       struct type {                                           \
               f1; f2; f3; f4;                                 \
               new_fields                                      \
       }

       /* BPF_OBJ_PIN command */
       __bpf_cmd4(bpf_obj_pin_attr,
               __aligned_u64   pathname,
               __u32           bpf_fd,
               __u32           file_flags,
               /* Same as dirfd in openat() syscall; see openat(2)
                * manpage for details of path FD and pathname semantics;
                * path_fd should accompanied by BPF_F_PATH_FD flag set in
                * file_flags field, otherwise it should be set to zero;
                * if BPF_F_PATH_FD flag is not set, AT_FDCWD is assumed.
                */
               __s32           path_fd,
               __u32           token_fd;
       ) obj_pin;

Note that I also added `__u32 token_fd;` as a demonstration how we can
new fields, and that new fields will have proper semicolons at the
end. The largest command (BPF_PROG_LOAD) will need 28 arg variant, but
that can be fit in few lines pretty cleanly, if the overall approach
would be deemed acceptable.

This approach also has a slight downside that we can rename fields
(e.g. for BPF_BTF_LOAD command). We still can split out dedicated new
named structs. So too big of a deal.


Second approach. If it's mostly about aesthetics, then we can add
include/uapi/linux/bpf_legacy.h, where we put all these unnamed fields
and structs in one stashed away place, and then in original
include/uapi/linux/bpf.h header we just

union bpf_attr {
   ... named structs and fields go here ...

/* include backwards compat legacy anon fields/structs */
#include "bpf_legacy.h"
};

This way this eyesore will be somewhat hidden away (but still lookup-able).


Curious if any of the above is more palatable?


>
> Thanks,
> Daniel