[PATCH bpf 0/5] Fix BPF multi-uprobe PID filtering logic

[PATCH bpf 0/5] Fix BPF multi-uprobe PID filtering logic

[Andrii Nakryiko]

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.

Andrii Nakryiko (5):
  bpf: fix multi-uprobe PID filtering logic
  bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe
    attach logic
  libbpf: detect broken PID filtering logic for multi-uprobe
  selftests/bpf: extend multi-uprobe tests with child thread case
  selftests/bpf: extend multi-uprobe tests with USDTs

 kernel/trace/bpf_trace.c                      |  10 +-
 tools/lib/bpf/features.c                      |  31 ++++-
 .../bpf/prog_tests/uprobe_multi_test.c        | 131 ++++++++++++++++--
 .../selftests/bpf/progs/uprobe_multi.c        |  50 ++++++-
 4 files changed, 203 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH bpf 1/5] bpf: fix multi-uprobe PID filtering logic

[Andrii Nakryiko]

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.

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

Re: [PATCH bpf 1/5] bpf: fix multi-uprobe PID filtering logic

[Jiri Olsa]

On Mon, May 20, 2024 at 04:47:16PM -0700, Andrii Nakryiko wrote:
> 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.
> 
> 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)

argh.. I guess we don't use filtering or usdt ATM, so we did not catch
this, thanks for fixing this

>  		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);

agreed,

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

thanks,
jirka

>  		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
> 
> 

[PATCH bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic

[Andrii Nakryiko]

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 improvements and has no correctness implications.

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

Re: [PATCH bpf 2/5] bpf: remove unnecessary rcu_read_{lock,unlock}() in multi-uprobe attach logic

[Jiri Olsa]

On Mon, May 20, 2024 at 04:47:17PM -0700, Andrii Nakryiko wrote:
> 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 improvements and has no correctness implications.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

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

jirka

> ---
>  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
> 
> 

[PATCH bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe

[Andrii Nakryiko]

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.

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..cff8640ca66f 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

Re: [PATCH bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe

[Jiri Olsa]

On Mon, May 20, 2024 at 04:47:18PM -0700, Andrii Nakryiko wrote:
> 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.
> 
> 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..cff8640ca66f 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 */

                                       ^ s/,/;/

so this affects just USDT load/attach, you right?

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

thanks,
jirka


> +	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
> 
> 

Re: [PATCH bpf 3/5] libbpf: detect broken PID filtering logic for multi-uprobe

[Andrii Nakryiko]

On Tue, May 21, 2024 at 3:04 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 04:47:18PM -0700, Andrii Nakryiko wrote:
> > 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.
> >
> > 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..cff8640ca66f 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 */
>
>                                        ^ s/,/;/
>
> so this affects just USDT load/attach, you right?

good eye, fixing :)

and yes, for libbpf this affects only USDTs. If user uses multi-uprobe
directly through bpf_program__attach_uprobe_multi(), they will need to
do similar feature detection, if they care about PID filtering.

>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
>
> > +     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
> >
> >

[PATCH bpf 4/5] selftests/bpf: extend multi-uprobe tests with child thread case

[Andrii Nakryiko]

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

[PATCH bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs

[Andrii Nakryiko]

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.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/prog_tests/uprobe_multi_test.c        | 22 +++++++++++++
 .../selftests/bpf/progs/uprobe_multi.c        | 33 +++++++++++++++++--
 2 files changed, 53 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..85d46e568e90 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 */
@@ -269,8 +275,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

Re: [PATCH bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs

[Andrii Nakryiko]

On Mon, May 20, 2024 at 4:47 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  .../bpf/prog_tests/uprobe_multi_test.c        | 22 +++++++++++++
>  .../selftests/bpf/progs/uprobe_multi.c        | 33 +++++++++++++++++--
>  2 files changed, 53 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..85d46e568e90 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 */
> @@ -269,8 +275,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
>

I lost the following during the final rebase before submitting,
sigh... With the piece below tests are passing again:

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 85d46e568e90..bf6ca8e3eb13 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -96,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);
        }
@@ -123,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);
@@ -188,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)


