Re: [PATCH v2 04/12] uprobes: revamp uprobe refcounting and lifetime management

Andrii Nakryiko
2024-07-09T20:59:46Z

On Tue, Jul 9, 2024 at 11:49 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 07/08, Andrii Nakryiko wrote:
> >
> > On Sun, Jul 7, 2024 at 7:48 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > And I forgot to mention...
> > >
> > > In any case __uprobe_unregister() can't ignore the error code from
> > > register_for_each_vma(). If it fails to restore the original insn,
> > > we should not remove this uprobe from uprobes_tree.
> > >
> > > Otherwise the next handle_swbp() will send SIGTRAP to the (no longer)
> > > probed application.
> >
> > Yep, that would be unfortunate (just like SIGILL sent when uretprobe
> > detects "improper" stack pointer progression, for example),
>
> In this case we a) assume that user-space tries to fool the kernel and

Well, it's a bad assumption. User space might just be using fibers and
managing its own stack. Not saying SIGILL is good, but it's part of
the uprobe system regardless.

> b) the kernel can't handle this case in any case, thus uprobe_warn().
>
> > but from
> > what I gather it's not really expected to fail on unregistration given
> > we successfully registered uprobe.
>
> Not really expected, and that is why the "TODO" comment in _unregister()
> was never implemented. Although the real reason is that we are lazy ;)

Worked fine for 10+ years, which says something ;)

>
> But register_for_each_vma(NULL) can fail. Say, simply because
> kmalloc(GFP_KERNEL) in build_map_info() can fail even if it "never" should.
> A lot of other reasons.
>
> > I guess it's a decision between
> > leaking memory with an uprobe stuck in the tree or killing process due
> > to some very rare (or buggy) condition?
>
> Yes. I think in this case it is better to leak uprobe than kill the
> no longer probed task.

Ok, I think it's not hard to keep uprobe around if
__uprobe_unregister() fails, should be an easy addition from what I
can see.

>
> Oleg.
>