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 V7:
- Rebased changes.
- Changed function scope.

Signed-off-by Carlos Neira <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 148 ++++++++++------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  26 +--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 105 insertions(+), 233 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 +15 -12

-       err = bpf_object__load(obj);
-       if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
-               goto cleanup;
+       tid = syscall(SYS_gettid);
+       pid = getpid();
+
+       id = ((__u64)tid << 32) | pid;
 
-       bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
-       if (CHECK(!bss_map, "find_bss_map", "failed\n"))
+       err = stat("/proc/self/ns/pid", &st);
+       if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
                goto cleanup;
 
-       prog = bpf_object__find_program_by_title(obj, probe_name);
-       if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
-                 probe_name))
+       bss = skel->bss;
+       bss->dev = st.st_dev;
+       bss->ino = st.st_ino;
+       bss->user_pid_tgid = 0;
+
+       err = test_ns_current_pid_tgid__attach(skel);
+       if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
                goto cleanup;
 
-       memset(&bss, 0, sizeof(bss));
-       pid_t tid = syscall(SYS_gettid);
-       pid_t pid = getpid();
+       /* trigger tracepoint */
+       usleep(1);
 
-       id = (__u64) tid << 32 | pid;

                goto cleanup;
        }

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +3 -2

-       bss.dev = st.st_dev;
-       bss.ino = st.st_ino;
+       tid = syscall(SYS_gettid);
+       pid = getpid();
+       id = ((__u64) tid << 32) | pid;

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +19 -0

+
+       if (CHECK(cpid == -1, "clone", strerror(errno)))
+               exit(EXIT_FAILURE);
+
+       if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
+        strerror(errno))) {
+               exit(EXIT_FAILURE);
+       }
+
+       CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
+        "failed");
+}
+
+void test_ns_current_pid_tgid(void)
+{
+       if (test__start_subtest("ns_current_pid_tgid_global_ns"))
+               test_ns_current_pid_tgid_global_ns();
+       if (test__start_subtest("ns_current_pid_tgid_new_ns"))
+               test_ns_current_pid_tgid_new_ns();
 }

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +0 -0

index 1dca70a6de2f..0daa12db0d83 100644

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +2 -1

+__u64 dev = 0;
+__u64 ino = 0;
 
 SEC("raw_tracepoint/sys_enter")
-int trace(void *ctx)

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +0 -4

                return 0;
-
-       ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
-       expected_pid = res.user_pid_tgid;
-

Quoted 10 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 V7:
- Rebased changes.
- Changed function scope.

Signed-off-by Carlos Neira <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

please drop my ack for the next version given the changes requested, thanks!

Quoted 9 lines

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 148 ++++++++++------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  26 +--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 105 insertions(+), 233 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 30

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +3 -3

-       obj = bpf_object__open_file(file, NULL);
-       if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
-               return;
+       skel = test_ns_current_pid_tgid__open_and_load();
+       CHECK(!skel, "skel_open_load", "failed to load skeleton\n");
+       goto cleanup;

Are you sure your test is doing what you think it's doing? Try running sudo ./test_progs -t ns_current_pid_tgid -v and see if you get all the CHECK()s you expect.

It's a long way of saying that you are missing if () and just unconditionally clean up after opening and loading the skeleton.

And if the skeleton is not open_and_load()'ed successfully, there is nothing to clean up, btw.

Quoted 34 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +15 -12

-       err = bpf_object__load(obj);
-       if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
-               goto cleanup;
+       tid = syscall(SYS_gettid);
+       pid = getpid();
+
+       id = ((__u64)tid << 32) | pid;
 
-       bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
-       if (CHECK(!bss_map, "find_bss_map", "failed\n"))
+       err = stat("/proc/self/ns/pid", &st);
+       if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
                goto cleanup;
 