I'll wait till tomorrow for any feedback and will post v2.

I'm also curious about logistics? Do we want to get everything through
the bpf tree? Or bpf-next? Or split somehow? Thoughts?

I think the fix in patch #1 is important enough to backport to stable
kernels (multi-uprobes went into upstream v6.6 kernel, FYI).

Re: [PATCH bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs

[Alexei Starovoitov]

On Mon, May 20, 2024 at 9:54 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 4:47 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 22 +++++++++++++
> >  .../selftests/bpf/progs/uprobe_multi.c        | 33 +++++++++++++++++--
> >  2 files changed, 53 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..85d46e568e90 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 */
> > @@ -269,8 +275,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
> >
>
> I lost the following during the final rebase before submitting,
> sigh... With the piece below tests are passing again:
>
> 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 85d46e568e90..bf6ca8e3eb13 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -96,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);
>         }
> @@ -123,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);
> @@ -188,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)
>
>
> I'll wait till tomorrow for any feedback and will post v2.

pls wait for review from Jiri.

> I'm also curious about logistics? Do we want to get everything through
> the bpf tree? Or bpf-next? Or split somehow? Thoughts?

I think the whole thing through bpf tree makes it the easiest for everyone.

Re: [PATCH bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs

[Jiri Olsa]

On Mon, May 20, 2024 at 09:54:40PM -0700, Andrii Nakryiko wrote:
> On Mon, May 20, 2024 at 4:47 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > 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.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  .../bpf/prog_tests/uprobe_multi_test.c        | 22 +++++++++++++
> >  .../selftests/bpf/progs/uprobe_multi.c        | 33 +++++++++++++++++--
> >  2 files changed, 53 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..85d46e568e90 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 */
> > @@ -269,8 +275,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
> >
> 
> I lost the following during the final rebase before submitting,
> sigh... With the piece below tests are passing again:
> 
> 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 85d46e568e90..bf6ca8e3eb13 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -96,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);
>         }
> @@ -123,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);
> @@ -188,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)
> 
> 
> I'll wait till tomorrow for any feedback and will post v2.

tests are passing for me with the changes above

> 
> I'm also curious about logistics? Do we want to get everything through
> the bpf tree? Or bpf-next? Or split somehow? Thoughts?
> 
> I think the fix in patch #1 is important enough to backport to stable
> kernels (multi-uprobes went into upstream v6.6 kernel, FYI).

agreed, perhaps also the patch #3 for libbpf detection?

jirka

Re: [PATCH bpf 5/5] selftests/bpf: extend multi-uprobe tests with USDTs

[Andrii Nakryiko]

On Tue, May 21, 2024 at 3:04 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Mon, May 20, 2024 at 09:54:40PM -0700, Andrii Nakryiko wrote:
> > On Mon, May 20, 2024 at 4:47 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> > >
> > > 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.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  .../bpf/prog_tests/uprobe_multi_test.c        | 22 +++++++++++++
> > >  .../selftests/bpf/progs/uprobe_multi.c        | 33 +++++++++++++++++--
> > >  2 files changed, 53 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..85d46e568e90 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 */
> > > @@ -269,8 +275,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
> > >
> >
> > I lost the following during the final rebase before submitting,
> > sigh... With the piece below tests are passing again:
> >
> > 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 85d46e568e90..bf6ca8e3eb13 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -96,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);
> >         }
> > @@ -123,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);
> > @@ -188,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)
> >
> >
> > I'll wait till tomorrow for any feedback and will post v2.
>
> tests are passing for me with the changes above
>
> >
> > I'm also curious about logistics? Do we want to get everything through
> > the bpf tree? Or bpf-next? Or split somehow? Thoughts?
> >
> > I think the fix in patch #1 is important enough to backport to stable
> > kernels (multi-uprobes went into upstream v6.6 kernel, FYI).
>
> agreed, perhaps also the patch #3 for libbpf detection?

no one should be building libbpf from kernel sources, Github is the
authoritative source code for packaging. So I think it's not necessary
to backport libbpf. Tests will keep passing with just patch #1.

>
> jirka