Messages in this thread | | | Date | Fri, 15 Sep 2017 17:38:25 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe |
| |
Hi Naveen,
I forgot almost everything about this code, but at first glance this patch needs more comments and the changelog should be updated, at least. And in any case this needs more changes iirc.
On 09/14, Naveen N. Rao wrote: > > If we try to install a uprobe on a breakpoint instruction, we register the > probe, but refuse to install it. In this case, when the breakpoint hits, we > incorrectly assume that the probe hit and end up looping.
This looks confusing to me...
If we try to install a uprobe on a breakpoint instruction, uprobe_register() should fail and remove uprobe. The task which hits this uprobe will loop until this uprobe goes away, this is fine.
But there is another case and probably this is what you mean. uprobe_register() can do nothing except insert_uprobe() if nobody mmaps this binary, and after that uprobe_mmap()->prepare_uprobe() can fail if the original isns is int3. In this case this !UPROBE_COPY_INSN uprobe won't go away, and the task can loop until it is killed or uprobe_unregister().
Right?
Now. The real fix should kill UPROBE_COPY_INSN altogether and simply move prepare_uprobe() from install_breakpoint() into __uprobe_register(), before it does register_for_each_vma().
The only problem is that read_mapping_page() needs "struct file *" and nobody confirmed that read_mapping_page(data => NULL) is fine on all filesystems. I think it should be fine though, and perhaps we should finally do this change.
until then we can probably make the things a bit better, but
> Fix this by checking that the trap was actually installed in > find_active_uprobe(). > > Reported-by: Anton Blanchard <anton@samba.org> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > kernel/events/uprobes.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index e14eb0a6e4f3..599078e6a092 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1752,6 +1752,13 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp) > uprobe = find_uprobe(inode, offset); > } > > + /* Ensure that the breakpoint was actually installed */ > + if (uprobe) { > + smp_rmb(); /* pairs with wmb() in prepare_uprobe() */ > + if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags))) > + uprobe = NULL; > + }
I need to recall how this code works... but if we add this change, shouldn't we remove another similar UPROBE_COPY_INSN check in handle_swbp() and add more comments?
Oleg.
| |