[bpf-next v1 1/2] bpf: Fix iter/task tid filtering

[bpf-next v1 1/2] bpf: Fix iter/task tid filtering

[Jordan Rome]

In userspace, you can add a tid filter by setting
the "task.tid" field for "bpf_iter_link_info".
However, `get_pid_task` when called for the
`BPF_TASK_ITER_TID` type should have been using
`PIDTYPE_PID` (tid) instead of `PIDTYPE_TGID` (pid).

Fixes: f0d74c4da1f0 ("bpf: Parameterize task iterators.")
Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 kernel/bpf/task_iter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 02aa9db8d796..5af9e130e500 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -99,7 +99,7 @@ static struct task_struct *task_seq_get_next(struct bpf_iter_seq_task_common *co
 		rcu_read_lock();
 		pid = find_pid_ns(common->pid, common->ns);
 		if (pid) {
-			task = get_pid_task(pid, PIDTYPE_TGID);
+			task = get_pid_task(pid, PIDTYPE_PID);
 			*tid = common->pid;
 		}
 		rcu_read_unlock();
--
2.43.5

[bpf-next v1 2/2] bpf: properly test iter/task tid filtering

[Jordan Rome]

Previously test_task_tid was setting `linfo.task.tid`
to `getpid()` which is the same as `gettid()` for the
parent process. Instead create a new child thread
and set `linfo.task.tid` to `gettid()` to make sure
the tid filtering logic is working as expected.

Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 .../selftests/bpf/prog_tests/bpf_iter.c       | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 52e6f7570475..5b056eb5d166 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -226,7 +226,7 @@ static void test_task_common_nocheck(struct bpf_iter_attach_opts *opts,
 	ASSERT_OK(pthread_create(&thread_id, NULL, &do_nothing_wait, NULL),
 		  "pthread_create");

-	skel->bss->tid = getpid();
+	skel->bss->tid = gettid();

 	do_dummy_read_opts(skel->progs.dump_task, opts);

@@ -249,25 +249,41 @@ static void test_task_common(struct bpf_iter_attach_opts *opts, int num_unknown,
 	ASSERT_EQ(num_known_tid, num_known, "check_num_known_tid");
 }

-static void test_task_tid(void)
+static void *run_test_task_tid(void *arg)
 {
+	ASSERT_NEQ(getpid(), gettid(), "check_new_thread_id");
 	LIBBPF_OPTS(bpf_iter_attach_opts, opts);
 	union bpf_iter_link_info linfo;
 	int num_unknown_tid, num_known_tid;

 	memset(&linfo, 0, sizeof(linfo));
-	linfo.task.tid = getpid();
+	linfo.task.tid = gettid();
 	opts.link_info = &linfo;
 	opts.link_info_len = sizeof(linfo);
 	test_task_common(&opts, 0, 1);

 	linfo.task.tid = 0;
 	linfo.task.pid = getpid();
-	test_task_common(&opts, 1, 1);
+	// This includes the parent thread, this thread, and the do_nothing_wait thread
+	test_task_common(&opts, 2, 1);

 	test_task_common_nocheck(NULL, &num_unknown_tid, &num_known_tid);
-	ASSERT_GT(num_unknown_tid, 1, "check_num_unknown_tid");
+	ASSERT_GT(num_unknown_tid, 2, "check_num_unknown_tid");
 	ASSERT_EQ(num_known_tid, 1, "check_num_known_tid");
+
+	pthread_exit(arg);
+}
+
+static void test_task_tid(void)
+{
+	pthread_t thread_id;
+	void *ret;
+
+	// Create a new thread so pid and tid aren't the same
+	ASSERT_OK(pthread_create(&thread_id, NULL, &run_test_task_tid, NULL),
+		  "pthread_create");
+	ASSERT_FALSE(pthread_join(thread_id, &ret) || ret != NULL,
+		     "pthread_join");
 }

 static void test_task_pid(void)
--
2.43.5

Re: [bpf-next v1 2/2] bpf: properly test iter/task tid filtering

[Andrii Nakryiko]

On Tue, Oct 15, 2024 at 11:41 AM Jordan Rome <linux@jordanrome.com> wrote:
>
> Previously test_task_tid was setting `linfo.task.tid`
> to `getpid()` which is the same as `gettid()` for the
> parent process. Instead create a new child thread
> and set `linfo.task.tid` to `gettid()` to make sure
> the tid filtering logic is working as expected.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  .../selftests/bpf/prog_tests/bpf_iter.c       | 26 +++++++++++++++----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index 52e6f7570475..5b056eb5d166 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -226,7 +226,7 @@ static void test_task_common_nocheck(struct bpf_iter_attach_opts *opts,
>         ASSERT_OK(pthread_create(&thread_id, NULL, &do_nothing_wait, NULL),
>                   "pthread_create");
>
> -       skel->bss->tid = getpid();
> +       skel->bss->tid = gettid();
>
>         do_dummy_read_opts(skel->progs.dump_task, opts);
>
> @@ -249,25 +249,41 @@ static void test_task_common(struct bpf_iter_attach_opts *opts, int num_unknown,
>         ASSERT_EQ(num_known_tid, num_known, "check_num_known_tid");
>  }
>
> -static void test_task_tid(void)
> +static void *run_test_task_tid(void *arg)
>  {
> +       ASSERT_NEQ(getpid(), gettid(), "check_new_thread_id");

this is variable declaration block, move assertion after it (and empty line)

>         LIBBPF_OPTS(bpf_iter_attach_opts, opts);
>         union bpf_iter_link_info linfo;
>         int num_unknown_tid, num_known_tid;
>

here

>         memset(&linfo, 0, sizeof(linfo));
> -       linfo.task.tid = getpid();
> +       linfo.task.tid = gettid();
>         opts.link_info = &linfo;
>         opts.link_info_len = sizeof(linfo);
>         test_task_common(&opts, 0, 1);
>
>         linfo.task.tid = 0;
>         linfo.task.pid = getpid();
> -       test_task_common(&opts, 1, 1);
> +       // This includes the parent thread, this thread, and the do_nothing_wait thread

we don't use C++-style comments in C code base, please use /* */

> +       test_task_common(&opts, 2, 1);
>
>         test_task_common_nocheck(NULL, &num_unknown_tid, &num_known_tid);
> -       ASSERT_GT(num_unknown_tid, 1, "check_num_unknown_tid");
> +       ASSERT_GT(num_unknown_tid, 2, "check_num_unknown_tid");
>         ASSERT_EQ(num_known_tid, 1, "check_num_known_tid");
> +
> +       pthread_exit(arg);

nit: wouldn't `return arg;` do the same?

> +}
> +
> +static void test_task_tid(void)
> +{
> +       pthread_t thread_id;
> +       void *ret;
> +
> +       // Create a new thread so pid and tid aren't the same

C++ comment

> +       ASSERT_OK(pthread_create(&thread_id, NULL, &run_test_task_tid, NULL),
> +                 "pthread_create");
> +       ASSERT_FALSE(pthread_join(thread_id, &ret) || ret != NULL,

it's best to avoid combining two check in single ASSERT_*(), so

ASSERT_OK(pthread_join(...), ...);
ASSERT_NULL(ret, ...);

is way easier to follow and debug, if something breaks

But also, why do we check ret? Do we ever return non-NULL?

pw-bot: cr

> +                    "pthread_join");
>  }
>
>  static void test_task_pid(void)
> --
> 2.43.5
>

Re: [bpf-next v1 1/2] bpf: Fix iter/task tid filtering

[Andrii Nakryiko]

On Tue, Oct 15, 2024 at 11:33 AM Jordan Rome <linux@jordanrome.com> wrote:
>
> In userspace, you can add a tid filter by setting
> the "task.tid" field for "bpf_iter_link_info".
> However, `get_pid_task` when called for the
> `BPF_TASK_ITER_TID` type should have been using
> `PIDTYPE_PID` (tid) instead of `PIDTYPE_TGID` (pid).
>
> Fixes: f0d74c4da1f0 ("bpf: Parameterize task iterators.")
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  kernel/bpf/task_iter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

This change is an important fix, so it has to target bpf tree, please
use [PATCH bpf] subject prefix

> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 02aa9db8d796..5af9e130e500 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -99,7 +99,7 @@ static struct task_struct *task_seq_get_next(struct bpf_iter_seq_task_common *co
>                 rcu_read_lock();
>                 pid = find_pid_ns(common->pid, common->ns);
>                 if (pid) {
> -                       task = get_pid_task(pid, PIDTYPE_TGID);
> +                       task = get_pid_task(pid, PIDTYPE_PID);
>                         *tid = common->pid;
>                 }
>                 rcu_read_unlock();
> --
> 2.43.5
>