Message ID | 16989c2daceb609f6538f132987a66a84aa2032a.1595489786.git.zhuyifei@google.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | Make BPF CGROUP_STORAGE map usable by different programs at once | expand |
On Thu, Jul 23, 2020 at 02:40:55AM -0500, YiFei Zhu wrote: > From: YiFei Zhu <zhuyifei@google.com> > > 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@google.com> > --- > .../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; > +} > +
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)) + return; +} + void test_cg_storage_multi(void) { int parent_cgroup_fd = -1, child_cgroup_fd = -1; @@ -157,6 +180,9 @@ void test_cg_storage_multi(void) if (test__start_subtest("egress_only")) test_egress_only(parent_cgroup_fd, child_cgroup_fd); + if (test__start_subtest("egress_ingress")) + test_egress_ingress(parent_cgroup_fd, child_cgroup_fd); + close_cgroup_fd: close(child_cgroup_fd); close(parent_cgroup_fd); diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi.h b/tools/testing/selftests/bpf/progs/cg_storage_multi.h new file mode 100644 index 000000000000..a0778fe7857a --- /dev/null +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __PROGS_CG_STORAGE_MULTI_H +#define __PROGS_CG_STORAGE_MULTI_H + +#include <asm/types.h> + +struct cgroup_value { + __u32 egress_pkts; + __u32 ingress_pkts; +}; + +#endif diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_ingress.c b/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_ingress.c new file mode 100644 index 000000000000..9ce386899365 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_ingress.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* + * Copyright 2020 Google LLC. + */ + +#include <errno.h> +#include <linux/bpf.h> +#include <linux/ip.h> +#include <linux/udp.h> +#include <bpf/bpf_helpers.h> + +#include "progs/cg_storage_multi.h" + +struct { + __uint(type, BPF_MAP_TYPE_CGROUP_STORAGE); + __type(key, struct bpf_cgroup_storage_key); + __type(value, struct cgroup_value); +} cgroup_storage SEC(".maps"); + +__u32 invocations = 0; + +SEC("cgroup_skb/egress") +int egress(struct __sk_buff *skb) +{ + struct cgroup_value *ptr_cg_storage = + bpf_get_local_storage(&cgroup_storage, 0); + + __sync_fetch_and_add(&ptr_cg_storage->egress_pkts, 1); + __sync_fetch_and_add(&invocations, 1); + + return 1; +} + +SEC("cgroup_skb/ingress") +int ingress(struct __sk_buff *skb) +{ + struct cgroup_value *ptr_cg_storage = + bpf_get_local_storage(&cgroup_storage, 0); + + __sync_fetch_and_add(&ptr_cg_storage->ingress_pkts, 1); + __sync_fetch_and_add(&invocations, 1); + + return 1; +} diff --git a/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_only.c b/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_only.c index ec0165d07105..44ad46b33539 100644 --- a/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_only.c +++ b/tools/testing/selftests/bpf/progs/cg_storage_multi_egress_only.c @@ -10,10 +10,12 @@ #include <linux/udp.h> #include <bpf/bpf_helpers.h> +#include "progs/cg_storage_multi.h" + struct { __uint(type, BPF_MAP_TYPE_CGROUP_STORAGE); __type(key, struct bpf_cgroup_storage_key); - __type(value, __u32); + __type(value, struct cgroup_value); } cgroup_storage SEC(".maps"); __u32 invocations = 0; @@ -21,9 +23,10 @@ __u32 invocations = 0; SEC("cgroup_skb/egress") int egress(struct __sk_buff *skb) { - __u32 *ptr_cg_storage = bpf_get_local_storage(&cgroup_storage, 0); + struct cgroup_value *ptr_cg_storage = + bpf_get_local_storage(&cgroup_storage, 0); - __sync_fetch_and_add(ptr_cg_storage, 1); + __sync_fetch_and_add(&ptr_cg_storage->egress_pkts, 1); __sync_fetch_and_add(&invocations, 1); return 1;