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