On Tue, Jul 9, 2024 at 11:49 AM Oleg Nesterov [off-list ref] wrote:

On 07/08, Andrii Nakryiko wrote:

quoted

On Sun, Jul 7, 2024 at 7:48 AM Oleg Nesterov [off-list ref] wrote:

quoted

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().

quoted

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.

quoted

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.