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

Commit Message

Andrii Nakryiko May 20, 2024, 11:47 p.m. UTC

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 <[e-mail]>
---
 kernel/trace/bpf_trace.c                                  | 8 ++++----
 .../testing/selftests/bpf/prog_tests/uprobe_multi_test.c  | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Jiri Olsa May 21, 2024, 10:04 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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 <[e-mail]>

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

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)