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?...
>
> 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.
>
>
> > > +
> > > + 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
