bpf: Support replacing cgroup-bpf program in MULTI mode

Introduce a new bpf_prog_attach_xattr function that, in addition to program fd, target fd and attach type, accepts an extendable struct bpf_prog_attach_opts.

bpf_prog_attach_opts relies on DECLARE_LIBBPF_OPTS macro to maintain backward and forward compatibility and has the following "optional" attach attributes:

* existing attach_flags, since it's not required when attaching in NONE mode. Even though it's quite often used in MULTI and OVERRIDE mode it seems to be a good idea to reduce number of arguments to bpf_prog_attach_xattr;

* newly introduced attribute of BPF_PROG_ATTACH command: replace_prog_fd that is fd of previously attached cgroup-bpf program to replace if BPF_F_REPLACE flag is used.

The new function is named to be consistent with other xattr-functions (bpf_prog_test_run_xattr, bpf_create_map_xattr, bpf_load_program_xattr).

The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS macro.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/lib/bpf/bpf.c      | 16 +++++++++++++++-
 tools/lib/bpf/bpf.h      | 11 +++++++++++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +4 -0

index 98596e15390f..ebb4f8d71bdb 100644
@@ -466,6 +466,17 @@ int bpf_obj_get(const char *pathname)
 
 int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
                    unsigned int flags)
+{
+       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts,
+               .flags = flags,
+       );

tools/lib/bpf/bpf.c +0 -0

        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

Quoted 43 lines

Introduce a new bpf_prog_attach_xattr function that, in addition to program fd, target fd and attach type, accepts an extendable struct bpf_prog_attach_opts.

bpf_prog_attach_opts relies on DECLARE_LIBBPF_OPTS macro to maintain backward and forward compatibility and has the following "optional" attach attributes:

* existing attach_flags, since it's not required when attaching in NONE mode. Even though it's quite often used in MULTI and OVERRIDE mode it seems to be a good idea to reduce number of arguments to bpf_prog_attach_xattr;

* newly introduced attribute of BPF_PROG_ATTACH command: replace_prog_fd that is fd of previously attached cgroup-bpf program to replace if BPF_F_REPLACE flag is used.

The new function is named to be consistent with other xattr-functions (bpf_prog_test_run_xattr, bpf_create_map_xattr, bpf_load_program_xattr).

The struct bpf_prog_attach_opts is supposed to be used with DECLARE_LIBBPF_OPTS macro.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/lib/bpf/bpf.c      | 16 +++++++++++++++-
 tools/lib/bpf/bpf.h      | 11 +++++++++++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

tools/lib/bpf/bpf.c +4 -0

index 98596e15390f..ebb4f8d71bdb 100644
@@ -466,6 +466,17 @@ int bpf_obj_get(const char *pathname)
 
 int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
                    unsigned int flags)
+{
+       DECLARE_LIBBPF_OPTS(bpf_prog_attach_opts, opts,
+               .flags = flags,
+       );

tools/lib/bpf/bpf.c int bpf_obj_get(const char *pathname) · line 482

Quoted 10 lines

tools/lib/bpf/bpf.c +7 -0

+
+       return bpf_prog_attach_xattr(prog_fd, target_fd, type, &opts);
+}
+
+int bpf_prog_attach_xattr(int prog_fd, int target_fd,
+                         enum bpf_attach_type type,
+                         const struct bpf_prog_attach_opts *opts)
 {
        union bpf_attr attr;
 

You need to validate opts with OPTS_VALID macro (see btf_dump__emit_type_decl() for simple example).

tools/lib/bpf/bpf.c int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type, · line 490

Quoted 9 lines

tools/lib/bpf/bpf.c +4 -1

@@ -473,7 +484,10 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
        attr.target_fd     = target_fd;
        attr.attach_bpf_fd = prog_fd;
        attr.attach_type   = type;
-       attr.attach_flags  = flags;
+       if (opts) {
+               attr.attach_flags = opts->flags;
+               attr.replace_bpf_fd = opts->replace_prog_fd;
+       }

Please use OPTS_GET() macro to fetch values from opts struct and provide default value if they are not specified.

Quoted 5 lines

tools/lib/bpf/bpf.c +0 -0

        return sys_bpf(BPF_PROG_ATTACH, &attr, sizeof(attr));
 }

