* [PATCH v2 bpf 1/5] bpf: fix multi-uprobe PID filtering logic
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
@ 2024-05-21 16:33 ` Andrii Nakryiko
2024-05-21 16:33 ` [PATCH v2 bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic Andrii Nakryiko
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2024-05-21 16:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, stable, Jiri Olsa
Current implementation of PID filtering logic for multi-uprobes in
uprobe_prog_run() is filtering down to exact *thread*, while the intent
for PID filtering it to filter by *process* instead. The check in
uprobe_prog_run() also differs from the analogous one in
uprobe_multi_link_filter() for some reason. The latter is correct,
checking task->mm, not the task itself.
Fix the check in uprobe_prog_run() to perform the same task->mm check.
While doing this, we also update get_pid_task() use to use PIDTYPE_TGID
type of lookup, given the intent is to get a representative task of an
entire process. This doesn't change behavior, but seems more logical. It
would hold task group leader task now, not any random thread task.
Last but not least, given multi-uprobe support is half-broken due to
this PID filtering logic (depending on whether PID filtering is
important or not), we need to make it easy for user space consumers
(including libbpf) to easily detect whether PID filtering logic was
already fixed.
We do it here by adding an early check on passed pid parameter. If it's
negative (and so has no chance of being a valid PID), we return -EINVAL.
Previous behavior would eventually return -ESRCH ("No process found"),
given there can't be any process with negative PID. This subtle change
won't make any practical change in behavior, but will allow applications
to detect PID filtering fixes easily. Libbpf fixes take advantage of
this in the next patch.
Cc: stable@vger.kernel.org
Acked-by: Jiri Olsa <jolsa@kernel.org>
Fixes: b733eeade420 ("bpf: Add pid filter support for uprobe_multi link")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/trace/bpf_trace.c | 8 ++++----
.../testing/selftests/bpf/prog_tests/uprobe_multi_test.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f5154c051d2c..1baaeb9ca205 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3295,7 +3295,7 @@ static int uprobe_prog_run(struct bpf_uprobe *uprobe,
struct bpf_run_ctx *old_run_ctx;
int err = 0;
- if (link->task && current != link->task)
+ if (link->task && current->mm != link->task->mm)
return 0;
if (sleepable)
@@ -3396,8 +3396,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
upath = u64_to_user_ptr(attr->link_create.uprobe_multi.path);
uoffsets = u64_to_user_ptr(attr->link_create.uprobe_multi.offsets);
cnt = attr->link_create.uprobe_multi.cnt;
+ pid = attr->link_create.uprobe_multi.pid;
- if (!upath || !uoffsets || !cnt)
+ if (!upath || !uoffsets || !cnt || pid < 0)
return -EINVAL;
if (cnt > MAX_UPROBE_MULTI_CNT)
return -E2BIG;
@@ -3421,10 +3422,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
goto error_path_put;
}
- pid = attr->link_create.uprobe_multi.pid;
if (pid) {
rcu_read_lock();
- task = get_pid_task(find_vpid(pid), PIDTYPE_PID);
+ task = get_pid_task(find_vpid(pid), PIDTYPE_TGID);
rcu_read_unlock();
if (!task) {
err = -ESRCH;
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index 8269cdee33ae..38fda42fd70f 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -397,7 +397,7 @@ static void test_attach_api_fails(void)
link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &opts);
if (!ASSERT_ERR(link_fd, "link_fd"))
goto cleanup;
- ASSERT_EQ(link_fd, -ESRCH, "pid_is_wrong");
+ ASSERT_EQ(link_fd, -EINVAL, "pid_is_wrong");
cleanup:
if (link_fd >= 0)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
2024-05-21 16:33 ` [PATCH v2 bpf 1/5] bpf: fix " Andrii Nakryiko
@ 2024-05-21 16:33 ` Andrii Nakryiko
2024-05-21 16:33 ` [PATCH v2 bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe Andrii Nakryiko
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2024-05-21 16:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Jiri Olsa
get_pid_task() internally already calls rcu_read_lock() and
rcu_read_unlock(), so there is no point to do this one extra time.
This is a drive-by improvement and has no correctness implications.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
kernel/trace/bpf_trace.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1baaeb9ca205..6249dac61701 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3423,9 +3423,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
}
if (pid) {
- rcu_read_lock();
task = get_pid_task(find_vpid(pid), PIDTYPE_TGID);
- rcu_read_unlock();
if (!task) {
err = -ESRCH;
goto error_path_put;
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
2024-05-21 16:33 ` [PATCH v2 bpf 1/5] bpf: fix " Andrii Nakryiko
2024-05-21 16:33 ` [PATCH v2 bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic Andrii Nakryiko
@ 2024-05-21 16:33 ` Andrii Nakryiko
2024-05-21 16:34 ` [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case Andrii Nakryiko
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2024-05-21 16:33 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Jiri Olsa
Libbpf is automatically (and transparently to user) detecting
multi-uprobe support in the kernel, and, if supported, uses
multi-uprobes to improve USDT attachment speed.
USDTs can be attached system-wide or for the specific process by PID. In
the latter case, we rely on correct kernel logic of not triggering USDT
for unrelated processes.
As such, on older kernels that do support multi-uprobes, but still have
broken PID filtering logic, we need to fall back to singular uprobes.
Unfortunately, whether user is using PID filtering or not is known at
the attachment time, which happens after relevant BPF programs were
loaded into the kernel. Also unfortunately, we need to make a call
whether to use multi-uprobes or singular uprobe for SEC("usdt") programs
during BPF object load time, at which point we have no information about
possible PID filtering.
The distinction between single and multi-uprobes is small, but important
for the kernel. Multi-uprobes get BPF_TRACE_UPROBE_MULTI attach type,
and kernel internally substitiute different implementation of some of
BPF helpers (e.g., bpf_get_attach_cookie()) depending on whether uprobe
is multi or singular. So, multi-uprobes and singular uprobes cannot be
intermixed.
All the above implies that we have to make an early and conservative
call about the use of multi-uprobes. And so this patch modifies libbpf's
existing feature detector for multi-uprobe support to also check correct
PID filtering. If PID filtering is not yet fixed, we fall back to
singular uprobes for USDTs.
This extension to feature detection is simple thanks to kernel's -EINVAL
addition for pid < 0.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/features.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/features.c b/tools/lib/bpf/features.c
index a336786a22a3..3df0125ed5fa 100644
--- a/tools/lib/bpf/features.c
+++ b/tools/lib/bpf/features.c
@@ -392,11 +392,40 @@ static int probe_uprobe_multi_link(int token_fd)
link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts);
err = -errno; /* close() can clobber errno */
+ if (link_fd >= 0 || err != -EBADF) {
+ close(link_fd);
+ close(prog_fd);
+ return 0;
+ }
+
+ /* Initial multi-uprobe support in kernel didn't handle PID filtering
+ * correctly (it was doing thread filtering, not process filtering).
+ * So now we'll detect if PID filtering logic was fixed, and, if not,
+ * we'll pretend multi-uprobes are not supported, if not.
+ * Multi-uprobes are used in USDT attachment logic, and we need to be
+ * conservative here, because multi-uprobe selection happens early at
+ * load time, while the use of PID filtering is known late at
+ * attachment time, at which point it's too late to undo multi-uprobe
+ * selection.
+ *
+ * Creating uprobe with pid == -1 for (invalid) '/' binary will fail
+ * early with -EINVAL on kernels with fixed PID filtering logic;
+ * otherwise -ESRCH would be returned if passed correct binary path
+ * (but we'll just get -BADF, of course).
+ */
+ link_opts.uprobe_multi.pid = -1; /* invalid PID */
+ link_opts.uprobe_multi.path = "/"; /* invalid path */
+ link_opts.uprobe_multi.offsets = &offset;
+ link_opts.uprobe_multi.cnt = 1;
+
+ link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts);
+ err = -errno; /* close() can clobber errno */
+
if (link_fd >= 0)
close(link_fd);
close(prog_fd);
- return link_fd < 0 && err == -EBADF;
+ return link_fd < 0 && err == -EINVAL;
}
static int probe_kern_bpf_cookie(int token_fd)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
` (2 preceding siblings ...)
2024-05-21 16:33 ` [PATCH v2 bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe Andrii Nakryiko
@ 2024-05-21 16:34 ` Andrii Nakryiko
2024-05-22 7:46 ` Jiri Olsa
2024-05-21 16:34 ` [PATCH v2 bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs Andrii Nakryiko
2024-05-25 17:50 ` [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic patchwork-bot+netdevbpf
5 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2024-05-21 16:34 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team
Extend existing multi-uprobe tests to test that PID filtering works
correctly. We already have child *process* tests, but we need also child
*thread* tests. This patch adds spawn_thread() helper to start child
thread, wait for it to be ready, and then instruct it to trigger desired
uprobes.
Additionally, we extend BPF-side code to track thread ID, not just
process ID. Also we detect whether extraneous triggerings with
unexpected process IDs happened, and validate that none of that happened
in practice.
These changes prove that fixed PID filtering logic for multi-uprobe
works as expected. These tests fail on old kernels.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../bpf/prog_tests/uprobe_multi_test.c | 107 ++++++++++++++++--
.../selftests/bpf/progs/uprobe_multi.c | 17 ++-
2 files changed, 115 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index 38fda42fd70f..677232d31432 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include <unistd.h>
+#include <pthread.h>
#include <test_progs.h>
#include "uprobe_multi.skel.h"
#include "uprobe_multi_bench.skel.h"
@@ -27,7 +28,10 @@ noinline void uprobe_multi_func_3(void)
struct child {
int go[2];
+ int c2p[2]; /* child -> parent channel */
int pid;
+ int tid;
+ pthread_t thread;
};
static void release_child(struct child *child)
@@ -38,6 +42,10 @@ static void release_child(struct child *child)
return;
close(child->go[1]);
close(child->go[0]);
+ if (child->thread)
+ pthread_join(child->thread, NULL);
+ close(child->c2p[0]);
+ close(child->c2p[1]);
if (child->pid > 0)
waitpid(child->pid, &child_status, 0);
}
@@ -63,7 +71,7 @@ static struct child *spawn_child(void)
if (pipe(child.go))
return NULL;
- child.pid = fork();
+ child.pid = child.tid = fork();
if (child.pid < 0) {
release_child(&child);
errno = EINVAL;
@@ -89,6 +97,66 @@ static struct child *spawn_child(void)
return &child;
}
+static void *child_thread(void *ctx)
+{
+ struct child *child = ctx;
+ int c = 0, err;
+
+ child->tid = syscall(SYS_gettid);
+
+ /* let parent know we are ready */
+ err = write(child->c2p[1], &c, 1);
+ if (err != 1)
+ pthread_exit(&err);
+
+ /* wait for parent's kick */
+ err = read(child->go[0], &c, 1);
+ if (err != 1)
+ pthread_exit(&err);
+
+ uprobe_multi_func_1();
+ uprobe_multi_func_2();
+ uprobe_multi_func_3();
+
+ err = 0;
+ pthread_exit(&err);
+}
+
+static struct child *spawn_thread(void)
+{
+ static struct child child;
+ int c, err;
+
+ /* pipe to notify child to execute the trigger functions */
+ if (pipe(child.go))
+ return NULL;
+ /* pipe to notify parent that child thread is ready */
+ if (pipe(child.c2p)) {
+ close(child.go[0]);
+ close(child.go[1]);
+ return NULL;
+ }
+
+ child.pid = getpid();
+
+ err = pthread_create(&child.thread, NULL, child_thread, &child);
+ if (err) {
+ err = -errno;
+ close(child.go[0]);
+ close(child.go[1]);
+ close(child.c2p[0]);
+ close(child.c2p[1]);
+ errno = -err;
+ return NULL;
+ }
+
+ err = read(child.c2p[0], &c, 1);
+ if (!ASSERT_EQ(err, 1, "child_thread_ready"))
+ return NULL;
+
+ return &child;
+}
+
static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child)
{
skel->bss->uprobe_multi_func_1_addr = (__u64) uprobe_multi_func_1;
@@ -103,15 +171,22 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child
* passed at the probe attach.
*/
skel->bss->pid = child ? 0 : getpid();
+ skel->bss->expect_pid = child ? child->pid : 0;
+
+ /* trigger all probes, if we are testing child *process*, just to make
+ * sure that PID filtering doesn't let through activations from wrong
+ * PIDs; when we test child *thread*, we don't want to do this to
+ * avoid double counting number of triggering events
+ */
+ if (!child || !child->thread) {
+ uprobe_multi_func_1();
+ uprobe_multi_func_2();
+ uprobe_multi_func_3();
+ }
if (child)
kick_child(child);
- /* trigger all probes */
- uprobe_multi_func_1();
- uprobe_multi_func_2();
- uprobe_multi_func_3();
-
/*
* There are 2 entry and 2 exit probe called for each uprobe_multi_func_[123]
* function and each slepable probe (6) increments uprobe_multi_sleep_result.
@@ -126,8 +201,12 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child
ASSERT_EQ(skel->bss->uprobe_multi_sleep_result, 6, "uprobe_multi_sleep_result");
- if (child)
+ ASSERT_FALSE(skel->bss->bad_pid_seen, "bad_pid_seen");
+
+ if (child) {
ASSERT_EQ(skel->bss->child_pid, child->pid, "uprobe_multi_child_pid");
+ ASSERT_EQ(skel->bss->child_tid, child->tid, "uprobe_multi_child_tid");
+ }
}
static void test_skel_api(void)
@@ -210,6 +289,13 @@ test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_multi
return;
__test_attach_api(binary, pattern, opts, child);
+
+ /* pid filter (thread) */
+ child = spawn_thread();
+ if (!ASSERT_OK_PTR(child, "spawn_thread"))
+ return;
+
+ __test_attach_api(binary, pattern, opts, child);
}
static void test_attach_api_pattern(void)
@@ -495,6 +581,13 @@ static void test_link_api(void)
return;
__test_link_api(child);
+
+ /* pid filter (thread) */
+ child = spawn_thread();
+ if (!ASSERT_OK_PTR(child, "spawn_thread"))
+ return;
+
+ __test_link_api(child);
}
static void test_bench_attach_uprobe(void)
diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c
index 419d9aa28fce..86a7ff5d3726 100644
--- a/tools/testing/selftests/bpf/progs/uprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c
@@ -22,6 +22,10 @@ __u64 uprobe_multi_sleep_result = 0;
int pid = 0;
int child_pid = 0;
+int child_tid = 0;
+
+int expect_pid = 0;
+bool bad_pid_seen = false;
bool test_cookie = false;
void *user_ptr = 0;
@@ -36,11 +40,19 @@ static __always_inline bool verify_sleepable_user_copy(void)
static void uprobe_multi_check(void *ctx, bool is_return, bool is_sleep)
{
- child_pid = bpf_get_current_pid_tgid() >> 32;
+ __u64 cur_pid_tgid = bpf_get_current_pid_tgid();
+ __u32 cur_pid;
- if (pid && child_pid != pid)
+ cur_pid = cur_pid_tgid >> 32;
+ if (pid && cur_pid != pid)
return;
+ if (expect_pid && cur_pid != expect_pid)
+ bad_pid_seen = true;
+
+ child_pid = cur_pid_tgid >> 32;
+ child_tid = (__u32)cur_pid_tgid;
+
__u64 cookie = test_cookie ? bpf_get_attach_cookie(ctx) : 0;
__u64 addr = bpf_get_func_ip(ctx);
@@ -97,5 +109,6 @@ int uretprobe_sleep(struct pt_regs *ctx)
SEC("uprobe.multi//proc/self/exe:uprobe_multi_func_*")
int uprobe_extra(struct pt_regs *ctx)
{
+ /* we need this one just to mix PID-filtered and global uprobes */
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case
2024-05-21 16:34 ` [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case Andrii Nakryiko
@ 2024-05-22 7:46 ` Jiri Olsa
0 siblings, 0 replies; 8+ messages in thread
From: Jiri Olsa @ 2024-05-22 7:46 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team
On Tue, May 21, 2024 at 09:34:00AM -0700, Andrii Nakryiko wrote:
> Extend existing multi-uprobe tests to test that PID filtering works
> correctly. We already have child *process* tests, but we need also child
> *thread* tests. This patch adds spawn_thread() helper to start child
> thread, wait for it to be ready, and then instruct it to trigger desired
> uprobes.
>
> Additionally, we extend BPF-side code to track thread ID, not just
> process ID. Also we detect whether extraneous triggerings with
> unexpected process IDs happened, and validate that none of that happened
> in practice.
>
> These changes prove that fixed PID filtering logic for multi-uprobe
> works as expected. These tests fail on old kernels.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> .../bpf/prog_tests/uprobe_multi_test.c | 107 ++++++++++++++++--
> .../selftests/bpf/progs/uprobe_multi.c | 17 ++-
> 2 files changed, 115 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> index 38fda42fd70f..677232d31432 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> #include <unistd.h>
> +#include <pthread.h>
> #include <test_progs.h>
> #include "uprobe_multi.skel.h"
> #include "uprobe_multi_bench.skel.h"
> @@ -27,7 +28,10 @@ noinline void uprobe_multi_func_3(void)
>
> struct child {
> int go[2];
> + int c2p[2]; /* child -> parent channel */
> int pid;
> + int tid;
> + pthread_t thread;
> };
>
> static void release_child(struct child *child)
> @@ -38,6 +42,10 @@ static void release_child(struct child *child)
> return;
> close(child->go[1]);
> close(child->go[0]);
> + if (child->thread)
> + pthread_join(child->thread, NULL);
> + close(child->c2p[0]);
> + close(child->c2p[1]);
> if (child->pid > 0)
> waitpid(child->pid, &child_status, 0);
> }
> @@ -63,7 +71,7 @@ static struct child *spawn_child(void)
> if (pipe(child.go))
> return NULL;
>
> - child.pid = fork();
> + child.pid = child.tid = fork();
> if (child.pid < 0) {
> release_child(&child);
> errno = EINVAL;
> @@ -89,6 +97,66 @@ static struct child *spawn_child(void)
> return &child;
> }
>
> +static void *child_thread(void *ctx)
> +{
> + struct child *child = ctx;
> + int c = 0, err;
> +
> + child->tid = syscall(SYS_gettid);
> +
> + /* let parent know we are ready */
> + err = write(child->c2p[1], &c, 1);
> + if (err != 1)
> + pthread_exit(&err);
> +
> + /* wait for parent's kick */
> + err = read(child->go[0], &c, 1);
> + if (err != 1)
> + pthread_exit(&err);
> +
> + uprobe_multi_func_1();
> + uprobe_multi_func_2();
> + uprobe_multi_func_3();
> +
> + err = 0;
> + pthread_exit(&err);
> +}
> +
> +static struct child *spawn_thread(void)
> +{
> + static struct child child;
> + int c, err;
> +
> + /* pipe to notify child to execute the trigger functions */
> + if (pipe(child.go))
> + return NULL;
> + /* pipe to notify parent that child thread is ready */
> + if (pipe(child.c2p)) {
> + close(child.go[0]);
> + close(child.go[1]);
> + return NULL;
> + }
> +
> + child.pid = getpid();
> +
> + err = pthread_create(&child.thread, NULL, child_thread, &child);
> + if (err) {
> + err = -errno;
> + close(child.go[0]);
> + close(child.go[1]);
> + close(child.c2p[0]);
> + close(child.c2p[1]);
> + errno = -err;
> + return NULL;
> + }
> +
> + err = read(child.c2p[0], &c, 1);
> + if (!ASSERT_EQ(err, 1, "child_thread_ready"))
> + return NULL;
> +
> + return &child;
> +}
> +
> static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child)
> {
> skel->bss->uprobe_multi_func_1_addr = (__u64) uprobe_multi_func_1;
> @@ -103,15 +171,22 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child
> * passed at the probe attach.
> */
> skel->bss->pid = child ? 0 : getpid();
> + skel->bss->expect_pid = child ? child->pid : 0;
> +
> + /* trigger all probes, if we are testing child *process*, just to make
> + * sure that PID filtering doesn't let through activations from wrong
> + * PIDs; when we test child *thread*, we don't want to do this to
> + * avoid double counting number of triggering events
> + */
> + if (!child || !child->thread) {
> + uprobe_multi_func_1();
> + uprobe_multi_func_2();
> + uprobe_multi_func_3();
> + }
>
> if (child)
> kick_child(child);
>
> - /* trigger all probes */
> - uprobe_multi_func_1();
> - uprobe_multi_func_2();
> - uprobe_multi_func_3();
> -
> /*
> * There are 2 entry and 2 exit probe called for each uprobe_multi_func_[123]
> * function and each slepable probe (6) increments uprobe_multi_sleep_result.
> @@ -126,8 +201,12 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child
>
> ASSERT_EQ(skel->bss->uprobe_multi_sleep_result, 6, "uprobe_multi_sleep_result");
>
> - if (child)
> + ASSERT_FALSE(skel->bss->bad_pid_seen, "bad_pid_seen");
> +
> + if (child) {
> ASSERT_EQ(skel->bss->child_pid, child->pid, "uprobe_multi_child_pid");
> + ASSERT_EQ(skel->bss->child_tid, child->tid, "uprobe_multi_child_tid");
> + }
> }
>
> static void test_skel_api(void)
> @@ -210,6 +289,13 @@ test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_multi
> return;
>
> __test_attach_api(binary, pattern, opts, child);
> +
> + /* pid filter (thread) */
> + child = spawn_thread();
> + if (!ASSERT_OK_PTR(child, "spawn_thread"))
> + return;
> +
> + __test_attach_api(binary, pattern, opts, child);
> }
>
> static void test_attach_api_pattern(void)
> @@ -495,6 +581,13 @@ static void test_link_api(void)
> return;
>
> __test_link_api(child);
> +
> + /* pid filter (thread) */
> + child = spawn_thread();
> + if (!ASSERT_OK_PTR(child, "spawn_thread"))
> + return;
> +
> + __test_link_api(child);
> }
>
> static void test_bench_attach_uprobe(void)
> diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c
> index 419d9aa28fce..86a7ff5d3726 100644
> --- a/tools/testing/selftests/bpf/progs/uprobe_multi.c
> +++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c
> @@ -22,6 +22,10 @@ __u64 uprobe_multi_sleep_result = 0;
>
> int pid = 0;
> int child_pid = 0;
> +int child_tid = 0;
> +
> +int expect_pid = 0;
> +bool bad_pid_seen = false;
>
> bool test_cookie = false;
> void *user_ptr = 0;
> @@ -36,11 +40,19 @@ static __always_inline bool verify_sleepable_user_copy(void)
>
> static void uprobe_multi_check(void *ctx, bool is_return, bool is_sleep)
> {
> - child_pid = bpf_get_current_pid_tgid() >> 32;
> + __u64 cur_pid_tgid = bpf_get_current_pid_tgid();
> + __u32 cur_pid;
>
> - if (pid && child_pid != pid)
> + cur_pid = cur_pid_tgid >> 32;
> + if (pid && cur_pid != pid)
> return;
>
> + if (expect_pid && cur_pid != expect_pid)
> + bad_pid_seen = true;
> +
> + child_pid = cur_pid_tgid >> 32;
> + child_tid = (__u32)cur_pid_tgid;
> +
> __u64 cookie = test_cookie ? bpf_get_attach_cookie(ctx) : 0;
> __u64 addr = bpf_get_func_ip(ctx);
>
> @@ -97,5 +109,6 @@ int uretprobe_sleep(struct pt_regs *ctx)
> SEC("uprobe.multi//proc/self/exe:uprobe_multi_func_*")
> int uprobe_extra(struct pt_regs *ctx)
> {
> + /* we need this one just to mix PID-filtered and global uprobes */
> return 0;
> }
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
` (3 preceding siblings ...)
2024-05-21 16:34 ` [PATCH v2 bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case Andrii Nakryiko
@ 2024-05-21 16:34 ` Andrii Nakryiko
2024-05-25 17:50 ` [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic patchwork-bot+netdevbpf
5 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2024-05-21 16:34 UTC (permalink / raw)
To: bpf, ast, daniel, martin.lau; +Cc: andrii, kernel-team, Jiri Olsa
Validate libbpf's USDT-over-multi-uprobe logic by adding USDTs to
existing multi-uprobe tests. This checks correct libbpf fallback to
singular uprobes (when run on older kernels with buggy PID filtering).
We reuse already established child process and child thread testing
infrastructure, so additions are minimal. These test fail on either
older kernels or older version of libbpf that doesn't detect PID
filtering problems.
Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
.../bpf/prog_tests/uprobe_multi_test.c | 25 ++++++++++++++
.../selftests/bpf/progs/uprobe_multi.c | 33 +++++++++++++++++--
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index 677232d31432..bf6ca8e3eb13 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -8,6 +8,7 @@
#include "uprobe_multi_usdt.skel.h"
#include "bpf/libbpf_internal.h"
#include "testing_helpers.h"
+#include "../sdt.h"
static char test_data[] = "test_data";
@@ -26,6 +27,11 @@ noinline void uprobe_multi_func_3(void)
asm volatile ("");
}
+noinline void usdt_trigger(void)
+{
+ STAP_PROBE(test, pid_filter_usdt);
+}
+
struct child {
int go[2];
int c2p[2]; /* child -> parent channel */
@@ -90,6 +96,7 @@ static struct child *spawn_child(void)
uprobe_multi_func_1();
uprobe_multi_func_2();
uprobe_multi_func_3();
+ usdt_trigger();
exit(errno);
}
@@ -117,6 +124,7 @@ static void *child_thread(void *ctx)
uprobe_multi_func_1();
uprobe_multi_func_2();
uprobe_multi_func_3();
+ usdt_trigger();
err = 0;
pthread_exit(&err);
@@ -182,6 +190,7 @@ static void uprobe_multi_test_run(struct uprobe_multi *skel, struct child *child
uprobe_multi_func_1();
uprobe_multi_func_2();
uprobe_multi_func_3();
+ usdt_trigger();
}
if (child)
@@ -269,8 +278,24 @@ __test_attach_api(const char *binary, const char *pattern, struct bpf_uprobe_mul
if (!ASSERT_OK_PTR(skel->links.uprobe_extra, "bpf_program__attach_uprobe_multi"))
goto cleanup;
+ /* Attach (uprobe-backed) USDTs */
+ skel->links.usdt_pid = bpf_program__attach_usdt(skel->progs.usdt_pid, pid, binary,
+ "test", "pid_filter_usdt", NULL);
+ if (!ASSERT_OK_PTR(skel->links.usdt_pid, "attach_usdt_pid"))
+ goto cleanup;
+
+ skel->links.usdt_extra = bpf_program__attach_usdt(skel->progs.usdt_extra, -1, binary,
+ "test", "pid_filter_usdt", NULL);
+ if (!ASSERT_OK_PTR(skel->links.usdt_extra, "attach_usdt_extra"))
+ goto cleanup;
+
uprobe_multi_test_run(skel, child);
+ ASSERT_FALSE(skel->bss->bad_pid_seen_usdt, "bad_pid_seen_usdt");
+ if (child) {
+ ASSERT_EQ(skel->bss->child_pid_usdt, child->pid, "usdt_multi_child_pid");
+ ASSERT_EQ(skel->bss->child_tid_usdt, child->tid, "usdt_multi_child_tid");
+ }
cleanup:
uprobe_multi__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_multi.c b/tools/testing/selftests/bpf/progs/uprobe_multi.c
index 86a7ff5d3726..44190efcdba2 100644
--- a/tools/testing/selftests/bpf/progs/uprobe_multi.c
+++ b/tools/testing/selftests/bpf/progs/uprobe_multi.c
@@ -1,8 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/bpf.h>
+#include "vmlinux.h"
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
-#include <stdbool.h>
+#include <bpf/usdt.bpf.h>
char _license[] SEC("license") = "GPL";
@@ -23,9 +23,12 @@ __u64 uprobe_multi_sleep_result = 0;
int pid = 0;
int child_pid = 0;
int child_tid = 0;
+int child_pid_usdt = 0;
+int child_tid_usdt = 0;
int expect_pid = 0;
bool bad_pid_seen = false;
+bool bad_pid_seen_usdt = false;
bool test_cookie = false;
void *user_ptr = 0;
@@ -112,3 +115,29 @@ int uprobe_extra(struct pt_regs *ctx)
/* we need this one just to mix PID-filtered and global uprobes */
return 0;
}
+
+SEC("usdt")
+int usdt_pid(struct pt_regs *ctx)
+{
+ __u64 cur_pid_tgid = bpf_get_current_pid_tgid();
+ __u32 cur_pid;
+
+ cur_pid = cur_pid_tgid >> 32;
+ if (pid && cur_pid != pid)
+ return 0;
+
+ if (expect_pid && cur_pid != expect_pid)
+ bad_pid_seen_usdt = true;
+
+ child_pid_usdt = cur_pid_tgid >> 32;
+ child_tid_usdt = (__u32)cur_pid_tgid;
+
+ return 0;
+}
+
+SEC("usdt")
+int usdt_extra(struct pt_regs *ctx)
+{
+ /* we need this one just to mix PID-filtered and global USDT probes */
+ return 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic
2024-05-21 16:33 [PATCH v2 bpf 0/5] Fix BPF multi-uprobe PID filtering logic Andrii Nakryiko
` (4 preceding siblings ...)
2024-05-21 16:34 ` [PATCH v2 bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs Andrii Nakryiko
@ 2024-05-25 17:50 ` patchwork-bot+netdevbpf
5 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-05-25 17:50 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, ast, daniel, martin.lau, kernel-team
Hello:
This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Tue, 21 May 2024 09:33:56 -0700 you wrote:
> It turns out that current implementation of multi-uprobe PID filtering logic
> is broken. It filters by thread, while the promise is filtering by process.
> Patch #1 fixes the logic trivially. The rest is testing and mitigations that
> are necessary for libbpf to not break users of USDT programs.
>
> v1->v2:
> - fix selftest in last patch (CI);
> - use semicolon in patch #3 (Jiri).
>
> [...]
Here is the summary with links:
- [v2,bpf,1/5] bpf: fix multi-uprobe PID filtering logic
https://git.kernel.org/bpf/bpf/c/46ba0e49b642
- [v2,bpf,2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic
https://git.kernel.org/bpf/bpf/c/4a8f635a6054
- [v2,bpf,3/5] libbpf: detect broken PID filtering logic for multi-uprobe
https://git.kernel.org/bpf/bpf/c/04d939a2ab22
- [v2,bpf,4/5] selftests/bpf: extend multi-uprobe tests with child thread case
https://git.kernel.org/bpf/bpf/c/70342420a1cf
- [v2,bpf,5/5] selftests/bpf: extend multi-uprobe tests with USDTs
https://git.kernel.org/bpf/bpf/c/198034a87dfe
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread