[PATCH bpf-next v2 0/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types

[PATCH bpf-next v2 0/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types

[Yonghong Song]

Currently bpf_get_current_pid_tgid() is allowed in tracing, cgroup
and sk_msg progs while bpf_get_ns_current_pid_tgid() is only allowed
in tracing progs.

We have an internal use case where for an application running
in a container (with pid namespace), user wants to get
the pid associated with the pid namespace in a cgroup bpf
program. Besides cgroup, the only prog type, supporting
bpf_get_current_pid_tgid() but not bpf_get_ns_current_pid_tgid(),
is sk_msg.

But actually both bpf_get_current_pid_tgid() and
bpf_get_ns_current_pid_tgid() helpers do not reveal kernel internal
data and there is no reason that they cannot be used in other
program types. This patch just did this and enabled these
two helpers for all program types.

Patch 1 added the kernel support and patches 2-5 added
the test for cgroup and sk_msg.

Change logs:
  v1 -> v2:
    - allow bpf_get_[ns_]current_pid_tgid() for all prog types.
    - for network related selftests, using netns.

Yonghong Song (5):
  bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types
  selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test
  selftests/bpf: Refactor out some functions in ns_current_pid_tgid test
  selftests/bpf: Add a cgroup prog bpf_get_ns_current_pid_tgid() test
  selftests/bpf: Add a sk_msg prog bpf_get_ns_current_pid_tgid() test

 kernel/bpf/cgroup.c                           |   2 -
 kernel/bpf/helpers.c                          |   4 +
 kernel/trace/bpf_trace.c                      |   4 -
 net/core/filter.c                             |   2 -
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 214 +++++++++++++++---
 .../bpf/progs/test_ns_current_pid_tgid.c      |  31 ++-
 6 files changed, 215 insertions(+), 42 deletions(-)

-- 
2.43.0

[PATCH bpf-next v2 1/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types

[Yonghong Song]

Currently bpf_get_current_pid_tgid() is allowed in tracing, cgroup
and sk_msg progs while bpf_get_ns_current_pid_tgid() is only allowed
in tracing progs.

We have an internal use case where for an application running
in a container (with pid namespace), user wants to get
the pid associated with the pid namespace in a cgroup bpf
program. Currently, cgroup bpf progs already allow
bpf_get_current_pid_tgid(). Let us allow bpf_get_ns_current_pid_tgid()
as well.

With auditing the code, bpf_get_current_pid_tgid() is also used
by sk_msg prog. But there are no side effect to expose these two
helpers to all prog types since they do not reveal any kernel specific
data. The detailed discussion is in [1].

So with this patch, both bpf_get_current_pid_tgid() and bpf_get_ns_current_pid_tgid()
are put in bpf_base_func_proto(), making them available to all
program types.

  [1] https://lore.kernel.org/bpf/20240307232659.1115872-1-yonghong.song@linux.dev/

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/cgroup.c      | 2 --
 kernel/bpf/helpers.c     | 4 ++++
 kernel/trace/bpf_trace.c | 4 ----
 net/core/filter.c        | 2 --
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 82243cb6c54d..8ba73042a239 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2575,8 +2575,6 @@ cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	switch (func_id) {
 	case BPF_FUNC_get_current_uid_gid:
 		return &bpf_get_current_uid_gid_proto;
-	case BPF_FUNC_get_current_pid_tgid:
-		return &bpf_get_current_pid_tgid_proto;
 	case BPF_FUNC_get_current_comm:
 		return &bpf_get_current_comm_proto;
 #ifdef CONFIG_CGROUP_NET_CLASSID
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index a89587859571..9234174ccb21 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1730,6 +1730,10 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_strtol_proto;
 	case BPF_FUNC_strtoul:
 		return &bpf_strtoul_proto;
+	case BPF_FUNC_get_current_pid_tgid:
+		return &bpf_get_current_pid_tgid_proto;
+	case BPF_FUNC_get_ns_current_pid_tgid:
+		return &bpf_get_ns_current_pid_tgid_proto;
 	default:
 		break;
 	}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 0a5c4efc73c3..1b041911b1d8 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1525,8 +1525,6 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_ktime_get_boot_ns_proto;
 	case BPF_FUNC_tail_call:
 		return &bpf_tail_call_proto;
-	case BPF_FUNC_get_current_pid_tgid:
-		return &bpf_get_current_pid_tgid_proto;
 	case BPF_FUNC_get_current_task:
 		return &bpf_get_current_task_proto;
 	case BPF_FUNC_get_current_task_btf:
@@ -1582,8 +1580,6 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_send_signal_thread_proto;
 	case BPF_FUNC_perf_event_read_value:
 		return &bpf_perf_event_read_value_proto;
-	case BPF_FUNC_get_ns_current_pid_tgid:
-		return &bpf_get_ns_current_pid_tgid_proto;
 	case BPF_FUNC_ringbuf_output:
 		return &bpf_ringbuf_output_proto;
 	case BPF_FUNC_ringbuf_reserve:
diff --git a/net/core/filter.c b/net/core/filter.c
index 8adf95765cdd..0c66e4a3fc5b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8342,8 +8342,6 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_event_output_data_proto;
 	case BPF_FUNC_get_current_uid_gid:
 		return &bpf_get_current_uid_gid_proto;
-	case BPF_FUNC_get_current_pid_tgid:
-		return &bpf_get_current_pid_tgid_proto;
 	case BPF_FUNC_sk_storage_get:
 		return &bpf_sk_storage_get_proto;
 	case BPF_FUNC_sk_storage_delete:
-- 
2.43.0

[PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Yonghong Song]

Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
I also shortened subtest name as the prefix of subtest name is covered
by the test name already.

This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
is not correct, the test still passed since the clone func returns 0.
I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
 1 file changed, 19 insertions(+), 17 deletions(-)

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 24d493482ffc..3a0664a86243 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
@@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
 {
 	struct test_ns_current_pid_tgid__bss  *bss;
 	struct test_ns_current_pid_tgid *skel;
-	int err = -1, duration = 0;
+	int ret = -1, err;
 	pid_t tgid, pid;
 	struct stat st;
 
 	skel = test_ns_current_pid_tgid__open_and_load();
-	if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
-		goto cleanup;
+	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
+		goto out;
 
 	pid = syscall(SYS_gettid);
 	tgid = getpid();
 
 	err = stat("/proc/self/ns/pid", &st);
-	if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
+	if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
 		goto cleanup;
 
 	bss = skel->bss;
@@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
 	bss->user_tgid = 0;
 
 	err = test_ns_current_pid_tgid__attach(skel);
-	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
 		goto cleanup;
 
 	/* trigger tracepoint */
 	usleep(1);
-	ASSERT_EQ(bss->user_pid, pid, "pid");
-	ASSERT_EQ(bss->user_tgid, tgid, "tgid");
-	err = 0;
+	if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
+		goto cleanup;
+	if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
+		goto cleanup;
+	ret = 0;
 
 cleanup:
-	 test_ns_current_pid_tgid__destroy(skel);
-
-	return err;
+	test_ns_current_pid_tgid__destroy(skel);
+out:
+	return ret;
 }
 
 static void test_ns_current_pid_tgid_new_ns(void)
 {
-	int wstatus, duration = 0;
+	int wstatus;
 	pid_t cpid;
 
 	/* Create a process in a new namespace, this process
@@ -68,21 +70,21 @@ static void test_ns_current_pid_tgid_new_ns(void)
 	cpid = clone(test_current_pid_tgid, child_stack + STACK_SIZE,
 		     CLONE_NEWPID | SIGCHLD, NULL);
 
-	if (CHECK(cpid == -1, "clone", "%s\n", strerror(errno)))
+	if (!ASSERT_NEQ(cpid, -1, "clone"))
 		return;
 
-	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", "%s\n", strerror(errno)))
+	if (!ASSERT_NEQ(waitpid(cpid, &wstatus, 0), -1, "waitpid"))
 		return;
 
-	if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid", "failed"))
+	if (!ASSERT_OK(WEXITSTATUS(wstatus), "newns_pidtgid"))
 		return;
 }
 
 /* TODO: use a different tracepoint */
 void serial_test_ns_current_pid_tgid(void)
 {
-	if (test__start_subtest("ns_current_pid_tgid_root_ns"))
+	if (test__start_subtest("root_ns_tp"))
 		test_current_pid_tgid(NULL);
-	if (test__start_subtest("ns_current_pid_tgid_new_ns"))
+	if (test__start_subtest("new_ns_tp"))
 		test_ns_current_pid_tgid_new_ns();
 }
-- 
2.43.0

[PATCH bpf-next v2 3/5] selftests/bpf: Refactor out some functions in ns_current_pid_tgid test

[Yonghong Song]

Refactor some functions in both user space code and bpf program
as these functions are used by later cgroup/sk_msg tests.
Another change is to mark tp program optional loading as later
patches will use optional loading as well since they have quite
different attachment and testing logic.

There is no functionality change.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 53 ++++++++++++-------
 .../bpf/progs/test_ns_current_pid_tgid.c      | 10 ++--
 2 files changed, 41 insertions(+), 22 deletions(-)

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 3a0664a86243..847d7b70e290 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
@@ -16,30 +16,46 @@
 #define STACK_SIZE (1024 * 1024)
 static char child_stack[STACK_SIZE];
 
-static int test_current_pid_tgid(void *args)
+static int get_pid_tgid(pid_t *pid, pid_t *tgid,
+			struct test_ns_current_pid_tgid__bss *bss)
 {
-	struct test_ns_current_pid_tgid__bss  *bss;
-	struct test_ns_current_pid_tgid *skel;
-	int ret = -1, err;
-	pid_t tgid, pid;
 	struct stat st;
+	int err;
 
-	skel = test_ns_current_pid_tgid__open_and_load();
-	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
-		goto out;
-
-	pid = syscall(SYS_gettid);
-	tgid = getpid();
+	*pid = syscall(SYS_gettid);
+	*tgid = getpid();
 
 	err = stat("/proc/self/ns/pid", &st);
 	if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
-		goto cleanup;
+		return err;
 
-	bss = skel->bss;
 	bss->dev = st.st_dev;
 	bss->ino = st.st_ino;
 	bss->user_pid = 0;
 	bss->user_tgid = 0;
+	return 0;
+}
+
+static int test_current_pid_tgid_tp(void *args)
+{
+	struct test_ns_current_pid_tgid__bss  *bss;
+	struct test_ns_current_pid_tgid *skel;
+	int ret = -1, err;
+	pid_t tgid, pid;
+
+	skel = test_ns_current_pid_tgid__open();
+	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open"))
+		return ret;
+
+	bpf_program__set_autoload(skel->progs.tp_handler, true);
+
+	err = test_ns_current_pid_tgid__load(skel);
+	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__load"))
+		goto cleanup;
+
+	bss = skel->bss;
+	if (get_pid_tgid(&pid, &tgid, bss))
+		goto cleanup;
 
 	err = test_ns_current_pid_tgid__attach(skel);
 	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
@@ -55,11 +71,10 @@ static int test_current_pid_tgid(void *args)
 
 cleanup:
 	test_ns_current_pid_tgid__destroy(skel);
-out:
 	return ret;
 }
 
-static void test_ns_current_pid_tgid_new_ns(void)
+static void test_ns_current_pid_tgid_new_ns(int (*fn)(void *), void *arg)
 {
 	int wstatus;
 	pid_t cpid;
@@ -67,8 +82,8 @@ static void test_ns_current_pid_tgid_new_ns(void)
 	/* Create a process in a new namespace, this process
 	 * will be the init process of this new namespace hence will be pid 1.
 	 */
-	cpid = clone(test_current_pid_tgid, child_stack + STACK_SIZE,
-		     CLONE_NEWPID | SIGCHLD, NULL);
+	cpid = clone(fn, child_stack + STACK_SIZE,
+		     CLONE_NEWPID | SIGCHLD, arg);
 
 	if (!ASSERT_NEQ(cpid, -1, "clone"))
 		return;
@@ -84,7 +99,7 @@ static void test_ns_current_pid_tgid_new_ns(void)
 void serial_test_ns_current_pid_tgid(void)
 {
 	if (test__start_subtest("root_ns_tp"))
-		test_current_pid_tgid(NULL);
+		test_current_pid_tgid_tp(NULL);
 	if (test__start_subtest("new_ns_tp"))
-		test_ns_current_pid_tgid_new_ns();
+		test_ns_current_pid_tgid_new_ns(test_current_pid_tgid_tp, NULL);
 }
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 0763d49f9c42..aa3ec7ca16d9 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
@@ -10,17 +10,21 @@ __u64 user_tgid = 0;
 __u64 dev = 0;
 __u64 ino = 0;
 
-SEC("tracepoint/syscalls/sys_enter_nanosleep")
-int handler(const void *ctx)
+static void get_pid_tgid(void)
 {
 	struct bpf_pidns_info nsdata;
 
 	if (bpf_get_ns_current_pid_tgid(dev, ino, &nsdata, sizeof(struct bpf_pidns_info)))
-		return 0;
+		return;
 
 	user_pid = nsdata.pid;
 	user_tgid = nsdata.tgid;
+}
 
+SEC("?tracepoint/syscalls/sys_enter_nanosleep")
+int tp_handler(const void *ctx)
+{
+	get_pid_tgid();
 	return 0;
 }
 
-- 
2.43.0

[PATCH bpf-next v2 4/5] selftests/bpf: Add a cgroup prog bpf_get_ns_current_pid_tgid() test

[Yonghong Song]

Add a cgroup bpf program test where the bpf program is running
in a pid namespace. The test is successfully:
  #165/3   ns_current_pid_tgid/new_ns_cgrp:OK

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 73 +++++++++++++++++++
 .../bpf/progs/test_ns_current_pid_tgid.c      |  7 ++
 2 files changed, 80 insertions(+)

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 847d7b70e290..fded38d24aae 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
@@ -12,6 +12,7 @@
 #include <sys/wait.h>
 #include <sys/mount.h>
 #include <sys/fcntl.h>
+#include "network_helpers.h"
 
 #define STACK_SIZE (1024 * 1024)
 static char child_stack[STACK_SIZE];
@@ -74,6 +75,50 @@ static int test_current_pid_tgid_tp(void *args)
 	return ret;
 }
 
+static int test_current_pid_tgid_cgrp(void *args)
+{
+	struct test_ns_current_pid_tgid__bss *bss;
+	struct test_ns_current_pid_tgid *skel;
+	int server_fd = -1, ret = -1, err;
+	int cgroup_fd = *(int *)args;
+	pid_t tgid, pid;
+
+	skel = test_ns_current_pid_tgid__open();
+	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open"))
+		return ret;
+
+	bpf_program__set_autoload(skel->progs.cgroup_bind4, true);
+
+	err = test_ns_current_pid_tgid__load(skel);
+	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__load"))
+		goto cleanup;
+
+	bss = skel->bss;
+	if (get_pid_tgid(&pid, &tgid, bss))
+		goto cleanup;
+
+	skel->links.cgroup_bind4 = bpf_program__attach_cgroup(
+		skel->progs.cgroup_bind4, cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.cgroup_bind4, "bpf_program__attach_cgroup"))
+		goto cleanup;
+
+	server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
+	if (!ASSERT_GE(server_fd, 0, "start_server"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
+		goto cleanup;
+	if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
+		goto cleanup;
+	ret = 0;
+
+cleanup:
+	if (server_fd >= 0)
+		close(server_fd);
+	test_ns_current_pid_tgid__destroy(skel);
+	return ret;
+}
+
 static void test_ns_current_pid_tgid_new_ns(int (*fn)(void *), void *arg)
 {
 	int wstatus;
@@ -95,6 +140,25 @@ static void test_ns_current_pid_tgid_new_ns(int (*fn)(void *), void *arg)
 		return;
 }
 
+static void test_in_netns(int (*fn)(void *), void *arg)
+{
+	struct nstoken *nstoken = NULL;
+
+	SYS(cleanup, "ip netns add ns_current_pid_tgid");
+	SYS(cleanup, "ip -net ns_current_pid_tgid link set dev lo up");
+
+	nstoken = open_netns("ns_current_pid_tgid");
+	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
+		goto cleanup;
+
+	test_ns_current_pid_tgid_new_ns(fn, arg);
+
+cleanup:
+	if (nstoken)
+		close_netns(nstoken);
+	SYS_NOFAIL("ip netns del ns_current_pid_tgid");
+}
+
 /* TODO: use a different tracepoint */
 void serial_test_ns_current_pid_tgid(void)
 {
@@ -102,4 +166,13 @@ void serial_test_ns_current_pid_tgid(void)
 		test_current_pid_tgid_tp(NULL);
 	if (test__start_subtest("new_ns_tp"))
 		test_ns_current_pid_tgid_new_ns(test_current_pid_tgid_tp, NULL);
+	if (test__start_subtest("new_ns_cgrp")) {
+		int cgroup_fd = -1;
+
+		cgroup_fd = test__join_cgroup("/sock_addr");
+		if (ASSERT_GE(cgroup_fd, 0, "join_cgroup")) {
+			test_in_netns(test_current_pid_tgid_cgrp, &cgroup_fd);
+			close(cgroup_fd);
+		}
+	}
 }
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 aa3ec7ca16d9..d0010e698f66 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
@@ -28,4 +28,11 @@ int tp_handler(const void *ctx)
 	return 0;
 }
 
+SEC("?cgroup/bind4")
+int cgroup_bind4(struct bpf_sock_addr *ctx)
+{
+	get_pid_tgid();
+	return 1;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.0

[PATCH bpf-next v2 5/5] selftests/bpf: Add a sk_msg prog bpf_get_ns_current_pid_tgid() test

[Yonghong Song]

Add a sk_msg bpf program test where the program is running in a pid
namespace. The test is successful:
  #165/4   ns_current_pid_tgid/new_ns_sk_msg:OK

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/ns_current_pid_tgid.c      | 62 +++++++++++++++++++
 .../bpf/progs/test_ns_current_pid_tgid.c      | 14 +++++
 2 files changed, 76 insertions(+)

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 fded38d24aae..e72d75d6baa7 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
@@ -119,6 +119,66 @@ static int test_current_pid_tgid_cgrp(void *args)
 	return ret;
 }
 
+static int test_current_pid_tgid_sk_msg(void *args)
+{
+	int verdict, map, server_fd = -1, client_fd = -1;
+	struct test_ns_current_pid_tgid__bss *bss;
+	static const char send_msg[] = "message";
+	struct test_ns_current_pid_tgid *skel;
+	int ret = -1, err, key = 0;
+	pid_t tgid, pid;
+
+	skel = test_ns_current_pid_tgid__open();
+	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open"))
+		return ret;
+
+	bpf_program__set_autoload(skel->progs.sk_msg, true);
+
+	err = test_ns_current_pid_tgid__load(skel);
+	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__load"))
+		goto cleanup;
+
+	bss = skel->bss;
+	if (get_pid_tgid(&pid, &tgid, skel->bss))
+		goto cleanup;
+
+	verdict = bpf_program__fd(skel->progs.sk_msg);
+	map = bpf_map__fd(skel->maps.sock_map);
+	err = bpf_prog_attach(verdict, map, BPF_SK_MSG_VERDICT, 0);
+	if (!ASSERT_OK(err, "prog_attach"))
+		goto cleanup;
+
+	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
+	if (!ASSERT_GE(server_fd, 0, "start_server"))
+		goto cleanup;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_GE(client_fd, 0, "connect_to_fd"))
+		goto cleanup;
+
+	err = bpf_map_update_elem(map, &key, &client_fd, BPF_ANY);
+	if (!ASSERT_OK(err, "bpf_map_update_elem"))
+		goto cleanup;
+
+	err = send(client_fd, send_msg, sizeof(send_msg), 0);
+	if (!ASSERT_EQ(err, sizeof(send_msg), "send(msg)"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
+		goto cleanup;
+	if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
+		goto cleanup;
+	ret = 0;
+
+cleanup:
+	if (server_fd >= 0)
+		close(server_fd);
+	if (client_fd >= 0)
+		close(client_fd);
+	test_ns_current_pid_tgid__destroy(skel);
+	return ret;
+}
+
 static void test_ns_current_pid_tgid_new_ns(int (*fn)(void *), void *arg)
 {
 	int wstatus;
@@ -175,4 +235,6 @@ void serial_test_ns_current_pid_tgid(void)
 			close(cgroup_fd);
 		}
 	}
+	if (test__start_subtest("new_ns_sk_msg"))
+		test_in_netns(test_current_pid_tgid_sk_msg, NULL);
 }
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 d0010e698f66..386315afad65 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,6 +5,13 @@
 #include <stdint.h>
 #include <bpf/bpf_helpers.h>
 
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__uint(max_entries, 2);
+	__type(key, __u32);
+	__type(value, __u32);
+} sock_map SEC(".maps");
+
 __u64 user_pid = 0;
 __u64 user_tgid = 0;
 __u64 dev = 0;
@@ -35,4 +42,11 @@ int cgroup_bind4(struct bpf_sock_addr *ctx)
 	return 1;
 }
 
+SEC("?sk_msg")
+int sk_msg(struct sk_msg_md *msg)
+{
+	get_pid_tgid();
+	return SK_PASS;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Jiri Olsa]

On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
> I also shortened subtest name as the prefix of subtest name is covered
> by the test name already.
> 
> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
> is not correct, the test still passed since the clone func returns 0.
> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> 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 24d493482ffc..3a0664a86243 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
> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
>  {
>  	struct test_ns_current_pid_tgid__bss  *bss;
>  	struct test_ns_current_pid_tgid *skel;
> -	int err = -1, duration = 0;
> +	int ret = -1, err;
>  	pid_t tgid, pid;
>  	struct stat st;
>  
>  	skel = test_ns_current_pid_tgid__open_and_load();
> -	if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
> -		goto cleanup;
> +	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
> +		goto out;

you could just return in here so there's no need for the out label
otherwise lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>


jirka

>  
>  	pid = syscall(SYS_gettid);
>  	tgid = getpid();
>  
>  	err = stat("/proc/self/ns/pid", &st);
> -	if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
> +	if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
>  		goto cleanup;
>  
>  	bss = skel->bss;
> @@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
>  	bss->user_tgid = 0;
>  
>  	err = test_ns_current_pid_tgid__attach(skel);
> -	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> +	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
>  		goto cleanup;
>  
>  	/* trigger tracepoint */
>  	usleep(1);
> -	ASSERT_EQ(bss->user_pid, pid, "pid");
> -	ASSERT_EQ(bss->user_tgid, tgid, "tgid");
> -	err = 0;
> +	if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
> +		goto cleanup;
> +	ret = 0;
>  
>  cleanup:
> -	 test_ns_current_pid_tgid__destroy(skel);
> -
> -	return err;
> +	test_ns_current_pid_tgid__destroy(skel);
> +out:
> +	return ret;
>  }
>  
>  static void test_ns_current_pid_tgid_new_ns(void)
>  {
> -	int wstatus, duration = 0;
> +	int wstatus;
>  	pid_t cpid;
>  
>  	/* Create a process in a new namespace, this process
> @@ -68,21 +70,21 @@ static void test_ns_current_pid_tgid_new_ns(void)
>  	cpid = clone(test_current_pid_tgid, child_stack + STACK_SIZE,
>  		     CLONE_NEWPID | SIGCHLD, NULL);
>  
> -	if (CHECK(cpid == -1, "clone", "%s\n", strerror(errno)))
> +	if (!ASSERT_NEQ(cpid, -1, "clone"))
>  		return;
>  
> -	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", "%s\n", strerror(errno)))
> +	if (!ASSERT_NEQ(waitpid(cpid, &wstatus, 0), -1, "waitpid"))
>  		return;
>  
> -	if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid", "failed"))
> +	if (!ASSERT_OK(WEXITSTATUS(wstatus), "newns_pidtgid"))
>  		return;
>  }
>  
>  /* TODO: use a different tracepoint */
>  void serial_test_ns_current_pid_tgid(void)
>  {
> -	if (test__start_subtest("ns_current_pid_tgid_root_ns"))
> +	if (test__start_subtest("root_ns_tp"))
>  		test_current_pid_tgid(NULL);
> -	if (test__start_subtest("ns_current_pid_tgid_new_ns"))
> +	if (test__start_subtest("new_ns_tp"))
>  		test_ns_current_pid_tgid_new_ns();
>  }
> -- 
> 2.43.0
> 
> 

Re: [PATCH bpf-next v2 1/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types

[Jiri Olsa]

On Fri, Mar 15, 2024 at 11:48:54AM -0700, Yonghong Song wrote:
> Currently bpf_get_current_pid_tgid() is allowed in tracing, cgroup
> and sk_msg progs while bpf_get_ns_current_pid_tgid() is only allowed
> in tracing progs.
> 
> We have an internal use case where for an application running
> in a container (with pid namespace), user wants to get
> the pid associated with the pid namespace in a cgroup bpf
> program. Currently, cgroup bpf progs already allow
> bpf_get_current_pid_tgid(). Let us allow bpf_get_ns_current_pid_tgid()
> as well.
> 
> With auditing the code, bpf_get_current_pid_tgid() is also used
> by sk_msg prog. But there are no side effect to expose these two
> helpers to all prog types since they do not reveal any kernel specific
> data. The detailed discussion is in [1].
> 
> So with this patch, both bpf_get_current_pid_tgid() and bpf_get_ns_current_pid_tgid()
> are put in bpf_base_func_proto(), making them available to all
> program types.
> 
>   [1] https://lore.kernel.org/bpf/20240307232659.1115872-1-yonghong.song@linux.dev/
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  kernel/bpf/cgroup.c      | 2 --
>  kernel/bpf/helpers.c     | 4 ++++
>  kernel/trace/bpf_trace.c | 4 ----
>  net/core/filter.c        | 2 --
>  4 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 82243cb6c54d..8ba73042a239 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -2575,8 +2575,6 @@ cgroup_current_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  	switch (func_id) {
>  	case BPF_FUNC_get_current_uid_gid:
>  		return &bpf_get_current_uid_gid_proto;
> -	case BPF_FUNC_get_current_pid_tgid:
> -		return &bpf_get_current_pid_tgid_proto;
>  	case BPF_FUNC_get_current_comm:
>  		return &bpf_get_current_comm_proto;
>  #ifdef CONFIG_CGROUP_NET_CLASSID
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a89587859571..9234174ccb21 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1730,6 +1730,10 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_strtol_proto;
>  	case BPF_FUNC_strtoul:
>  		return &bpf_strtoul_proto;
> +	case BPF_FUNC_get_current_pid_tgid:
> +		return &bpf_get_current_pid_tgid_proto;
> +	case BPF_FUNC_get_ns_current_pid_tgid:
> +		return &bpf_get_ns_current_pid_tgid_proto;
>  	default:
>  		break;
>  	}
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 0a5c4efc73c3..1b041911b1d8 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1525,8 +1525,6 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_ktime_get_boot_ns_proto;
>  	case BPF_FUNC_tail_call:
>  		return &bpf_tail_call_proto;
> -	case BPF_FUNC_get_current_pid_tgid:
> -		return &bpf_get_current_pid_tgid_proto;
>  	case BPF_FUNC_get_current_task:
>  		return &bpf_get_current_task_proto;
>  	case BPF_FUNC_get_current_task_btf:
> @@ -1582,8 +1580,6 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_send_signal_thread_proto;
>  	case BPF_FUNC_perf_event_read_value:
>  		return &bpf_perf_event_read_value_proto;
> -	case BPF_FUNC_get_ns_current_pid_tgid:
> -		return &bpf_get_ns_current_pid_tgid_proto;
>  	case BPF_FUNC_ringbuf_output:
>  		return &bpf_ringbuf_output_proto;
>  	case BPF_FUNC_ringbuf_reserve:
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8adf95765cdd..0c66e4a3fc5b 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -8342,8 +8342,6 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_event_output_data_proto;
>  	case BPF_FUNC_get_current_uid_gid:
>  		return &bpf_get_current_uid_gid_proto;
> -	case BPF_FUNC_get_current_pid_tgid:
> -		return &bpf_get_current_pid_tgid_proto;
>  	case BPF_FUNC_sk_storage_get:
>  		return &bpf_sk_storage_get_proto;
>  	case BPF_FUNC_sk_storage_delete:
> -- 
> 2.43.0
> 
> 

Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Yonghong Song]

On 3/18/24 4:36 AM, Jiri Olsa wrote:
> On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
>> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
>> I also shortened subtest name as the prefix of subtest name is covered
>> by the test name already.
>>
>> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
>> is not correct, the test still passed since the clone func returns 0.
>> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
>>   1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> 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 24d493482ffc..3a0664a86243 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
>> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
>>   {
>>   	struct test_ns_current_pid_tgid__bss  *bss;
>>   	struct test_ns_current_pid_tgid *skel;
>> -	int err = -1, duration = 0;
>> +	int ret = -1, err;
>>   	pid_t tgid, pid;
>>   	struct stat st;
>>   
>>   	skel = test_ns_current_pid_tgid__open_and_load();
>> -	if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
>> -		goto cleanup;
>> +	if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
>> +		goto out;
> you could just return in here so there's no need for the out label
> otherwise lgtm

Since this patch intends to just replace CHECK with ASSERT_*.
I tried to keep other parts of codes the same as before. But
I can certainly do this. I will wait for other comments
before sending another revision.

>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
>
> jirka
>
>>   
>>   	pid = syscall(SYS_gettid);
>>   	tgid = getpid();
>>   
>>   	err = stat("/proc/self/ns/pid", &st);
>> -	if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
>> +	if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
>>   		goto cleanup;
>>   
>>   	bss = skel->bss;
>> @@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
>>   	bss->user_tgid = 0;
>>   
>>   	err = test_ns_current_pid_tgid__attach(skel);
>> -	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
>> +	if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
>>   		goto cleanup;
>>   
>>   	/* trigger tracepoint */
>>   	usleep(1);
>> -	ASSERT_EQ(bss->user_pid, pid, "pid");
>> -	ASSERT_EQ(bss->user_tgid, tgid, "tgid");
>> -	err = 0;
>> +	if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
>> +		goto cleanup;
>> +	if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
>> +		goto cleanup;
>> +	ret = 0;
>>   
>>   cleanup:
>> -	 test_ns_current_pid_tgid__destroy(skel);
>> -
>> -	return err;
>> +	test_ns_current_pid_tgid__destroy(skel);
>> +out:
>> +	return ret;
>>   }
[...]

Re: [PATCH bpf-next v2 0/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Fri, 15 Mar 2024 11:48:49 -0700 you wrote:
> Currently bpf_get_current_pid_tgid() is allowed in tracing, cgroup
> and sk_msg progs while bpf_get_ns_current_pid_tgid() is only allowed
> in tracing progs.
> 
> We have an internal use case where for an application running
> in a container (with pid namespace), user wants to get
> the pid associated with the pid namespace in a cgroup bpf
> program. Besides cgroup, the only prog type, supporting
> bpf_get_current_pid_tgid() but not bpf_get_ns_current_pid_tgid(),
> is sk_msg.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/5] bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types
    https://git.kernel.org/bpf/bpf-next/c/eb166e522c77
  - [bpf-next,v2,2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test
    https://git.kernel.org/bpf/bpf-next/c/84239a24d101
  - [bpf-next,v2,3/5] selftests/bpf: Refactor out some functions in ns_current_pid_tgid test
    https://git.kernel.org/bpf/bpf-next/c/4d4bd29e363c
  - [bpf-next,v2,4/5] selftests/bpf: Add a cgroup prog bpf_get_ns_current_pid_tgid() test
    https://git.kernel.org/bpf/bpf-next/c/87ade6cd859e
  - [bpf-next,v2,5/5] selftests/bpf: Add a sk_msg prog bpf_get_ns_current_pid_tgid() test
    https://git.kernel.org/bpf/bpf-next/c/4c195ee4865d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Andrii Nakryiko]

On Mon, Mar 18, 2024 at 8:30 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/18/24 4:36 AM, Jiri Olsa wrote:
> > On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
> >> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
> >> I also shortened subtest name as the prefix of subtest name is covered
> >> by the test name already.
> >>
> >> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
> >> is not correct, the test still passed since the clone func returns 0.
> >> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
> >>
> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >> ---
> >>   .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
> >>   1 file changed, 19 insertions(+), 17 deletions(-)
> >>
> >> 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 24d493482ffc..3a0664a86243 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
> >> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
> >>   {
> >>      struct test_ns_current_pid_tgid__bss  *bss;
> >>      struct test_ns_current_pid_tgid *skel;
> >> -    int err = -1, duration = 0;
> >> +    int ret = -1, err;
> >>      pid_t tgid, pid;
> >>      struct stat st;
> >>
> >>      skel = test_ns_current_pid_tgid__open_and_load();
> >> -    if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
> >> -            goto cleanup;
> >> +    if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
> >> +            goto out;
> > you could just return in here so there's no need for the out label
> > otherwise lgtm
>
> Since this patch intends to just replace CHECK with ASSERT_*.
> I tried to keep other parts of codes the same as before. But
> I can certainly do this. I will wait for other comments
> before sending another revision.
>

I applied it as is, but feel free to send suggested improvements as a follow up.


> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> >
> >
> > jirka
> >
> >>
> >>      pid = syscall(SYS_gettid);
> >>      tgid = getpid();
> >>
> >>      err = stat("/proc/self/ns/pid", &st);
> >> -    if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
> >> +    if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
> >>              goto cleanup;
> >>
> >>      bss = skel->bss;
> >> @@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
> >>      bss->user_tgid = 0;
> >>
> >>      err = test_ns_current_pid_tgid__attach(skel);
> >> -    if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> >> +    if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
> >>              goto cleanup;
> >>
> >>      /* trigger tracepoint */
> >>      usleep(1);
> >> -    ASSERT_EQ(bss->user_pid, pid, "pid");
> >> -    ASSERT_EQ(bss->user_tgid, tgid, "tgid");
> >> -    err = 0;
> >> +    if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
> >> +            goto cleanup;
> >> +    if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
> >> +            goto cleanup;
> >> +    ret = 0;
> >>
> >>   cleanup:
> >> -     test_ns_current_pid_tgid__destroy(skel);
> >> -
> >> -    return err;
> >> +    test_ns_current_pid_tgid__destroy(skel);
> >> +out:
> >> +    return ret;
> >>   }
> [...]

Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Yonghong Song]

On 3/19/24 2:52 PM, Andrii Nakryiko wrote:
> On Mon, Mar 18, 2024 at 8:30 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/18/24 4:36 AM, Jiri Olsa wrote:
>>> On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
>>>> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
>>>> I also shortened subtest name as the prefix of subtest name is covered
>>>> by the test name already.
>>>>
>>>> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
>>>> is not correct, the test still passed since the clone func returns 0.
>>>> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
>>>>
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>>    .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
>>>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> 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 24d493482ffc..3a0664a86243 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
>>>> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
>>>>    {
>>>>       struct test_ns_current_pid_tgid__bss  *bss;
>>>>       struct test_ns_current_pid_tgid *skel;
>>>> -    int err = -1, duration = 0;
>>>> +    int ret = -1, err;
>>>>       pid_t tgid, pid;
>>>>       struct stat st;
>>>>
>>>>       skel = test_ns_current_pid_tgid__open_and_load();
>>>> -    if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
>>>> -            goto cleanup;
>>>> +    if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
>>>> +            goto out;
>>> you could just return in here so there's no need for the out label
>>> otherwise lgtm
>> Since this patch intends to just replace CHECK with ASSERT_*.
>> I tried to keep other parts of codes the same as before. But
>> I can certainly do this. I will wait for other comments
>> before sending another revision.
>>
> I applied it as is, but feel free to send suggested improvements as a follow up.

Thanks! Will send a followup later.

>
>
>>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>>
>>> jirka
>>>
>>>>       pid = syscall(SYS_gettid);
>>>>       tgid = getpid();
>>>>
>>>>       err = stat("/proc/self/ns/pid", &st);
>>>> -    if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
>>>> +    if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
>>>>               goto cleanup;
>>>>
>>>>       bss = skel->bss;
>>>> @@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
>>>>       bss->user_tgid = 0;
>>>>
>>>>       err = test_ns_current_pid_tgid__attach(skel);
>>>> -    if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
>>>> +    if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
>>>>               goto cleanup;
>>>>
>>>>       /* trigger tracepoint */
>>>>       usleep(1);
>>>> -    ASSERT_EQ(bss->user_pid, pid, "pid");
>>>> -    ASSERT_EQ(bss->user_tgid, tgid, "tgid");
>>>> -    err = 0;
>>>> +    if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
>>>> +            goto cleanup;
>>>> +    if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
>>>> +            goto cleanup;
>>>> +    ret = 0;
>>>>
>>>>    cleanup:
>>>> -     test_ns_current_pid_tgid__destroy(skel);
>>>> -
>>>> -    return err;
>>>> +    test_ns_current_pid_tgid__destroy(skel);
>>>> +out:
>>>> +    return ret;
>>>>    }
>> [...]

Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Yonghong Song]

On 3/19/24 2:52 PM, Andrii Nakryiko wrote:
> On Mon, Mar 18, 2024 at 8:30 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/18/24 4:36 AM, Jiri Olsa wrote:
>>> On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
>>>> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
>>>> I also shortened subtest name as the prefix of subtest name is covered
>>>> by the test name already.
>>>>
>>>> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
>>>> is not correct, the test still passed since the clone func returns 0.
>>>> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
>>>>
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>>    .../bpf/prog_tests/ns_current_pid_tgid.c      | 36 ++++++++++---------
>>>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> 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 24d493482ffc..3a0664a86243 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
>>>> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
>>>>    {
>>>>       struct test_ns_current_pid_tgid__bss  *bss;
>>>>       struct test_ns_current_pid_tgid *skel;
>>>> -    int err = -1, duration = 0;
>>>> +    int ret = -1, err;
>>>>       pid_t tgid, pid;
>>>>       struct stat st;
>>>>
>>>>       skel = test_ns_current_pid_tgid__open_and_load();
>>>> -    if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
>>>> -            goto cleanup;
>>>> +    if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
>>>> +            goto out;
>>> you could just return in here so there's no need for the out label
>>> otherwise lgtm
>> Since this patch intends to just replace CHECK with ASSERT_*.
>> I tried to keep other parts of codes the same as before. But
>> I can certainly do this. I will wait for other comments
>> before sending another revision.
>>
> I applied it as is, but feel free to send suggested improvements as a follow up.

Actually the patch (after this commit)
   4d4bd29e363c  selftests/bpf: Refactor out some functions in ns_current_pid_tgid test
already did the refactoring. So I don't need to do anything.

>
>
>>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>>
>>> jirka
[...]