Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Wed, 2019-12-18 09:25 -0800]:
> On Wed, Dec 18, 2019 at 8:58 AM Andrey Ignatov <rdna@xxxxxx> wrote:
> >
> > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Thu, 2019-12-12 23:01 -0800]:
> > > On Thu, Dec 12, 2019 at 3:34 PM Andrey Ignatov <rdna@xxxxxx> 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@xxxxxx>
> > > > ---
> > > > .../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
