netdev - Re: [PATCH v2 bpf-next] fold test_current_pid_tgid_new_ns into 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzYSKXE2aYkbE2XKa9z1Wc8Zv9-bkTmh=8unOM+Za-6uMw@mail.gmail.com>
Date:   Tue, 23 Jun 2020 11:10:51 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Carlos Neira <cneirabustos@...il.com>
Cc:     Networking <netdev@...r.kernel.org>, Yonghong Song <yhs@...com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH v2 bpf-next] fold test_current_pid_tgid_new_ns into into test_progs

On Tue, Jun 23, 2020 at 5:48 AM Carlos Neira <cneirabustos@...il.com> wrote:
>
> folds tests from test_current_pid_tgid_new_ns into test_progs.
>
> Signed-off-by: Carlos Neira <cneirabustos@...il.com>
> ---
>  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();
> +}

[...]