On Tue, Sep 03, 2024 at 11:11:06AM -0700, Andrii Nakryiko wrote:
SNIP
> > 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).
>
> > 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
> >
> > This thread is almost 50 emails deep now, we should break out of it.
> > We can argue on your actual fixes. :)
> >
> > >
> > > 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
