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@xxxxxxxxx> wrote:
> >
> > folds tests from test_current_pid_tgid_new_ns into test_progs.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@xxxxxxxxx>
> > ---
> > 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
