[PATCH bpf-next 0/4] selftests/bpf: convert three other cgroup tests to test_progs

[PATCH bpf-next 0/4] selftests/bpf: convert three other cgroup tests to test_progs

[Alexis Lothoré (eBPF Foundation)]

Hello,
this series brings a new set of test converted to the test_progs framework.
Since the tests are quite small, I chose to group three tests conversion in
the same series, but feel free to let me know if I should keep one series
per test. The series focuses on cgroup testing and converts the following
tests:
- get_cgroup_id_user
- cgroup_storage
- test_skb_cgroup_id_user

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
Alexis Lothoré (eBPF Foundation) (4):
      selftests/bpf: convert get_current_cgroup_id_user to test_progs
      selftests/bpf: convert test_cgroup_storage to test_progs
      selftests/bpf: add proper section name to bpf prog and rename it
      selftests/bpf: convert test_skb_cgroup_id_user to test_progs

 tools/testing/selftests/bpf/.gitignore             |   3 -
 tools/testing/selftests/bpf/Makefile               |   8 +-
 tools/testing/selftests/bpf/get_cgroup_id_user.c   | 151 -----------------
 .../selftests/bpf/prog_tests/cgroup_ancestor.c     | 159 ++++++++++++++++++
 .../bpf/prog_tests/cgroup_get_current_cgroup_id.c  |  58 +++++++
 .../selftests/bpf/prog_tests/cgroup_storage.c      |  65 ++++++++
 ...test_skb_cgroup_id_kern.c => cgroup_ancestor.c} |   2 +-
 tools/testing/selftests/bpf/progs/cgroup_storage.c |  24 +++
 tools/testing/selftests/bpf/test_cgroup_storage.c  | 174 --------------------
 tools/testing/selftests/bpf/test_skb_cgroup_id.sh  |  63 -------
 .../selftests/bpf/test_skb_cgroup_id_user.c        | 183 ---------------------
 11 files changed, 309 insertions(+), 581 deletions(-)
---
base-commit: 0e2eaf4b33f65e904b69bae6b956f3f610dbba9a
change-id: 20240725-convert_cgroup_tests-d07c66053225

Best regards,
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alexis Lothoré (eBPF Foundation)]

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@bootlin.com>
---
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;
+
+	/* 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;
+
+	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();
+}

-- 
2.45.2

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@bootlin.com>

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();
> +}
>

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

[Alexis Lothoré]

Hello Alan,

On 7/31/24 19:23, Alan Maguire wrote:
> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>> +	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.

That sounds like a good improvement, thanks for the hint and the example :) I'll
spin a new revision with this, and make sure to use it in my next test
conversion patches too when relevant.

TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
clear for me when I should prefer it over classic maps. From some quick search I
feel like it should be the default choice when needing basic controls
knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
more broadly by default ?

>> +	/* 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.

ACK, I'll update this too.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alan Maguire]

On 31/07/2024 19:53, Alexis Lothoré wrote:
> Hello Alan,
> 
> On 7/31/24 19:23, Alan Maguire wrote:
>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> 
> [...]
> 
>>> +	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.
> 
> That sounds like a good improvement, thanks for the hint and the example :) I'll
> spin a new revision with this, and make sure to use it in my next test
> conversion patches too when relevant.
> 
> TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
> clear for me when I should prefer it over classic maps. From some quick search I
> feel like it should be the default choice when needing basic controls
> knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
> more broadly by default ?
> 

Yeah, it's certainly what I use by default, unless I need multiple
instances of an object. Under the hood, the BPF skeleton creates
single-element array maps for .bss, .data and .rodata sections which
contain all the initialized, uninitialized and constant globals in the
BPF object and mmaps() them so you can read/update the values in
userspace via skel->bss/skel->data without needing a map-related syscalls.


>>> +	/* 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.
> 
> ACK, I'll update this too.
>

Great, thank you!

Alan

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

[Alexis Lothoré]

Hello Alan,

On 8/1/24 10:17, Alan Maguire wrote:
> On 31/07/2024 19:53, Alexis Lothoré wrote:
>> Hello Alan,
>>
>> On 7/31/24 19:23, Alan Maguire wrote:
>>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>>
>> [...]
>>
>>>> +	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.
>>
>> That sounds like a good improvement, thanks for the hint and the example :) I'll
>> spin a new revision with this, and make sure to use it in my next test
>> conversion patches too when relevant.
>>
>> TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
>> clear for me when I should prefer it over classic maps. From some quick search I
>> feel like it should be the default choice when needing basic controls
>> knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
>> more broadly by default ?
>>
> 
> Yeah, it's certainly what I use by default, unless I need multiple
> instances of an object. Under the hood, the BPF skeleton creates
> single-element array maps for .bss, .data and .rodata sections which
> contain all the initialized, uninitialized and constant globals in the
> BPF object and mmaps() them so you can read/update the values in
> userspace via skel->bss/skel->data without needing a map-related syscalls.

Thanks a lot for the additional details, much appreciated :)

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alexis Lothoré (eBPF Foundation)]

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@bootlin.com>
---
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"
+
+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);
+	__sync_fetch_and_add(counter, 1);
+
+	/* Drop one out of every two packets */
+	return (*counter & 1);
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_cgroup_storage.c b/tools/testing/selftests/bpf/test_cgroup_storage.c
deleted file mode 100644
index 0861ea60dcdd..000000000000
--- a/tools/testing/selftests/bpf/test_cgroup_storage.c
+++ /dev/null
@@ -1,174 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <assert.h>
-#include <bpf/bpf.h>
-#include <linux/filter.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/sysinfo.h>
-
-#include "bpf_util.h"
-#include "cgroup_helpers.h"
-#include "testing_helpers.h"
-
-char bpf_log_buf[BPF_LOG_BUF_SIZE];
-
-#define TEST_CGROUP "/test-bpf-cgroup-storage-buf/"
-
-int main(int argc, char **argv)
-{
-	struct bpf_insn prog[] = {
-		BPF_LD_MAP_FD(BPF_REG_1, 0), /* percpu map fd */
-		BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
-		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
-			     BPF_FUNC_get_local_storage),
-		BPF_LDX_MEM(BPF_DW, BPF_REG_3, BPF_REG_0, 0),
-		BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 0x1),
-		BPF_STX_MEM(BPF_DW, BPF_REG_0, BPF_REG_3, 0),
-
-		BPF_LD_MAP_FD(BPF_REG_1, 0), /* map fd */
-		BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
-		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
-			     BPF_FUNC_get_local_storage),
-		BPF_MOV64_IMM(BPF_REG_1, 1),
-		BPF_ATOMIC_OP(BPF_DW, BPF_ADD, BPF_REG_0, BPF_REG_1, 0),
-		BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, 0),
-		BPF_ALU64_IMM(BPF_AND, BPF_REG_1, 0x1),
-		BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
-		BPF_EXIT_INSN(),
-	};
-	size_t insns_cnt = ARRAY_SIZE(prog);
-	int error = EXIT_FAILURE;
-	int map_fd, percpu_map_fd, prog_fd, cgroup_fd;
-	struct bpf_cgroup_storage_key key;
-	unsigned long long value;
-	unsigned long long *percpu_value;
-	int cpu, nproc;
-
-	nproc = bpf_num_possible_cpus();
-	percpu_value = malloc(sizeof(*percpu_value) * nproc);
-	if (!percpu_value) {
-		printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
-		goto err;
-	}
-
-	/* Use libbpf 1.0 API mode */
-	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
-
-	map_fd = bpf_map_create(BPF_MAP_TYPE_CGROUP_STORAGE, NULL, sizeof(key),
-				sizeof(value), 0, NULL);
-	if (map_fd < 0) {
-		printf("Failed to create map: %s\n", strerror(errno));
-		goto out;
-	}
-
-	percpu_map_fd = bpf_map_create(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, NULL,
-				       sizeof(key), sizeof(value), 0, NULL);
-	if (percpu_map_fd < 0) {
-		printf("Failed to create map: %s\n", strerror(errno));
-		goto out;
-	}
-
-	prog[0].imm = percpu_map_fd;
-	prog[7].imm = map_fd;
-	prog_fd = bpf_test_load_program(BPF_PROG_TYPE_CGROUP_SKB,
-				   prog, insns_cnt, "GPL", 0,
-				   bpf_log_buf, BPF_LOG_BUF_SIZE);
-	if (prog_fd < 0) {
-		printf("Failed to load bpf program: %s\n", bpf_log_buf);
-		goto out;
-	}
-
-	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
-
-	/* Attach the bpf program */
-	if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
-		printf("Failed to attach bpf program\n");
-		goto err;
-	}
-
-	if (bpf_map_get_next_key(map_fd, NULL, &key)) {
-		printf("Failed to get the first key in cgroup storage\n");
-		goto err;
-	}
-
-	if (bpf_map_lookup_elem(map_fd, &key, &value)) {
-		printf("Failed to lookup cgroup storage 0\n");
-		goto err;
-	}
-
-	for (cpu = 0; cpu < nproc; cpu++)
-		percpu_value[cpu] = 1000;
-
-	if (bpf_map_update_elem(percpu_map_fd, &key, percpu_value, 0)) {
-		printf("Failed to update the data in the cgroup storage\n");
-		goto err;
-	}
-
-	/* Every second packet should be dropped */
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null") == 0);
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null"));
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null") == 0);
-
-	/* Check the counter in the cgroup local storage */
-	if (bpf_map_lookup_elem(map_fd, &key, &value)) {
-		printf("Failed to lookup cgroup storage\n");
-		goto err;
-	}
-
-	if (value != 3) {
-		printf("Unexpected data in the cgroup storage: %llu\n", value);
-		goto err;
-	}
-
-	/* Bump the counter in the cgroup local storage */
-	value++;
-	if (bpf_map_update_elem(map_fd, &key, &value, 0)) {
-		printf("Failed to update the data in the cgroup storage\n");
-		goto err;
-	}
-
-	/* Every second packet should be dropped */
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null") == 0);
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null"));
-	assert(system("ping localhost -c 1 -W 1 -q > /dev/null") == 0);
-
-	/* Check the final value of the counter in the cgroup local storage */
-	if (bpf_map_lookup_elem(map_fd, &key, &value)) {
-		printf("Failed to lookup the cgroup storage\n");
-		goto err;
-	}
-
-	if (value != 7) {
-		printf("Unexpected data in the cgroup storage: %llu\n", value);
-		goto err;
-	}
-
-	/* Check the final value of the counter in the percpu local storage */
-
-	for (cpu = 0; cpu < nproc; cpu++)
-		percpu_value[cpu] = 0;
-
-	if (bpf_map_lookup_elem(percpu_map_fd, &key, percpu_value)) {
-		printf("Failed to lookup the per-cpu cgroup storage\n");
-		goto err;
-	}
-
-	value = 0;
-	for (cpu = 0; cpu < nproc; cpu++)
-		value += percpu_value[cpu];
-
-	if (value != nproc * 1000 + 6) {
-		printf("Unexpected data in the per-cpu cgroup storage\n");
-		goto err;
-	}
-
-	error = 0;
-	printf("test_cgroup_storage:PASS\n");
-
-err:
-	cleanup_cgroup_environment();
-	free(percpu_value);
-
-out:
-	return error;
-}