[...]

Convert test_cgroup_attach to prog_tests.

This change does a lot of things but in many cases it's pretty expensive to separate them, so they go in one commit. Nevertheless the logic is ketp as is and changes made are just moving things around, simplifying them (w/o changing the meaning of the tests) and making prog_tests compatible:

* split the 3 tests in the file into 3 separate files in prog_tests/;

* rename the test functions to test_<file_base_name>;

* remove unused includes, constants, variables and functions from every test;

* replace if-s with or if (CHECK()) where additional context should be logged and with if (CHECK_FAIL()) where line number is enough;

* switch from log_err() to logging via CHECK();

* replace assert-s with CHECK_FAIL() to avoid crashing the whole test_progs if one assertion fails;

* replace cgroup_helpers with test__join_cgroup() in cgroup_attach_override only, other tests need more fine-grained control for cgroup creation/deletion so cgroup_helpers are still used there;

* simplify cgroup_attach_autodetach by switching to easiest possible program since this test doesn't really need such a complicated program as cgroup_attach_multi does;

* remove test_cgroup_attach.c itself.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++
 .../bpf/prog_tests/cgroup_attach_multi.c      | 238 ++++++++
 .../bpf/prog_tests/cgroup_attach_override.c   | 148 +++++
 .../selftests/bpf/test_cgroup_attach.c        | 571 ------------------
 6 files changed, 498 insertions(+), 574 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c
 delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +46 -0

+
+       if (CHECK_FAIL(setup_cgroup_environment()))
+               goto err;
+
+       cg1 = create_and_get_cgroup("/cg1");
+       if (CHECK_FAIL(cg1 < 0))
+               goto err;
+       cg2 = create_and_get_cgroup("/cg1/cg2");
+       if (CHECK_FAIL(cg2 < 0))
+               goto err;
+       cg3 = create_and_get_cgroup("/cg1/cg2/cg3");
+       if (CHECK_FAIL(cg3 < 0))
+               goto err;
+       cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4");
+       if (CHECK_FAIL(cg4 < 0))
+               goto err;
+       cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5");
+       if (CHECK_FAIL(cg5 < 0))
+               goto err;
+
+       if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5")))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog0_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                  BPF_F_ALLOW_MULTI),
+                 "fail_same_prog_attach_to_cg1", "unexpected success\n"))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog1_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_OVERRIDE),
+                 "prog2_attach_to_cg2_override", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog3_attach_to_cg3_multi", "errno=%d\n", errno))

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +5 -0

+
+       CHECK_FAIL(system(PING_CMD));
+       CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value));
+       CHECK_FAIL(value != 1 + 2 + 8 + 32);
+

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -9

-
-       if (errors)
-               printf("test_cgroup_attach:FAIL\n");
-       else
-               printf("test_cgroup_attach:PASS\n");
-
-       return errors ? EXIT_FAILURE : EXIT_SUCCESS;
-}
--

2.17.1

Quoted 49 lines

Convert test_cgroup_attach to prog_tests.

This change does a lot of things but in many cases it's pretty expensive to separate them, so they go in one commit. Nevertheless the logic is ketp as is and changes made are just moving things around, simplifying them (w/o changing the meaning of the tests) and making prog_tests compatible:

* split the 3 tests in the file into 3 separate files in prog_tests/;

* rename the test functions to test_<file_base_name>;

* remove unused includes, constants, variables and functions from every test;

* replace if-s with or if (CHECK()) where additional context should be logged and with if (CHECK_FAIL()) where line number is enough;

* switch from log_err() to logging via CHECK();

* replace assert-s with CHECK_FAIL() to avoid crashing the whole test_progs if one assertion fails;

