[v2,1/4] uprobes: add trap variant helper

From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Some architectures like powerpc have multiple variants of the trap
instruction. Introduce an additional helper is_trap_insn() for run-time
handling of non-uprobe traps on such architectures.

While there, change is_swbp_at_addr() to is_trap_at_addr() for reading
clarity.

With this change, the uprobe registration path will supercede any trap
instruction inserted at the requested location, while taking care of
delivering the SIGTRAP for cases where the trap notification came in
for an address without a uprobe. See [1] for a more detailed explanation.

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-March/104771.html

This change was suggested by Oleg Nesterov.

Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |   32 ++++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)

On 03/22, Ananth N Mavinakayanahalli wrote:
>
> Some architectures like powerpc have multiple variants of the trap
> instruction. Introduce an additional helper is_trap_insn() for run-time
> handling of non-uprobe traps on such architectures.

Looks fine to me.

I am going to take this into my tree, hopefully Ben won't object. The
main change is arch-agnostic and the changes in arch/powerpc are trivial.

Oleg.

* Ananth N Mavinakayanahalli <ananth@in.ibm.com> [2013-03-22 20:46:27]:

> From: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> 
> Some architectures like powerpc have multiple variants of the trap
> instruction. Introduce an additional helper is_trap_insn() for run-time
> handling of non-uprobe traps on such architectures.
> 
> While there, change is_swbp_at_addr() to is_trap_at_addr() for reading
> clarity.
> 
> With this change, the uprobe registration path will supercede any trap
> instruction inserted at the requested location, while taking care of
> delivering the SIGTRAP for cases where the trap notification came in
> for an address without a uprobe. See [1] for a more detailed explanation.
> 
> [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-March/104771.html
> 
> This change was suggested by Oleg Nesterov.
> 
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  include/linux/uprobes.h |    1 +
>  kernel/events/uprobes.c |   32 ++++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> Index: linux-3.9-rc3/include/linux/uprobes.h
> ===================================================================
> --- linux-3.9-rc3.orig/include/linux/uprobes.h
> +++ linux-3.9-rc3/include/linux/uprobes.h
> @@ -100,6 +100,7 @@ struct uprobes_state {
>  extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
> +extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
>  extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> Index: linux-3.9-rc3/kernel/events/uprobes.c
> ===================================================================
> --- linux-3.9-rc3.orig/kernel/events/uprobes.c
> +++ linux-3.9-rc3/kernel/events/uprobes.c
> @@ -173,6 +173,20 @@ bool __weak is_swbp_insn(uprobe_opcode_t
>  	return *insn == UPROBE_SWBP_INSN;
>  }
> 
> +/**
> + * is_trap_insn - check if instruction is breakpoint instruction.
> + * @insn: instruction to be checked.
> + * Default implementation of is_trap_insn
> + * Returns true if @insn is a breakpoint instruction.
> + *
> + * This function is needed for the case where an architecture has multiple
> + * trap instructions (like powerpc).
> + */
> +bool __weak is_trap_insn(uprobe_opcode_t *insn)
> +{
> +	return is_swbp_insn(insn);
> +}
> +
>  static void copy_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *opcode)
>  {
>  	void *kaddr = kmap_atomic(page);
> @@ -185,6 +199,15 @@ static int verify_opcode(struct page *pa
>  	uprobe_opcode_t old_opcode;
>  	bool is_swbp;
> 
> +	/*
> +	 * Note: We only check if the old_opcode is UPROBE_SWBP_INSN here.
> +	 * We do not check if it is any other 'trap variant' which could
> +	 * be conditional trap instruction such as the one powerpc supports.
> +	 *
> +	 * The logic is that we do not care if the underlying instruction
> +	 * is a trap variant; uprobes always wins over any other (gdb)
> +	 * breakpoint.
> +	 */
>  	copy_opcode(page, vaddr, &old_opcode);
>  	is_swbp = is_swbp_insn(&old_opcode);
> 
> @@ -204,7 +227,7 @@ static int verify_opcode(struct page *pa
>   * Expect the breakpoint instruction to be the smallest size instruction for
>   * the architecture. If an arch has variable length instruction and the
>   * breakpoint instruction is not of the smallest length instruction
> - * supported by that architecture then we need to modify is_swbp_at_addr and
> + * supported by that architecture then we need to modify is_trap_at_addr and
>   * write_opcode accordingly. This would never be a problem for archs that
>   * have fixed length instructions.
>   */
> @@ -1431,7 +1454,7 @@ static void mmf_recalc_uprobes(struct mm
>  	clear_bit(MMF_HAS_UPROBES, &mm->flags);
>  }
> 
> -static int is_swbp_at_addr(struct mm_struct *mm, unsigned long vaddr)
> +static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
>  {
>  	struct page *page;
>  	uprobe_opcode_t opcode;
> @@ -1452,7 +1475,8 @@ static int is_swbp_at_addr(struct mm_str
>  	copy_opcode(page, vaddr, &opcode);
>  	put_page(page);
>   out:
> -	return is_swbp_insn(&opcode);
> +	/* This needs to return true for any variant of the trap insn */
> +	return is_trap_insn(&opcode);
>  }
> 
>  static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> @@ -1472,7 +1496,7 @@ static struct uprobe *find_active_uprobe
>  		}
> 
>  		if (!uprobe)
> -			*is_swbp = is_swbp_at_addr(mm, bp_vaddr);
> +			*is_swbp = is_trap_at_addr(mm, bp_vaddr);
>  	} else {
>  		*is_swbp = -EFAULT;
>  	}