Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Wed, 2019-12-18 21:59 -0800]:
> On Wed, Dec 18, 2019 at 6:17 PM Andrey Ignatov <rdna@xxxxxx> 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@xxxxxx>
> > ---
> > .../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
> >
--
Andrey Ignatov
