Re: [PATCH v2 bpf-next 6/6] selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

[Andrey Ignatov <rdna@fb.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 09:25 -0800]:
> On Wed, Dec 18, 2019 at 8:58 AM Andrey Ignatov <rdna@fb.com> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-12 23:01 -0800]:
> > > On Thu, Dec 12, 2019 at 3:34 PM Andrey Ignatov <rdna@fb.com> wrote:
> > > >
> > > > Test replacement of a cgroup-bpf program attached with BPF_F_ALLOW_MULTI
> > > > and possible failure modes: invalid combination of flags, invalid
> > > > replace_bpf_fd, replacing a non-attachd to specified cgroup program.
> > > >
> > > > Example of program replacing:
> > > >
> > > >   # gdb -q ./test_cgroup_attach
> > > >   Reading symbols from /data/users/rdna/bin/test_cgroup_attach...done.
> > > >   ...
> > > >   Breakpoint 1, test_multiprog () at test_cgroup_attach.c:443
> > > >   443     test_cgroup_attach.c: No such file or directory.
> > > >   (gdb)
> > > >   [2]+  Stopped                 gdb -q ./test_cgroup_attach
> > > >   # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
> > > >   ID       AttachType      AttachFlags     Name
> > > >   35       egress          multi
> > > >   36       egress          multi
> > > >   # fg gdb -q ./test_cgroup_attach
> > > >   c
> > > >   Continuing.
> > > >   Detaching after fork from child process 361.
> > > >
> > > >   Breakpoint 2, test_multiprog () at test_cgroup_attach.c:454
> > > >   454     in test_cgroup_attach.c
> > > >   (gdb)
> > > >   [2]+  Stopped                 gdb -q ./test_cgroup_attach
> > > >   # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
> > > >   ID       AttachType      AttachFlags     Name
> > > >   41       egress          multi
> > > >   36       egress          multi
> > > >
> > > > Signed-off-by: Andrey Ignatov <rdna@fb.com>
> > > > ---
> > > >  .../selftests/bpf/test_cgroup_attach.c        | 62 +++++++++++++++++--
> > > >  1 file changed, 57 insertions(+), 5 deletions(-)
> > > >
> > >
> > > I second Alexei's sentiment. Having this as part of test_progs would
> > > certainly be better in terms of ensuring this doesn't accidentally
> > > breaks.
> >
> > OK, I converted both existing test_cgroup_attach and my test for
> > BPF_F_REPLACE to test_progs and will send v3 with this change.
> >
> 
> Great, thanks!
> 
> >
> > > [...]
> > >
> > > >
> > > > +       /* invalid input */
> > > > +
> > > > +       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
> > > > +               .target_fd              = cg1,
> > > > +               .prog_fd                = allow_prog[6],
> > > > +               .replace_prog_fd        = allow_prog[0],
> > > > +               .type                   = BPF_CGROUP_INET_EGRESS,
> > > > +               .flags                  = BPF_F_ALLOW_MULTI | BPF_F_REPLACE,
> > > > +       );
> > >
> > > This might cause compiler warnings (depending on compiler settings, of
> > > course). DECLARE_LIBBPF_OPTS does declare variable, so this is a
> > > situation of mixing code and variable declarations, which under C89
> > > (or whatever it's named, the older standard that kernel is trying to
> > > stick to for the most part) is not allowed.
> >
> > Yeah, I know about such a warning and expected it but didn't get it with
> > the current setup (what surprised me btw) and decided to keep it.
> 
> yeah, selftests compiler flags must not be as strict as kernel's, I guess?...

That I don't know :) I thought they should be similar but apparently
it's not the case.

> > The main reason I kept it is it's not actually clear how to separate
> > declaration and initialization of opts structure when
> > DECLARE_LIBBPF_OPTS is used since the macro does both things at once. In
> > selftests I can just switch to direct initialization (w/o the macro)
> > since libbpf and selftests are in sync, but for real use-cases there
> > should be smth else (e.g. INIT_LIBBPF_OPTS macro that does only
> > initialization of already declared struct).
> 
> For compiler, DECLARE_LIBBPF_OPTS is purely declaration, in the same
> sense as struct declaration+initialization is still just declaration.
> If you need to postpone some of the field initialization, then you can
> do that by assinging that field explicitly:
> 
> DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts,
>     .target_fd = cgl,
> );
> 
> 
> ... some code here ...
> attach_opts.prog_fd = allow_prog[6];
> 
> It is just a struct, DECLARE_LIBBPF_OPTS just hides memset to 0 and
> setting .sz correctly, nothing more.

Lol that makes sense of course. I'm not sure why I decided that all
fields should be specified in DECLARE_LIBBPF_OPTS() at once .. Thanks!

> > > > +
> > > > +       attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
> > > > +       if (!bpf_prog_attach_xattr(&attach_opts)) {
> > > > +               log_err("Unexpected success with OVERRIDE | REPLACE");
> > > > +               goto err;
> > > > +       }
> > > > +       assert(errno == EINVAL);
> > > > +
> >
> > --
> > Andrey Ignatov

-- 
Andrey Ignatov