Re: [PATCH v2] tracing/uprobe: Add missing PID filter for uretprobe

Tianyi Liu
2024-09-07T19:19:25Z

On Mon, Sep 06, 2024 at 18:43:00AM +0800, Jiri Olsa wrote:

SNIP

> > > ---------------------------------------------------------------------------
> > > Now lets return to BPF and this particular problem. I won't really argue
> > > with this patch, but
> > >
> > > - Please change the subject and update the changelog,
> > > "Fixes: c1ae5c75e103" and the whole reasoning is misleading
> > > and wrong, IMO.
> > >
> > > - This patch won't fix all problems because uprobe_perf_filter()
> > > filters by mm, not by pid. The changelog/patch assumes that it
> > > is a "PID filter", but it is not.
> > >
> > > See https://lore.kernel.org/linux-trace-kernel/20240825224018.GD3906@redhat.com/
> > > If the traced process does clone(CLONE_VM), bpftrace will hit the
> > > similar problem, with uprobe or uretprobe.
> > >
> > > - So I still think that the "right" fix should change the
> > > bpf_prog_run_array_uprobe() paths somehow, but I know nothing
> > > about bpf.
> >
> > I agree that this patch does not address the issue correctly.
> > The PID filter should be implemented within bpf_prog_run_array_uprobe,
> > or alternatively, bpf_prog_run_array_uprobe should be called after
> > perf_tp_event_match to reuse the filtering mechanism provided by perf.
> >
> > Also, uretprobe may need UPROBE_HANDLER_REMOVE, similar to uprobe.
> >
> > For now, please forget the original patch as we need a new solution ;)
>
> hi,
> any chance we could go with your fix until we find better solution?
>
> it's simple and it fixes most of the cases for return uprobe pid filter
> for events with bpf programs.. I know during the discussion we found
> that standard perf record path won't work if there's bpf program
> attached on the same event, but I think that likely it needs more
> changes and also it's not a common use case
>
> would you consider sending another version addressing Oleg's points
> for changelog above?

My pleasure, I'll resend the updated patch in a new thread.

Based on previous discussions, `uprobe_perf_filter` acts as a preliminary
filter that removes breakpoints when they are no longer needed.
More complex filtering mechanisms related to perf are implemented in
perf-specific paths.

From my understanding, the original patch attempted to partially implement
UPROBE_HANDLER_REMOVE (since it didn't actually remove the breakpoint but
only prevented it from entering the BPF-related code).
I'm trying to provide a complete implementation, i.e., removing the
breakpoint when `uprobe_perf_filter` returns false, similar to how uprobe
functions. However, this would require merging the following functions,
because they will almost be the same:

uprobe_perf_func / uretprobe_perf_func
uprobe_dispatcher / uretprobe_dispatcher
handler_chain / handle_uretprobe_chain

I'm not sure why the paths of uprobe and uretprobe are entirely different.
I suspect that uretprobe might have been implemented later than uprobe and
was only partially implemented.

Oleg, do you have more insights on this?
In your opinion, does uretprobe need UPROBE_HANDLER_REMOVE?
If so, is it a good idea to merge the paths of uprobe and uretprobe?

Regardless, if we only need a temporary and incomplete fix, I will modify
only the commit message according to Oleg's suggestions and resend it.

I'm aware that using `uprobe_perf_filter` in `uretprobe_perf_func` is not
the solution for BPF filtering. I'm just trying to alleviate the issue
in some simple cases.

Sorry for the late reply as I spent a long time looking at the details
discussed above.

Thanks,