Date:   Wed, 16 Dec 2020 16:08:47 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Carlos Neira <cneirabustos@...il.com>
Cc:     Networking <netdev@...r.kernel.org>,
        Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v9 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns
 into test_progs.

On Wed, Dec 16, 2020 at 6:18 AM Carlos Neira <cneirabustos@...il.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@...il.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);
>  }

[...]

Powered by blists - more mailing lists