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

[Oleg Nesterov <oleg@redhat.com>]

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.

Oleg.