On Tue, Sep 03, 2024 at 11:11:06AM -0700, Andrii Nakryiko wrote:

SNIP

quoted

Aren't we conflating two things here? Yes, from what Oleg explained,
it's clear that using task->mm is wrong. So that is what I feel is the
main issue. We shouldn't use task->mm at all, only task->signal should
be used instead. We should fix that (in bpf tree, please).

But I don't get the concern about linux->mm or linux->signal becoming

correction, we shouldn't worry about *linux->signal* becoming NULL.
linux->mm can become NULL, but we don't care about that (once we fix
filtering logic in multi-uprobe).

quoted

NULL because of a task existing. Look at put_task_struct(), it WILL
NOT call __put_task_struct() (which then calls put_signal_struct()),
so task->signal at least will be there and valid until multi-uprobe is
detached and we call put_task().

So. Can you please send fixes against the bpf tree, switching to
task->signal? And maybe also include the fix to prevent
UPROBE_HANDLER_REMOVE to be returned from the BPF program?

ok, it's uprobe-multi specific, let's discuss that over the change
itself, I'll try to send it soon

jirka

quoted

This thread is almost 50 emails deep now, we should break out of it.
We can argue on your actual fixes. :)

quoted

Oleg suggested change below (in addition to same_thread_group change)
to take that in account

jirka


---

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 98e395f1baae..9e6b390aa6da 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c

@@ -3235,9 +3235,23 @@ uprobe_multi_link_filter(struct uprobe_consumer *con, enum uprobe_filter_ctx ctx
                         struct mm_struct *mm)
 {
        struct bpf_uprobe *uprobe;
+       struct task_struct *task, *t;
+       bool ret = false;

        uprobe = container_of(con, struct bpf_uprobe, consumer);
-       return uprobe->link->task->mm == mm;
+       task = uprobe->link->task;
+
+       rcu_read_lock();
+       for_each_thread(task, t) {
+               struct mm_struct *mm = READ_ONCE(t->mm);
+               if (mm) {
+                       ret = t->mm == mm;
+                       break;
+               }
+       }
+       rcu_read_unlock();
+
+       return ret;
 }

 static int