On 2017/09/15 05:38PM, Oleg Nesterov wrote:
> Hi Naveen,
Hi Oleg,
>
> 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.
Sure, thanks for the review!
>
> 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?
You're right -- I should have elaborated.
>
>
> 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.
Sure. I will take a stab at this after these fixes.
>
>
> 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 <an...@samba.org>
> > Signed-off-by: Naveen N. Rao <naveen.n....@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?
Yes, you're right. The check in handle_swbp() won't be needed anymore. I
will make that change and re-spin.
Thanks,
Naveen