* replace cgroup_helpers with test__join_cgroup() in cgroup_attach_override only, other tests need more fine-grained control for cgroup creation/deletion so cgroup_helpers are still used there;

* simplify cgroup_attach_autodetach by switching to easiest possible program since this test doesn't really need such a complicated program as cgroup_attach_multi does;

* remove test_cgroup_attach.c itself.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++
 .../bpf/prog_tests/cgroup_attach_multi.c      | 238 ++++++++
 .../bpf/prog_tests/cgroup_attach_override.c   | 148 +++++
 .../selftests/bpf/test_cgroup_attach.c        | 571 ------------------
 6 files changed, 498 insertions(+), 574 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c
 delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c

[...]

Quoted 50 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +50 -0

+
+       if (CHECK_FAIL(setup_cgroup_environment()))
+               goto err;
+
+       cg1 = create_and_get_cgroup("/cg1");
+       if (CHECK_FAIL(cg1 < 0))
+               goto err;
+       cg2 = create_and_get_cgroup("/cg1/cg2");
+       if (CHECK_FAIL(cg2 < 0))
+               goto err;
+       cg3 = create_and_get_cgroup("/cg1/cg2/cg3");
+       if (CHECK_FAIL(cg3 < 0))
+               goto err;
+       cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4");
+       if (CHECK_FAIL(cg4 < 0))
+               goto err;
+       cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5");
+       if (CHECK_FAIL(cg5 < 0))
+               goto err;
+
+       if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5")))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog0_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                  BPF_F_ALLOW_MULTI),
+                 "fail_same_prog_attach_to_cg1", "unexpected success\n"))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog1_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_OVERRIDE),
+                 "prog2_attach_to_cg2_override", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog3_attach_to_cg3_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS,
+                           BPF_F_ALLOW_OVERRIDE),

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c line 147

Quoted 6 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +6 -0

+                 "prog4_attach_to_cg4_override", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0),
+                 "prog5_attach_to_cg5_none", "errno=%d\n", errno))
+               goto err;

nit: this looks like a good candidate for a loop...

Quoted 4 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +4 -0

+       CHECK_FAIL(system(PING_CMD));
+       CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value));
+       CHECK_FAIL(value != 1 + 2 + 8 + 32);
+

[...]

Quoted 11 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -11

