| Message ID | bc55a274ea572d237bd091819f38502fa837abb5.1576193131.git.rdna@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Support replacing cgroup-bpf program in MULTI mode | expand |
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. [...] > > + /* 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. > + > + 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); > +
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. > [...] > > > > > + /* 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); > > +
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?... > > 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
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
diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c index 7671909ee1cb..6c7971ffe683 100644 --- a/tools/testing/selftests/bpf/test_cgroup_attach.c +++ b/tools/testing/selftests/bpf/test_cgroup_attach.c @@ -250,7 +250,7 @@ static int prog_load_cnt(int verdict, int val) BPF_LD_MAP_FD(BPF_REG_1, map_fd), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2), - BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */ + BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = val */ BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */ BPF_LD_MAP_FD(BPF_REG_1, cgroup_storage_fd), @@ -290,11 +290,11 @@ static int test_multiprog(void) { __u32 prog_ids[4], prog_cnt = 0, attach_flags, saved_prog_id; int cg1 = 0, cg2 = 0, cg3 = 0, cg4 = 0, cg5 = 0, key = 0; - int drop_prog, allow_prog[6] = {}, rc = 0; + int drop_prog, allow_prog[7] = {}, rc = 0; unsigned long long value; int i = 0; - for (i = 0; i < 6; i++) { + for (i = 0; i < ARRAY_SIZE(allow_prog); i++) { allow_prog[i] = prog_load_cnt(1, 1 << i); if (!allow_prog[i]) goto err; @@ -400,6 +400,58 @@ static int test_multiprog(void) assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); assert(value == 1 + 2 + 8 + 16); + /* 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, + ); + + 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); + + attach_opts.flags = BPF_F_REPLACE; + if (!bpf_prog_attach_xattr(&attach_opts)) { + log_err("Unexpected success with REPLACE alone"); + goto err; + } + assert(errno == EINVAL); + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; + + attach_opts.replace_prog_fd = -1; + if (!bpf_prog_attach_xattr(&attach_opts)) { + log_err("Unexpected success with bad replace fd"); + goto err; + } + assert(errno == EBADF); + + /* replacing a program that is not attached to cgroup should fail */ + attach_opts.replace_prog_fd = allow_prog[3]; + if (!bpf_prog_attach_xattr(&attach_opts)) { + log_err("Unexpected success: replace not-attached prog on cg1"); + goto err; + } + assert(errno == ENOENT); + attach_opts.replace_prog_fd = allow_prog[0]; + + /* replace 1st from the top program */ + if (bpf_prog_attach_xattr(&attach_opts)) { + log_err("Replace prog1 with prog7 on cg1"); + goto err; + } + value = 0; + assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0); + assert(system(PING_CMD) == 0); + assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); + assert(value == 64 + 2 + 8 + 16); + /* detach 3rd from bottom program and ping again */ errno = 0; if (!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS)) { @@ -414,7 +466,7 @@ static int test_multiprog(void) assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0); assert(system(PING_CMD) == 0); assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); - assert(value == 1 + 2 + 16); + assert(value == 64 + 2 + 16); /* detach 2nd from bottom program and ping again */ if (bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS)) { @@ -425,7 +477,7 @@ static int test_multiprog(void) assert(bpf_map_update_elem(map_fd, &key, &value, 0) == 0); assert(system(PING_CMD) == 0); assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); - assert(value == 1 + 2 + 4); + assert(value == 64 + 2 + 4); prog_cnt = 4; assert(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, BPF_F_QUERY_EFFECTIVE,
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(-)