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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sun, Aug 2, 2020 at 2:36 PM Carlos Neira <cneirabustos@gmail.com> 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@gmail.com>
> ---
>  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
>