netdev - Re: [PATCH v11 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 4 Jan 2021 13:00:36 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Carlos Neira <cneirabustos@...il.com>
Cc:     Networking <netdev@...r.kernel.org>,
        Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v11 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns
 into test_progs.

On Mon, Dec 28, 2020 at 7:48 AM Carlos Neira <cneirabustos@...il.com> wrote:
>
> Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
> This change folds test cases into test_progs.
>
> Changes from v10:
>
>  - Code style fixes.
>  - Remove redundant code.
>
> Signed-off-by: Carlos Neira <cneirabustos@...il.com>
> ---
>  tools/testing/selftests/bpf/.gitignore        |   1 -
>  tools/testing/selftests/bpf/Makefile          |   3 +-
>  .../bpf/prog_tests/ns_current_pid_tgid.c      | 118 ++++++-------
>  .../bpf/progs/test_ns_current_pid_tgid.c      |  28 +--
>  .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
>  5 files changed, 69 insertions(+), 241 deletions(-)
>  delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
>

[...]

> -void test_ns_current_pid_tgid(void)
> +static int test_current_pid_tgid(void *args)
>  {
> -       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 test_ns_current_pid_tgid__bss  *bss;
> +       struct test_ns_current_pid_tgid *skel;
> +       int err = 0, duration = 0;
> +       pid_t tgid, pid;
>         struct stat st;
> -       __u64 id;
> -
> -       obj = bpf_object__open_file(file, NULL);
> -       if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
> -               return;
>
> -       err = bpf_object__load(obj);
> -       if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
> +       skel = test_ns_current_pid_tgid__open_and_load();
> +       if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
>                 goto cleanup;

err will be 0 here, and test failure won't be detected (if it happens
in a child process), right? please check all the other cases. It might
be better to assign err = -1 at the beginning and just assign err = 0
after all tests succeeded, right before the clean up.

>
> -       bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
> -       if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> -               goto cleanup;
> +       pid = syscall(SYS_gettid);
> +       tgid = getpid();
>

[...]

> -cleanup:
> -       bpf_link__destroy(link);
> -       bpf_object__close(obj);
> +       if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
> +               exit(EXIT_FAILURE);
> +
> +       if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid", "failed"))
> +               exit(EXIT_FAILURE);

this exit will happen in the parent process, which is test_progs test
runner itself, right? Which will prematurely stop all of the remaining
tests to run. I.e., you can't just exit(), you need to just return and
let test_progs keep running.

> +}
> +
> +void test_ns_current_pid_tgid(void)
> +{
> +       if (test__start_subtest("ns_current_pid_tgid_root_ns"))
> +               test_current_pid_tgid(NULL);
> +       if (test__start_subtest("ns_current_pid_tgid_new_ns"))
> +               test_ns_current_pid_tgid_new_ns();
>  }

[...]