bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

PATCH Superseded Carlos Neira 2 participants 3 messages Dec 21, 2020 – Dec 24, 2020

Subsystems (5)

Networking Network drivers Kselftest BPF +1 more subsystems

lore

Share & tools

lore

Carlos Neira

author

2020-12-21 22:24 UTC #0 lore raw

PATCH v10 1/1 bpf/selftests: fold test_current_pid_tgid_new_ns into test_progs.

Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs. This change folds test cases into test_progs.

Changes from v9:

- Added test in root namespace.
- Fixed changed tracepoint from sys_enter to sys_usleep.
- Fixed pid, tgid values were inverted.
- Used CLONE(2) for namespaced test, the new process pid will be 1.
- Used ASSERTEQ on pid/tgid validation.
- Added comment on CLONE(2) call

Signed-off-by Carlos Neira <[email protected]>

---
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 149 ++++++++++------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  29 ++--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 106 insertions(+), 236 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c

tools/testing/selftests/bpf/.gitignore +0 -1

index f5b7ef93618c..9abca0616ec0 100644
@@ -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

tools/testing/selftests/bpf/Makefile +1 -2

index 8c33e999319a..886577bc2bb6 100644
@@ -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),)

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +95 ~~-54~~

180 lines not on this page · raw message

tools/testing/selftests/bpf/progs/test_ns_current_pid_tgid.c +10 ~~-19~~

index 1dca70a6de2f..3a57825da33e 100644
@@ -5,31 +5,22 @@
 #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 = 0;
+__u64 user_tgid = 0;
+__u64 dev = 0;
+__u64 ino = 0;
 
-SEC("raw_tracepoint/sys_enter")
-int trace(void *ctx)
+SEC("tracepoint/syscalls/sys_enter_nanosleep")
+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 = nsdata.pid;
+	user_tgid = nsdata.tgid;
 
 	return 0;
 }

tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c deleted +0 ~~-160~~

160 lines not on this page · raw message

Signature · 1 line

2.20.1

Andrii Nakryiko

2020-12-22 20:27 UTC #1 lore raw

Quoted 13 lines · Carlos Neira

Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs. This change folds test cases into test_progs.
Changes from v9:
- Added test in root namespace.
- Fixed changed tracepoint from sys_enter to sys_usleep.
- Fixed pid, tgid values were inverted.
- Used CLONE(2) for namespaced test, the new process pid will be 1.
- Used ASSERTEQ on pid/tgid validation.
- Added comment on CLONE(2) call

Quoted 2 lines

Signed-off-by Carlos Neira <[email protected]>
---

See checkpatch.pl errors ([0]), ignore the "do not initialize globals with zero" ones. Next time, please don't wait for me to point out every single instance where you didn't align wrapped around parameters.

[0] https://patchwork.hopto.org/static/nipa/405025/11985347/checkpatch/stdout

Quoted 8 lines

 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 149 ++++++++++------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  29 ++--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 106 insertions(+), 236 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c

[...]

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c line 57

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +5 -2

-       id = (__u64) tid << 32 | pid;
-       bss.user_pid_tgid = id;
+cleanup:
+        test_ns_current_pid_tgid__destroy(skel);
+}
+
+static int newns_pidtgid(void *arg)

newns_pidtgid and test_ns_current_pid_tgid_global_ns look identical to me, am I missing something on why you didn't reuse the testing logic between the two?

Quoted 7 lines

tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +6 -0

+{
+       struct test_ns_current_pid_tgid__bss  *bss;
+       int err = 0, duration = 0;
+       struct test_ns_current_pid_tgid *skel;
+       pid_t pid, tgid;
+       struct stat st;

[...]

Carlos Antonio Neira Bustos

author

2020-12-24 17:40 UTC #2 lore raw

Quoted 14 lines · Andrii Nakryiko

Quoted 13 lines · Carlos Neira
Currently tests for bpf_get_ns_current_pid_tgid() are outside test_progs. This change folds test cases into test_progs.
Changes from v9:
- Added test in root namespace.
- Fixed changed tracepoint from sys_enter to sys_usleep.
- Fixed pid, tgid values were inverted.
- Used CLONE(2) for namespaced test, the new process pid will be 1.
- Used ASSERTEQ on pid/tgid validation.
- Added comment on CLONE(2) call

Quoted 2 lines

Quoted 2 lines
Signed-off-by Carlos Neira <[email protected]>
---

Andrii,

Thanks for taking the time to check this out.

For the code style issues I found out that I was not using the --strict flag for checkpatch.pl so I missed those warnings, now I'm using--strict as default. I should look into using cindent or clang-format to avoid this.

Quoted 16 lines

See checkpatch.pl errors ([0]), ignore the "do not initialize globals with zero" ones. Next time, please don't wait for me to point out every single instance where you didn't align wrapped around parameters.
[0] https://patchwork.hopto.org/static/nipa/405025/11985347/checkpatch/stdout
Quoted 8 lines
 tools/testing/selftests/bpf/.gitignore        |   1 -
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 149 ++++++++++------
 .../bpf/progs/test_ns_current_pid_tgid.c      |  29 ++--
 .../bpf/test_current_pid_tgid_new_ns.c        | 160 ------------------
 5 files changed, 106 insertions(+), 236 deletions(-)
 delete mode 100644 tools/testing/selftests/bpf/test_current_pid_tgid_new_ns.c

You are right, the code is redundant I'll reuse it.

Quoted 13 lines

newns_pidtgid and test_ns_current_pid_tgid_global_ns look identical to me, am I missing something on why you didn't reuse the testing logic between the two?
Quoted 7 lines
tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c +6 -0
+{
+       struct test_ns_current_pid_tgid__bss  *bss;
+       int err = 0, duration = 0;
+       struct test_ns_current_pid_tgid *skel;
+       pid_t pid, tgid;
+       struct stat st;
[...]