| Message ID | 31ac56887591418c2c098fabc14ad00de008e603.1576720240.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 Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > ... > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > (gdb) > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > ID AttachType AttachFlags Name > 2133 egress multi > 2134 egress multi > # fg > gdb -q --args ./test_progs --name=cgroup_attach_multi > (gdb) c > Continuing. > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > (gdb) > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > ID AttachType AttachFlags Name > 2139 egress multi > 2134 egress multi > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > --- > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > 1 file changed, 50 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > index 4eaab7435044..2ff21dbce179 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > + int allow_prog[7] = {-1}; > unsigned long long value; > __u32 duration = 0; > int i = 0; > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > + /* test replace */ > + > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > + attach_opts.replace_prog_fd = allow_prog[0]; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_override", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EINVAL); CHECK macro above can prints both in success and failure scenarios, which means that errno of bpf_prog_attach_xattr can be overriden by a bunch of other functions. So if this check is critical, you'd have to remember errno before calling CHECK. Same for all the check below. > + > + attach_opts.flags = BPF_F_REPLACE; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_no_multi", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EINVAL); > + > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > + attach_opts.replace_prog_fd = -1; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_bad_fd", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != EBADF); > + > + /* replacing a program that is not attached to cgroup should fail */ > + attach_opts.replace_prog_fd = allow_prog[3]; > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "fail_prog_replace_no_ent", "unexpected success\n")) > + goto err; > + CHECK_FAIL(errno != ENOENT); > + > + /* replace 1st from the top program */ > + attach_opts.replace_prog_fd = allow_prog[0]; > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > + BPF_CGROUP_INET_EGRESS, &attach_opts), > + "prog_replace", "errno=%d\n", errno)) > + goto err; > + > + value = 0; > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > + CHECK_FAIL(system(PING_CMD)); > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > + > /* detach 3rd from bottom program and ping again */ > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > "fail_prog_detach_from_cg3", "unexpected success\n")) > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > CHECK_FAIL(system(PING_CMD)); > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > - CHECK_FAIL(value != 1 + 2 + 16); > + CHECK_FAIL(value != 64 + 2 + 16); > > /* detach 2nd from bottom program and ping again */ > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > CHECK_FAIL(system(PING_CMD)); > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > - CHECK_FAIL(value != 1 + 2 + 4); > + CHECK_FAIL(value != 64 + 2 + 4); > > prog_cnt = 4; > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > -- > 2.17.1 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > ... > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > (gdb) > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > ID AttachType AttachFlags Name > > 2133 egress multi > > 2134 egress multi > > # fg > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > (gdb) c > > Continuing. > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > (gdb) > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > ID AttachType AttachFlags Name > > 2139 egress multi > > 2134 egress multi > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > --- > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > index 4eaab7435044..2ff21dbce179 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > + int allow_prog[7] = {-1}; > > unsigned long long value; > > __u32 duration = 0; > > int i = 0; > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > + /* test replace */ > > + > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > + attach_opts.replace_prog_fd = allow_prog[0]; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_override", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EINVAL); > > CHECK macro above can prints both in success and failure scenarios, > which means that errno of bpf_prog_attach_xattr can be overriden by a > bunch of other functions. So if this check is critical, you'd have to > remember errno before calling CHECK. Same for all the check below. If bpf_prog_attach_xattr finishes successfully (what is unexpected here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't be run at all so "success" case is not a problem. If bpf_prog_attach_xattr fails (what is expected) it has to set errno and this is the errno that will be checked by CHECK_FAIL, i.e. failure case is not a problem at all. If you mean printf() that is called from "PASS" branch of CHECK then I don't actually see a way to distinguish errno from failed bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and printf() from the CHECK() w/o changing CHECK() itself. I think CHECK() can be improved wrt errno so that it saves errno before calling anything that can affect it and restore it afterwards. But this is not specific to this patch so IMO it should be done separately with, ideally, checking that it doesn't break some other tests. > > + > > + attach_opts.flags = BPF_F_REPLACE; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EINVAL); > > + > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > + attach_opts.replace_prog_fd = -1; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != EBADF); > > + > > + /* replacing a program that is not attached to cgroup should fail */ > > + attach_opts.replace_prog_fd = allow_prog[3]; > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > + goto err; > > + CHECK_FAIL(errno != ENOENT); > > + > > + /* replace 1st from the top program */ > > + attach_opts.replace_prog_fd = allow_prog[0]; > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > + "prog_replace", "errno=%d\n", errno)) > > + goto err; > > + > > + value = 0; > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > + CHECK_FAIL(system(PING_CMD)); > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > + > > /* detach 3rd from bottom program and ping again */ > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > CHECK_FAIL(system(PING_CMD)); > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > - CHECK_FAIL(value != 1 + 2 + 16); > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > /* detach 2nd from bottom program and ping again */ > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > CHECK_FAIL(system(PING_CMD)); > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > - CHECK_FAIL(value != 1 + 2 + 4); > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > prog_cnt = 4; > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > -- > > 2.17.1 > >
On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > ... > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > (gdb) > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > ID AttachType AttachFlags Name > > > 2133 egress multi > > > 2134 egress multi > > > # fg > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > (gdb) c > > > Continuing. > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > (gdb) > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > ID AttachType AttachFlags Name > > > 2139 egress multi > > > 2134 egress multi > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > --- > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > index 4eaab7435044..2ff21dbce179 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > + int allow_prog[7] = {-1}; > > > unsigned long long value; > > > __u32 duration = 0; > > > int i = 0; > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > + /* test replace */ > > > + > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_override", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EINVAL); > > > > CHECK macro above can prints both in success and failure scenarios, > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > bunch of other functions. So if this check is critical, you'd have to > > remember errno before calling CHECK. Same for all the check below. > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > be run at all so "success" case is not a problem. > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > case is not a problem at all. > > If you mean printf() that is called from "PASS" branch of CHECK then I > don't actually see a way to distinguish errno from failed > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > printf() from the CHECK() w/o changing CHECK() itself. well, of course you can do that: err = bpf_xxx(...); saved_errno = errno; if (CHECK(!err, ...)) goto handle_err; if (CHECK_FAIL(saved_errno)) { ... } It's just more cumbersome, but nothing impossible. > > I think CHECK() can be improved wrt errno so that it saves errno before > calling anything that can affect it and restore it afterwards. But this > is not specific to this patch so IMO it should be done separately with, > ideally, checking that it doesn't break some other tests. That might be a way to do this, of course. We can add that later. I think that errno for error code is quite inconvenient, I wonder if it might be possible to retrofit kernel-style error code as a result instead. That's what we do for high-level libbpf API, it might be too late for low-level one though. > > > > + > > > + attach_opts.flags = BPF_F_REPLACE; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EINVAL); > > > + > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > + attach_opts.replace_prog_fd = -1; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != EBADF); > > > + > > > + /* replacing a program that is not attached to cgroup should fail */ > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > + goto err; > > > + CHECK_FAIL(errno != ENOENT); > > > + > > > + /* replace 1st from the top program */ > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > + "prog_replace", "errno=%d\n", errno)) > > > + goto err; > > > + > > > + value = 0; > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > + CHECK_FAIL(system(PING_CMD)); > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > + > > > /* detach 3rd from bottom program and ping again */ > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > CHECK_FAIL(system(PING_CMD)); > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > /* detach 2nd from bottom program and ping again */ > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > CHECK_FAIL(system(PING_CMD)); > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > prog_cnt = 4; > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > -- > > > 2.17.1 > > > > > -- > Andrey Ignatov
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > ... > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > (gdb) > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > ID AttachType AttachFlags Name > > > > 2133 egress multi > > > > 2134 egress multi > > > > # fg > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > (gdb) c > > > > Continuing. > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > (gdb) > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > ID AttachType AttachFlags Name > > > > 2139 egress multi > > > > 2134 egress multi > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > --- > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > + int allow_prog[7] = {-1}; > > > > unsigned long long value; > > > > __u32 duration = 0; > > > > int i = 0; > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > + /* test replace */ > > > > + > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > bunch of other functions. So if this check is critical, you'd have to > > > remember errno before calling CHECK. Same for all the check below. > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > be run at all so "success" case is not a problem. > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > case is not a problem at all. > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > don't actually see a way to distinguish errno from failed > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > printf() from the CHECK() w/o changing CHECK() itself. > > well, of course you can do that: > > err = bpf_xxx(...); > saved_errno = errno; > if (CHECK(!err, ...)) > goto handle_err; > if (CHECK_FAIL(saved_errno)) { ... } > > It's just more cumbersome, but nothing impossible. Oh, yeah, that would work, but I agree that it doesn't look great :) > > I think CHECK() can be improved wrt errno so that it saves errno before > > calling anything that can affect it and restore it afterwards. But this > > is not specific to this patch so IMO it should be done separately with, > > ideally, checking that it doesn't break some other tests. > > That might be a way to do this, of course. We can add that later. Sounds good. I can follow-up on the CHECK() change separately so that every test that does similar checks doesn't have to have additional code to deal with errno. > I > think that errno for error code is quite inconvenient, I wonder if it > might be possible to retrofit kernel-style error code as a result > instead. That's what we do for high-level libbpf API, it might be too > late for low-level one though. There are a bunch of places that actually do it in bpf.c, e.g. bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL for error conditions unrelated to sys_bpf. I also added similar thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. But for vast majority of cases where wrappers simply return whatever sys_bpf returned IMO it makes sense to preserve the semantics similar to that of sys_bpf since it's just thin wrappers. Again, just my opinion. > > > > + > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EINVAL); > > > > + > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > + attach_opts.replace_prog_fd = -1; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != EBADF); > > > > + > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > + goto err; > > > > + CHECK_FAIL(errno != ENOENT); > > > > + > > > > + /* replace 1st from the top program */ > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > + "prog_replace", "errno=%d\n", errno)) > > > > + goto err; > > > > + > > > > + value = 0; > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > + CHECK_FAIL(system(PING_CMD)); > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > + > > > > /* detach 3rd from bottom program and ping again */ > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > CHECK_FAIL(system(PING_CMD)); > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > CHECK_FAIL(system(PING_CMD)); > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > prog_cnt = 4; > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > -- > > > > 2.17.1 > > > > > > > > -- > > Andrey Ignatov
On Thu, Dec 19, 2019 at 1:09 PM Andrey Ignatov <rdna@fb.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > > ... > > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > > (gdb) > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > ID AttachType AttachFlags Name > > > > > 2133 egress multi > > > > > 2134 egress multi > > > > > # fg > > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > (gdb) c > > > > > Continuing. > > > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > > (gdb) > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > ID AttachType AttachFlags Name > > > > > 2139 egress multi > > > > > 2134 egress multi > > > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > > --- > > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > > + int allow_prog[7] = {-1}; > > > > > unsigned long long value; > > > > > __u32 duration = 0; > > > > > int i = 0; > > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > > > + /* test replace */ > > > > > + > > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > > bunch of other functions. So if this check is critical, you'd have to > > > > remember errno before calling CHECK. Same for all the check below. > > > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > > be run at all so "success" case is not a problem. > > > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > > case is not a problem at all. > > > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > > don't actually see a way to distinguish errno from failed > > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > > printf() from the CHECK() w/o changing CHECK() itself. > > > > well, of course you can do that: > > > > err = bpf_xxx(...); > > saved_errno = errno; > > if (CHECK(!err, ...)) > > goto handle_err; > > if (CHECK_FAIL(saved_errno)) { ... } > > > > It's just more cumbersome, but nothing impossible. > > Oh, yeah, that would work, but I agree that it doesn't look great :) > > > > I think CHECK() can be improved wrt errno so that it saves errno before > > > calling anything that can affect it and restore it afterwards. But this > > > is not specific to this patch so IMO it should be done separately with, > > > ideally, checking that it doesn't break some other tests. > > > > That might be a way to do this, of course. We can add that later. > > Sounds good. I can follow-up on the CHECK() change separately so that > every test that does similar checks doesn't have to have additional code > to deal with errno. great > > > I > > think that errno for error code is quite inconvenient, I wonder if it > > might be possible to retrofit kernel-style error code as a result > > instead. That's what we do for high-level libbpf API, it might be too > > late for low-level one though. > > There are a bunch of places that actually do it in bpf.c, e.g. > bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL > for error conditions unrelated to sys_bpf. I also added similar > thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. > > But for vast majority of cases where wrappers simply return whatever > sys_bpf returned IMO it makes sense to preserve the semantics similar to > that of sys_bpf since it's just thin wrappers. Again, just my opinion. > can we have both? keep setting errno, but return -errno as a result? If it's already a hybrid approach we are using, I don't think this will break anything. > > > > > > + > > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > + > > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > > + attach_opts.replace_prog_fd = -1; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != EBADF); > > > > > + > > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > > + goto err; > > > > > + CHECK_FAIL(errno != ENOENT); > > > > > + > > > > > + /* replace 1st from the top program */ > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > + "prog_replace", "errno=%d\n", errno)) > > > > > + goto err; > > > > > + > > > > > + value = 0; > > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > + CHECK_FAIL(system(PING_CMD)); > > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > > + > > > > > /* detach 3rd from bottom program and ping again */ > > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > > > prog_cnt = 4; > > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > -- > > > Andrey Ignatov > > -- > Andrey Ignatov
Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 13:19 -0800]: > On Thu, Dec 19, 2019 at 1:09 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Thu, 2019-12-19 12:44 -0800]: > > > On Wed, Dec 18, 2019 at 11:35 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> [Wed, 2019-12-18 21:59 -0800]: > > > > > On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@fb.com> wrote: > > > > > > > > > > > > Test replacing 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 --args ./test_progs --name=cgroup_attach_multi > > > > > > ... > > > > > > Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 > > > > > > (gdb) > > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > > ID AttachType AttachFlags Name > > > > > > 2133 egress multi > > > > > > 2134 egress multi > > > > > > # fg > > > > > > gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > (gdb) c > > > > > > Continuing. > > > > > > > > > > > > Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 > > > > > > (gdb) > > > > > > [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi > > > > > > # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 > > > > > > ID AttachType AttachFlags Name > > > > > > 2139 egress multi > > > > > > 2134 egress multi > > > > > > > > > > > > Signed-off-by: Andrey Ignatov <rdna@fb.com> > > > > > > --- > > > > > > .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- > > > > > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > index 4eaab7435044..2ff21dbce179 100644 > > > > > > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c > > > > > > @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; > > > > > > + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); > > > > > > + int allow_prog[7] = {-1}; > > > > > > unsigned long long value; > > > > > > __u32 duration = 0; > > > > > > int i = 0; > > > > > > @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > CHECK_FAIL(value != 1 + 2 + 8 + 16); > > > > > > > > > > > > + /* test replace */ > > > > > > + > > > > > > + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; > > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_override", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > > > > > CHECK macro above can prints both in success and failure scenarios, > > > > > which means that errno of bpf_prog_attach_xattr can be overriden by a > > > > > bunch of other functions. So if this check is critical, you'd have to > > > > > remember errno before calling CHECK. Same for all the check below. > > > > > > > > If bpf_prog_attach_xattr finishes successfully (what is unexpected > > > > here), `goto err` will be taken and `CHECK_FAIL(errno != EINVAL)` won't > > > > be run at all so "success" case is not a problem. > > > > > > > > If bpf_prog_attach_xattr fails (what is expected) it has to set errno > > > > and this is the errno that will be checked by CHECK_FAIL, i.e. failure > > > > case is not a problem at all. > > > > > > > > If you mean printf() that is called from "PASS" branch of CHECK then I > > > > don't actually see a way to distinguish errno from failed > > > > bpf_prog_attach_xattr (what would mean "PASS" for the CHECK) and > > > > printf() from the CHECK() w/o changing CHECK() itself. > > > > > > well, of course you can do that: > > > > > > err = bpf_xxx(...); > > > saved_errno = errno; > > > if (CHECK(!err, ...)) > > > goto handle_err; > > > if (CHECK_FAIL(saved_errno)) { ... } > > > > > > It's just more cumbersome, but nothing impossible. > > > > Oh, yeah, that would work, but I agree that it doesn't look great :) > > > > > > I think CHECK() can be improved wrt errno so that it saves errno before > > > > calling anything that can affect it and restore it afterwards. But this > > > > is not specific to this patch so IMO it should be done separately with, > > > > ideally, checking that it doesn't break some other tests. > > > > > > That might be a way to do this, of course. We can add that later. > > > > Sounds good. I can follow-up on the CHECK() change separately so that > > every test that does similar checks doesn't have to have additional code > > to deal with errno. > > great > > > > > > I > > > think that errno for error code is quite inconvenient, I wonder if it > > > might be possible to retrofit kernel-style error code as a result > > > instead. That's what we do for high-level libbpf API, it might be too > > > late for low-level one though. > > > > There are a bunch of places that actually do it in bpf.c, e.g. > > bpf_prog_test_run_xattr() and bpf_load_program_xattr() return -EINVAL > > for error conditions unrelated to sys_bpf. I also added similar > > thing in patch 4 (libbpf part) for `!OPTS_VALID` scenario. > > > > But for vast majority of cases where wrappers simply return whatever > > sys_bpf returned IMO it makes sense to preserve the semantics similar to > > that of sys_bpf since it's just thin wrappers. Again, just my opinion. > > > > can we have both? keep setting errno, but return -errno as a result? > If it's already a hybrid approach we are using, I don't think this > will break anything. I'd be hesitant to do it for all existing interfaces in bpf.h, since it's quite common patter to have code like: if (some_func() == -1) /* handle_error */ , instead of `if (some_func() < 0)`. Even `git grep ' == -1' tools/testing/selftests/bpf` shows a lot of such call sites, so it may break a lot of stuff. Anyway IMO it's out of the scope of my patch set :) > > > > > > + > > > > > > + attach_opts.flags = BPF_F_REPLACE; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_no_multi", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EINVAL); > > > > > > + > > > > > > + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; > > > > > > + attach_opts.replace_prog_fd = -1; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_bad_fd", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != EBADF); > > > > > > + > > > > > > + /* replacing a program that is not attached to cgroup should fail */ > > > > > > + attach_opts.replace_prog_fd = allow_prog[3]; > > > > > > + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "fail_prog_replace_no_ent", "unexpected success\n")) > > > > > > + goto err; > > > > > > + CHECK_FAIL(errno != ENOENT); > > > > > > + > > > > > > + /* replace 1st from the top program */ > > > > > > + attach_opts.replace_prog_fd = allow_prog[0]; > > > > > > + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, > > > > > > + BPF_CGROUP_INET_EGRESS, &attach_opts), > > > > > > + "prog_replace", "errno=%d\n", errno)) > > > > > > + goto err; > > > > > > + > > > > > > + value = 0; > > > > > > + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > + CHECK_FAIL(system(PING_CMD)); > > > > > > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > + CHECK_FAIL(value != 64 + 2 + 8 + 16); > > > > > > + > > > > > > /* detach 3rd from bottom program and ping again */ > > > > > > if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), > > > > > > "fail_prog_detach_from_cg3", "unexpected success\n")) > > > > > > @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > - CHECK_FAIL(value != 1 + 2 + 16); > > > > > > + CHECK_FAIL(value != 64 + 2 + 16); > > > > > > > > > > > > /* detach 2nd from bottom program and ping again */ > > > > > > if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), > > > > > > @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) > > > > > > CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); > > > > > > CHECK_FAIL(system(PING_CMD)); > > > > > > CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); > > > > > > - CHECK_FAIL(value != 1 + 2 + 4); > > > > > > + CHECK_FAIL(value != 64 + 2 + 4); > > > > > > > > > > > > prog_cnt = 4; > > > > > > CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS, > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > > > -- > > > > Andrey Ignatov > > > > -- > > Andrey Ignatov
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c index 4eaab7435044..2ff21dbce179 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c @@ -78,7 +78,8 @@ void test_cgroup_attach_multi(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 allow_prog[6] = {-1}; + DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, attach_opts); + int allow_prog[7] = {-1}; unsigned long long value; __u32 duration = 0; int i = 0; @@ -189,6 +190,52 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); CHECK_FAIL(value != 1 + 2 + 8 + 16); + /* test replace */ + + attach_opts.flags = BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE; + attach_opts.replace_prog_fd = allow_prog[0]; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_override", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EINVAL); + + attach_opts.flags = BPF_F_REPLACE; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_no_multi", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EINVAL); + + attach_opts.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE; + attach_opts.replace_prog_fd = -1; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_bad_fd", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != EBADF); + + /* replacing a program that is not attached to cgroup should fail */ + attach_opts.replace_prog_fd = allow_prog[3]; + if (CHECK(!bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "fail_prog_replace_no_ent", "unexpected success\n")) + goto err; + CHECK_FAIL(errno != ENOENT); + + /* replace 1st from the top program */ + attach_opts.replace_prog_fd = allow_prog[0]; + if (CHECK(bpf_prog_attach_xattr(allow_prog[6], cg1, + BPF_CGROUP_INET_EGRESS, &attach_opts), + "prog_replace", "errno=%d\n", errno)) + goto err; + + value = 0; + CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); + CHECK_FAIL(system(PING_CMD)); + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); + CHECK_FAIL(value != 64 + 2 + 8 + 16); + /* detach 3rd from bottom program and ping again */ if (CHECK(!bpf_prog_detach2(0, cg3, BPF_CGROUP_INET_EGRESS), "fail_prog_detach_from_cg3", "unexpected success\n")) @@ -202,7 +249,7 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); CHECK_FAIL(system(PING_CMD)); CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); - CHECK_FAIL(value != 1 + 2 + 16); + CHECK_FAIL(value != 64 + 2 + 16); /* detach 2nd from bottom program and ping again */ if (CHECK(bpf_prog_detach2(-1, cg4, BPF_CGROUP_INET_EGRESS), @@ -213,7 +260,7 @@ void test_cgroup_attach_multi(void) CHECK_FAIL(bpf_map_update_elem(map_fd, &key, &value, 0)); CHECK_FAIL(system(PING_CMD)); CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value)); - CHECK_FAIL(value != 1 + 2 + 4); + CHECK_FAIL(value != 64 + 2 + 4); prog_cnt = 4; CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS,
Test replacing 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 --args ./test_progs --name=cgroup_attach_multi ... Breakpoint 1, test_cgroup_attach_multi () at cgroup_attach_multi.c:227 (gdb) [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 ID AttachType AttachFlags Name 2133 egress multi 2134 egress multi # fg gdb -q --args ./test_progs --name=cgroup_attach_multi (gdb) c Continuing. Breakpoint 2, test_cgroup_attach_multi () at cgroup_attach_multi.c:233 (gdb) [1]+ Stopped gdb -q --args ./test_progs --name=cgroup_attach_multi # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1 ID AttachType AttachFlags Name 2139 egress multi 2134 egress multi Signed-off-by: Andrey Ignatov <rdna@fb.com> --- .../bpf/prog_tests/cgroup_attach_multi.c | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-)