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

[Jiri Olsa <olsajiri@gmail.com>]

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