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

Andrii Nakryiko
2024-09-09T01:16:19Z

On Sun, Sep 8, 2024 at 6:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 09/08, Tianyi Liu wrote:
> >
> > On Mon, Sep 06, 2024 at 18:43:00AM +0800, Jiri Olsa wrote:
> >
> > > 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.
>
> Well. Not only. See the usage of consumer_filter() and filter_chain() in
> register_for_each_vma().
>
> > More complex filtering mechanisms related to perf are implemented in
> > perf-specific paths.
>
> The perf paths in __uprobe_perf_func() do the filtering based on
> perf_event->hw.target, that is all.
>
> But uprobe_perf_filter() or any other consumer->filter() simply can't rely
> on pid/task, it has to check ->mm.
>
> > 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).
>
> Confused...
>
> Your patch can help bpftrace although it (or any other change in
> trace_uprobe.c) can't not actually fix all the problems with bpf/filtering
> even if we forget about ret-probes.
>
> And I don't understand how this relates to UPROBE_HANDLER_REMOVE...
>
> > 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
>
> Sorry, I don't understand... Yes, uprobe_dispatcher and uretprobe_dispatcher
> can share more code or even unified, but
>
> > I suspect that uretprobe might have been implemented later than uprobe
>
> Yes,
>
> > and was only partially implemented.
>
> what do you mean?
>
> But whatever you meant, I agree that this code doesn't look pretty and can
> be cleanuped.
>
> > In your opinion, does uretprobe need UPROBE_HANDLER_REMOVE?
>
> Probably. But this has absolutely nothing to do with the filtering problem?
> Can we discuss this separately?
>
> > 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.
>
> Agreed.
>
> -------------------------------------------------------------------------------
> To summarise.
>
> This code is very old, and it was written for /usr/bin/perf which attaches
> to the tracepoint. So multiple instances of perf-record will share the same
> consumer/trace_event_call/filter. uretprobe_perf_func() doesn't call
> uprobe_perf_filter() because (if /usr/bin/perf is the only user) in the likely
> case it would burn CPU and return true. Quite possibly this design was not
> optimal from the very beginning, I simply can't recall why the is_ret_probe()
> consumer has ->handler != NULL, but it was not buggy.
>
> Now we have bpf, create_local_trace_uprobe(), etc. So lets add another
> uprobe_perf_filter() into uretprobe_perf_func() as your patch did.
>
> Then we can probably change uprobe_handle_trampoline() to do
> unapply_uprobe() if all the ret-handlers return UPROBE_HANDLER_REMOVE, like
> handler_chain() does.
>
> Then we can probably cleanup/simplify trace_uprobe.c, in partucular we can
> change alloc_trace_uprobe()
>
> - tu->consumer.handler = uprobe_dispatcher;
> - if (is_ret)
> - tu->consumer.ret_handler = uretprobe_dispatcher;
> + if (is_ret)
> + tu->consumer.ret_handler = uretprobe_dispatcher;
> + else
> + tu->consumer.handler = uprobe_dispatcher;
>
> and do more (including unrelated) cleanups.
>
> But lets do this step-by-step.
>
> And lets not mix the filtering issues with the UPROBE_HANDLER_REMOVE logic,
> to me this adds the unnecessary confusion.

+100 to all of the above. This has been a very long and winding
thread, but uretprobes are still being triggered unnecessarily :)
Let's fix the bleeding and talk and work on improving all of the other
issues in separate efforts.

>
> Oleg.
>