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

[Oleg Nesterov]

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@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?

Oleg.