Re: [PATCH v4 4/8] uprobes: travers uprobe's consumer list locklessly under SRCU protection

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Aug 30, 2024 at 7:33 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 08/30, Jiri Olsa wrote:
> >
> > with this change the probe will not get removed in the attached test,
> > it'll get 2 hits, without this change just 1 hit
>
> I don't understand the code in tools/...bpf../ at all, can't comment,
>
> > but I'm not sure it's a big problem, because seems like that's not the
> > intended way the removal should be used anyway, as explained by Oleg [1]
>
> It seems that I confused you again ;)
>
> No, I think you found a problem. UPROBE_HANDLER_REMOVE can be lost if
> uc->filter == NULL of if it returns true. See another reply I sent a
> minute ago.
>

For better or worse, but I think there is (or has to be) and implicit
contract that if uprobe (or uretprobe for that matter as well, but
that's a separate issue) handler can return UPROBE_HANDLER_REMOVE,
then it *has to* also provide filter. If it doesn't provide filter
callback, it doesn't care about PID filtering and thus can't and
shouldn't cause unregistration.

In ideal world, we wouldn't need handler to do the filtering, and
instead generic uprobe/uretprobe code would just call uc->filter to
know whether to trigger consumer or not. Unfortunately, that's a lot
of overhead due to indirect function call, especially with retpolines
and stuff like that.

So I think it's reasonable to have an (implicit, yeah...) contract
that whoever cares about UPROBE_HANDLER_REMOVE has to provide filter,
they go together.

Jiri, the fact that uprobe/uretprobe can cause detachment by returning
1 is a bug, we should not allow that. But that's a separate issue
which we can fix in bpf-next tree. Please send a patch.

> I think the fix is simple, plus we need to cleanup this logic anyway,
> I'll try to send some code on Monday.

Can we please let me land these patches first? It's been a while. I
don't think anything is really broken with the logic.

>
> Oleg.
>