On Thu, Jul 23, 2020 at 02:40:55AM -0500, YiFei Zhu wrote:
> From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
>
> The current assumption is that the lifetime of a cgroup storage
> is tied to the program's attachment. The storage is created in
> cgroup_bpf_attach, and released upon cgroup_bpf_detach and
> cgroup_bpf_release.
>
> Because the current semantics is that each attachment gets a
> completely independent cgroup storage, and you can have multiple
> programs attached to the same (cgroup, attach type) pair, the key
> of the CGROUP_STORAGE map, looking up the map with this pair could
> yield multiple storages, and that is not permitted. Therefore,
> the kernel verifier checks that two programs cannot share the same
> CGROUP_STORAGE map, even if they have different expected attach
> types, considering that the actual attach type does not always
> have to be equal to the expected attach type.
>
> The test creates a CGROUP_STORAGE map and make it shared across
> two different programs, one cgroup_skb/egress and one /ingress.
> It asserts that the two programs cannot be both loaded, due to
> verifier failure from the above reason.
>
> Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> ---
> .../bpf/prog_tests/cg_storage_multi.c | 42 +++++++++++++----
> .../selftests/bpf/progs/cg_storage_multi.h | 13 ++++++
> .../progs/cg_storage_multi_egress_ingress.c | 45 +++++++++++++++++++
> .../bpf/progs/cg_storage_multi_egress_only.c | 9 ++--
> 4 files changed, 98 insertions(+), 11 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/progs/cg_storage_multi.h
> create mode 100644 tools/testing/selftests/bpf/progs/cg_storage_multi_egress_ingress.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/cg_storage_multi.c b/tools/testing/selftests/bpf/prog_tests/cg_storage_multi.c
> index 6d5a2194e036..1f4ab437ddb9 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cg_storage_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cg_storage_multi.c
> @@ -8,7 +8,10 @@
> #include <cgroup_helpers.h>
> #include <network_helpers.h>
>
> +#include "progs/cg_storage_multi.h"
> +
> #include "cg_storage_multi_egress_only.skel.h"
> +#include "cg_storage_multi_egress_ingress.skel.h"
>
> #define PARENT_CGROUP "/cgroup_storage"
> #define CHILD_CGROUP "/cgroup_storage/child"
> @@ -16,10 +19,10 @@
> static int duration;
>
> static bool assert_storage(struct bpf_map *map, const char *cgroup_path,
> - __u32 expected)
> + struct cgroup_value *expected)
> {
> struct bpf_cgroup_storage_key key = {0};
> - __u32 value;
> + struct cgroup_value value;
> int map_fd;
>
> map_fd = bpf_map__fd(map);
> @@ -29,8 +32,8 @@ static bool assert_storage(struct bpf_map *map, const char *cgroup_path,
> if (CHECK(bpf_map_lookup_elem(map_fd, &key, &value) < 0,
> "map-lookup", "errno %d", errno))
> return true;
> - if (CHECK(value != expected,
> - "assert-storage", "got %u expected %u", value, expected))
> + if (CHECK(memcmp(&value, expected, sizeof(struct cgroup_value)),
> + "assert-storage", "storages differ"))
> return true;
>
> return false;
> @@ -39,7 +42,7 @@ static bool assert_storage(struct bpf_map *map, const char *cgroup_path,
> static bool assert_storage_noexist(struct bpf_map *map, const char *cgroup_path)
> {
> struct bpf_cgroup_storage_key key = {0};
> - __u32 value;
> + struct cgroup_value value;
> int map_fd;
>
> map_fd = bpf_map__fd(map);
> @@ -86,6 +89,7 @@ static bool connect_send(const char *cgroup_path)
> static void test_egress_only(int parent_cgroup_fd, int child_cgroup_fd)
> {
> struct cg_storage_multi_egress_only *obj;
> + struct cgroup_value expected_cgroup_value;
> struct bpf_link *parent_link = NULL, *child_link = NULL;
> bool err;
>
> @@ -109,7 +113,9 @@ static void test_egress_only(int parent_cgroup_fd, int child_cgroup_fd)
> if (CHECK(obj->bss->invocations != 1,
> "first-invoke", "invocations=%d", obj->bss->invocations))
> goto close_bpf_object;
> - if (assert_storage(obj->maps.cgroup_storage, PARENT_CGROUP, 1))
> + expected_cgroup_value = (struct cgroup_value) { .egress_pkts = 1 };
> + if (assert_storage(obj->maps.cgroup_storage,
> + PARENT_CGROUP, &expected_cgroup_value))
> goto close_bpf_object;
> if (assert_storage_noexist(obj->maps.cgroup_storage, CHILD_CGROUP))
> goto close_bpf_object;
> @@ -129,9 +135,13 @@ static void test_egress_only(int parent_cgroup_fd, int child_cgroup_fd)
> if (CHECK(obj->bss->invocations != 3,
> "second-invoke", "invocations=%d", obj->bss->invocations))
> goto close_bpf_object;
> - if (assert_storage(obj->maps.cgroup_storage, PARENT_CGROUP, 2))
> + expected_cgroup_value = (struct cgroup_value) { .egress_pkts = 2 };
> + if (assert_storage(obj->maps.cgroup_storage,
> + PARENT_CGROUP, &expected_cgroup_value))
> goto close_bpf_object;
> - if (assert_storage(obj->maps.cgroup_storage, CHILD_CGROUP, 1))
> + expected_cgroup_value = (struct cgroup_value) { .egress_pkts = 1 };
> + if (assert_storage(obj->maps.cgroup_storage,
> + CHILD_CGROUP, &expected_cgroup_value))
> goto close_bpf_object;
>
> close_bpf_object:
> @@ -143,6 +153,19 @@ static void test_egress_only(int parent_cgroup_fd, int child_cgroup_fd)
> cg_storage_multi_egress_only__destroy(obj);
> }
>
> +static void test_egress_ingress(int parent_cgroup_fd, int child_cgroup_fd)
> +{
> + struct cg_storage_multi_egress_ingress *obj;
> +
> + /* Cannot load both programs due to verifier failure:
> + * "only one cgroup storage of each type is allowed"
> + */
> + obj = cg_storage_multi_egress_ingress__open_and_load();
> + if (CHECK(obj || errno != EBUSY,
> + "skel-load", "errno %d, expected EBUSY", errno))
obj may theoretically need to be freed.
Probably not big deal since this case will be gone in
the latter test.
> + return;
> +}
> +
