[PATCH v9 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

[PATCH v9 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

[Carlos Neira]

Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
This change folds test cases into test_progs.

Changes from v8:

 - Fixed code style
 - Fixed CHECK macro usage
 - Removed root namespace sub-test
 - Split pid_tgid variable

Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 115 ++++++-------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  27 +--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 68 insertions(+), 238 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index f5b7ef93618c..9abca0616ec0 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -26,7 +26,6 @@ test_tcpnotify_user
 test_libbpf
 test_tcp_check_syncookie_user
 test_sysctl
-test_current_pid_tgid_new_ns
 xdping
 test_cpp
 *.skel.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 8c33e999319a..886577bc2bb6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -35,8 +35,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_sock test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sysctl \
-	test_progs-no_alu32 \
-	test_current_pid_tgid_new_ns
+	test_progs-no_alu32
 
 # Also test bpf-gcc, if present
 ifneq ($(BPF_GCC),)
diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
index e74dc501b27f..038195704f2d 100644
--- a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
+++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
@@ -1,85 +1,86 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020 Carlos Neira cneirabustos@gmail.com */
+
+#define _GNU_SOURCE
 #include <test_progs.h>
+#include "test_ns_current_pid_tgid.skel.h"
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <sys/syscall.h>
+#include <sched.h>
+#include <sys/wait.h>
+#include <sys/mount.h>
+#include <sys/fcntl.h>
 
-struct bss {
-	__u64 dev;
-	__u64 ino;
-	__u64 pid_tgid;
-	__u64 user_pid_tgid;
-};
+#define STACK_SIZE (1024 * 1024)
+static char child_stack[STACK_SIZE];
 
-void test_ns_current_pid_tgid(void)
+static int newns_pid_tgid(void *arg)
 {
-	const char *probe_name = "raw_tracepoint/sys_enter";
-	const char *file = "test_ns_current_pid_tgid.o";
-	int err, key = 0, duration = 0;
-	struct bpf_link *link = NULL;
-	struct bpf_program *prog;
-	struct bpf_map *bss_map;
-	struct bpf_object *obj;
-	struct bss bss;
+	struct test_ns_current_pid_tgid__bss  *bss;
+	int err = -1, duration = 0;
+	struct test_ns_current_pid_tgid *skel;
+	pid_t pid, tid;
 	struct stat st;
-	__u64 id;
-
-	obj = bpf_object__open_file(file, NULL);
-	if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
-		return;
 
-	err = bpf_object__load(obj);
-	if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
+	skel = test_ns_current_pid_tgid__open_and_load();
+	if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
 		goto cleanup;
 
-	bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
-	if (CHECK(!bss_map, "find_bss_map", "failed\n"))
-		goto cleanup;
+	tid = syscall(SYS_gettid);
+	pid = getpid();
 
-	prog = bpf_object__find_program_by_title(obj, probe_name);
-	if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
-		  probe_name))
+	err = stat("/proc/self/ns/pid", &st);
+	if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
 		goto cleanup;
 
-	memset(&bss, 0, sizeof(bss));
-	pid_t tid = syscall(SYS_gettid);
-	pid_t pid = getpid();
-
-	id = (__u64) tid << 32 | pid;
-	bss.user_pid_tgid = id;
+	bss = skel->bss;
+	bss->dev = st.st_dev;
+	bss->ino = st.st_ino;
+	bss->user_pid = 0;
+	bss->user_tgid = 0;
 
-	if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
-		perror("Failed to stat /proc/self/ns/pid");
+	err = test_ns_current_pid_tgid__attach(skel);
+	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
 		goto cleanup;
-	}
 
-	bss.dev = st.st_dev;
-	bss.ino = st.st_ino;
+	/* trigger tracepoint */
+	usleep(1);
 
-	err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
-	if (CHECK(err, "setting_bss", "failed to set bss : %d\n", err))
+	if (CHECK((pid_t)bss->user_pid != pid, "pid", "got %d != exp %d\n",
+	 (pid_t)bss->user_pid, pid))
 		goto cleanup;
 
-	link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
-	if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n",
-		  PTR_ERR(link))) {
-		link = NULL;
+	if (CHECK((pid_t)bss->user_tgid != tid, "tgid", "got %d != exp %d\n",
+	 (pid_t)bss->user_tgid, tid))
 		goto cleanup;
-	}
 
