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

[Carlos Antonio Neira Bustos <cneirabustos@gmail.com>]

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 <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 V7:
> >  - Rebased changes.
> >  - Changed function scope.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
> please drop my ack for the next version given the changes requested, thanks!
> 
> > ---
> >  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
> >
> 
> [...]
> 
> >
> > -       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.
> 
> >
> > -       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?
> 
> > +{
> > +       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()
> 
> >                 goto cleanup;
> >         }
> >
> > -       bss.dev = st.st_dev;
> > -       bss.ino = st.st_ino;
> > +       tid = syscall(SYS_gettid);
> > +       pid = getpid();
> > +       id = ((__u64) tid << 32) | pid;
> >
> 
> [...]
> 
> > +
> > +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.
> 
> > +
> > +       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?
> 
> > +__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)
> 
> >                 return 0;
> > -
> > -       ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
> > -       expected_pid = res.user_pid_tgid;
> > -
> 
> [...]