Re: [PATCH v2 3/3] kernel/uprobes: Fix check for active uprobe

[Naveen N. Rao]

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 <anton@xxxxxxxxx>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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