-int main(void)
-{
-       int (*tests[])(void) = {
-               test_foo_bar,
-               test_multiprog,
-               test_autodetach,
-       };
-       int errors = 0;
-       int i;
-
-       for (i = 0; i < ARRAY_SIZE(tests); i++)

tools/testing/selftests/bpf/test_cgroup_attach.c line 563

Quoted 2 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -2

-               if (tests[i]())
-                       errors++;

Depending on what you think is better structure (I couldn't follow through entire test logic...), you could have done this as a single test with 3 subtests. Search for test__start_subtest(). If that saves you some duplication, I think it's worth converting (which will be 1-to-1 with original structure).

But either way thanks a lot for doing the convertion, brings as another step closer to more uniform and consolidated testing infra.

Quoted 11 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -9

-
-       if (errors)
-               printf("test_cgroup_attach:FAIL\n");
-       else
-               printf("test_cgroup_attach:PASS\n");
-
-       return errors ? EXIT_FAILURE : EXIT_SUCCESS;
-}
--

2.17.1

Andrii Nakryiko <[email protected]> [Wed, 2019-12-18 22:11 -0800]:

Quoted 103 lines · Andrey Ignatov

Quoted 49 lines

Convert test_cgroup_attach to prog_tests.

This change does a lot of things but in many cases it's pretty expensive to separate them, so they go in one commit. Nevertheless the logic is ketp as is and changes made are just moving things around, simplifying them (w/o changing the meaning of the tests) and making prog_tests compatible:

* split the 3 tests in the file into 3 separate files in prog_tests/;

* rename the test functions to test_<file_base_name>;

* remove unused includes, constants, variables and functions from every test;

* replace if-s with or if (CHECK()) where additional context should be logged and with if (CHECK_FAIL()) where line number is enough;

* switch from log_err() to logging via CHECK();

* replace assert-s with CHECK_FAIL() to avoid crashing the whole test_progs if one assertion fails;

* replace cgroup_helpers with test__join_cgroup() in cgroup_attach_override only, other tests need more fine-grained control for cgroup creation/deletion so cgroup_helpers are still used there;

* simplify cgroup_attach_autodetach by switching to easiest possible program since this test doesn't really need such a complicated program as cgroup_attach_multi does;

* remove test_cgroup_attach.c itself.

Signed-off-by Andrey Ignatov <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++
 .../bpf/prog_tests/cgroup_attach_multi.c      | 238 ++++++++
 .../bpf/prog_tests/cgroup_attach_override.c   | 148 +++++
 .../selftests/bpf/test_cgroup_attach.c        | 571 ------------------
 6 files changed, 498 insertions(+), 574 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c
 delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c

[...]

Quoted 50 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +50 -0

+
+       if (CHECK_FAIL(setup_cgroup_environment()))
+               goto err;
+
+       cg1 = create_and_get_cgroup("/cg1");
+       if (CHECK_FAIL(cg1 < 0))
+               goto err;
+       cg2 = create_and_get_cgroup("/cg1/cg2");
+       if (CHECK_FAIL(cg2 < 0))
+               goto err;
+       cg3 = create_and_get_cgroup("/cg1/cg2/cg3");
+       if (CHECK_FAIL(cg3 < 0))
+               goto err;
+       cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4");
+       if (CHECK_FAIL(cg4 < 0))
+               goto err;
+       cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5");
+       if (CHECK_FAIL(cg5 < 0))
+               goto err;
+
+       if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5")))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog0_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
+                                  BPF_F_ALLOW_MULTI),
+                 "fail_same_prog_attach_to_cg1", "unexpected success\n"))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog1_attach_to_cg1_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_OVERRIDE),
+                 "prog2_attach_to_cg2_override", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS,
+                                 BPF_F_ALLOW_MULTI),
+                 "prog3_attach_to_cg3_multi", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS,
+                           BPF_F_ALLOW_OVERRIDE),

Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c line 147

Quoted 6 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +6 -0

+                 "prog4_attach_to_cg4_override", "errno=%d\n", errno))
+               goto err;
+
+       if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0),
+                 "prog5_attach_to_cg5_none", "errno=%d\n", errno))
+               goto err;

nit: this looks like a good candidate for a loop...

These tests can benefit from a lot of cleanup steps and yeah, this can be one of them. I'd prefer to deal with it separately though since it shouldn't be a blocker for this patch set. That's why I preserved this logic with just adding checks.

Quoted 19 lines

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c new file +4 -0

+       CHECK_FAIL(system(PING_CMD));
+       CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value));
+       CHECK_FAIL(value != 1 + 2 + 8 + 32);
+

[...]

Quoted 11 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -11

-int main(void)
-{
-       int (*tests[])(void) = {
-               test_foo_bar,
-               test_multiprog,
-               test_autodetach,
-       };
-       int errors = 0;
-       int i;
-
-       for (i = 0; i < ARRAY_SIZE(tests); i++)

Quoted 8 lines

tools/testing/selftests/bpf/test_cgroup_attach.c line 563

Quoted 2 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -2

-               if (tests[i]())
-                       errors++;

Depending on what you think is better structure (I couldn't follow through entire test logic...), you could have done this as a single test with 3 subtests. Search for test__start_subtest(). If that saves you some duplication, I think it's worth converting (which will be 1-to-1 with original structure).

Yeah, I saw test__start_subtest() and checked if it would fit here. IMO it wouldn't since these tests don't have much in common and can't be converted to, say, parametrized tests that differ only in input that can be easily described by a struct. They do cover parts of same feature (cgroup attach) but focus on different parts of it. That's why separate files. This actually helped to identify and remove a bunch of junk from each test (like simplifying testing prog in autodetach).

Quoted 5 lines

But either way thanks a lot for doing the convertion, brings as another step closer to more uniform and consolidated testing infra.

Quoted 11 lines

Quoted 11 lines

tools/testing/selftests/bpf/test_cgroup_attach.c deleted +0 -9

-
-       if (errors)
-               printf("test_cgroup_attach:FAIL\n");
-       else
-               printf("test_cgroup_attach:PASS\n");
-
-       return errors ? EXIT_FAILURE : EXIT_SUCCESS;
-}
--

2.17.1

--
Andrey Ignatov

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -3

+
+       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

Quoted 52 lines

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);
 

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c void test_cgroup_attach_multi(void) · line 201

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +9 -0

