Skip to contentSkip to conversation

bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

PATCH Superseded Carlos Neira 2 participants 2 messages Dec 16, 2020 – Dec 17, 2020

Subsystems (5)

v9 of 12 Superseded Latest is v12 →

lore
Share & tools
lore
#0 lore raw

PATCH v9 1/1 bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

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

---
 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
tools/testing/selftests/bpf/.gitignore +0 -1
index f5b7ef93618c..9abca0616ec0 100644
@@ -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
tools/testing/selftests/bpf/Makefile +1 -2
index 8c33e999319a..886577bc2bb6 100644
@@ -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),)
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +58 -57

143 lines not on this page · raw message

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +9 -18
index 1dca70a6de2f..805ceef9103a 100644
@@ -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;
 }
tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c deleted +0 -160

160 lines not on this page · raw message

Signature · 1 line
2.20.1
#1 lore raw
Quoted 21 lines · 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

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

[...]

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c line 32

Quoted 12 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +4 -5
-       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.

Quoted 34 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +11 -16
-       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))

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c line 56

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +4 -4
+       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");

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c line 60

Quoted 7 lines
                goto cleanup;
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +2 -3
-       }
 
-       /* trigger some syscalls */
-       usleep(1);
+cleanup:
+       if (skel)

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

Quoted 12 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +3 -7
+               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:

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c line 73

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +8 -2
-       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.

Quoted 13 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +12 -0
+
+       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);
 }

[...]