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