+       /* 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.

Quoted 61 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -3

+
+       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 <[email protected]> [Wed, 2019-12-18 21:59 -0800]:

Quoted 53 lines · Andrey Ignatov

Quoted 52 lines

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);
 

Quoted 14 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c void test_cgroup_attach_multi(void) · line 201

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +9 -0

+       /* 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.

Quoted 57 lines

Quoted 57 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -2

+
+       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);
 

Quoted 5 lines

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +0 -1

        prog_cnt = 4;
        CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS,
--

2.17.1

--
Andrey Ignatov

Quoted 74 lines

Andrii Nakryiko <[email protected]> [Wed, 2019-12-18 21:59 -0800]:

Quoted 53 lines · Andrey Ignatov

Quoted 52 lines

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);
 

Quoted 14 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c void test_cgroup_attach_multi(void) · line 201

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +9 -0

+       /* 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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 64 lines

Quoted 56 lines

Quoted 56 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -2

+
+       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);
 

Quoted 5 lines

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +0 -1

        prog_cnt = 4;
        CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS,
--

2.17.1

--
Andrey Ignatov

Andrii Nakryiko <[email protected]> [Thu, 2019-12-19 12:44 -0800]:

Quoted 86 lines · Andrey Ignatov

Quoted 74 lines

Andrii Nakryiko <[email protected]> [Wed, 2019-12-18 21:59 -0800]:

Quoted 53 lines · Andrey Ignatov

Quoted 52 lines

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);
 

Quoted 14 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c void test_cgroup_attach_multi(void) · line 201

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +9 -0

+       /* 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.

Quoted 8 lines

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:

Quoted 7 lines

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 :)

Quoted 5 lines

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 6 lines

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.

Quoted 57 lines

Quoted 57 lines

Quoted 57 lines

Quoted 57 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -2

+
+       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);
 

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +0 -1

        prog_cnt = 4;
        CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS,
--

2.17.1

--
Andrey Ignatov

--
Andrey Ignatov

Quoted 103 lines

Andrii Nakryiko <[email protected]> [Thu, 2019-12-19 12:44 -0800]:

Quoted 86 lines · Andrey Ignatov

Quoted 74 lines

Andrii Nakryiko <[email protected]> [Wed, 2019-12-18 21:59 -0800]:

Quoted 53 lines · Andrey Ignatov

Quoted 52 lines

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 <[email protected]>

---
 .../bpf/prog_tests/cgroup_attach_multi.c      | 53 +++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +2 -1

index 4eaab7435044..2ff21dbce179 100644
@@ -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);
 

Quoted 14 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c void test_cgroup_attach_multi(void) · line 201

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +9 -0

+       /* 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.

Quoted 8 lines

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:

Quoted 7 lines

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 :)

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 1 line

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

Quoted 12 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 67 lines

Quoted 56 lines

Quoted 56 lines

Quoted 56 lines

Quoted 56 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +39 -2

+
+       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);
 

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c +0 -1

        prog_cnt = 4;
        CHECK_FAIL(bpf_prog_query(cg5, BPF_CGROUP_INET_EGRESS,
--

2.17.1

--
Andrey Ignatov

--
Andrey Ignatov