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