-- 
2.45.2

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@bootlin.com>

Two small things below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
> 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?

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

[Alexis Lothoré]

On 8/1/24 10:27, Alan Maguire wrote:
> 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@bootlin.com>
> 
> Two small things below, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

[...]

>> +#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.

That's in fact performed automatically by SYS_NOFAIL :)

#define SYS_NOFAIL(fmt, ...)						\
	({								\
		char cmd[1024];						\
		int n;							\
		n = snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
		if (n < sizeof(cmd) && sizeof(cmd) - n >= sizeof(ALL_TO_DEV_NULL)) \
			strcat(cmd, ALL_TO_DEV_NULL);			\
		system(cmd);						\
	})

[...]

>> +{
>> +	__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?

Good question. Since the verifier accepted the prog during my tests, I indeed
assume that the returned pointer is always valid. Amongst all calls to this
function in progs involved in selftests, I found only one performing a check
before using the value (lsm_cgroup.c). So I guess it is fine ?

Thanks for the review !

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alan Maguire]

On 01/08/2024 10:21, Alexis Lothoré wrote:
> On 8/1/24 10:27, Alan Maguire wrote:
>> 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@bootlin.com>
>>
>> Two small things below, but
>>
>> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> [...]
> 
>>> +#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.
> 
> That's in fact performed automatically by SYS_NOFAIL :)
> > #define SYS_NOFAIL(fmt, ...)						\
> 	({								\
> 		char cmd[1024];						\
> 		int n;							\
> 		n = snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
> 		if (n < sizeof(cmd) && sizeof(cmd) - n >= sizeof(ALL_TO_DEV_NULL)) \
> 			strcat(cmd, ALL_TO_DEV_NULL);			\
> 		system(cmd);						\
> 	})
> 
> [...]
> 

Perfect, I missed that.

>>> +{
>>> +	__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?
> 
> Good question. Since the verifier accepted the prog during my tests, I indeed
> assume that the returned pointer is always valid. Amongst all calls to this
> function in progs involved in selftests, I found only one performing a check
> before using the value (lsm_cgroup.c). So I guess it is fine ?
> 

Looks like the prototype for the helper specifies a return type of
RET_PTR_TO_MAP_VALUE ; if it was RET_PTR_TO_MAP_VALUE_OR_NULL we'd need
the NULL check, but because it's a guaranteed map ptr we are good here
without a NULL check.

Thanks!

Alan

[PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alexis Lothoré (eBPF Foundation)]

test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
current form, it can not be used with the auto-generated skeleton APIs,
because the section name is not valid to allow libbpf to deduce the program
type.

Update section name to allow skeleton APIs usage. Also rename the program
name to make it shorter and more straighforward regarding the API it is
testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
a working reference before converting it to test_progs
- update the obj name
- fix loading issue (verifier rejecting the program when loaded through tc,
  because of map not found), by preloading the whole obj with bpftool

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
 .../progs/{test_skb_cgroup_id_kern.c => cgroup_ancestor.c}   |  2 +-
 tools/testing/selftests/bpf/test_skb_cgroup_id.sh            | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
similarity index 97%
rename from tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
rename to tools/testing/selftests/bpf/progs/cgroup_ancestor.c
index 37aacc66cd68..4879645f5827 100644
--- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
+++ b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
@@ -28,7 +28,7 @@ static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level)
 	bpf_map_update_elem(&cgroup_ids, &level, &id, 0);
 }
 
