Re: [PATCH v2 bpf-next] fold test_current_pid_tgid_new_ns into into test_progs

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

On Tue, Jun 23, 2020 at 11:10:51AM -0700, Andrii Nakryiko wrote:
> On Tue, Jun 23, 2020 at 5:48 AM Carlos Neira <cneirabustos@gmail.com> wrote:
> >
> > folds tests from test_current_pid_tgid_new_ns into test_progs.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> > ---
> >  tools/testing/selftests/bpf/Makefile          |   3 +-
> >  .../bpf/prog_tests/ns_current_pid_tgid.c      | 112 +++++++++++-
> >  .../bpf/test_current_pid_tgid_new_ns.c        | 159 ------------------
> >  3 files changed, 112 insertions(+), 162 deletions(-)
> >  delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 22aaec74ea0a..7b2ea7adccb0 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -36,8 +36,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> >         test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
> >         test_cgroup_storage \
> >         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> > -       test_progs-no_alu32 \
> > -       test_current_pid_tgid_new_ns
> > +       test_progs-no_alu32
> 
> Please update .gitignore as well.
> 
> >
> >  # Also test bpf-gcc, if present
> >  ifneq ($(BPF_GCC),)
> 
> [...]
> 
> > +
> > +       snprintf(nspath, sizeof(nspath) - 1, "/proc/%d/ns/pid", ppid);
> > +       pidns_fd = open(nspath, O_RDONLY);
> > +
> > +       if (CHECK(unshare(CLONE_NEWPID),
> > +               "unshare CLONE_NEWPID",
> > +               "error: %s\n", strerror(errno)))
> > +               return;
> > +
> > +       pid = vfork();
> 
> is vfork necessary()? Maybe just stick to fork(), as in original implementation?
> 
> > +       if (CHECK(pid < 0, "ns_current_pid_tgid_new_ns", "vfork error: %s\n",
> > +           strerror(errno))) {
> > +               return;
> > +       }
> > +       if (pid > 0) {
> > +       printf("waiting pid is %u\n", pid);
> 
> indentation off?
> 
> > +               usleep(5);
> > +               wait(NULL);
> 
> waitpid() for specific child would be more reliable, no?
> 
> > +               return;
> > +       } else {
> 
> what if fork failed?
> 
> > +               const char *probe_name = "raw_tracepoint/sys_enter";
> > +               const char *file = "test_ns_current_pid_tgid.o";
> > +               int err, key = 0, duration = 0;
> > +               struct bpf_link *link = NULL;
> > +               struct bpf_program *prog;
> > +               struct bpf_map *bss_map;
> > +               struct bpf_object *obj;
> > +               struct bss bss;
> > +               struct stat st;
> > +               __u64 id;
> > +
> 
> [...]
> 
> > +               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;
> > +
> > +               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;
> 
> 
> Good half of all this code could be removed if you used BPF skeleton,
> see other tests utilizing *.skel.h for inspiration.
> 
> > +cleanup:
> > +               setns(pidns_fd, CLONE_NEWPID);
> > +               bpf_link__destroy(link);
> > +               bpf_object__close(obj);
> > +       }
> > +}
> > +
> > +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();
> > +}
> 
> [...]
Thanks!.
I'll include your feedback on the next version, thanks for reviewing this.

Bests