-	/* trigger some syscalls */
-	usleep(1);
+cleanup:
+	if (skel)
+		test_ns_current_pid_tgid__destroy(skel);
 
-	err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
-	if (CHECK(err, "set_bss", "failed to get bss : %d\n", err))
-		goto cleanup;
+	return err;
+}
 
-	if (CHECK(id != bss.pid_tgid, "Compare user pid/tgid vs. bpf pid/tgid",
-		  "User pid/tgid %llu BPF pid/tgid %llu\n", id, bss.pid_tgid))
-		goto cleanup;
-cleanup:
-	bpf_link__destroy(link);
-	bpf_object__close(obj);
+void test_ns_current_pid_tgid(void)
+{
+	int wstatus, duration = 0;
+	pid_t cpid;
+
+	cpid = clone(newns_pid_tgid,
+	  child_stack + STACK_SIZE,
+	  CLONE_NEWPID | SIGCHLD, NULL);
+
+	if (CHECK(cpid == -1, "clone", strerror(errno)))
+		exit(EXIT_FAILURE);
+
+	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
+	 strerror(errno)))
+		exit(EXIT_FAILURE);
+
+
+	if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
+	 "failed"))
+		exit(EXIT_FAILURE);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
index 1dca70a6de2f..805ceef9103a 100644
--- a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
+++ b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
@@ -5,31 +5,22 @@
 #include <stdint.h>
 #include <bpf/bpf_helpers.h>
 
-static volatile struct {
-	__u64 dev;
-	__u64 ino;
-	__u64 pid_tgid;
-	__u64 user_pid_tgid;
-} res;
+__u32 user_pid = 0;
+__u32 user_tgid = 0;
+__u64 dev = 0;
+__u64 ino = 0;
 
 SEC("raw_tracepoint/sys_enter")
-int trace(void *ctx)
+int handler(const void *ctx)
 {
-	__u64  ns_pid_tgid, expected_pid;
 	struct bpf_pidns_info nsdata;
-	__u32 key = 0;
 
-	if (bpf_get_ns_current_pid_tgid(res.dev, res.ino, &nsdata,
-		   sizeof(struct bpf_pidns_info)))
+	if (bpf_get_ns_current_pid_tgid(dev, ino, &nsdata,
+		sizeof(struct bpf_pidns_info)))
 		return 0;
 
-	ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
-	expected_pid = res.user_pid_tgid;
-
-	if (expected_pid != ns_pid_tgid)
-		return 0;
-
-	res.pid_tgid = ns_pid_tgid;
+	user_pid =  nsdata.pid;
+	user_tgid = nsdata.tgid;
 
 	return 0;
 }
