* [PATCH v8 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.
@ 2020-12-14 17:49 Carlos Neira
2020-12-15 19:05 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Carlos Neira @ 2020-12-14 17:49 UTC (permalink / raw)
To: netdev; +Cc: andriin, yhs, ebiederm, brouer, bpf, cneirabustos
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@gmail.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
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
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index f5b7ef93618c..9abca0616ec0 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -26,7 +26,6 @@ test_tcpnotify_user
test_libbpf
test_tcp_check_syncookie_user
test_sysctl
-test_current_pid_tgid_new_ns
xdping
test_cpp
*.skel.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 8c33e999319a..886577bc2bb6 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -35,8 +35,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_sock test_sockmap get_cgroup_id_user test_socket_cookie \
test_cgroup_storage \
test_netcnt test_tcpnotify_user test_sysctl \
- test_progs-no_alu32 \
- test_current_pid_tgid_new_ns
+ test_progs-no_alu32
# Also test bpf-gcc, if present
ifneq ($(BPF_GCC),)
diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
index e74dc501b27f..c838c77ced26 100644
--- a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
+++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
@@ -1,85 +1,131 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2020 Carlos Neira cneirabustos@gmail.com */
+
+#define _GNU_SOURCE
#include <test_progs.h>
+#include "test_ns_current_pid_tgid.skel.h"
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/syscall.h>
+#include <sched.h>
+#include <sys/wait.h>
+#include <sys/mount.h>
+#include <sys/fcntl.h>
-struct bss {
- __u64 dev;
- __u64 ino;
- __u64 pid_tgid;
- __u64 user_pid_tgid;
-};
+#define STACK_SIZE (1024 * 1024)
+static char child_stack[STACK_SIZE];
-void test_ns_current_pid_tgid(void)
+static void test_ns_current_pid_tgid_global_ns(void)
{
- 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, duration = 0;
struct stat st;
+ pid_t tid, pid;
__u64 id;
- 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;
- 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)
+{
+ 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");
goto cleanup;
}
- bss.dev = st.st_dev;
- bss.ino = st.st_ino;
+ tid = syscall(SYS_gettid);
+ pid = getpid();
+ id = ((__u64) tid << 32) | pid;
- err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
- if (CHECK(err, "setting_bss", "failed to set bss : %d\n", err))
+ err = stat("/proc/self/ns/pid", &st);
+ if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d", err))
goto cleanup;
- link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
- if (CHECK(IS_ERR(link), "attach_raw_tp", "err %ld\n",
- PTR_ERR(link))) {
- link = NULL;
+ 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;
- }
- /* trigger some syscalls */
+ /* trigger tracepoint */
usleep(1);
- 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;
+ CHECK(bss->user_pid_tgid != id, "pid/tgid", "got %llu != exp %llu\n",
+ bss->user_pid_tgid, id);
- 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;
cleanup:
- bpf_link__destroy(link);
- bpf_object__close(obj);
+ setns(pidns_fd, CLONE_NEWPID);
+ test_ns_current_pid_tgid__destroy(skel);
+
+ return err;
+}
+
+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);
+
+ 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;
+__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)))
return 0;
-
- ns_pid_tgid = (__u64)nsdata.tgid << 32 | nsdata.pid;
- expected_pid = res.user_pid_tgid;
-
- if (expected_pid != ns_pid_tgid)
- return 0;
-
- res.pid_tgid = ns_pid_tgid;
+ user_pid_tgid = ((__u64)nsdata.tgid << 32) | nsdata.pid;
return 0;
}
diff --git a/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c b/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
deleted file mode 100644
index ec53b1ef90d2..000000000000
--- a/tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c
+++ /dev/null
@@ -1,160 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright (c) 2020 Carlos Neira cneirabustos@gmail.com */
-#define _GNU_SOURCE
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
-#include <sys/syscall.h>
-#include <sched.h>
-#include <sys/wait.h>
-#include <sys/mount.h>
-#include "test_progs.h"
-
-#define CHECK_NEWNS(condition, tag, format...) ({ \
- int __ret = !!(condition); \
- if (__ret) { \
- printf("%s:FAIL:%s ", __func__, tag); \
- printf(format); \
- } else { \
- printf("%s:PASS:%s\n", __func__, tag); \
- } \
- __ret; \
-})
-
-struct bss {
- __u64 dev;
- __u64 ino;
- __u64 pid_tgid;
- __u64 user_pid_tgid;
-};
-
-int main(int argc, char **argv)
-{
- pid_t pid;
- int exit_code = 1;
- struct stat st;
-
- printf("Testing bpf_get_ns_current_pid_tgid helper in new ns\n");
-
- if (stat("/proc/self/ns/pid", &st)) {
- perror("stat failed on /proc/self/ns/pid ns\n");
- printf("%s:FAILED\n", argv[0]);
- return exit_code;
- }
-
- if (CHECK_NEWNS(unshare(CLONE_NEWPID | CLONE_NEWNS),
- "unshare CLONE_NEWPID | CLONE_NEWNS", "error errno=%d\n", errno))
- return exit_code;
-
- pid = fork();
- if (pid == -1) {
- perror("Fork() failed\n");
- printf("%s:FAILED\n", argv[0]);
- return exit_code;
- }
-
- if (pid > 0) {
- int status;
-
- usleep(5);
- waitpid(pid, &status, 0);
- return 0;
- } else {
-
- pid = fork();
- if (pid == -1) {
- perror("Fork() failed\n");
- printf("%s:FAILED\n", argv[0]);
- return exit_code;
- }
-
- if (pid > 0) {
- int status;
- waitpid(pid, &status, 0);
- return 0;
- } else {
- if (CHECK_NEWNS(mount("none", "/proc", NULL, MS_PRIVATE|MS_REC, NULL),
- "Unmounting proc", "Cannot umount proc! errno=%d\n", errno))
- return exit_code;
-
- if (CHECK_NEWNS(mount("proc", "/proc", "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL),
- "Mounting proc", "Cannot mount proc! errno=%d\n", errno))
- return exit_code;
-
- const char *probe_name = "raw_tracepoint/sys_enter";
- const char *file = "test_ns_current_pid_tgid.o";
- struct bpf_link *link = NULL;
- struct bpf_program *prog;
- struct bpf_map *bss_map;
- struct bpf_object *obj;
- int exit_code = 1;
- int err, key = 0;
- struct bss bss;
- struct stat st;
- __u64 id;
-
- obj = bpf_object__open_file(file, NULL);
- if (CHECK_NEWNS(IS_ERR(obj), "obj_open", "err %ld\n", PTR_ERR(obj)))
- return exit_code;
-
- err = bpf_object__load(obj);
- if (CHECK_NEWNS(err, "obj_load", "err %d errno %d\n", err, errno))
- goto cleanup;
-
- bss_map = bpf_object__find_map_by_name(obj, "test_ns_.bss");
- if (CHECK_NEWNS(!bss_map, "find_bss_map", "failed\n"))
- goto cleanup;
-
- prog = bpf_object__find_program_by_title(obj, probe_name);
- if (CHECK_NEWNS(!prog, "find_prog", "prog '%s' not found\n",
- probe_name))
- goto cleanup;
-
- memset(&bss, 0, sizeof(bss));
- pid_t tid = syscall(SYS_gettid);
- pid_t pid = getpid();
-
- id = (__u64) tid << 32 | pid;
- bss.user_pid_tgid = id;
-
- if (CHECK_NEWNS(stat("/proc/self/ns/pid", &st),
- "stat new ns", "Failed to stat /proc/self/ns/pid errno=%d\n", errno))
- goto cleanup;
-
- bss.dev = st.st_dev;
- bss.ino = st.st_ino;
-
- err = bpf_map_update_elem(bpf_map__fd(bss_map), &key, &bss, 0);
- if (CHECK_NEWNS(err, "setting_bss", "failed to set bss : %d\n", err))
- goto cleanup;
-
- link = bpf_program__attach_raw_tracepoint(prog, "sys_enter");
- if (CHECK_NEWNS(IS_ERR(link), "attach_raw_tp", "err %ld\n",
- PTR_ERR(link))) {
- link = NULL;
- goto cleanup;
- }
-
- /* trigger some syscalls */
- usleep(1);
-
- err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &bss);
- if (CHECK_NEWNS(err, "set_bss", "failed to get bss : %d\n", err))
- goto cleanup;
-
- if (CHECK_NEWNS(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;
-
- exit_code = 0;
- printf("%s:PASS\n", argv[0]);
-cleanup:
- if (!link) {
- bpf_link__destroy(link);
- link = NULL;
- }
- bpf_object__close(obj);
- }
- }
- return 0;
-}
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v8 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.
2020-12-14 17:49 [PATCH v8 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs Carlos Neira
@ 2020-12-15 19:05 ` Andrii Nakryiko
2020-12-15 20:42 ` Carlos Antonio Neira Bustos
0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2020-12-15 19:05 UTC (permalink / raw)
To: Carlos Neira
Cc: Networking, Andrii Nakryiko, Yonghong Song, Eric W. Biederman,
Jesper Dangaard Brouer, bpf
On Mon, Dec 14, 2020 at 10:39 AM Carlos Neira <cneirabustos@gmail.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 V7:
> - Rebased changes.
> - Changed function scope.
>
> Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
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;
> -
[...]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v8 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.
2020-12-15 19:05 ` Andrii Nakryiko
@ 2020-12-15 20:42 ` Carlos Antonio Neira Bustos
0 siblings, 0 replies; 3+ messages in thread
From: Carlos Antonio Neira Bustos @ 2020-12-15 20:42 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, Andrii Nakryiko, Yonghong Song, Eric W. Biederman,
Jesper Dangaard Brouer, bpf
Andrii,
Thank you very much for checking this out!, you were right the test was
incorrect. I'll work on your feedback and resend.
Thanks!
On Tue, Dec 15, 2020 at 11:05:50AM -0800, Andrii Nakryiko wrote:
> On Mon, Dec 14, 2020 at 10:39 AM Carlos Neira <cneirabustos@gmail.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 V7:
> > - Rebased changes.
> > - Changed function scope.
> >
> > Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> 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;
> > -
>
> [...]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-15 20:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-14 17:49 [PATCH v8 bpf-next] bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs Carlos Neira
2020-12-15 19:05 ` Andrii Nakryiko
2020-12-15 20:42 ` Carlos Antonio Neira Bustos
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.