-SEC("cgroup_id_logger")
+SEC("tc")
 int log_cgroup_id(struct __sk_buff *skb)
 {
 	/* Loop unroll can't be used here due to [1]. Unrolling manually.
diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
index 515c2eafc97f..d7dad49175c2 100755
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
+++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
@@ -30,8 +30,10 @@ setup()
 	wait_for_ip
 
 	tc qdisc add dev ${TEST_IF} clsact
-	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
-		sec ${BPF_PROG_SECTION} da
+	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
+	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
+	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
+		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
 
 	BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \
 			awk '/ id / {sub(/.* id /, "", $0); print($1)}')
@@ -41,6 +43,7 @@ cleanup()
 {
 	ip link del ${TEST_IF} 2>/dev/null || :
 	ip link del ${TEST_IF_PEER} 2>/dev/null || :
+	rm -rf /sys/fs/bpf/${BPF_PROG_PIN}
 }
 
 main()
@@ -54,8 +57,9 @@ DIR=$(dirname $0)
 TEST_IF="test_cgid_1"
 TEST_IF_PEER="test_cgid_2"
 MAX_PING_TRIES=5
-BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.bpf.o"
-BPF_PROG_SECTION="cgroup_id_logger"
+BPF_PROG_PIN="cgroup_ancestor"
+BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o"
+BPF_PROG_NAME="log_cgroup_id"
 BPF_PROG_ID=0
 PROG="${DIR}/test_skb_cgroup_id_user"
 type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"

-- 
2.45.2

Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alan Maguire]

On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
> current form, it can not be used with the auto-generated skeleton APIs,
> because the section name is not valid to allow libbpf to deduce the program
> type.
> 
> Update section name to allow skeleton APIs usage. Also rename the program
> name to make it shorter and more straighforward regarding the API it is
> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
> a working reference before converting it to test_progs
> - update the obj name
> - fix loading issue (verifier rejecting the program when loaded through tc,
>   because of map not found), by preloading the whole obj with bpftool
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  .../progs/{test_skb_cgroup_id_kern.c => cgroup_ancestor.c}   |  2 +-
>  tools/testing/selftests/bpf/test_skb_cgroup_id.sh            | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> similarity index 97%
> rename from tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
> rename to tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> index 37aacc66cd68..4879645f5827 100644
> --- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
> +++ b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> @@ -28,7 +28,7 @@ static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level)
>  	bpf_map_update_elem(&cgroup_ids, &level, &id, 0);
>  }
>  
> -SEC("cgroup_id_logger")
> +SEC("tc")
>  int log_cgroup_id(struct __sk_buff *skb)
>  {
>  	/* Loop unroll can't be used here due to [1]. Unrolling manually.
> diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> index 515c2eafc97f..d7dad49175c2 100755
> --- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> +++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> @@ -30,8 +30,10 @@ setup()
>  	wait_for_ip
>  
>  	tc qdisc add dev ${TEST_IF} clsact
> -	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
> -		sec ${BPF_PROG_SECTION} da
> +	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
> +	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
> +	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
> +		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
>

Just out of curiosity; did you find that the pin is needed? I would have
thought tc attach would be enough to keep the program aroud.

>  	BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \
>  			awk '/ id / {sub(/.* id /, "", $0); print($1)}')
> @@ -41,6 +43,7 @@ cleanup()
>  {
>  	ip link del ${TEST_IF} 2>/dev/null || :
>  	ip link del ${TEST_IF_PEER} 2>/dev/null || :
> +	rm -rf /sys/fs/bpf/${BPF_PROG_PIN}
>  }
>  
>  main()
> @@ -54,8 +57,9 @@ DIR=$(dirname $0)
>  TEST_IF="test_cgid_1"
>  TEST_IF_PEER="test_cgid_2"
>  MAX_PING_TRIES=5
> -BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.bpf.o"
> -BPF_PROG_SECTION="cgroup_id_logger"
> +BPF_PROG_PIN="cgroup_ancestor"
> +BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o"
> +BPF_PROG_NAME="log_cgroup_id"
>  BPF_PROG_ID=0
>  PROG="${DIR}/test_skb_cgroup_id_user"
>  type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
> 

Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alexis Lothoré]

On 8/1/24 10:35, Alan Maguire wrote:
> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
>> current form, it can not be used with the auto-generated skeleton APIs,
>> because the section name is not valid to allow libbpf to deduce the program
>> type.
>>
>> Update section name to allow skeleton APIs usage. Also rename the program
>> name to make it shorter and more straighforward regarding the API it is
>> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
>> a working reference before converting it to test_progs
>> - update the obj name
>> - fix loading issue (verifier rejecting the program when loaded through tc,
>>   because of map not found), by preloading the whole obj with bpftool
>>
>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

[...]

>>  	tc qdisc add dev ${TEST_IF} clsact
>> -	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
>> -		sec ${BPF_PROG_SECTION} da
>> +	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
>> +	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
>> +	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
>> +		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
>>
> 
> Just out of curiosity; did you find that the pin is needed? I would have
> thought tc attach would be enough to keep the program aroud.

That's more because that's the only way I found to make the filter addition
work. With the current command, the tc command fails because of the verifier
rejecting the program:

Verifier analysis:

func#0 @0
0: R1=ctx() R10=fp0
0: (bf) r6 = r1                       ; R1=ctx() R6_w=ctx()
1: (b4) w1 = 0                        ; R1_w=0
2: (63) *(u32 *)(r10 -4) = r1
mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0
3: R1_w=0 R10=fp0 fp-8=0000????
3: (bf) r1 = r6                       ; R1_w=ctx() R6_w=ctx()
4: (b4) w2 = 0                        ; R2_w=0
5: (85) call bpf_skb_ancestor_cgroup_id#83    ; R0_w=scalar()
6: (7b) *(u64 *)(r10 -16) = r0        ; R0_w=scalar(id=1) R10=fp0
fp-16_w=scalar(id=1)
7: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
8: (07) r2 += -4                      ; R2_w=fp-4
9: (bf) r3 = r10                      ; R3_w=fp0 R10=fp0
10: (07) r3 += -16                    ; R3_w=fp-16
11: (18) r1 = 0x0                     ; R1_w=0
13: (b7) r4 = 0                       ; R4_w=0
14: (85) call bpf_map_update_elem#2
R1 type=scalar expected=map_ptr
processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0

(I also tried to remove the sec argument from the tc commandline, in case it
could allow tc to load everything, but the issue remains the same)

IIUC the verifier output, there's an issue with the variable representing the map.
When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall
but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map
correctly. That's why I used bpftool to make sure everything is loaded, but as a
consequence I need to provide a pin path when using load/loadall. But maybe I am
missing something ?

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alan Maguire]

On 01/08/2024 11:00, Alexis Lothoré wrote:
> On 8/1/24 10:35, Alan Maguire wrote:
>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>>> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
>>> current form, it can not be used with the auto-generated skeleton APIs,
>>> because the section name is not valid to allow libbpf to deduce the program
>>> type.
>>>
>>> Update section name to allow skeleton APIs usage. Also rename the program
>>> name to make it shorter and more straighforward regarding the API it is
>>> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
>>> a working reference before converting it to test_progs
>>> - update the obj name
>>> - fix loading issue (verifier rejecting the program when loaded through tc,
>>>   because of map not found), by preloading the whole obj with bpftool
>>>
>>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
>>
>> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> [...]
> 
>>>  	tc qdisc add dev ${TEST_IF} clsact
>>> -	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
>>> -		sec ${BPF_PROG_SECTION} da
>>> +	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
>>> +	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
>>> +	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
>>> +		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
>>>
>>
>> Just out of curiosity; did you find that the pin is needed? I would have
>> thought tc attach would be enough to keep the program aroud.
> 
> That's more because that's the only way I found to make the filter addition
> work. With the current command, the tc command fails because of the verifier
> rejecting the program:
> 
> Verifier analysis:
> 
> func#0 @0
> 0: R1=ctx() R10=fp0
> 0: (bf) r6 = r1                       ; R1=ctx() R6_w=ctx()
> 1: (b4) w1 = 0                        ; R1_w=0
> 2: (63) *(u32 *)(r10 -4) = r1
> mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1
> mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0
> 3: R1_w=0 R10=fp0 fp-8=0000????
> 3: (bf) r1 = r6                       ; R1_w=ctx() R6_w=ctx()
> 4: (b4) w2 = 0                        ; R2_w=0
> 5: (85) call bpf_skb_ancestor_cgroup_id#83    ; R0_w=scalar()
> 6: (7b) *(u64 *)(r10 -16) = r0        ; R0_w=scalar(id=1) R10=fp0
> fp-16_w=scalar(id=1)
> 7: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
> 8: (07) r2 += -4                      ; R2_w=fp-4
> 9: (bf) r3 = r10                      ; R3_w=fp0 R10=fp0
> 10: (07) r3 += -16                    ; R3_w=fp-16
> 11: (18) r1 = 0x0                     ; R1_w=0
> 13: (b7) r4 = 0                       ; R4_w=0
> 14: (85) call bpf_map_update_elem#2
> R1 type=scalar expected=map_ptr
> processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0
> peak_states 0 mark_read 0
> 
> (I also tried to remove the sec argument from the tc commandline, in case it
> could allow tc to load everything, but the issue remains the same)
> 
> IIUC the verifier output, there's an issue with the variable representing the map.
> When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall
> but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map
> correctly. That's why I used bpftool to make sure everything is loaded, but as a
> consequence I need to provide a pin path when using load/loadall. But maybe I am
> missing something ?
> 

No I think you're absolutely right. It seems like there have been
problems with BTF-defined maps in the past with tc filter loading [1],
but more recent tc fixes this since it uses libbpf under the hood. I
tried with the section name update only and the test passes, so it might
just be a tc version issue. As in [1] I'd suggest compiling an
up-to-date iproute2/tc and re-testing. Thanks!

[1] https://lore.kernel.org/bpf/87zgkx9l6y.fsf@toke.dk/

Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alexis Lothoré]

On 8/6/24 14:33, Alan Maguire wrote:
> On 01/08/2024 11:00, Alexis Lothoré wrote:
>> On 8/1/24 10:35, Alan Maguire wrote:
>>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>>>> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
>>>> current form, it can not be used with the auto-generated skeleton APIs,
>>>> because the section name is not valid to allow libbpf to deduce the program
>>>> type.
>>>>
>>>> Update section name to allow skeleton APIs usage. Also rename the program
>>>> name to make it shorter and more straighforward regarding the API it is
>>>> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
>>>> a working reference before converting it to test_progs
>>>> - update the obj name
>>>> - fix loading issue (verifier rejecting the program when loaded through tc,
>>>>   because of map not found), by preloading the whole obj with bpftool
>>>>
>>>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
>>>
>>> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>>
>> [...]
>>
>>>>  	tc qdisc add dev ${TEST_IF} clsact
>>>> -	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
>>>> -		sec ${BPF_PROG_SECTION} da
>>>> +	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
>>>> +	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
>>>> +	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
>>>> +		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
>>>>
>>>
>>> Just out of curiosity; did you find that the pin is needed? I would have
>>> thought tc attach would be enough to keep the program aroud.
>>
>> That's more because that's the only way I found to make the filter addition
>> work. With the current command, the tc command fails because of the verifier
>> rejecting the program:
>>
>> Verifier analysis:
>>
>> func#0 @0
>> 0: R1=ctx() R10=fp0
>> 0: (bf) r6 = r1                       ; R1=ctx() R6_w=ctx()
>> 1: (b4) w1 = 0                        ; R1_w=0
>> 2: (63) *(u32 *)(r10 -4) = r1
>> mark_precise: frame0: last_idx 2 first_idx 0 subseq_idx -1
>> mark_precise: frame0: regs=r1 stack= before 1: (b4) w1 = 0
>> 3: R1_w=0 R10=fp0 fp-8=0000????
>> 3: (bf) r1 = r6                       ; R1_w=ctx() R6_w=ctx()
>> 4: (b4) w2 = 0                        ; R2_w=0
>> 5: (85) call bpf_skb_ancestor_cgroup_id#83    ; R0_w=scalar()
>> 6: (7b) *(u64 *)(r10 -16) = r0        ; R0_w=scalar(id=1) R10=fp0
>> fp-16_w=scalar(id=1)
>> 7: (bf) r2 = r10                      ; R2_w=fp0 R10=fp0
>> 8: (07) r2 += -4                      ; R2_w=fp-4
>> 9: (bf) r3 = r10                      ; R3_w=fp0 R10=fp0
>> 10: (07) r3 += -16                    ; R3_w=fp-16
>> 11: (18) r1 = 0x0                     ; R1_w=0
>> 13: (b7) r4 = 0                       ; R4_w=0
>> 14: (85) call bpf_map_update_elem#2
>> R1 type=scalar expected=map_ptr
>> processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 0
>> peak_states 0 mark_read 0
>>
>> (I also tried to remove the sec argument from the tc commandline, in case it
>> could allow tc to load everything, but the issue remains the same)
>>
>> IIUC the verifier output, there's an issue with the variable representing the map.
>> When stracing the tc filter add command, I indeed see the BPF_PROG_LOAD syscall
>> but no BPF_MAP_CREATE at all, so I assumed tc did not read/create the map
>> correctly. That's why I used bpftool to make sure everything is loaded, but as a
>> consequence I need to provide a pin path when using load/loadall. But maybe I am
>> missing something ?
>>
> 
> No I think you're absolutely right. It seems like there have been
> problems with BTF-defined maps in the past with tc filter loading [1],
> but more recent tc fixes this since it uses libbpf under the hood. I
> tried with the section name update only and the test passes, so it might
> just be a tc version issue. As in [1] I'd suggest compiling an
> up-to-date iproute2/tc and re-testing. Thanks!
> 
> [1] https://lore.kernel.org/bpf/87zgkx9l6y.fsf@toke.dk/

I have checked my tc binary and indeed it was not built with libbpf support. So
applying what is suggested in the thread you have attached, I rebuilt tc with
proper libbpf support, and now the original tc command works properly (bpf
program *and* map properly loaded through tc). So in the end, the issue was on
my testing setup.

I have already sent a v2 updating the other topics you have suggested, if it
raises new comments I will use the opportunity to put back the previous tc
filter command.

Thanks for the review and the helpful comments !

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH bpf-next 4/4] selftests/bpf: convert test_skb_cgroup_id_user to test_progs

[Alexis Lothoré (eBPF Foundation)]

test_skb_cgroup_id_user allows testing skb cgroup id retrieval at different
levels, but is not integrated in test_progs, so it is not run
automatically in CI. The test overlaps a bit with
cgroup_skb_sk_lookup_kern, which is integrated in test_progs and test
extensively skb cgroup helpers, but there is still one major difference
between the two tests which justifies the conversion:
cgroup_skb_sk_lookup_kern deals with a BPF_PROG_TYPE_CGROUP_SKB (attached
on a cgroup), while test_skb_cgroup_id_user deals with a
BPF_PROG_TYPE_SCHED_CLS (attached on a qdisc)

Convert test_skb_cgroup_id_user into test_progs framework in order to run
it automatically in CI. The main differences with the original test are the
following:
- rename the test to make it shorter and more straightforward regarding
  tested feature
- the wrapping shell script has been dropped since every setup step is now
  handled in the main C test file
- the test has been renamed for a shorter name and reflecting the tested
  API
- add dedicated assert log per level to ease test failure debugging

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
The new test has been tested in a qemu environment:

  ./test_progs -a cgroup_ancestor
  44      cgroup_ancestor:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
---
 tools/testing/selftests/bpf/.gitignore             |   1 -
 tools/testing/selftests/bpf/Makefile               |   3 +-
 .../selftests/bpf/prog_tests/cgroup_ancestor.c     | 159 ++++++++++++++++++
 tools/testing/selftests/bpf/test_skb_cgroup_id.sh  |  67 --------
 .../selftests/bpf/test_skb_cgroup_id_user.c        | 183 ---------------------
 5 files changed, 160 insertions(+), 253 deletions(-)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index a45f11f81337..c46950d4ef53 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
-test_skb_cgroup_id_user
 test_flow_dissector
 flow_dissector_load
 test_tcpnotify_user
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0ac0f9dbc2f8..057e6ba003f1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -138,7 +138,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
 	test_xdp_vlan.sh test_bpftool.py
 
 # Compile but not part of 'make run_tests'
-TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
+TEST_GEN_PROGS_EXTENDED = \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
 	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
@@ -292,7 +292,6 @@ CAP_HELPERS	:= $(OUTPUT)/cap_helpers.o
 NETWORK_HELPERS := $(OUTPUT)/network_helpers.o
 
 $(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
-$(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)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c b/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c
new file mode 100644
index 000000000000..796aa3af75e8
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "test_progs.h"
+#include "network_helpers.h"
+#include "cgroup_helpers.h"
+#include "cgroup_ancestor.skel.h"
+
+#define VETH_PREFIX "test_cgid_"
+#define VETH_1 VETH_PREFIX "1"
+#define VETH_2 VETH_PREFIX "2"
+#define CGROUP_PATH "/skb_cgroup_test"
+#define NUM_CGROUP_LEVELS 4
+#define WAIT_AUTO_IP_MAX_ATTEMPT 10
+#define DST_ADDR "ff02::1"
+#define DST_PORT 1234
+#define MAX_ASSERT_NAME 32
+
+struct test_data {
+	struct cgroup_ancestor *skel;
+	struct bpf_tc_hook qdisc;
+	struct bpf_tc_opts tc_attach;
+};
+
+static int send_datagram(void)
+{
+	unsigned char buf[] = "some random test data";
+	struct sockaddr_in6 addr = { .sin6_family = AF_INET6,
+				     .sin6_port = htons(DST_PORT),
+				     .sin6_scope_id = if_nametoindex(VETH_1) };
+	int sock, n;
+
+	if (!ASSERT_EQ(inet_pton(AF_INET6, DST_ADDR, &addr.sin6_addr), 1,
+		       "inet_pton"))
+		return -1;
+
+	sock = socket(AF_INET6, SOCK_DGRAM, 0);
+	if (!ASSERT_OK_FD(sock, "create socket"))
+		return sock;
+
+	n = sendto(sock, buf, sizeof(buf), 0, (const struct sockaddr *)&addr,
+		   sizeof(addr));
+	if (!ASSERT_EQ(n, sizeof(buf), "send data"))
+		return n;
+
+	return 0;
+}
+
+static int wait_local_ip(void)
+{
+	char *ping_cmd = ping_command(AF_INET6);
+	int i, err;
+
+	for (i = 0; i < WAIT_AUTO_IP_MAX_ATTEMPT; i++) {
+		err = SYS_NOFAIL("%s -c 1 -W 1 %s%%%s", ping_cmd, DST_ADDR,
+				 VETH_1);
+		if (!err)
+			break;
+	}
+
+	return err;
+}
+
+static int setup_network(struct test_data *t)
+{
+	int ret;
+
+	SYS(fail, "ip link add dev %s type veth peer name %s", VETH_1, VETH_2);
+	SYS(fail, "ip link set %s up", VETH_1);
+	SYS(fail, "ip link set %s up", VETH_2);
+
+	ret = wait_local_ip();
+	if (!ASSERT_EQ(ret, 0, "wait local ip"))
+		goto fail;
+
+	memset(&t->qdisc, 0, sizeof(t->qdisc));
+	t->qdisc.sz = sizeof(t->qdisc);
+	t->qdisc.attach_point = BPF_TC_EGRESS;
+	t->qdisc.ifindex = if_nametoindex(VETH_1);
+	if (!ASSERT_NEQ(t->qdisc.ifindex, 0, "if_nametoindex"))
+		goto cleanup_interfaces;
+	if (!ASSERT_OK(bpf_tc_hook_create(&t->qdisc), "qdisc add"))
+		goto cleanup_interfaces;
+
+	memset(&t->tc_attach, 0, sizeof(t->tc_attach));
+	t->tc_attach.sz = sizeof(t->tc_attach);
+	t->tc_attach.prog_fd = bpf_program__fd(t->skel->progs.log_cgroup_id);
+	if (!ASSERT_OK(bpf_tc_attach(&t->qdisc, &t->tc_attach), "filter add"))
+		goto cleanup_qdisc;
+
+	return 0;
+
+cleanup_qdisc:
+	bpf_tc_hook_destroy(&t->qdisc);
+cleanup_interfaces:
+	SYS_NOFAIL("ip link del %s", VETH_1);
+fail:
+	return 1;
+}
+
+static void cleanup_network(struct test_data *t)
+{
+	bpf_tc_detach(&t->qdisc, &t->tc_attach);
+	bpf_tc_hook_destroy(&t->qdisc);
+	/* Deleting first interface will also delete peer interface */
+	SYS_NOFAIL("ip link del %s", VETH_1);
+}
+
+static void check_ancestors_ids(struct test_data *t)
+{
+	__u64 actual_ids[NUM_CGROUP_LEVELS], expected_ids[NUM_CGROUP_LEVELS];
+	char assert_name[MAX_ASSERT_NAME];
+	__u32 level;
+	int err;
+
+	expected_ids[0] = get_cgroup_id("/.."); /* root cgroup */
+	expected_ids[1] = get_cgroup_id("");
+	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
+	expected_ids[3] = 0; /* non-existent cgroup */
+
+	for (level = 0; level < NUM_CGROUP_LEVELS; level++) {
+		err = bpf_map__lookup_elem(t->skel->maps.cgroup_ids, &level,
+					   sizeof(level), &actual_ids[level],
+					   sizeof(__u64), 0);
+		if (!ASSERT_OK(err, "read map"))
+			continue;
+		snprintf(assert_name, MAX_ASSERT_NAME,
+			 "ancestor id at level %d", level);
+		ASSERT_EQ(actual_ids[level], expected_ids[level], assert_name);
+	}
+}
+
+void test_cgroup_ancestor(void)
+{
+	struct test_data t;
+	int cgroup_fd;
+
+	t.skel = cgroup_ancestor__open_and_load();
+	if (!ASSERT_OK_PTR(t.skel, "open and load"))
+		return;
+
+	if (setup_network(&t))
+		goto cleanup_progs;
+
+	cgroup_fd = cgroup_setup_and_join(CGROUP_PATH);
+	if (cgroup_fd < 0)
+		goto cleanup_network;
+
+	if (send_datagram())
+		goto cleanup_cgroups;
+
+	check_ancestors_ids(&t);
+
+cleanup_cgroups:
+	cleanup_cgroup_environment();
+cleanup_network:
+	cleanup_network(&t);
+cleanup_progs:
+	cgroup_ancestor__destroy(t.skel);
+}
diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
deleted file mode 100755
index d7dad49175c2..000000000000
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
+++ /dev/null
@@ -1,67 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-# Copyright (c) 2018 Facebook
-
-set -eu
-
-wait_for_ip()
-{
-	local _i
-	echo -n "Wait for testing link-local IP to become available "
-	for _i in $(seq ${MAX_PING_TRIES}); do
-		echo -n "."
-		if $PING6 -c 1 -W 1 ff02::1%${TEST_IF} >/dev/null 2>&1; then
-			echo " OK"
-			return
-		fi
-		sleep 1
-	done
-	echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
-	exit 1
-}
-
-setup()
-{
-	# Create testing interfaces not to interfere with current environment.
-	ip link add dev ${TEST_IF} type veth peer name ${TEST_IF_PEER}
-	ip link set ${TEST_IF} up
-	ip link set ${TEST_IF_PEER} up
-
-	wait_for_ip
-
-	tc qdisc add dev ${TEST_IF} clsact
-	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
-	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
-	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
-		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
-
-	BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \
-			awk '/ id / {sub(/.* id /, "", $0); print($1)}')
-}
-
-cleanup()
-{
-	ip link del ${TEST_IF} 2>/dev/null || :
-	ip link del ${TEST_IF_PEER} 2>/dev/null || :
-	rm -rf /sys/fs/bpf/${BPF_PROG_PIN}
-}
-
-main()
-{
-	trap cleanup EXIT 2 3 6 15
-	setup
-	${PROG} ${TEST_IF} ${BPF_PROG_ID}
-}
-
-DIR=$(dirname $0)
-TEST_IF="test_cgid_1"
-TEST_IF_PEER="test_cgid_2"
-MAX_PING_TRIES=5
-BPF_PROG_PIN="cgroup_ancestor"
-BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o"
-BPF_PROG_NAME="log_cgroup_id"
-BPF_PROG_ID=0
-PROG="${DIR}/test_skb_cgroup_id_user"
-type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
-
-main
diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
deleted file mode 100644
index ed518d075d1d..000000000000
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
+++ /dev/null
@@ -1,183 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-
-#include <arpa/inet.h>
-#include <net/if.h>
-#include <netinet/in.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "cgroup_helpers.h"
-
-#define CGROUP_PATH		"/skb_cgroup_test"
-#define NUM_CGROUP_LEVELS	4
-
-/* RFC 4291, Section 2.7.1 */
-#define LINKLOCAL_MULTICAST	"ff02::1"
-
-static int mk_dst_addr(const char *ip, const char *iface,
-		       struct sockaddr_in6 *dst)
-{
-	memset(dst, 0, sizeof(*dst));
-
-	dst->sin6_family = AF_INET6;
-	dst->sin6_port = htons(1025);
-
-	if (inet_pton(AF_INET6, ip, &dst->sin6_addr) != 1) {
-		log_err("Invalid IPv6: %s", ip);
-		return -1;
-	}
-
-	dst->sin6_scope_id = if_nametoindex(iface);
-	if (!dst->sin6_scope_id) {
-		log_err("Failed to get index of iface: %s", iface);
-		return -1;
-	}
-
-	return 0;
-}
-
-static int send_packet(const char *iface)
-{
-	struct sockaddr_in6 dst;
-	char msg[] = "msg";
-	int err = 0;
-	int fd = -1;
-
-	if (mk_dst_addr(LINKLOCAL_MULTICAST, iface, &dst))
-		goto err;
-
-	fd = socket(AF_INET6, SOCK_DGRAM, 0);
-	if (fd == -1) {
-		log_err("Failed to create UDP socket");
-		goto err;
-	}
-
-	if (sendto(fd, &msg, sizeof(msg), 0, (const struct sockaddr *)&dst,
-		   sizeof(dst)) == -1) {
-		log_err("Failed to send datagram");
-		goto err;
-	}
-
-	goto out;
-err:
-	err = -1;
-out:
-	if (fd >= 0)
-		close(fd);
-	return err;
-}
-
-int get_map_fd_by_prog_id(int prog_id)
-{
-	struct bpf_prog_info info = {};
-	__u32 info_len = sizeof(info);
-	__u32 map_ids[1];
-	int prog_fd = -1;
-	int map_fd = -1;
-
-	prog_fd = bpf_prog_get_fd_by_id(prog_id);
-	if (prog_fd < 0) {
-		log_err("Failed to get fd by prog id %d", prog_id);
-		goto err;
-	}
-
-	info.nr_map_ids = 1;
-	info.map_ids = (__u64) (unsigned long) map_ids;
-
-	if (bpf_prog_get_info_by_fd(prog_fd, &info, &info_len)) {
-		log_err("Failed to get info by prog fd %d", prog_fd);
-		goto err;
-	}
-
-	if (!info.nr_map_ids) {
-		log_err("No maps found for prog fd %d", prog_fd);
-		goto err;
-	}
-
-	map_fd = bpf_map_get_fd_by_id(map_ids[0]);
-	if (map_fd < 0)
-		log_err("Failed to get fd by map id %d", map_ids[0]);
-err:
-	if (prog_fd >= 0)
-		close(prog_fd);
-	return map_fd;
-}
-
-int check_ancestor_cgroup_ids(int prog_id)
-{
-	__u64 actual_ids[NUM_CGROUP_LEVELS], expected_ids[NUM_CGROUP_LEVELS];
-	__u32 level;
-	int err = 0;
-	int map_fd;
-
-	expected_ids[0] = get_cgroup_id("/..");	/* root cgroup */
-	expected_ids[1] = get_cgroup_id("");
-	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
-	expected_ids[3] = 0; /* non-existent cgroup */
-
-	map_fd = get_map_fd_by_prog_id(prog_id);
-	if (map_fd < 0)
-		goto err;
-
-	for (level = 0; level < NUM_CGROUP_LEVELS; ++level) {
-		if (bpf_map_lookup_elem(map_fd, &level, &actual_ids[level])) {
-			log_err("Failed to lookup key %d", level);
-			goto err;
-		}
-		if (actual_ids[level] != expected_ids[level]) {
-			log_err("%llx (actual) != %llx (expected), level: %u\n",
-				actual_ids[level], expected_ids[level], level);
-			goto err;
-		}
-	}
-
-	goto out;
-err:
-	err = -1;
-out:
-	if (map_fd >= 0)
-		close(map_fd);
-	return err;
-}
-
-int main(int argc, char **argv)
-{
-	int cgfd = -1;
-	int err = 0;
-
-	if (argc < 3) {
-		fprintf(stderr, "Usage: %s iface prog_id\n", argv[0]);
-		exit(EXIT_FAILURE);
-	}
-
-	/* Use libbpf 1.0 API mode */
-	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
-
-	cgfd = cgroup_setup_and_join(CGROUP_PATH);
-	if (cgfd < 0)
-		goto err;
-
-	if (send_packet(argv[1]))
-		goto err;
-
-	if (check_ancestor_cgroup_ids(atoi(argv[2])))
-		goto err;
-
-	goto out;
-err:
-	err = -1;
-out:
-	close(cgfd);
-	cleanup_cgroup_environment();
-	printf("[%s]\n", err ? "FAIL" : "PASS");
-	return err;
-}