diff --git a/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c b/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
deleted file mode 100644
index ec53b1ef90d2..000000000000
--- a/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
+++ /dev/null
@@ -1,160 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright (c) 2020 Carlos Neira cneirabustos@gmail.com */
-#define _GNU_SOURCE
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
-#include <sys/syscall.h>
-#include <sched.h>
-#include <sys/wait.h>
-#include <sys/mount.h>
-#include "test_progs.h"
-
-#define CHECK_NEWNS(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;						\
-})
-
-struct bss {
-	__u64 dev;
-	__u64 ino;
-	__u64 pid_tgid;
-	__u64 user_pid_tgid;
-};
-
-int main(int argc, char **argv)
-{
-	pid_t pid;
-	int exit_code = 1;
-	struct stat st;
-
-	printf("Testing bpf_get_ns_current_pid_tgid helper in new ns\n");
-
-	if (stat("/proc/self/ns/pid", &st)) {
-		perror("stat failed on /proc/self/ns/pid ns\n");
-		printf("%s:FAILED\n", argv[0]);
-		return exit_code;
-	}
-
-	if (CHECK_NEWNS(unshare(CLONE_NEWPID | CLONE_NEWNS),
-			"unshare CLONE_NEWPID | CLONE_NEWNS", "error errno=%d\n", errno))
-		return exit_code;
-
-	pid = fork();
-	if (pid == -1) {
-		perror("Fork() failed\n");
-		printf("%s:FAILED\n", argv[0]);
-		return exit_code;
-	}
-
-	if (pid > 0) {
-		int status;
-
-		usleep(5);
-		waitpid(pid, &status, 0);
-		return 0;
-	} else {
-
-		pid = fork();
-		if (pid == -1) {
-			perror("Fork() failed\n");
-			printf("%s:FAILED\n", argv[0]);
-			return exit_code;
-		}
-
-		if (pid > 0) {
-			int status;
-			waitpid(pid, &status, 0);
-			return 0;
-		} else {
-			if (CHECK_NEWNS(mount("none", "/proc", NULL, MS_PRIVATE|MS_REC, NULL),
-				"Unmounting proc", "Cannot umount proc! errno=%d\n", errno))
-				return exit_code;
-
-			if (CHECK_NEWNS(mount("proc", "/proc", "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL),
-				"Mounting proc", "Cannot mount proc! errno=%d\n", errno))
-				return exit_code;
-
-			const char *probe_name = "raw_tracepoint/sys_enter";
-			const char *file = "test_ns_current_pid_tgid.o";
-			struct bpf_link *link = NULL;
-			struct bpf_program *prog;
-			struct bpf_map *bss_map;
-			struct bpf_object *obj;
-			int exit_code = 1;
-			int err, key = 0;
-			struct bss bss;
-			struct stat st;
-			__u64 id;
-
-			obj = bpf_object__open_file(file, NULL);
-			if (CHECK_NEWNS(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
-				return exit_code;
-
-			err = bpf_object__load(obj);
-			if (CHECK_NEWNS(err, "obj_load", "err %d errno %d\n", err, errno))
-				goto cleanup;
-
-			bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
-			if (CHECK_NEWNS(!bss_map, "find_bss_map", "failed\n"))
-				goto cleanup;
-
-			prog = bpf_object__find_program_by_title(obj, probe_name);
-			if (CHECK_NEWNS(!prog, "find_prog", "prog '%s' not found\n",
-						probe_name))
-				goto cleanup;
-
-			memset(&bss, 0, sizeof(bss));
-			pid_t tid = syscall(SYS_gettid);
-			pid_t pid = getpid();
-
-			id = (__u64) tid << 32 | pid;
-			bss.user_pid_tgid = id;
-
-			if (CHECK_NEWNS(stat("/proc/self/ns/pid", &st),
-				"stat new ns", "Failed to stat /proc/self/ns/pid errno=%d\n", errno))
-				goto cleanup;
-
-			bss.dev = st.st_dev;
-			bss.ino = st.st_ino;
-
-			err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
-			if (CHECK_NEWNS(err, "setting_bss", "failed to set bss : %d\n", err))
-				goto cleanup;
-
-			link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
-			if (CHECK_NEWNS(IS_ERR(link), "attach_raw_tp", "err %ld\n",
-						PTR_ERR(link))) {
-				link = NULL;
-				goto cleanup;
-			}
-
-			/* trigger some syscalls */
-			usleep(1);
-
-			err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
-			if (CHECK_NEWNS(err, "set_bss", "failed to get bss : %d\n", err))
-				goto cleanup;
-
-			if (CHECK_NEWNS(id != bss.pid_tgid, "Compare user pid/tgid vs. bpf pid/tgid",
-						"User pid/tgid %llu BPF pid/tgid %llu\n", id, bss.pid_tgid))
-				goto cleanup;
-
-			exit_code = 0;
-			printf("%s:PASS\n", argv[0]);
-cleanup:
-			if (!link) {
-				bpf_link__destroy(link);
-				link = NULL;
-			}
-			bpf_object__close(obj);
-		}
-	}
-	return 0;
-}
-- 
2.20.1

Re: [PATCH v9 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

[Andrii Nakryiko]

On Wed, Dec 16, 2020 at 6:18 AM Carlos Neira <cneirabustos@gmail.com> wrote:
>
> Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
> This change folds test cases into test_progs.
>
> Changes from v8:
>
>  - Fixed code style
>  - Fixed CHECK macro usage
>  - Removed root namespace sub-test
>  - Split pid_tgid variable
>
> Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> ---
>  tools/testing/selftests/bpf/.gitignore        |   1 -
>  tools/testing/selftests/bpf/Makefile          |   3 +-
>  .../bpf/prog_tests/ns_current_pid_tgid.c      | 115 ++++++-------
>  .../bpf/progs/test_ns_current_pid_tgid.c      |  27 +--
>  .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
>  5 files changed, 68 insertions(+), 238 deletions(-)
>  delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
>

[...]

>
> -       err = bpf_object__load(obj);
> -       if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
> +       skel = test_ns_current_pid_tgid__open_and_load();
> +       if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
>                 goto cleanup;
>
> -       bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
> -       if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> -               goto cleanup;
> +       tid = syscall(SYS_gettid);
> +       pid = getpid();

So pid here corresponds to tgid from the BPF program
tid is thread ID, which is the same as pid in Linux kernel
terminology. So checks below are wrong and just by coincidence pass.
Which also might mean that test doesn't test enough?

Would it be possible to also check non-namespaced pid/tgid and see if
they are at least different? As is, this test doesn't give me enough
confidence it would catch issues.

>
> -       prog = bpf_object__find_program_by_title(obj, probe_name);
> -       if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
> -                 probe_name))
> +       err = stat("/proc/self/ns/pid", &st);
> +       if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
>                 goto cleanup;
>
> -       memset(&bss, 0, sizeof(bss));
> -       pid_t tid = syscall(SYS_gettid);
> -       pid_t pid = getpid();
> -
> -       id = (__u64) tid << 32 | pid;
> -       bss.user_pid_tgid = id;
> +       bss = skel->bss;
> +       bss->dev = st.st_dev;
> +       bss->ino = st.st_ino;
> +       bss->user_pid = 0;
> +       bss->user_tgid = 0;
>
> -       if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
> -               perror("Failed to stat /proc/self/ns/pid");
> +       err = test_ns_current_pid_tgid__attach(skel);
> +       if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
>                 goto cleanup;
> -       }
>
> -       bss.dev = st.st_dev;
> -       bss.ino = st.st_ino;
> +       /* trigger tracepoint */
> +       usleep(1);
>
> -       err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
> -       if (CHECK(err, "setting_bss", "failed to set bss : %d\n", err))
> +       if (CHECK((pid_t)bss->user_pid != pid, "pid", "got %d != exp %d\n",
> +        (pid_t)bss->user_pid, pid))
>                 goto cleanup;
>
> -       link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
> -       if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n",
> -                 PTR_ERR(link))) {
> -               link = NULL;
> +       if (CHECK((pid_t)bss->user_tgid != tid, "tgid", "got %d != exp %d\n",
> +        (pid_t)bss->user_tgid, tid))

wrapped arguments need to be aligned with the first argument on the
first line, please pay attention to that, you have a similar problem
above.

But better yet in this case, just use ASSERT_EQ, which is more succinct:

ASSERT_EQ(bss->user_pid, pid, "pid");
ASSERT_EQ(bss->user_tgid, tid, "pid");

>                 goto cleanup;
> -       }
>
> -       /* trigger some syscalls */
> -       usleep(1);
> +cleanup:
> +       if (skel)

