On Sun, Aug 2, 2020 at 2:36 PM Carlos Neira <cneirabustos@xxxxxxxxx> wrote:
>
> Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
> This change folds a test case into test_progs.
>
> Changes from V2:
> - Tests are now using skeleton.
> - Test not creating a new namespace has been included in test_progs.
> - Test creating a new pid namespace has been added as a standalone test.
>
> Signed-off-by: Carlos Neira <cneirabustos@xxxxxxxxx>
> ---
> tools/testing/selftests/bpf/.gitignore | 2 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> .../bpf/prog_tests/ns_current_pid_tgid.c | 85 -----------------
> .../bpf/prog_tests/ns_current_pidtgid.c | 59 ++++++++++++
> .../bpf/progs/test_ns_current_pid_tgid.c | 37 --------
> .../bpf/progs/test_ns_current_pidtgid.c | 25 +++++
> ...new_ns.c => test_current_pidtgid_new_ns.c} | 0
> .../bpf/test_ns_current_pidtgid_newns.c | 91 +++++++++++++++++++
> 8 files changed, 178 insertions(+), 124 deletions(-)
> delete mode 100644 tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> create mode 100644 tools/testing/selftests/bpf/prog_tests/ns_current_pidtgid.c
> delete mode 100644 tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_ns_current_pidtgid.c
> rename tools/testing/selftests/bpf/{test_current_pid_tgid_new_ns.c => test_current_pidtgid_new_ns.c} (100%)
> create mode 100644 tools/testing/selftests/bpf/test_ns_current_pidtgid_newns.c
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 1bb204cee853..022055f23592 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -30,8 +30,8 @@ test_tcpnotify_user
> test_libbpf
> test_tcp_check_syncookie_user
> test_sysctl
> -test_current_pid_tgid_new_ns
> xdping
> +test_ns_current_pidtgid_newns
> test_cpp
> *.skel.h
> /no_alu32
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e7a8cf83ba48..c1ba9c947196 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -37,7 +37,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> test_cgroup_storage \
> test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> test_progs-no_alu32 \
> - test_current_pid_tgid_new_ns
> + test_ns_current_pidtgid_newns
>
> # Also test bpf-gcc, if present
> ifneq ($(BPF_GCC),)
> @@ -163,6 +163,7 @@ $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> $(OUTPUT)/test_netcnt: cgroup_helpers.c
> $(OUTPUT)/test_sock_fields: cgroup_helpers.c
> $(OUTPUT)/test_sysctl: cgroup_helpers.c
> +$(OUTPUT)/test_ns_current_pidtgid_newns: test_ns_current_pidtgid_newns.c
This dependency is implicit and not necessary. What you see above is
*additional* .c files that tests need.
>
> DEFAULT_BPFTOOL := $(SCRATCH_DIR)/sbin/bpftool
> BPFTOOL ?= $(DEFAULT_BPFTOOL)
[...]
> +void test_ns_current_pidtgid(void)
> +{
> + int err, duration = 0;
> + struct test_ns_current_pidtgid *skel;
> + struct test_ns_current_pidtgid__bss *bss;
> + struct stat st;
> + __u64 id;
> +
> + skel = test_ns_current_pidtgid__open();
> + if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> + return;
> +
> + err = test_ns_current_pidtgid__load(skel);
> + if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
> + goto cleanup;
nit: could be combined into a single test_ns_current_pidtgid__open_and_load()
> +
> + pid_t tid = syscall(SYS_gettid);
> + pid_t pid = getpid();
> +
> + id = (__u64) tid << 32 | pid;
> +
> + if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
please use CHECK instead of CHECK_FAIL
> + perror("Failed to stat /proc/self/ns/pid");
> + goto cleanup;
> + }
> +
> + bss = skel->bss;
> + bss->dev = st.st_dev;
> + bss->ino = st.st_ino;
> + bss->user_pid_tgid = 0;
> +
[...]
> +static int newns_pidtgid(void *arg)
> +{
> + struct test_ns_current_pidtgid__bss *bss;
> + struct test_ns_current_pidtgid *skel;
> + int pidns_fd = 0, err = 0;
> + pid_t pid, tid;
> + struct stat st;
> + __u64 id;
> +
> + skel = test_ns_current_pidtgid__open();
> + err = test_ns_current_pidtgid__load(skel);
if open() fails, NULL pointer dereference happens here. But also
better use open_and_load() variant.
> + if (err) {
> + perror("Failed to load skeleton");
> + goto cleanup;
> + }
> +
> + tid = syscall(SYS_gettid);
> + pid = getpid();
> + id = (__u64) tid << 32 | pid;
> +
> + if (stat("/proc/self/ns/pid", &st)) {
> + printf("Failed to stat /proc/self/ns/pid: %s\n",
> + strerror(errno));
> + goto cleanup;
> + }
> +
> + bss = skel->bss;
> + bss->dev = st.st_dev;
> + bss->ino = st.st_ino;
> + bss->user_pid_tgid = 0;
> +
> + err = test_ns_current_pidtgid__attach(skel);
> + if (err) {
> + printf("Failed to attach: %s err: %d\n", strerror(errno), err);
> + goto cleanup;
> + }
> + /* trigger tracepoint */
> + usleep(1);
> +
> + if (bss->user_pid_tgid != id) {
> + printf("test_ns_current_pidtgid_newns:FAIL\n");
> + err = EXIT_FAILURE;
> + } else {
> + printf("test_ns_current_pidtgid_newns:PASS\n");
> + err = EXIT_SUCCESS;
> + }
> +
> +cleanup:
> + setns(pidns_fd, CLONE_NEWPID);
> + test_ns_current_pidtgid__destroy(skel);
> +
> + return 0;
you don't return err from above
> +}
> +
> +int main(int argc, char **argv)
> +{
> + pid_t cpid;
> + int wstatus;
> +
> + cpid = clone(newns_pidtgid,
> + child_stack + STACK_SIZE,
> + CLONE_NEWPID | SIGCHLD, NULL);
> + if (cpid == -1) {
> + printf("test_ns_current_pidtgid_newns:Failed on CLONE: %s\n",
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + if (waitpid(cpid, &wstatus, 0) == -1) {
> + printf("test_ns_current_pidtgid_newns:Failed on waitpid: %s\n",
> + strerror(errno));
exit(EXIT_FAILURE) here?
> + }
> + return (WEXITSTATUS(wstatus));
nit: unnecessary ()
> +}
> --
> 2.20.1
>
