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.
> [...]
>
> >
> > + /* 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.
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).
> > +
> > + 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