-- 
2.45.2

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

[Alan Maguire]

On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> test_skb_cgroup_id_user allows testing skb cgroup id retrieval at different
> levels, but is not integrated in test_progs, so it is not run
> automatically in CI. The test overlaps a bit with
> cgroup_skb_sk_lookup_kern, which is integrated in test_progs and test
> extensively skb cgroup helpers, but there is still one major difference
> between the two tests which justifies the conversion:
> cgroup_skb_sk_lookup_kern deals with a BPF_PROG_TYPE_CGROUP_SKB (attached
> on a cgroup), while test_skb_cgroup_id_user deals with a
> BPF_PROG_TYPE_SCHED_CLS (attached on a qdisc)
> 
> Convert test_skb_cgroup_id_user into test_progs framework in order to run
> it automatically in CI. The main differences with the original test are the
> following:
> - rename the test to make it shorter and more straightforward regarding
>   tested feature
> - the wrapping shell script has been dropped since every setup step is now
>   handled in the main C test file
> - the test has been renamed for a shorter name and reflecting the tested
>   API
> - add dedicated assert log per level to ease test failure debugging
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

Looks great! A few things below...

> ---
> The new test has been tested in a qemu environment:
> 
>   ./test_progs -a cgroup_ancestor
>   44      cgroup_ancestor:OK
>   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> ---
>  tools/testing/selftests/bpf/.gitignore             |   1 -
>  tools/testing/selftests/bpf/Makefile               |   3 +-
>  .../selftests/bpf/prog_tests/cgroup_ancestor.c     | 159 ++++++++++++++++++
>  tools/testing/selftests/bpf/test_skb_cgroup_id.sh  |  67 --------
>  .../selftests/bpf/test_skb_cgroup_id_user.c        | 183 ---------------------
>  5 files changed, 160 insertions(+), 253 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index a45f11f81337..c46950d4ef53 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
> -test_skb_cgroup_id_user
>  test_flow_dissector
>  flow_dissector_load
>  test_tcpnotify_user
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 0ac0f9dbc2f8..057e6ba003f1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -138,7 +138,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
>  	test_xdp_vlan.sh test_bpftool.py
>  
>  # Compile but not part of 'make run_tests'
> -TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
> +TEST_GEN_PROGS_EXTENDED = \
>  	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>  	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
>  	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> @@ -292,7 +292,6 @@ CAP_HELPERS	:= $(OUTPUT)/cap_helpers.o
>  NETWORK_HELPERS := $(OUTPUT)/network_helpers.o
>  
>  $(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> -$(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)
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c b/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c
> new file mode 100644
> index 000000000000..796aa3af75e8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_ancestor.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "test_progs.h"
> +#include "network_helpers.h"
> +#include "cgroup_helpers.h"
> +#include "cgroup_ancestor.skel.h"
> +
> +#define VETH_PREFIX "test_cgid_"
> +#define VETH_1 VETH_PREFIX "1"
> +#define VETH_2 VETH_PREFIX "2"
> +#define CGROUP_PATH "/skb_cgroup_test"
> +#define NUM_CGROUP_LEVELS 4
> +#define WAIT_AUTO_IP_MAX_ATTEMPT 10
> +#define DST_ADDR "ff02::1"
> +#define DST_PORT 1234
> +#define MAX_ASSERT_NAME 32
> +
> +struct test_data {
> +	struct cgroup_ancestor *skel;
> +	struct bpf_tc_hook qdisc;
> +	struct bpf_tc_opts tc_attach;
> +};
> +
> +static int send_datagram(void)
> +{
> +	unsigned char buf[] = "some random test data";
> +	struct sockaddr_in6 addr = { .sin6_family = AF_INET6,
> +				     .sin6_port = htons(DST_PORT),
> +				     .sin6_scope_id = if_nametoindex(VETH_1) };
> +	int sock, n;
> +
> +	if (!ASSERT_EQ(inet_pton(AF_INET6, DST_ADDR, &addr.sin6_addr), 1,
> +		       "inet_pton"))
> +		return -1;
> +
> +	sock = socket(AF_INET6, SOCK_DGRAM, 0);
> +	if (!ASSERT_OK_FD(sock, "create socket"))
> +		return sock;
> +
> +	n = sendto(sock, buf, sizeof(buf), 0, (const struct sockaddr *)&addr,
> +		   sizeof(addr));
> +	if (!ASSERT_EQ(n, sizeof(buf), "send data"))
> +		return n;
> +
> +	return 0;
> +}
> +
> +static int wait_local_ip(void)
> +{
> +	char *ping_cmd = ping_command(AF_INET6);
> +	int i, err;
> +
> +	for (i = 0; i < WAIT_AUTO_IP_MAX_ATTEMPT; i++) {
> +		err = SYS_NOFAIL("%s -c 1 -W 1 %s%%%s", ping_cmd, DST_ADDR,
> +				 VETH_1);
> +		if (!err)
> +			break;
> +	}


thinking about the risks of CI flakiness, would a small sleep between
checks be worth doing here?
> +
> +	return err;
> +}
> +
> +static int setup_network(struct test_data *t)
> +{
> +	int ret;
> +
> +	SYS(fail, "ip link add dev %s type veth peer name %s", VETH_1, VETH_2);
> +	SYS(fail, "ip link set %s up", VETH_1);
> +	SYS(fail, "ip link set %s up", VETH_2);
> +
> +	ret = wait_local_ip();
> +	if (!ASSERT_EQ(ret, 0, "wait local ip"))
> +		goto fail;
> +
> +	memset(&t->qdisc, 0, sizeof(t->qdisc));
> +	t->qdisc.sz = sizeof(t->qdisc);
> +	t->qdisc.attach_point = BPF_TC_EGRESS;
> +	t->qdisc.ifindex = if_nametoindex(VETH_1);
> +	if (!ASSERT_NEQ(t->qdisc.ifindex, 0, "if_nametoindex"))
> +		goto cleanup_interfaces;
> +	if (!ASSERT_OK(bpf_tc_hook_create(&t->qdisc), "qdisc add"))
> +		goto cleanup_interfaces;
> +
> +	memset(&t->tc_attach, 0, sizeof(t->tc_attach));
> +	t->tc_attach.sz = sizeof(t->tc_attach);
> +	t->tc_attach.prog_fd = bpf_program__fd(t->skel->progs.log_cgroup_id);
> +	if (!ASSERT_OK(bpf_tc_attach(&t->qdisc, &t->tc_attach), "filter add"))
> +		goto cleanup_qdisc;
> +
> +	return 0;
> +
> +cleanup_qdisc:
> +	bpf_tc_hook_destroy(&t->qdisc);
> +cleanup_interfaces:
> +	SYS_NOFAIL("ip link del %s", VETH_1);
> +fail:
> +	return 1;
> +}
> +
> +static void cleanup_network(struct test_data *t)
> +{
> +	bpf_tc_detach(&t->qdisc, &t->tc_attach);
> +	bpf_tc_hook_destroy(&t->qdisc);
> +	/* Deleting first interface will also delete peer interface */
> +	SYS_NOFAIL("ip link del %s", VETH_1);
> +}
> +
> +static void check_ancestors_ids(struct test_data *t)
> +{
> +	__u64 actual_ids[NUM_CGROUP_LEVELS], expected_ids[NUM_CGROUP_LEVELS];
> +	char assert_name[MAX_ASSERT_NAME];
> +	__u32 level;
> +	int err;
> +
> +	expected_ids[0] = get_cgroup_id("/.."); /* root cgroup */
> +	expected_ids[1] = get_cgroup_id("");
> +	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
> +	expected_ids[3] = 0; /* non-existent cgroup */
> +
> +	for (level = 0; level < NUM_CGROUP_LEVELS; level++) {
> +		err = bpf_map__lookup_elem(t->skel->maps.cgroup_ids, &level,
> +					   sizeof(level), &actual_ids[level],
> +					   sizeof(__u64), 0);

could probably simplify this + the BPF prog using a global array of
actual_ids[], then compare it to the expected values using
skel->bss->actual_ids

> +		if (!ASSERT_OK(err, "read map"))
> +			continue;
> +		snprintf(assert_name, MAX_ASSERT_NAME,
> +			 "ancestor id at level %d", level);
> +		ASSERT_EQ(actual_ids[level], expected_ids[level], assert_name);
> +	}
> +}
> +
> +void test_cgroup_ancestor(void)
> +{
> +	struct test_data t;
> +	int cgroup_fd;
> +
> +	t.skel = cgroup_ancestor__open_and_load();
> +	if (!ASSERT_OK_PTR(t.skel, "open and load"))
> +		return;
> +
> +	if (setup_network(&t))
> +		goto cleanup_progs;
> +
> +	cgroup_fd = cgroup_setup_and_join(CGROUP_PATH);
> +	if (cgroup_fd < 0)
> +		goto cleanup_network;
> +
> +	if (send_datagram())
> +		goto cleanup_cgroups;
> +
> +	check_ancestors_ids(&t);
> +
> +cleanup_cgroups:
> +	cleanup_cgroup_environment();
> +cleanup_network:
> +	cleanup_network(&t);
> +cleanup_progs:
> +	cgroup_ancestor__destroy(t.skel);
> +}
> diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> deleted file mode 100755
> index d7dad49175c2..000000000000
> --- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -#!/bin/sh
> -# SPDX-License-Identifier: GPL-2.0
> -# Copyright (c) 2018 Facebook
> -
> -set -eu
> -
> -wait_for_ip()
> -{
> -	local _i
> -	echo -n "Wait for testing link-local IP to become available "
> -	for _i in $(seq ${MAX_PING_TRIES}); do
> -		echo -n "."
> -		if $PING6 -c 1 -W 1 ff02::1%${TEST_IF} >/dev/null 2>&1; then
> -			echo " OK"
> -			return
> -		fi
> -		sleep 1
> -	done
> -	echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
> -	exit 1
> -}
> -
> -setup()
> -{
> -	# Create testing interfaces not to interfere with current environment.
> -	ip link add dev ${TEST_IF} type veth peer name ${TEST_IF_PEER}
> -	ip link set ${TEST_IF} up
> -	ip link set ${TEST_IF_PEER} up
> -
> -	wait_for_ip
> -
> -	tc qdisc add dev ${TEST_IF} clsact
> -	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
> -	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
> -	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
> -		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
> -
> -	BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \
> -			awk '/ id / {sub(/.* id /, "", $0); print($1)}')
> -}
> -
> -cleanup()
> -{
> -	ip link del ${TEST_IF} 2>/dev/null || :
> -	ip link del ${TEST_IF_PEER} 2>/dev/null || :
> -	rm -rf /sys/fs/bpf/${BPF_PROG_PIN}
> -}
> -
> -main()
> -{
> -	trap cleanup EXIT 2 3 6 15
> -	setup
> -	${PROG} ${TEST_IF} ${BPF_PROG_ID}
> -}
> -
> -DIR=$(dirname $0)
> -TEST_IF="test_cgid_1"
> -TEST_IF_PEER="test_cgid_2"
> -MAX_PING_TRIES=5
> -BPF_PROG_PIN="cgroup_ancestor"
> -BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o"
> -BPF_PROG_NAME="log_cgroup_id"
> -BPF_PROG_ID=0
> -PROG="${DIR}/test_skb_cgroup_id_user"
> -type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
> -
> -main
> diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
> deleted file mode 100644
> index ed518d075d1d..000000000000
> --- a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
> +++ /dev/null
> @@ -1,183 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2018 Facebook
> -
> -#include <stdlib.h>
> -#include <string.h>
> -#include <unistd.h>
> -
> -#include <arpa/inet.h>
> -#include <net/if.h>
> -#include <netinet/in.h>
> -#include <sys/socket.h>
> -#include <sys/types.h>
> -
> -
> -#include <bpf/bpf.h>
> -#include <bpf/libbpf.h>
> -
> -#include "cgroup_helpers.h"
> -
> -#define CGROUP_PATH		"/skb_cgroup_test"
> -#define NUM_CGROUP_LEVELS	4
> -
> -/* RFC 4291, Section 2.7.1 */
> -#define LINKLOCAL_MULTICAST	"ff02::1"
> -
> -static int mk_dst_addr(const char *ip, const char *iface,
> -		       struct sockaddr_in6 *dst)
> -{
> -	memset(dst, 0, sizeof(*dst));
> -
> -	dst->sin6_family = AF_INET6;
> -	dst->sin6_port = htons(1025);
> -
> -	if (inet_pton(AF_INET6, ip, &dst->sin6_addr) != 1) {
> -		log_err("Invalid IPv6: %s", ip);
> -		return -1;
> -	}
> -
> -	dst->sin6_scope_id = if_nametoindex(iface);
> -	if (!dst->sin6_scope_id) {
> -		log_err("Failed to get index of iface: %s", iface);
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
> -static int send_packet(const char *iface)
> -{
> -	struct sockaddr_in6 dst;
> -	char msg[] = "msg";
> -	int err = 0;
> -	int fd = -1;
> -
> -	if (mk_dst_addr(LINKLOCAL_MULTICAST, iface, &dst))
> -		goto err;
> -
> -	fd = socket(AF_INET6, SOCK_DGRAM, 0);
> -	if (fd == -1) {
> -		log_err("Failed to create UDP socket");
> -		goto err;
> -	}
> -
> -	if (sendto(fd, &msg, sizeof(msg), 0, (const struct sockaddr *)&dst,
> -		   sizeof(dst)) == -1) {
> -		log_err("Failed to send datagram");
> -		goto err;
> -	}
> -
> -	goto out;
> -err:
> -	err = -1;
> -out:
> -	if (fd >= 0)
> -		close(fd);
> -	return err;
> -}
> -
> -int get_map_fd_by_prog_id(int prog_id)
> -{
> -	struct bpf_prog_info info = {};
> -	__u32 info_len = sizeof(info);
> -	__u32 map_ids[1];
> -	int prog_fd = -1;
> -	int map_fd = -1;
> -
> -	prog_fd = bpf_prog_get_fd_by_id(prog_id);
> -	if (prog_fd < 0) {
> -		log_err("Failed to get fd by prog id %d", prog_id);
> -		goto err;
> -	}
> -
> -	info.nr_map_ids = 1;
> -	info.map_ids = (__u64) (unsigned long) map_ids;
> -
> -	if (bpf_prog_get_info_by_fd(prog_fd, &info, &info_len)) {
> -		log_err("Failed to get info by prog fd %d", prog_fd);
> -		goto err;
> -	}
> -
> -	if (!info.nr_map_ids) {
> -		log_err("No maps found for prog fd %d", prog_fd);
> -		goto err;
> -	}
> -
> -	map_fd = bpf_map_get_fd_by_id(map_ids[0]);
> -	if (map_fd < 0)
> -		log_err("Failed to get fd by map id %d", map_ids[0]);
> -err:
> -	if (prog_fd >= 0)
> -		close(prog_fd);
> -	return map_fd;
> -}
> -
> -int check_ancestor_cgroup_ids(int prog_id)
> -{
> -	__u64 actual_ids[NUM_CGROUP_LEVELS], expected_ids[NUM_CGROUP_LEVELS];
> -	__u32 level;
> -	int err = 0;
> -	int map_fd;
> -
> -	expected_ids[0] = get_cgroup_id("/..");	/* root cgroup */
> -	expected_ids[1] = get_cgroup_id("");
> -	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
> -	expected_ids[3] = 0; /* non-existent cgroup */
> -
> -	map_fd = get_map_fd_by_prog_id(prog_id);
> -	if (map_fd < 0)
> -		goto err;
> -
> -	for (level = 0; level < NUM_CGROUP_LEVELS; ++level) {
> -		if (bpf_map_lookup_elem(map_fd, &level, &actual_ids[level])) {
> -			log_err("Failed to lookup key %d", level);
> -			goto err;
> -		}
> -		if (actual_ids[level] != expected_ids[level]) {
> -			log_err("%llx (actual) != %llx (expected), level: %u\n",
> -				actual_ids[level], expected_ids[level], level);
> -			goto err;
> -		}
> -	}
> -
> -	goto out;
> -err:
> -	err = -1;
> -out:
> -	if (map_fd >= 0)
> -		close(map_fd);
> -	return err;
> -}
> -
> -int main(int argc, char **argv)
> -{
> -	int cgfd = -1;
> -	int err = 0;
> -
> -	if (argc < 3) {
> -		fprintf(stderr, "Usage: %s iface prog_id\n", argv[0]);
> -		exit(EXIT_FAILURE);
> -	}
> -
> -	/* Use libbpf 1.0 API mode */
> -	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> -
> -	cgfd = cgroup_setup_and_join(CGROUP_PATH);
> -	if (cgfd < 0)
> -		goto err;
> -
> -	if (send_packet(argv[1]))
> -		goto err;
> -
> -	if (check_ancestor_cgroup_ids(atoi(argv[2])))
> -		goto err;
> -
> -	goto out;
> -err:
> -	err = -1;
> -out:
> -	close(cgfd);
> -	cleanup_cgroup_environment();
> -	printf("[%s]\n", err ? "FAIL" : "PASS");
> -	return err;
> -}
> 

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

[Alexis Lothoré]

On 8/1/24 10:49, Alan Maguire wrote:
> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>> +static int wait_local_ip(void)
>> +{
>> +	char *ping_cmd = ping_command(AF_INET6);
>> +	int i, err;
>> +
>> +	for (i = 0; i < WAIT_AUTO_IP_MAX_ATTEMPT; i++) {
>> +		err = SYS_NOFAIL("%s -c 1 -W 1 %s%%%s", ping_cmd, DST_ADDR,
>> +				 VETH_1);
>> +		if (!err)
>> +			break;
>> +	}
> 
> 
> thinking about the risks of CI flakiness, would a small sleep between
> checks be worth doing here?

I assumed that adding -W 1 (ping timeout duration) to the command would be
enough to make sure that there is a proper wait between each attempt (so
currently, waiting at most 10s for network configuration between the 2 veths).
Don't you think it is enough to prevent issues in CI ?

>> +

[...]

>> +
>> +	expected_ids[0] = get_cgroup_id("/.."); /* root cgroup */
>> +	expected_ids[1] = get_cgroup_id("");
>> +	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
>> +	expected_ids[3] = 0; /* non-existent cgroup */
>> +
>> +	for (level = 0; level < NUM_CGROUP_LEVELS; level++) {
>> +		err = bpf_map__lookup_elem(t->skel->maps.cgroup_ids, &level,
>> +					   sizeof(level), &actual_ids[level],
>> +					   sizeof(__u64), 0);
> 
> could probably simplify this + the BPF prog using a global array of
> actual_ids[], then compare it to the expected values using
> skel->bss->actual_ids

ACK, I'll update this.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Alan Maguire]

On 01/08/2024 11:12, Alexis Lothoré wrote:
> On 8/1/24 10:49, Alan Maguire wrote:
>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> 
> [...]
> 
>>> +static int wait_local_ip(void)
>>> +{
>>> +	char *ping_cmd = ping_command(AF_INET6);
>>> +	int i, err;
>>> +
>>> +	for (i = 0; i < WAIT_AUTO_IP_MAX_ATTEMPT; i++) {
>>> +		err = SYS_NOFAIL("%s -c 1 -W 1 %s%%%s", ping_cmd, DST_ADDR,
>>> +				 VETH_1);
>>> +		if (!err)
>>> +			break;
>>> +	}
>>
>>
>> thinking about the risks of CI flakiness, would a small sleep between
>> checks be worth doing here?
> 
> I assumed that adding -W 1 (ping timeout duration) to the command would be
> enough to make sure that there is a proper wait between each attempt (so
> currently, waiting at most 10s for network configuration between the 2 veths).
> Don't you think it is enough to prevent issues in CI ?
>

Yep, that should be fine, I missed the wait option.

>>> +
> 
> [...]
> 
>>> +
>>> +	expected_ids[0] = get_cgroup_id("/.."); /* root cgroup */
>>> +	expected_ids[1] = get_cgroup_id("");
>>> +	expected_ids[2] = get_cgroup_id(CGROUP_PATH);
>>> +	expected_ids[3] = 0; /* non-existent cgroup */
>>> +
>>> +	for (level = 0; level < NUM_CGROUP_LEVELS; level++) {
>>> +		err = bpf_map__lookup_elem(t->skel->maps.cgroup_ids, &level,
>>> +					   sizeof(level), &actual_ids[level],
>>> +					   sizeof(__u64), 0);
>>
>> could probably simplify this + the BPF prog using a global array of
>> actual_ids[], then compare it to the expected values using
>> skel->bss->actual_ids
> 
> ACK, I'll update this.
>

Great, thanks!

Alan