Re: [PATCH bpf-next 1/4] selftests/bpf: convert get_current_cgroup_id_user to test_progs

[Alan Maguire]

On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> get_current_cgroup_id_user allows testing for bpf_get_current_cgroup_id()
> bpf API but is not integrated into test_progs, and so is not tested
> automatically in CI.
> 
> Convert it to the test_progs framework to allow running it automatically.
> The most notable differences with the old test are the following:
> - the new test relies on autoattach instead of manually hooking/enabling
>   the targeted tracepoint through perf_event, which reduces quite a lot the
>   test code size
> - sleep duration passed to nanosleep syscall has been reduced to its
>   minimum to not impact overall CI duration (we only care about the syscall
>   being properly triggered, not about the passed duration)
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@xxxxxxxxxxx>

Nice work! Two small suggestions below..

> ---
> The new test_progs part has been tested in a local qemu environment:
> 
>   ./test_progs -a cgroup_get_current_cgroup_id
>   47      cgroup_get_current_cgroup_id:OK
>   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> ---
>  tools/testing/selftests/bpf/.gitignore             |   1 -
>  tools/testing/selftests/bpf/Makefile               |   3 +-
>  tools/testing/selftests/bpf/get_cgroup_id_user.c   | 151 ---------------------
>  .../bpf/prog_tests/cgroup_get_current_cgroup_id.c  |  58 ++++++++
>  4 files changed, 59 insertions(+), 154 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 4e4aae8aa7ec..108eb10626b9 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -20,7 +20,6 @@ test_sock
>  urandom_read
>  test_sockmap
>  test_lirc_mode2_user
> -get_cgroup_id_user
>  test_skb_cgroup_id_user
>  test_cgroup_storage
>  test_flow_dissector
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 1d7a62e7deff..8d8483f81009 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -68,7 +68,7 @@ endif
>  # Order correspond to 'make run_tests' order
>  TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
>  	test_dev_cgroup \
> -	test_sock test_sockmap get_cgroup_id_user \
> +	test_sock test_sockmap \
>  	test_cgroup_storage \
>  	test_tcpnotify_user test_sysctl \
>  	test_progs-no_alu32
> @@ -297,7 +297,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)/get_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_cgroup_storage: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> diff --git a/tools/testing/selftests/bpf/get_cgroup_id_user.c b/tools/testing/selftests/bpf/get_cgroup_id_user.c
> deleted file mode 100644
> index aefd83ebdcd7..000000000000
> --- a/tools/testing/selftests/bpf/get_cgroup_id_user.c
> +++ /dev/null
> @@ -1,151 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2018 Facebook
> -
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <syscall.h>
> -#include <unistd.h>
> -#include <linux/perf_event.h>
> -#include <sys/ioctl.h>
> -#include <sys/time.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -
> -#include <linux/bpf.h>
> -#include <bpf/bpf.h>
> -#include <bpf/libbpf.h>
> -
> -#include "cgroup_helpers.h"
> -#include "testing_helpers.h"
> -
> -#define CHECK(condition, tag, format...) ({		\
> -	int __ret = !!(condition);			\
> -	if (__ret) {					\
> -		printf("%s:FAIL:%s ", __func__, tag);	\
> -		printf(format);				\
> -	} else {					\
> -		printf("%s:PASS:%s\n", __func__, tag);	\
> -	}						\
> -	__ret;						\
> -})
> -
> -static int bpf_find_map(const char *test, struct bpf_object *obj,
> -			const char *name)
> -{
> -	struct bpf_map *map;
> -
> -	map = bpf_object__find_map_by_name(obj, name);
> -	if (!map)
> -		return -1;
> -	return bpf_map__fd(map);
> -}
> -
> -#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
> -
> -int main(int argc, char **argv)
> -{
> -	const char *probe_name = "syscalls/sys_enter_nanosleep";
> -	const char *file = "get_cgroup_id_kern.bpf.o";
> -	int err, bytes, efd, prog_fd, pmu_fd;
> -	int cgroup_fd, cgidmap_fd, pidmap_fd;
> -	struct perf_event_attr attr = {};
> -	struct bpf_object *obj;
> -	__u64 kcgid = 0, ucgid;
> -	__u32 key = 0, pid;
> -	int exit_code = 1;
> -	char buf[256];
> -	const struct timespec req = {
> -		.tv_sec = 1,
> -		.tv_nsec = 0,
> -	};
> -
> -	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
> -	if (CHECK(cgroup_fd < 0, "cgroup_setup_and_join", "err %d errno %d\n", cgroup_fd, errno))
> -		return 1;
> -
> -	/* Use libbpf 1.0 API mode */
> -	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> -
> -	err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
> -	if (CHECK(err, "bpf_prog_test_load", "err %d errno %d\n", err, errno))
> -		goto cleanup_cgroup_env;
> -
> -	cgidmap_fd = bpf_find_map(__func__, obj, "cg_ids");
> -	if (CHECK(cgidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
> -		  cgidmap_fd, errno))
> -		goto close_prog;
> -
> -	pidmap_fd = bpf_find_map(__func__, obj, "pidmap");
> -	if (CHECK(pidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
> -		  pidmap_fd, errno))
> -		goto close_prog;
> -
> -	pid = getpid();
> -	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
> -
> -	if (access("/sys/kernel/tracing/trace", F_OK) == 0) {
> -		snprintf(buf, sizeof(buf),
> -			 "/sys/kernel/tracing/events/%s/id", probe_name);
> -	} else {
> -		snprintf(buf, sizeof(buf),
> -			 "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> -	}
> -	efd = open(buf, O_RDONLY, 0);
> -	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
> -		goto close_prog;
> -	bytes = read(efd, buf, sizeof(buf));
> -	close(efd);
> -	if (CHECK(bytes <= 0 || bytes >= sizeof(buf), "read",
> -		  "bytes %d errno %d\n", bytes, errno))
> -		goto close_prog;
> -
> -	attr.config = strtol(buf, NULL, 0);
> -	attr.type = PERF_TYPE_TRACEPOINT;
> -	attr.sample_type = PERF_SAMPLE_RAW;
> -	attr.sample_period = 1;
> -	attr.wakeup_events = 1;
> -
> -	/* attach to this pid so the all bpf invocations will be in the
> -	 * cgroup associated with this pid.
> -	 */
> -	pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
> -	if (CHECK(pmu_fd < 0, "perf_event_open", "err %d errno %d\n", pmu_fd,
> -		  errno))
> -		goto close_prog;
> -
> -	err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> -	if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n", err,
> -		  errno))
> -		goto close_pmu;
> -
> -	err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> -	if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n", err,
> -		  errno))
> -		goto close_pmu;
> -
> -	/* trigger some syscalls */
> -	syscall(__NR_nanosleep, &req, NULL);
> -
> -	err = bpf_map_lookup_elem(cgidmap_fd, &key, &kcgid);
> -	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
> -		goto close_pmu;
> -
> -	ucgid = get_cgroup_id(TEST_CGROUP);
> -	if (CHECK(kcgid != ucgid, "compare_cgroup_id",
> -		  "kern cgid %llx user cgid %llx", kcgid, ucgid))
> -		goto close_pmu;
> -
> -	exit_code = 0;
> -	printf("%s:PASS\n", argv[0]);
> -
> -close_pmu:
> -	close(pmu_fd);
> -close_prog:
> -	bpf_object__close(obj);
> -cleanup_cgroup_env:
> -	cleanup_cgroup_environment();
> -	return exit_code;
> -}
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
> new file mode 100644
> index 000000000000..dd8835a63a00
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <sys/stat.h>
> +#include <sys/sysmacros.h>
> +#include "test_progs.h"
> +#include "cgroup_helpers.h"
> +#include "get_cgroup_id_kern.skel.h"
> +
> +#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
> +
> +void test_cgroup_get_current_cgroup_id(void)
> +{
> +	struct get_cgroup_id_kern *skel;
> +	const struct timespec req = {
> +		.tv_sec = 0,
> +		.tv_nsec = 1,
> +	};
> +	__u64 kcgid, ucgid;
> +	int cgroup_fd;
> +	int key = 0;
> +	int pid;
> +
> +	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
> +	if (!ASSERT_OK_FD(cgroup_fd, "cgroup switch"))
> +		return;
> +
> +	skel = get_cgroup_id_kern__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "load program"))
> +		goto cleanup_cgroup;
> +
> +	if (!ASSERT_OK(get_cgroup_id_kern__attach(skel), "attach bpf program"))
> +		goto cleanup_progs;
> +
> +	pid = getpid();
> +	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
> +					    sizeof(key), &pid, sizeof(pid), 0),
> +		       "write pid"))
> +		goto cleanup_progs;
> +

I think it would be worth using a global variable in the BPF program
my_pid, and setting skel->bss->my_pid here as other more up-to-date
tests do (example progs/test_usdt.c, prog_tests/usdt.c). No need for a
separate map anymore.

> +	/* trigger the syscall on which is attached the tested prog */
> +	if (!ASSERT_OK(syscall(__NR_nanosleep, &req, NULL), "nanosleep"))
> +		goto cleanup_progs;
> +
> +	if (!ASSERT_OK(bpf_map__lookup_elem(skel->maps.cg_ids, &key,
> +					    sizeof(key), &kcgid, sizeof(kcgid),
> +					    0),
> +		       "read bpf cgroup id"))
> +		goto cleanup_progs;
> +

ditto here, cg_ids could be a global var cg_id that the bpf prog sets
and we check here via skel->bss->cg_id.

> +	ucgid = get_cgroup_id(TEST_CGROUP);
> +
> +	ASSERT_EQ(kcgid, ucgid, "compare cgroup ids");
> +
> +cleanup_progs:
> +	get_cgroup_id_kern__destroy(skel);
> +cleanup_cgroup:
> +	cleanup_cgroup_environment();
> +}
>