-       prog = bpf_object__find_program_by_title(obj, probe_name);
-       if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
-                 probe_name))
+       bss = skel->bss;
+       bss->dev = st.st_dev;
+       bss->ino = st.st_ino;
+       bss->user_pid_tgid = 0;
+
+       err = test_ns_current_pid_tgid__attach(skel);
+       if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
                goto cleanup;
 
-       memset(&bss, 0, sizeof(bss));
-       pid_t tid = syscall(SYS_gettid);
-       pid_t pid = getpid();
+       /* trigger tracepoint */
+       usleep(1);
 
-       id = (__u64) tid << 32 | pid;

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

Quoted 10 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +6 -3

-       bss.user_pid_tgid = id;
+       CHECK(bss->user_pid_tgid != id, "pid/tgid", "got %llu != exp %llu\n",
+        bss->user_pid_tgid, id);
+cleanup:
+        test_ns_current_pid_tgid__destroy(skel);
+}
 
-       if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
-               perror("Failed to stat /proc/self/ns/pid");
+static int newns_pidtgid(void *arg)

nit: pid_tgid?

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

Quoted 11 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +11 -0

+{
+       struct test_ns_current_pid_tgid__bss  *bss;
+       int pidns_fd = 0, err = 0, duration = 0;
+       struct test_ns_current_pid_tgid *skel;
+       pid_t pid, tid;
+       struct stat st;
+       __u64 id;
+
+       skel = test_ns_current_pid_tgid__open_and_load();
+       if (!skel) {
+               perror("Failed to load skeleton");

please use CHECK() or ASSERT_OK_PTR() instead of perror()

Quoted 9 lines

                goto cleanup;
        }

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +3 -2

-       bss.dev = st.st_dev;
-       bss.ino = st.st_ino;
+       tid = syscall(SYS_gettid);
+       pid = getpid();
+       id = ((__u64) tid << 32) | pid;

[...]

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

Quoted 9 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +9 -0

+
+static void test_ns_current_pid_tgid_new_ns(void)
+{
+       int wstatus, duration = 0;
+       pid_t cpid;
+
+       cpid = clone(newns_pidtgid,
+         child_stack + STACK_SIZE,
+         CLONE_NEWPID | SIGCHLD, NULL);

formatting here and below for wrapped arguments looks wrong. Please double-check whitespaces and align arguments. There is also scripts/checkpatch.pl -f <path-to-file> which will help.

Quoted 25 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +19 -0

+
+       if (CHECK(cpid == -1, "clone", strerror(errno)))
+               exit(EXIT_FAILURE);
+
+       if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
+        strerror(errno))) {
+               exit(EXIT_FAILURE);
+       }
+
+       CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
+        "failed");
+}
+
+void test_ns_current_pid_tgid(void)
+{
+       if (test__start_subtest("ns_current_pid_tgid_global_ns"))
+               test_ns_current_pid_tgid_global_ns();
+       if (test__start_subtest("ns_current_pid_tgid_new_ns"))
+               test_ns_current_pid_tgid_new_ns();
 }

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +0 -0

index 1dca70a6de2f..0daa12db0d83 100644

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c line 8

Quoted 10 lines

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +1 -6

@@ -5,31 +5,19 @@
 #include <stdint.h>
 #include <bpf/bpf_helpers.h>
 
-static volatile struct {
-       __u64 dev;
-       __u64 ino;
-       __u64 pid_tgid;
-       __u64 user_pid_tgid;
-} res;
+__u64 user_pid_tgid = 0;

imo, no need to combine pid and tgid into a single u64, why not using two separate global variables and keep it simple?

Quoted 5 lines

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +2 -1

+__u64 dev = 0;
+__u64 ino = 0;
 
 SEC("raw_tracepoint/sys_enter")
-int trace(void *ctx)

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c line 18

Quoted 10 lines

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +3 -4

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

here as well, wrapped argument looks misaligned (unless it's my email client)

Quoted 5 lines

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +0 -4

                return 0;
-
-       ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
-       expected_pid = res.user_pid_tgid;
-

[...]