On Mon, Dec 14, 2020 at 10:39 AM Carlos Neira <cneirabustos@xxxxxxxxx> wrote:
>
> Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs.
> This change folds test cases into test_progs.
>
> Changes from V7:
> - Rebased changes.
> - Changed function scope.
>
> Signed-off-by: Carlos Neira <cneirabustos@xxxxxxxxx>
> Acked-by: Andrii Nakryiko <andriin@xxxxxx>
please drop my ack for the next version given the changes requested, thanks!
> ---
> tools/testing/selftests/bpf/.gitignore | 1 -
> tools/testing/selftests/bpf/Makefile | 3 +-
> .../bpf/prog_tests/ns_current_pid_tgid.c | 148 ++++++++++------
> .../bpf/progs/test_ns_current_pid_tgid.c | 26 +--
> .../bpf/test_current_pid_tgid_new_ns.c | 160 ------------------
> 5 files changed, 105 insertions(+), 233 deletions(-)
> delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
>
[...]
>
> - obj = bpf_object__open_file(file, NULL);
> - if (CHECK(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
> - return;
> + skel = test_ns_current_pid_tgid__open_and_load();
> + CHECK(!skel, "skel_open_load", "failed to load skeleton\n");
> + goto cleanup;
Are you sure your test is doing what you think it's doing? Try running
`sudo ./test_progs -t ns_current_pid_tgid -v` and see if you get all
the CHECK()s you expect.
It's a long way of saying that you are missing `if ()` and just
unconditionally clean up after opening and loading the skeleton.
And if the skeleton is not open_and_load()'ed successfully, there is
nothing to clean up, btw.
>
> - err = bpf_object__load(obj);
> - if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
> - goto cleanup;
> + tid = syscall(SYS_gettid);
> + pid = getpid();
> +
> + id = ((__u64)tid << 32) | pid;
>
> - bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
> - if (CHECK(!bss_map, "find_bss_map", "failed\n"))
> + err = stat("/proc/self/ns/pid", &st);
> + if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
> goto cleanup;
>
> - prog = bpf_object__find_program_by_title(obj, probe_name);
> - if (CHECK(!prog, "find_prog", "prog '%s' not found\n",
> - probe_name))
> + bss = skel->bss;
> + bss->dev = st.st_dev;
> + bss->ino = st.st_ino;
> + bss->user_pid_tgid = 0;
> +
> + err = test_ns_current_pid_tgid__attach(skel);
> + if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> goto cleanup;
>
> - memset(&bss, 0, sizeof(bss));
> - pid_t tid = syscall(SYS_gettid);
> - pid_t pid = getpid();
> + /* trigger tracepoint */
> + usleep(1);
>
> - id = (__u64) tid << 32 | pid;
> - bss.user_pid_tgid = id;
> + CHECK(bss->user_pid_tgid != id, "pid/tgid", "got %llu != exp %llu\n",
> + bss->user_pid_tgid, id);
> +cleanup:
> + test_ns_current_pid_tgid__destroy(skel);
> +}
>
> - if (CHECK_FAIL(stat("/proc/self/ns/pid", &st))) {
> - perror("Failed to stat /proc/self/ns/pid");
> +static int newns_pidtgid(void *arg)
nit: pid_tgid?
> +{
> + struct test_ns_current_pid_tgid__bss *bss;
> + int pidns_fd = 0, err = 0, duration = 0;
> + struct test_ns_current_pid_tgid *skel;
> + pid_t pid, tid;
> + struct stat st;
> + __u64 id;
> +
> + skel = test_ns_current_pid_tgid__open_and_load();
> + if (!skel) {
> + perror("Failed to load skeleton");
please use CHECK() or ASSERT_OK_PTR() instead of perror()
> goto cleanup;
> }
>
> - bss.dev = st.st_dev;
> - bss.ino = st.st_ino;
> + tid = syscall(SYS_gettid);
> + pid = getpid();
> + id = ((__u64) tid << 32) | pid;
>
[...]
> +
> +static void test_ns_current_pid_tgid_new_ns(void)
> +{
> + int wstatus, duration = 0;
> + pid_t cpid;
> +
> + cpid = clone(newns_pidtgid,
> + child_stack + STACK_SIZE,
> + CLONE_NEWPID | SIGCHLD, NULL);
formatting here and below for wrapped arguments looks wrong. Please
double-check whitespaces and align arguments. There is also
`scripts/checkpatch.pl -f <path-to-file>` which will help.
> +
> + if (CHECK(cpid == -1, "clone", strerror(errno)))
> + exit(EXIT_FAILURE);
> +
> + if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid",
> + strerror(errno))) {
> + exit(EXIT_FAILURE);
> + }
> +
> + CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid",
> + "failed");
> +}
> +
> +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();
> }
> diff --git a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> index 1dca70a6de2f..0daa12db0d83 100644
> --- a/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> +++ b/tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c
> @@ -5,31 +5,19 @@
> #include <stdint.h>
> #include <bpf/bpf_helpers.h>
>
> -static volatile struct {
> - __u64 dev;
> - __u64 ino;
> - __u64 pid_tgid;
> - __u64 user_pid_tgid;
> -} res;
> +__u64 user_pid_tgid = 0;
imo, no need to combine pid and tgid into a single u64, why not using
two separate global variables and keep it simple?
> +__u64 dev = 0;
> +__u64 ino = 0;
>
> SEC("raw_tracepoint/sys_enter")
> -int trace(void *ctx)
> +int handler(const void *ctx)
> {
> - __u64 ns_pid_tgid, expected_pid;
> struct bpf_pidns_info nsdata;
> - __u32 key = 0;
>
> - if (bpf_get_ns_current_pid_tgid(res.dev, res.ino, &nsdata,
> - sizeof(struct bpf_pidns_info)))
> + if (bpf_get_ns_current_pid_tgid(dev, ino, &nsdata,
> + sizeof(struct bpf_pidns_info)))
here as well, wrapped argument looks misaligned (unless it's my email client)
> return 0;
> -
> - ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
> - expected_pid = res.user_pid_tgid;
> -
[...]
