Re: [PATCH bpf-next 2/4] selftests/bpf: convert test_cgroup_storage to test_progs

[Alan Maguire]

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?