Re: [PATCH 1/3] uprobes: install_breakpoint() should fail ifis_swbp_insn() == T

[Peter Zijlstra]

On Wed, 2012-05-30 at 18:58 +0200, Oleg Nesterov wrote:
> install_breakpoint() returns -EEXIST if is_swbp_insn(orig_insn) == T,
> the caller treats this code as success.
> 
> This is doubly wrong. The successful return should set UPROBE_COPY_INSN,
> but the real problem is that it shouldn't succeed. If the probed insn is
> int3 the application should get SIGTRAP, this won't happen with uprobe.
> 
> Probably we can fix this, we can add the UPROBE_SHARED_BP flag and teach
> handle_swbp/set_orig_insn to handle this case correctly. But this needs
> some complications and we have other insns which can't be probed, lets
> make a simple fix for now.
> 
> I think this needs a cleanup. UPROBE_COPY_INSN should die, copy_insn()
> should be called by alloc_uprobe(). arch_uprobe_analyze_insn() depends
> on ->mm (ia32_compat) but it is called only once.
> 
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  kernel/events/uprobes.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 8c5e043..1593b43 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -704,7 +704,7 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			return ret;
>  
>  		if (is_swbp_insn((uprobe_opcode_t *)uprobe->arch.insn))
> -			return -EEXIST;
> +			return -ENOTSUPP;
>  
>  		ret = arch_uprobe_analyze_insn(&uprobe->arch, mm);
>  		if (ret)

IIRC this -EEXIST existed because the vma iteration it does is racy and
one can encounter the same vma twice or so. See the special -EEXIST
handling in register_for_each_vma().

Changing it like this would break stuff. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/