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 1:21 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 08/30, Andrii Nakryiko wrote:
> >
>
> Andrii, let me reply to your email "out of order". First of all:
>
> > 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.
>
> OK, agreed.
>
> I'll probably write another email (too late for me today), but I agree
> that "avoid register_rwsem in handler_chain" is obviously a good goal,
> lets discuss the possible cleanups or even fixlets later, when this
> series is already applied.
>

Sounds good. It seems like I'll need another revision due to missing
include, so if there is any reasonably straightforward clean up we
should do, I can just incorporate that into my series.

>
>
> > On Fri, Aug 30, 2024 at 7:33 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > 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.
>
> IOW, uc->handler and uc->filter must be consistent. But the current API
> doesn't require this contract, so this patch adds a difference which I
> didn't notice when I reviewed this change.
>
> (In fact I noticed the difference, but I thought that it should be fine).
>
> > If it doesn't provide filter
> > callback, it doesn't care about PID filtering and thus can't and
> > shouldn't cause unregistration.
>
> At first glance I disagree, but see above.

I still think it's fine, tbh. Which uprobe user violates this contract
in the kernel? Maybe we should just fix that while at it? Because
otherwise we are allowing some frankly too-dynamic and unnecessarily
complicated behavior where we can dynamically unsubscribe without
actually having corresponding filter logic.

As I mentioned earlier, I actually considered calling filter
explicitly to enforce this contract, but then got concerned about
indirect callback overhead and dropped that idea.

>
> > > I think the fix is simple, plus we need to cleanup this logic anyway,
> > > I'll try to send some code on Monday.
>
> Damn I am stupid. Nothing new ;) The "simple" fix I had in mind can't work.
> But we can do other things which we can discuss later.
>

I actually don't see how anything reasonably simple and
straightforward (short of just taking register_rwsem) can work if we
want this weird out-of-sync filter and dynamic UPROBE_HANDLER_REMOVE
behavior to remain. But again, does anyone actually rely on that and
should it be even allowed?

> Oleg.
>