| Message ID | 829ef294f0395649f459334b48d4d9a6103a4fc1.1576031228.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 Tue, Dec 10, 2019 at 06:33:31PM -0800, Andrey Ignatov 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:442 > 442 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:453 > 453 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 | 61 +++++++++++++++++-- > 1 file changed, 56 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c > index 7671909ee1cb..b9148d752207 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,12 @@ 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; > + struct bpf_prog_attach_attr attach_attr; > 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 +401,56 @@ static int test_multiprog(void) > assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); > assert(value == 1 + 2 + 8 + 16); > > + /* invalid input */ > + > + memset(&attach_attr, 0, sizeof(attach_attr)); > + attach_attr.target_fd = cg1; > + attach_attr.prog_fd = allow_prog[6]; > + attach_attr.replace_prog_fd = allow_prog[0]; > + attach_attr.type = BPF_CGROUP_INET_EGRESS; > + attach_attr.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > + > + if (!bpf_prog_attach_xattr(&attach_attr)) { > + log_err("Unexpected success with OVERRIDE | REPLACE"); > + goto err; > + } > + assert(errno == EINVAL); > + > + attach_attr.flags = BPF_F_REPLACE; > + if (!bpf_prog_attach_xattr(&attach_attr)) { > + log_err("Unexpected success with REPLACE alone"); > + goto err; > + } > + assert(errno == EINVAL); > + attach_attr.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > + > + attach_attr.replace_prog_fd = -1; > + if (!bpf_prog_attach_xattr(&attach_attr)) { The whole set LGTM. I expect this attach bit will change based on the discussion in patch 4. > + log_err("Unexpected success with bad replace fd"); > + goto err; > + }
Martin Lau <kafai@fb.com> [Thu, 2019-12-12 10:26 -0800]: > On Tue, Dec 10, 2019 at 06:33:31PM -0800, Andrey Ignatov wrote: ... > > + attach_attr.replace_prog_fd = -1; > > + if (!bpf_prog_attach_xattr(&attach_attr)) { > The whole set LGTM. I expect this attach bit will change based on > the discussion in patch 4. Right, I'll change libbpf part and this test according to the feedback from Andrii and send v2. Thank you!
diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c index 7671909ee1cb..b9148d752207 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,12 @@ 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; + struct bpf_prog_attach_attr attach_attr; 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 +401,56 @@ static int test_multiprog(void) assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0); assert(value == 1 + 2 + 8 + 16); + /* invalid input */ + + memset(&attach_attr, 0, sizeof(attach_attr)); + attach_attr.target_fd = cg1; + attach_attr.prog_fd = allow_prog[6]; + attach_attr.replace_prog_fd = allow_prog[0]; + attach_attr.type = BPF_CGROUP_INET_EGRESS; + attach_attr.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; + + if (!bpf_prog_attach_xattr(&attach_attr)) { + log_err("Unexpected success with OVERRIDE | REPLACE"); + goto err; + } + assert(errno == EINVAL); + + attach_attr.flags = BPF_F_REPLACE; + if (!bpf_prog_attach_xattr(&attach_attr)) { + log_err("Unexpected success with REPLACE alone"); + goto err; + } + assert(errno == EINVAL); + attach_attr.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; + + attach_attr.replace_prog_fd = -1; + if (!bpf_prog_attach_xattr(&attach_attr)) { + 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_attr.replace_prog_fd = allow_prog[3]; + if (!bpf_prog_attach_xattr(&attach_attr)) { + log_err("Unexpected success: replace not-attached prog on cg1"); + goto err; + } + assert(errno == ENOENT); + attach_attr.replace_prog_fd = allow_prog[0]; + + /* replace 1st from the top program */ + if (bpf_prog_attach_xattr(&attach_attr)) { + 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 +465,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 +476,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:442 442 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:453 453 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 | 61 +++++++++++++++++-- 1 file changed, 56 insertions(+), 5 deletions(-)