On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> test_cgroup_storage is currently a standalone program which is not run
> when executing test_progs.
>
> Convert it to the test_progs framework so it can be automatically executed
> in CI. The conversion led to the following changes:
> - converted the raw bpf program in the userspace test file into a dedicated
> test program in progs/ dir
> - reduced the scope of cgroup_storage test: the content from this test
> overlaps with some other tests already present in test_progs, most
> notably netcnt and cgroup_storage_multi*. Those tests already check
> extensively local storage, per-cpu local storage, cgroups interaction,
> etc. So the new test only keep the part testing that the program return
> code (based on map content) properly leads to packet being passed or
> dropped.
>
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@xxxxxxxxxxx>
Two small things below, but
Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> Tested in a local qemu environment:
>
> ./test_progs -a cgroup_storage
> 53 cgroup_storage:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> ---
> tools/testing/selftests/bpf/.gitignore | 1 -
> tools/testing/selftests/bpf/Makefile | 2 -
> .../selftests/bpf/prog_tests/cgroup_storage.c | 65 ++++++++
> tools/testing/selftests/bpf/progs/cgroup_storage.c | 24 +++
> tools/testing/selftests/bpf/test_cgroup_storage.c | 174 ---------------------
> 5 files changed, 89 insertions(+), 177 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 108eb10626b9..a45f11f81337 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -21,7 +21,6 @@ urandom_read
> test_sockmap
> test_lirc_mode2_user
> test_skb_cgroup_id_user
> -test_cgroup_storage
> test_flow_dissector
> flow_dissector_load
> test_tcpnotify_user
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 8d8483f81009..0ac0f9dbc2f8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -69,7 +69,6 @@ endif
> TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> test_dev_cgroup \
> test_sock test_sockmap \
> - test_cgroup_storage \
> test_tcpnotify_user test_sysctl \
> test_progs-no_alu32
> TEST_INST_SUBDIRS := no_alu32
> @@ -297,7 +296,6 @@ $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
> -$(OUTPUT)/test_cgroup_storage: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> $(OUTPUT)/test_tag: $(TESTING_HELPERS)
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
> new file mode 100644
> index 000000000000..c116fc22b460
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_storage.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "cgroup_helpers.h"
> +#include "cgroup_storage.skel.h"
> +
> +#define TEST_CGROUP "/test-bpf-cgroup-storage-buf/"
> +#define PING_CMD "ping localhost -c 1 -W 1 -q"
other tests seem to redirect ping stdout output to /dev/null ; might be
worth doing that too.
> +
> +void test_cgroup_storage(void)
> +{
> + struct bpf_cgroup_storage_key key;
> + struct cgroup_storage *skel;
> + unsigned long long value;
> + int cgroup_fd;
> + int err;
> +
> + cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
> + if (!ASSERT_OK_FD(cgroup_fd, "create cgroup"))
> + return;
> +
> + skel = cgroup_storage__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "load program"))
> + goto cleanup_cgroup;
> +
> + skel->links.bpf_prog =
> + bpf_program__attach_cgroup(skel->progs.bpf_prog, cgroup_fd);
> + if (!ASSERT_OK_PTR(skel->links.bpf_prog, "attach program"))
> + goto cleanup_progs;
> +
> + /* Check that one out of every two packets is dropped */
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_OK(err, "first ping");
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_NEQ(err, 0, "second ping");
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_OK(err, "third ping");
> +
> + err = bpf_map__get_next_key(skel->maps.cgroup_storage, NULL, &key,
> + sizeof(key));
> + if (!ASSERT_OK(err, "get first key"))
> + goto cleanup_progs;
> + err = bpf_map__lookup_elem(skel->maps.cgroup_storage, &key, sizeof(key),
> + &value, sizeof(value), 0);
> + if (!ASSERT_OK(err, "first packet count read"))
> + goto cleanup_progs;
> +
> + /* Add one to the packet counter, check again packet filtering */
> + value++;
> + err = bpf_map__update_elem(skel->maps.cgroup_storage, &key, sizeof(key),
> + &value, sizeof(value), 0);
> + if (!ASSERT_OK(err, "increment packet counter"))
> + goto cleanup_progs;
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_OK(err, "fourth ping");
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_NEQ(err, 0, "fifth ping");
> + err = SYS_NOFAIL(PING_CMD);
> + ASSERT_OK(err, "sixth ping");
> +
> +cleanup_progs:
> + cgroup_storage__destroy(skel);
> +cleanup_cgroup:
> + cleanup_cgroup_environment();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/cgroup_storage.c b/tools/testing/selftests/bpf/progs/cgroup_storage.c
> new file mode 100644
> index 000000000000..db1e4d2d3281
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/cgroup_storage.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> + __uint(type, BPF_MAP_TYPE_CGROUP_STORAGE);
> + __type(key, struct bpf_cgroup_storage_key);
> + __type(value, __u64);
> +} cgroup_storage SEC(".maps");
> +
> +SEC("cgroup_skb/egress")
> +int bpf_prog(struct __sk_buff *skb)
> +{
> + __u64 *counter;
> +
> + counter = bpf_get_local_storage(&cgroup_storage, 0);
don't we need a NULL check for counter here? Or does the verifier know
bpf_get_local_storage never fails?
