On Tue, Jul 9, 2024 at 11:49 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > On 07/08, Andrii Nakryiko wrote: > > > > On Sun, Jul 7, 2024 at 7:48 AM Oleg Nesterov <oleg@xxxxxxxxxx> 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. >