no need to check for null, skeleton's destroy() handles that already

> +               test_ns_current_pid_tgid__destroy(skel);
>
> -       err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
> -       if (CHECK(err, "set_bss", "failed to get bss : %d\n", err))
> -               goto cleanup;
> +       return err;
> +}
>
> -       if (CHECK(id != bss.pid_tgid, "Compare user pid/tgid vs. bpf pid/tgid",
> -                 "User pid/tgid %llu BPF pid/tgid %llu\n", id, bss.pid_tgid))
> -               goto cleanup;
> -cleanup:
> -       bpf_link__destroy(link);
> -       bpf_object__close(obj);
> +void test_ns_current_pid_tgid(void)
> +{
> +       int wstatus, duration = 0;
> +       pid_t cpid;
> +
> +       cpid = clone(newns_pid_tgid,
> +         child_stack + STACK_SIZE,
> +         CLONE_NEWPID | SIGCHLD, NULL);

I know nothing about namespaces, but seems like you are not doing
unshare(CLONE_NEWPID | CLONE_NEWNS) anymore, which previously was done
in the tests. What's up with that? You also used fork() before, now
you use clone(). It would be nice to explain the changes in the commit
message, so that reviewers don't have to dig through `man clone` that
much.

> +
> +       if (CHECK(cpid == -1, "clone", strerror(errno)))
> +               exit(EXIT_FAILURE);
> +
> +       if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
> +        strerror(errno)))
> +               exit(EXIT_FAILURE);
> +
> +
> +       if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
> +        "failed"))
> +               exit(EXIT_FAILURE);
>  }

[...]