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

Andrii,

Thank you very much for checking this out!, you were right the test was
incorrect. I'll work on your feedback and resend.
Thanks!

On Tue, Dec 15, 2020 at 11:05:50AM -0800, Andrii Nakryiko wrote:

On Mon, Dec 14, 2020 at 10:39 AM Carlos Neira [off-list ref] wrote:

quoted

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 <redacted>
Acked-by: Andrii Nakryiko <redacted>

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

quoted

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

[...]

quoted

-       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

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

quoted

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

                goto cleanup;
        }

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

[...]

quoted

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

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

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..0daa12db0d83 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,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

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

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

quoted

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

[...]