[PATCH 1/9] uprobes: move function declarations out of arch

[PATCH 1/9] uprobes: move function declarations out of arch

[Rabin Vincent]

It seems odd to keep the function declarations in the arch header where
they will need to be copy/pasted verbatim across arches.  Move them to
the common header.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/x86/include/asm/uprobes.h |    6 ------
 include/linux/uprobes.h        |    8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..b20b4d6 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -49,10 +49,4 @@ struct arch_uprobe_task {
 	unsigned int			saved_tf;
 };
 
-extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
-extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
-extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
-extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 #endif	/* _ASM_UPROBES_H */
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index e6f0331..ac90704 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -30,6 +30,7 @@
 struct vm_area_struct;
 struct mm_struct;
 struct inode;
+struct notifier_block;
 
 #ifdef CONFIG_ARCH_SUPPORTS_UPROBES
 # include <asm/uprobes.h>
@@ -120,6 +121,13 @@ extern void uprobe_notify_resume(struct pt_regs *regs);
 extern bool uprobe_deny_signal(void);
 extern bool __weak arch_uprobe_skip_sstep(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void uprobe_clear_state(struct mm_struct *mm);
+extern void uprobe_reset_state(struct mm_struct *mm);
+extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm,unsigned long addr);
+extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
-- 
1.7.9.5

[PATCH 1/9] uprobes: move function declarations out of arch

[Rabin Vincent]

It seems odd to keep the function declarations in the arch header where
they will need to be copy/pasted verbatim across arches.  Move them to
the common header.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/x86/include/asm/uprobes.h |    6 ------
 include/linux/uprobes.h        |    8 ++++++++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..b20b4d6 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -49,10 +49,4 @@ struct arch_uprobe_task {
 	unsigned int			saved_tf;
 };
 
-extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
-extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
-extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
-extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
-extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 #endif	/* _ASM_UPROBES_H */
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index e6f0331..ac90704 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -30,6 +30,7 @@
 struct vm_area_struct;
 struct mm_struct;
 struct inode;
+struct notifier_block;
 
 #ifdef CONFIG_ARCH_SUPPORTS_UPROBES
 # include <asm/uprobes.h>
@@ -120,6 +121,13 @@ extern void uprobe_notify_resume(struct pt_regs *regs);
 extern bool uprobe_deny_signal(void);
 extern bool __weak arch_uprobe_skip_sstep(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void uprobe_clear_state(struct mm_struct *mm);
+extern void uprobe_reset_state(struct mm_struct *mm);
+extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm,unsigned long addr);
+extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
+extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
+extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
-- 
1.7.9.5

[PATCH 2/9] uprobes: check for single step support

[Rabin Vincent]

Check for single step support before calling user_enable_single_step(),
since user_enable_single_step() just BUG()s if support does not exist.
Needed by ARM.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 kernel/events/uprobes.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 98256bc..db4e3ab 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1450,7 +1450,8 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
 {
-	user_enable_single_step(current);
+	if (arch_has_single_step())
+		user_enable_single_step(current);
 }
 
 void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
-- 
1.7.9.5

[PATCH 2/9] uprobes: check for single step support

[Rabin Vincent]

Check for single step support before calling user_enable_single_step(),
since user_enable_single_step() just BUG()s if support does not exist.
Needed by ARM.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 kernel/events/uprobes.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 98256bc..db4e3ab 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1450,7 +1450,8 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 
 void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
 {
-	user_enable_single_step(current);
+	if (arch_has_single_step())
+		user_enable_single_step(current);
 }
 
 void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
-- 
1.7.9.5

[PATCH 2/9] uprobes: check for single step support

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:06]:

> Check for single step support before calling user_enable_single_step(),
> since user_enable_single_step() just BUG()s if support does not exist.
> Needed by ARM.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  kernel/events/uprobes.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 98256bc..db4e3ab 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1450,7 +1450,8 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> 
>  void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
>  {
> -	user_enable_single_step(current);
> +	if (arch_has_single_step())
> +		user_enable_single_step(current);
>  }
> 
>  void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)

This change is fine. But I am wondering if should have a dummy
arch_uprobe_enable_step / arch_uprobe_disable_step in uprobes ARM.

If arch_uprobe_enable_step() wasnt a weak function, then the fix you
suggested would have been the only way to go.

Again, I am not against this change. But I am hoping that we get
feedback on which option is prefered, having this check or having a
dummy function in archs like ARM.

-- 
Thanks and Regards
Srikar

Re: [PATCH 2/9] uprobes: check for single step support

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:06]:

> Check for single step support before calling user_enable_single_step(),
> since user_enable_single_step() just BUG()s if support does not exist.
> Needed by ARM.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  kernel/events/uprobes.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 98256bc..db4e3ab 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1450,7 +1450,8 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> 
>  void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
>  {
> -	user_enable_single_step(current);
> +	if (arch_has_single_step())
> +		user_enable_single_step(current);
>  }
> 
>  void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)

This change is fine. But I am wondering if should have a dummy
arch_uprobe_enable_step / arch_uprobe_disable_step in uprobes ARM.

If arch_uprobe_enable_step() wasnt a weak function, then the fix you
suggested would have been the only way to go.

Again, I am not against this change. But I am hoping that we get
feedback on which option is prefered, having this check or having a
dummy function in archs like ARM.

-- 
Thanks and Regards
Srikar

[PATCH 2/9] uprobes: check for single step support

[Oleg Nesterov]

On 10/17, Srikar Dronamraju wrote:
>
> * Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:06]:
>
> >  void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
> >  {
> > -	user_enable_single_step(current);
> > +	if (arch_has_single_step())
> > +		user_enable_single_step(current);
> >  }
> >
> >  void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
>
> This change is fine. But I am wondering if should have a dummy
> arch_uprobe_enable_step / arch_uprobe_disable_step in uprobes ARM.

Or, better, we can kill it. We wertr going to do this anyway, we were
waiting for powerpc port.

Just I do not know how this change should be routed, it should update
both x86/powerpc.

Or do you think arch_uprobe_enable_step() still makes any sense?

Oleg.

Re: [PATCH 2/9] uprobes: check for single step support

[Oleg Nesterov]

On 10/17, Srikar Dronamraju wrote:
>
> * Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:06]:
>
> >  void __weak arch_uprobe_enable_step(struct arch_uprobe *arch)
> >  {
> > -	user_enable_single_step(current);
> > +	if (arch_has_single_step())
> > +		user_enable_single_step(current);
> >  }
> >
> >  void __weak arch_uprobe_disable_step(struct arch_uprobe *arch)
>
> This change is fine. But I am wondering if should have a dummy
> arch_uprobe_enable_step / arch_uprobe_disable_step in uprobes ARM.

Or, better, we can kill it. We wertr going to do this anyway, we were
waiting for powerpc port.

Just I do not know how this change should be routed, it should update
both x86/powerpc.

Or do you think arch_uprobe_enable_step() still makes any sense?

Oleg.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

Allow arches to decided to ignore a probe hit.  ARM will use this to
only call handlers if the conditions to execute a conditionally executed
instruction are satisfied.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index ac90704..da21b66 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -128,6 +128,7 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index db4e3ab..a0e1a38 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1419,6 +1419,11 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)
 	clear_bit(MMF_HAS_UPROBES, &mm->flags);
 }
 
+bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
+{
+	return false;
+}
+
 static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 {
 	struct mm_struct *mm = current->mm;
@@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
 	struct uprobe *uprobe;
 	unsigned long bp_vaddr;
 	int uninitialized_var(is_swbp);
+	bool ignored = false;
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
 	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
@@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
 			goto cleanup_ret;
 	}
 	utask->active_uprobe = uprobe;
+
+	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
+		ignored = true;
+		goto cleanup_ret;
+	}
+
 	handler_chain(uprobe, regs);
 	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
 		goto cleanup_ret;
@@ -1514,7 +1526,7 @@ cleanup_ret:
 		utask->active_uprobe = NULL;
 		utask->state = UTASK_RUNNING;
 	}
-	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
+	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
 
 		/*
 		 * cannot singlestep; cannot skip instruction;
-- 
1.7.9.5

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

Allow arches to decided to ignore a probe hit.  ARM will use this to
only call handlers if the conditions to execute a conditionally executed
instruction are satisfied.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index ac90704..da21b66 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -128,6 +128,7 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
+extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index db4e3ab..a0e1a38 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1419,6 +1419,11 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)
 	clear_bit(MMF_HAS_UPROBES, &mm->flags);
 }
 
+bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs)
+{
+	return false;
+}
+
 static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 {
 	struct mm_struct *mm = current->mm;
@@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
 	struct uprobe *uprobe;
 	unsigned long bp_vaddr;
 	int uninitialized_var(is_swbp);
+	bool ignored = false;
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
 	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
@@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
 			goto cleanup_ret;
 	}
 	utask->active_uprobe = uprobe;
+
+	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
+		ignored = true;
+		goto cleanup_ret;
+	}
+
 	handler_chain(uprobe, regs);
 	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
 		goto cleanup_ret;
@@ -1514,7 +1526,7 @@ cleanup_ret:
 		utask->active_uprobe = NULL;
 		utask->state = UTASK_RUNNING;
 	}
-	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
+	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
 
 		/*
 		 * cannot singlestep; cannot skip instruction;
-- 
1.7.9.5

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> Allow arches to decided to ignore a probe hit.  ARM will use this to
> only call handlers if the conditions to execute a conditionally executed
> instruction are satisfied.

Not sure I understand why we shouldn't call handlers in this case,
but OK, I know nothing about arm.

>  static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  {
>  	struct mm_struct *mm = current->mm;
> @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
>  	int uninitialized_var(is_swbp);
> +	bool ignored = false;
>  
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
>  			goto cleanup_ret;
>  	}
>  	utask->active_uprobe = uprobe;
> +
> +	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
> +		ignored = true;
> +		goto cleanup_ret;
> +	}
> +
>  	handler_chain(uprobe, regs);
>  	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
>  		goto cleanup_ret;
> @@ -1514,7 +1526,7 @@ cleanup_ret:
>  		utask->active_uprobe = NULL;
>  		utask->state = UTASK_RUNNING;
>  	}
> -	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> +	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
>  
>  		/*
>  		 * cannot singlestep; cannot skip instruction;

This conflicts with other changes in

	git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core

Could you re-diff?

I'll try to read 1-7 tomorrow.

Oleg.

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> Allow arches to decided to ignore a probe hit.  ARM will use this to
> only call handlers if the conditions to execute a conditionally executed
> instruction are satisfied.

Not sure I understand why we shouldn't call handlers in this case,
but OK, I know nothing about arm.

>  static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  {
>  	struct mm_struct *mm = current->mm;
> @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
>  	int uninitialized_var(is_swbp);
> +	bool ignored = false;
>  
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
>  			goto cleanup_ret;
>  	}
>  	utask->active_uprobe = uprobe;
> +
> +	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
> +		ignored = true;
> +		goto cleanup_ret;
> +	}
> +
>  	handler_chain(uprobe, regs);
>  	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
>  		goto cleanup_ret;
> @@ -1514,7 +1526,7 @@ cleanup_ret:
>  		utask->active_uprobe = NULL;
>  		utask->state = UTASK_RUNNING;
>  	}
> -	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> +	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
>  
>  		/*
>  		 * cannot singlestep; cannot skip instruction;

This conflicts with other changes in

	git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core

Could you re-diff?

I'll try to read 1-7 tomorrow.

Oleg.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> On 10/14, Rabin Vincent wrote:
>> Allow arches to decided to ignore a probe hit.  ARM will use this to
>> only call handlers if the conditions to execute a conditionally executed
>> instruction are satisfied.
>
> Not sure I understand why we shouldn't call handlers in this case,
> but OK, I know nothing about arm.

This old discussion about kprobes should be useful:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/046544.html

> This conflicts with other changes in
>
>         git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
>
> Could you re-diff?

OK, I will rebase them.

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> On 10/14, Rabin Vincent wrote:
>> Allow arches to decided to ignore a probe hit.  ARM will use this to
>> only call handlers if the conditions to execute a conditionally executed
>> instruction are satisfied.
>
> Not sure I understand why we shouldn't call handlers in this case,
> but OK, I know nothing about arm.

This old discussion about kprobes should be useful:

http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/046544.html

> This conflicts with other changes in
>
>         git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
>
> Could you re-diff?

OK, I will rebase them.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/16, Rabin Vincent wrote:
>
> 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> >
> > Not sure I understand why we shouldn't call handlers in this case,
> > but OK, I know nothing about arm.
>
> This old discussion about kprobes should be useful:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html

Thanks... Not sure I understand this discussion...

And, to clarify, I am not arguing. Just curious.

So, is this like cmov on x86? And this patch allows to not report if
the condition is not true? Or there are other issues on arm?

Oleg.

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/16, Rabin Vincent wrote:
>
> 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> >
> > Not sure I understand why we shouldn't call handlers in this case,
> > but OK, I know nothing about arm.
>
> This old discussion about kprobes should be useful:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html

Thanks... Not sure I understand this discussion...

And, to clarify, I am not arguing. Just curious.

So, is this like cmov on x86? And this patch allows to not report if
the condition is not true? Or there are other issues on arm?

Oleg.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote:
> On 10/16, Rabin Vincent wrote:
> > 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > > Not sure I understand why we shouldn't call handlers in this case,
> > > but OK, I know nothing about arm.
> >
> > This old discussion about kprobes should be useful:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html
> 
> Thanks... Not sure I understand this discussion...
> 
> And, to clarify, I am not arguing. Just curious.
> 
> So, is this like cmov on x86? And this patch allows to not report if
> the condition is not true? Or there are other issues on arm?

Yes, I guess this is like CMOV on x86.  In the ARM instruction set most
instructions can be conditionally executed.

In order to set the probe on a conditional instruction, we use an
undefined instruction with the same condition as the instruction we
replace.  However, it is implementation defined whether an undefined
instruction with a failing condition code will trigger an undefined
instruction exception or just be executed as a NOP.  So for those
processor implementations where we do get the undefined instruction
exception even for a failing condition code, we have to ignore it in
order to provide consistent behaviour.

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Rabin Vincent]

On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote:
> On 10/16, Rabin Vincent wrote:
> > 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > > Not sure I understand why we shouldn't call handlers in this case,
> > > but OK, I know nothing about arm.
> >
> > This old discussion about kprobes should be useful:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-March/045755.html
> 
> Thanks... Not sure I understand this discussion...
> 
> And, to clarify, I am not arguing. Just curious.
> 
> So, is this like cmov on x86? And this patch allows to not report if
> the condition is not true? Or there are other issues on arm?

Yes, I guess this is like CMOV on x86.  In the ARM instruction set most
instructions can be conditionally executed.

In order to set the probe on a conditional instruction, we use an
undefined instruction with the same condition as the instruction we
replace.  However, it is implementation defined whether an undefined
instruction with a failing condition code will trigger an undefined
instruction exception or just be executed as a NOP.  So for those
processor implementations where we do get the undefined instruction
exception even for a failing condition code, we have to ignore it in
order to provide consistent behaviour.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/21, Rabin Vincent wrote:
>
> On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote:
> >
> > And, to clarify, I am not arguing. Just curious.
> >
> > So, is this like cmov on x86? And this patch allows to not report if
> > the condition is not true? Or there are other issues on arm?
>
> Yes, I guess this is like CMOV on x86.  In the ARM instruction set most
> instructions can be conditionally executed.
>
> In order to set the probe on a conditional instruction, we use an
> undefined instruction with the same condition as the instruction we
> replace.  However, it is implementation defined whether an undefined
> instruction with a failing condition code will trigger an undefined
> instruction exception or just be executed as a NOP.  So for those
> processor implementations where we do get the undefined instruction
> exception even for a failing condition code, we have to ignore it in
> order to provide consistent behaviour.

OK, I see, thanks for your explanation.

Oleg.

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Oleg Nesterov]

On 10/21, Rabin Vincent wrote:
>
> On Wed, Oct 17, 2012 at 07:35:10PM +0200, Oleg Nesterov wrote:
> >
> > And, to clarify, I am not arguing. Just curious.
> >
> > So, is this like cmov on x86? And this patch allows to not report if
> > the condition is not true? Or there are other issues on arm?
>
> Yes, I guess this is like CMOV on x86.  In the ARM instruction set most
> instructions can be conditionally executed.
>
> In order to set the probe on a conditional instruction, we use an
> undefined instruction with the same condition as the instruction we
> replace.  However, it is implementation defined whether an undefined
> instruction with a failing condition code will trigger an undefined
> instruction exception or just be executed as a NOP.  So for those
> processor implementations where we do get the undefined instruction
> exception even for a failing condition code, we have to ignore it in
> order to provide consistent behaviour.

OK, I see, thanks for your explanation.

Oleg.

[PATCH 3/9] uprobes: allow ignoring of probe hits

[Srikar Dronamraju]

>  static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  {
>  	struct mm_struct *mm = current->mm;
> @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
>  	int uninitialized_var(is_swbp);
> +	bool ignored = false;
> 
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
>  			goto cleanup_ret;
>  	}
>  	utask->active_uprobe = uprobe;
> +
> +	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
> +		ignored = true;
> +		goto cleanup_ret;
> +	}
> +

With Oleg's changes, this should become simple. Something like:

	if (arch_uprobe_ignore(&uprobe->arch, regs))
		goto out;

>  	handler_chain(uprobe, regs);
>  	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
>  		goto cleanup_ret;
> @@ -1514,7 +1526,7 @@ cleanup_ret:
>  		utask->active_uprobe = NULL;
>  		utask->state = UTASK_RUNNING;
>  	}
> -	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> +	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
> 
>  		/*
>  		 * cannot singlestep; cannot skip instruction;
> -- 
> 1.7.9.5
> 

Re: [PATCH 3/9] uprobes: allow ignoring of probe hits

[Srikar Dronamraju]

>  static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  {
>  	struct mm_struct *mm = current->mm;
> @@ -1469,6 +1474,7 @@ static void handle_swbp(struct pt_regs *regs)
>  	struct uprobe *uprobe;
>  	unsigned long bp_vaddr;
>  	int uninitialized_var(is_swbp);
> +	bool ignored = false;
> 
>  	bp_vaddr = uprobe_get_swbp_addr(regs);
>  	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> @@ -1499,6 +1505,12 @@ static void handle_swbp(struct pt_regs *regs)
>  			goto cleanup_ret;
>  	}
>  	utask->active_uprobe = uprobe;
> +
> +	if (arch_uprobe_ignore(&uprobe->arch, regs)) {
> +		ignored = true;
> +		goto cleanup_ret;
> +	}
> +

With Oleg's changes, this should become simple. Something like:

	if (arch_uprobe_ignore(&uprobe->arch, regs))
		goto out;

>  	handler_chain(uprobe, regs);
>  	if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
>  		goto cleanup_ret;
> @@ -1514,7 +1526,7 @@ cleanup_ret:
>  		utask->active_uprobe = NULL;
>  		utask->state = UTASK_RUNNING;
>  	}
> -	if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> +	if (!ignored && !(uprobe->flags & UPROBE_SKIP_SSTEP))
> 
>  		/*
>  		 * cannot singlestep; cannot skip instruction;
> -- 
> 1.7.9.5
> 

[PATCH 4/9] uprobes: allow arch access to xol slot

[Rabin Vincent]

Allow arches to customize how the instruction is filled into the xol
slot.  ARM will use this to insert an undefined instruction after the
real instruction in order to simulate a single step of the instruction
without hardware support.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |    7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index da21b66..b4380ad 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -129,6 +129,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
+extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a0e1a38..f7ff3a4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1211,6 +1211,11 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
 	return slot_addr;
 }
 
+void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
+{
+	memcpy(vaddr, auprobe->insn, MAX_UINSN_BYTES);
+}
+
 /*
  * xol_get_insn_slot - If was not allocated a slot, then
  * allocate a slot.
@@ -1240,7 +1245,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	current->utask->vaddr = slot_addr;
 	offset = current->utask->xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
-	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
+	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
 	kunmap_atomic(vaddr);
 
 	return current->utask->xol_vaddr;
-- 
1.7.9.5

[PATCH 4/9] uprobes: allow arch access to xol slot

[Rabin Vincent]

Allow arches to customize how the instruction is filled into the xol
slot.  ARM will use this to insert an undefined instruction after the
real instruction in order to simulate a single step of the instruction
without hardware support.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |    7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index da21b66..b4380ad 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -129,6 +129,7 @@ extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
+extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index a0e1a38..f7ff3a4 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1211,6 +1211,11 @@ static unsigned long xol_take_insn_slot(struct xol_area *area)
 	return slot_addr;
 }
 
+void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
+{
+	memcpy(vaddr, auprobe->insn, MAX_UINSN_BYTES);
+}
+
 /*
  * xol_get_insn_slot - If was not allocated a slot, then
  * allocate a slot.
@@ -1240,7 +1245,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	current->utask->vaddr = slot_addr;
 	offset = current->utask->xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
-	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
+	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
 	kunmap_atomic(vaddr);
 
 	return current->utask->xol_vaddr;
-- 
1.7.9.5

[PATCH 4/9] uprobes: allow arch access to xol slot

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:08]:

> Allow arches to customize how the instruction is filled into the xol
> slot.  ARM will use this to insert an undefined instruction after the
> real instruction in order to simulate a single step of the instruction
> without hardware support.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---

The rest of the patches 4,5,6,7 look good. 

-- 
Thanks and Regards
Srikar

Re: [PATCH 4/9] uprobes: allow arch access to xol slot

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:08]:

> Allow arches to customize how the instruction is filled into the xol
> slot.  ARM will use this to insert an undefined instruction after the
> real instruction in order to simulate a single step of the instruction
> without hardware support.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---

The rest of the patches 4,5,6,7 look good. 

-- 
Thanks and Regards
Srikar

[PATCH 5/9] uprobes: allow arch-specific initialization

[Rabin Vincent]

Add a weak function for any architecture-specific initialization.  ARM
will use this to register the handlers for the undefined instructions it
uses to implement uprobes.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |   10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b4380ad..c3dc5de 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -130,6 +130,7 @@ extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned l
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
+extern int __weak arch_uprobes_init(void);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f7ff3a4..ca000a9 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1634,8 +1634,14 @@ static struct notifier_block uprobe_exception_nb = {
 	.priority		= INT_MAX-1,	/* notified after kprobes, kgdb */
 };
 
+int __weak __init arch_uprobes_init(void)
+{
+	return 0;
+}
+
 static int __init init_uprobes(void)
 {
+	int ret;
 	int i;
 
 	for (i = 0; i < UPROBES_HASH_SZ; i++) {
@@ -1643,6 +1649,10 @@ static int __init init_uprobes(void)
 		mutex_init(&uprobes_mmap_mutex[i]);
 	}
 
+	ret = arch_uprobes_init();
+	if (ret)
+		return ret;
+
 	return register_die_notifier(&uprobe_exception_nb);
 }
 module_init(init_uprobes);
-- 
1.7.9.5

[PATCH 5/9] uprobes: allow arch-specific initialization

[Rabin Vincent]

Add a weak function for any architecture-specific initialization.  ARM
will use this to register the handlers for the undefined instructions it
uses to implement uprobes.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    1 +
 kernel/events/uprobes.c |   10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b4380ad..c3dc5de 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -130,6 +130,7 @@ extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned l
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
+extern int __weak arch_uprobes_init(void);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f7ff3a4..ca000a9 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1634,8 +1634,14 @@ static struct notifier_block uprobe_exception_nb = {
 	.priority		= INT_MAX-1,	/* notified after kprobes, kgdb */
 };
 
+int __weak __init arch_uprobes_init(void)
+{
+	return 0;
+}
+
 static int __init init_uprobes(void)
 {
+	int ret;
 	int i;
 
 	for (i = 0; i < UPROBES_HASH_SZ; i++) {
@@ -1643,6 +1649,10 @@ static int __init init_uprobes(void)
 		mutex_init(&uprobes_mmap_mutex[i]);
 	}
 
+	ret = arch_uprobes_init();
+	if (ret)
+		return ret;
+
 	return register_die_notifier(&uprobe_exception_nb);
 }
 module_init(init_uprobes);
-- 
1.7.9.5

[PATCH 5/9] uprobes: allow arch-specific initialization

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:09]:

> Add a weak function for any architecture-specific initialization.  ARM
> will use this to register the handlers for the undefined instructions it
> uses to implement uprobes.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  include/linux/uprobes.h |    1 +
>  kernel/events/uprobes.c |   10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index b4380ad..c3dc5de 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -130,6 +130,7 @@ extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned l
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
> +extern int __weak arch_uprobes_init(void);
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {
>  };
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f7ff3a4..ca000a9 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1634,8 +1634,14 @@ static struct notifier_block uprobe_exception_nb = {
>  	.priority		= INT_MAX-1,	/* notified after kprobes, kgdb */
>  };
> 
> +int __weak __init arch_uprobes_init(void)
> +{
> +	return 0;
> +}
> +
>  static int __init init_uprobes(void)
>  {
> +	int ret;
>  	int i;
> 
>  	for (i = 0; i < UPROBES_HASH_SZ; i++) {
> @@ -1643,6 +1649,10 @@ static int __init init_uprobes(void)
>  		mutex_init(&uprobes_mmap_mutex[i]);
>  	}
> 
> +	ret = arch_uprobes_init();
> +	if (ret)
> +		return ret;
> +
>  	return register_die_notifier(&uprobe_exception_nb);
>  }
>  module_init(init_uprobes);
> -- 

We should be able to move the register_die_notifier and
uprobe_exception_nb structure into the default arch_uprobes_init.

Right? 

-- 
thanks and regards
Srikar

Re: [PATCH 5/9] uprobes: allow arch-specific initialization

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:09]:

> Add a weak function for any architecture-specific initialization.  ARM
> will use this to register the handlers for the undefined instructions it
> uses to implement uprobes.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  include/linux/uprobes.h |    1 +
>  kernel/events/uprobes.c |   10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index b4380ad..c3dc5de 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -130,6 +130,7 @@ extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned l
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
>  extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
> +extern int __weak arch_uprobes_init(void);
>  #else /* !CONFIG_UPROBES */
>  struct uprobes_state {
>  };
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f7ff3a4..ca000a9 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1634,8 +1634,14 @@ static struct notifier_block uprobe_exception_nb = {
>  	.priority		= INT_MAX-1,	/* notified after kprobes, kgdb */
>  };
> 
> +int __weak __init arch_uprobes_init(void)
> +{
> +	return 0;
> +}
> +
>  static int __init init_uprobes(void)
>  {
> +	int ret;
>  	int i;
> 
>  	for (i = 0; i < UPROBES_HASH_SZ; i++) {
> @@ -1643,6 +1649,10 @@ static int __init init_uprobes(void)
>  		mutex_init(&uprobes_mmap_mutex[i]);
>  	}
> 
> +	ret = arch_uprobes_init();
> +	if (ret)
> +		return ret;
> +
>  	return register_die_notifier(&uprobe_exception_nb);
>  }
>  module_init(init_uprobes);
> -- 

We should be able to move the register_die_notifier and
uprobe_exception_nb structure into the default arch_uprobes_init.

Right? 

-- 
thanks and regards
Srikar

[PATCH 6/9] uprobes: flush cache after xol write

[Rabin Vincent]

Flush the cache so that the instructions written to the XOL area are
visible.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 kernel/events/uprobes.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ca000a9..8c52f93 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	offset = current->utask->xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
 	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
+	flush_dcache_page(area->page);
 	kunmap_atomic(vaddr);
 
 	return current->utask->xol_vaddr;
-- 
1.7.9.5

[PATCH 6/9] uprobes: flush cache after xol write

[Rabin Vincent]

Flush the cache so that the instructions written to the XOL area are
visible.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 kernel/events/uprobes.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ca000a9..8c52f93 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
 	offset = current->utask->xol_vaddr & ~PAGE_MASK;
 	vaddr = kmap_atomic(area->page);
 	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
+	flush_dcache_page(area->page);
 	kunmap_atomic(vaddr);
 
 	return current->utask->xol_vaddr;
-- 
1.7.9.5

[PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> Flush the cache so that the instructions written to the XOL area are
> visible.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  kernel/events/uprobes.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ca000a9..8c52f93 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>  	offset = current->utask->xol_vaddr & ~PAGE_MASK;
>  	vaddr = kmap_atomic(area->page);
>  	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> +	flush_dcache_page(area->page);
>  	kunmap_atomic(vaddr);

I agree... but why under kmap_atomic?

Oleg.

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> Flush the cache so that the instructions written to the XOL area are
> visible.
>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  kernel/events/uprobes.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ca000a9..8c52f93 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>  	offset = current->utask->xol_vaddr & ~PAGE_MASK;
>  	vaddr = kmap_atomic(area->page);
>  	arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> +	flush_dcache_page(area->page);
>  	kunmap_atomic(vaddr);

I agree... but why under kmap_atomic?

Oleg.

[PATCH 6/9] uprobes: flush cache after xol write

[Rabin Vincent]

2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> On 10/14, Rabin Vincent wrote:
>> Flush the cache so that the instructions written to the XOL area are
>> visible.
>>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>> ---
>>  kernel/events/uprobes.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index ca000a9..8c52f93 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
>>       vaddr = kmap_atomic(area->page);
>>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
>> +     flush_dcache_page(area->page);
>>       kunmap_atomic(vaddr);
>
> I agree... but why under kmap_atomic?

No real reason; I'll move it to after the unmap.

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Rabin Vincent]

2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> On 10/14, Rabin Vincent wrote:
>> Flush the cache so that the instructions written to the XOL area are
>> visible.
>>
>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>> ---
>>  kernel/events/uprobes.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index ca000a9..8c52f93 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
>>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
>>       vaddr = kmap_atomic(area->page);
>>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
>> +     flush_dcache_page(area->page);
>>       kunmap_atomic(vaddr);
>
> I agree... but why under kmap_atomic?

No real reason; I'll move it to after the unmap.

[PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/16, Rabin Vincent wrote:
>
> 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > On 10/14, Rabin Vincent wrote:
> >> Flush the cache so that the instructions written to the XOL area are
> >> visible.
> >>
> >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> >> ---
> >>  kernel/events/uprobes.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> >> index ca000a9..8c52f93 100644
> >> --- a/kernel/events/uprobes.c
> >> +++ b/kernel/events/uprobes.c
> >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> >>       vaddr = kmap_atomic(area->page);
> >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> >> +     flush_dcache_page(area->page);
> >>       kunmap_atomic(vaddr);
> >
> > I agree... but why under kmap_atomic?
>
> No real reason; I'll move it to after the unmap.

OK. I assume you will send v2.

But this patch looks like a bugfix, flush_dcache_page() is not a nop
on powerpc. So perhaps we should apply this fix right now?

OTOH, I do not understand this stuff, everything is nop on x86. And
when I look into Documentation/cachetlb.txt I am starting to think
that may be this needs flush_icache_user_range instead?

Rabin, Ananth could you clarify this?

Oleg.

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/16, Rabin Vincent wrote:
>
> 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > On 10/14, Rabin Vincent wrote:
> >> Flush the cache so that the instructions written to the XOL area are
> >> visible.
> >>
> >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> >> ---
> >>  kernel/events/uprobes.c |    1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> >> index ca000a9..8c52f93 100644
> >> --- a/kernel/events/uprobes.c
> >> +++ b/kernel/events/uprobes.c
> >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> >>       vaddr = kmap_atomic(area->page);
> >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> >> +     flush_dcache_page(area->page);
> >>       kunmap_atomic(vaddr);
> >
> > I agree... but why under kmap_atomic?
>
> No real reason; I'll move it to after the unmap.

OK. I assume you will send v2.

But this patch looks like a bugfix, flush_dcache_page() is not a nop
on powerpc. So perhaps we should apply this fix right now?

OTOH, I do not understand this stuff, everything is nop on x86. And
when I look into Documentation/cachetlb.txt I am starting to think
that may be this needs flush_icache_user_range instead?

Rabin, Ananth could you clarify this?

Oleg.

[PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> On 10/16, Rabin Vincent wrote:
> >
> > 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > > On 10/14, Rabin Vincent wrote:
> > >> Flush the cache so that the instructions written to the XOL area are
> > >> visible.
> > >>
> > >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> > >> ---
> > >>  kernel/events/uprobes.c |    1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > >> index ca000a9..8c52f93 100644
> > >> --- a/kernel/events/uprobes.c
> > >> +++ b/kernel/events/uprobes.c
> > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > >>       vaddr = kmap_atomic(area->page);
> > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > >> +     flush_dcache_page(area->page);
> > >>       kunmap_atomic(vaddr);
> > >
> > > I agree... but why under kmap_atomic?
> >
> > No real reason; I'll move it to after the unmap.
> 
> OK. I assume you will send v2.
> 
> But this patch looks like a bugfix, flush_dcache_page() is not a nop
> on powerpc. So perhaps we should apply this fix right now?

Starting Power5, all Power processers have coherent caches.

> OTOH, I do not understand this stuff, everything is nop on x86. And
> when I look into Documentation/cachetlb.txt I am starting to think
> that may be this needs flush_icache_user_range instead?
>
> Rabin, Ananth could you clarify this?

Yes. We need flush_icache_user_range(). Though for x86 its always been a
nop, one never knows if there is some Power4 or older machine out there
that is still being used. We are fine for Power5 and later.

Ananth

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> On 10/16, Rabin Vincent wrote:
> >
> > 2012/10/15 Oleg Nesterov <oleg@redhat.com>:
> > > On 10/14, Rabin Vincent wrote:
> > >> Flush the cache so that the instructions written to the XOL area are
> > >> visible.
> > >>
> > >> Signed-off-by: Rabin Vincent <rabin@rab.in>
> > >> ---
> > >>  kernel/events/uprobes.c |    1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > >> index ca000a9..8c52f93 100644
> > >> --- a/kernel/events/uprobes.c
> > >> +++ b/kernel/events/uprobes.c
> > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > >>       vaddr = kmap_atomic(area->page);
> > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > >> +     flush_dcache_page(area->page);
> > >>       kunmap_atomic(vaddr);
> > >
> > > I agree... but why under kmap_atomic?
> >
> > No real reason; I'll move it to after the unmap.
> 
> OK. I assume you will send v2.
> 
> But this patch looks like a bugfix, flush_dcache_page() is not a nop
> on powerpc. So perhaps we should apply this fix right now?

Starting Power5, all Power processers have coherent caches.

> OTOH, I do not understand this stuff, everything is nop on x86. And
> when I look into Documentation/cachetlb.txt I am starting to think
> that may be this needs flush_icache_user_range instead?
>
> Rabin, Ananth could you clarify this?

Yes. We need flush_icache_user_range(). Though for x86 its always been a
nop, one never knows if there is some Power4 or older machine out there
that is still being used. We are fine for Power5 and later.

Ananth

[PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/26, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > On 10/16, Rabin Vincent wrote:
> > >
> > > >> --- a/kernel/events/uprobes.c
> > > >> +++ b/kernel/events/uprobes.c
> > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > >>       vaddr = kmap_atomic(area->page);
> > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > >> +     flush_dcache_page(area->page);
> > > >>       kunmap_atomic(vaddr);
> > > >
> > > > I agree... but why under kmap_atomic?
> > >
> > > No real reason; I'll move it to after the unmap.
> >
> > OK. I assume you will send v2.
> >
> > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > on powerpc. So perhaps we should apply this fix right now?
>
> Starting Power5, all Power processers have coherent caches.
>
> > OTOH, I do not understand this stuff, everything is nop on x86. And
> > when I look into Documentation/cachetlb.txt I am starting to think
> > that may be this needs flush_icache_user_range instead?
> >
> > Rabin, Ananth could you clarify this?
>
> Yes. We need flush_icache_user_range(). Though for x86 its always been a
> nop, one never knows if there is some Power4 or older machine out there
> that is still being used. We are fine for Power5 and later.

This is bad...

flush_icache_user needs vma. perhaps just to check VM_EXEC...

So let me repeat to be sure I really understand, do you confirm that
_in general_ flush_dcache_page() is not enough?

Oleg.

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/26, Ananth N Mavinakayanahalli wrote:
>
> On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > On 10/16, Rabin Vincent wrote:
> > >
> > > >> --- a/kernel/events/uprobes.c
> > > >> +++ b/kernel/events/uprobes.c
> > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > >>       vaddr = kmap_atomic(area->page);
> > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > >> +     flush_dcache_page(area->page);
> > > >>       kunmap_atomic(vaddr);
> > > >
> > > > I agree... but why under kmap_atomic?
> > >
> > > No real reason; I'll move it to after the unmap.
> >
> > OK. I assume you will send v2.
> >
> > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > on powerpc. So perhaps we should apply this fix right now?
>
> Starting Power5, all Power processers have coherent caches.
>
> > OTOH, I do not understand this stuff, everything is nop on x86. And
> > when I look into Documentation/cachetlb.txt I am starting to think
> > that may be this needs flush_icache_user_range instead?
> >
> > Rabin, Ananth could you clarify this?
>
> Yes. We need flush_icache_user_range(). Though for x86 its always been a
> nop, one never knows if there is some Power4 or older machine out there
> that is still being used. We are fine for Power5 and later.

This is bad...

flush_icache_user needs vma. perhaps just to check VM_EXEC...

So let me repeat to be sure I really understand, do you confirm that
_in general_ flush_dcache_page() is not enough?

Oleg.

[PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> On 10/26, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > > On 10/16, Rabin Vincent wrote:
> > > >
> > > > >> --- a/kernel/events/uprobes.c
> > > > >> +++ b/kernel/events/uprobes.c
> > > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > > >>       vaddr = kmap_atomic(area->page);
> > > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > > >> +     flush_dcache_page(area->page);
> > > > >>       kunmap_atomic(vaddr);
> > > > >
> > > > > I agree... but why under kmap_atomic?
> > > >
> > > > No real reason; I'll move it to after the unmap.
> > >
> > > OK. I assume you will send v2.
> > >
> > > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > > on powerpc. So perhaps we should apply this fix right now?
> >
> > Starting Power5, all Power processers have coherent caches.
> >
> > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > when I look into Documentation/cachetlb.txt I am starting to think
> > > that may be this needs flush_icache_user_range instead?
> > >
> > > Rabin, Ananth could you clarify this?
> >
> > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > nop, one never knows if there is some Power4 or older machine out there
> > that is still being used. We are fine for Power5 and later.
> 
> This is bad...
> 
> flush_icache_user needs vma. perhaps just to check VM_EXEC...
> 
> So let me repeat to be sure I really understand, do you confirm that
> _in general_ flush_dcache_page() is not enough?

flush_dcache_page() on powerpc already checks for
CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Ananth

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> On 10/26, Ananth N Mavinakayanahalli wrote:
> >
> > On Thu, Oct 25, 2012 at 04:58:39PM +0200, Oleg Nesterov wrote:
> > > On 10/16, Rabin Vincent wrote:
> > > >
> > > > >> --- a/kernel/events/uprobes.c
> > > > >> +++ b/kernel/events/uprobes.c
> > > > >> @@ -1246,6 +1246,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe, unsigned long slot
> > > > >>       offset = current->utask->xol_vaddr & ~PAGE_MASK;
> > > > >>       vaddr = kmap_atomic(area->page);
> > > > >>       arch_uprobe_xol_copy(&uprobe->arch, vaddr + offset);
> > > > >> +     flush_dcache_page(area->page);
> > > > >>       kunmap_atomic(vaddr);
> > > > >
> > > > > I agree... but why under kmap_atomic?
> > > >
> > > > No real reason; I'll move it to after the unmap.
> > >
> > > OK. I assume you will send v2.
> > >
> > > But this patch looks like a bugfix, flush_dcache_page() is not a nop
> > > on powerpc. So perhaps we should apply this fix right now?
> >
> > Starting Power5, all Power processers have coherent caches.
> >
> > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > when I look into Documentation/cachetlb.txt I am starting to think
> > > that may be this needs flush_icache_user_range instead?
> > >
> > > Rabin, Ananth could you clarify this?
> >
> > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > nop, one never knows if there is some Power4 or older machine out there
> > that is still being used. We are fine for Power5 and later.
> 
> This is bad...
> 
> flush_icache_user needs vma. perhaps just to check VM_EXEC...
> 
> So let me repeat to be sure I really understand, do you confirm that
> _in general_ flush_dcache_page() is not enough?

flush_dcache_page() on powerpc already checks for
CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Ananth

[PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/29, Ananth N Mavinakayanahalli wrote:
>
> On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > >
> > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > that may be this needs flush_icache_user_range instead?
> > > >
> > > > Rabin, Ananth could you clarify this?
> > >
> > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > nop, one never knows if there is some Power4 or older machine out there
> > > that is still being used. We are fine for Power5 and later.
> >
> > This is bad...
> >
> > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> >
> > So let me repeat to be sure I really understand, do you confirm that
> > _in general_ flush_dcache_page() is not enough?
>
> flush_dcache_page() on powerpc already checks for
> CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Thanks Ananth.

Still it is not clear to me if flush_dcache_page() would be always right
if we add the new port.

OK. So I assume we need the fix and I am going to apply the patch below.

Ananth, Rabin, will you ack it (including the comment I affed) ?

Oleg.

------------------------------------------------------------------------------
[PATCH] uprobes: flush cache after xol write

From: Rabin Vincent <rabin@rab.in>

Flush the cache so that the instructions written to the XOL area are
visible.

Signed-off-by: Rabin Vincent <rabin@rab.in>

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
 	vaddr = kmap_atomic(area->page);
 	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
 	kunmap_atomic(vaddr);
+	/*
+	 * We probably need flush_icache_user_range() but it needs vma.
+	 * This should work on supported architectures too.
+	 */
+	flush_dcache_page(area->page);
 
 	return current->utask->xol_vaddr;
 }

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 10/29, Ananth N Mavinakayanahalli wrote:
>
> On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > >
> > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > that may be this needs flush_icache_user_range instead?
> > > >
> > > > Rabin, Ananth could you clarify this?
> > >
> > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > nop, one never knows if there is some Power4 or older machine out there
> > > that is still being used. We are fine for Power5 and later.
> >
> > This is bad...
> >
> > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> >
> > So let me repeat to be sure I really understand, do you confirm that
> > _in general_ flush_dcache_page() is not enough?
>
> flush_dcache_page() on powerpc already checks for
> CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.

Thanks Ananth.

Still it is not clear to me if flush_dcache_page() would be always right
if we add the new port.

OK. So I assume we need the fix and I am going to apply the patch below.

Ananth, Rabin, will you ack it (including the comment I affed) ?

Oleg.

------------------------------------------------------------------------------
[PATCH] uprobes: flush cache after xol write

From: Rabin Vincent <rabin@rab.in>

Flush the cache so that the instructions written to the XOL area are
visible.

Signed-off-by: Rabin Vincent <rabin@rab.in>

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
 	vaddr = kmap_atomic(area->page);
 	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
 	kunmap_atomic(vaddr);
+	/*
+	 * We probably need flush_icache_user_range() but it needs vma.
+	 * This should work on supported architectures too.
+	 */
+	flush_dcache_page(area->page);
 
 	return current->utask->xol_vaddr;
 }

[PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:
> On 10/29, Ananth N Mavinakayanahalli wrote:
> >
> > On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > > >
> > > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > > that may be this needs flush_icache_user_range instead?
> > > > >
> > > > > Rabin, Ananth could you clarify this?
> > > >
> > > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > > nop, one never knows if there is some Power4 or older machine out there
> > > > that is still being used. We are fine for Power5 and later.
> > >
> > > This is bad...
> > >
> > > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> > >
> > > So let me repeat to be sure I really understand, do you confirm that
> > > _in general_ flush_dcache_page() is not enough?
> >
> > flush_dcache_page() on powerpc already checks for
> > CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.
> 
> Thanks Ananth.
> 
> Still it is not clear to me if flush_dcache_page() would be always right
> if we add the new port.
> 
> OK. So I assume we need the fix and I am going to apply the patch below.
> 
> Ananth, Rabin, will you ack it (including the comment I affed) ?
> 
> Oleg.
> 
> ------------------------------------------------------------------------------
> [PATCH] uprobes: flush cache after xol write
> 
> From: Rabin Vincent <rabin@rab.in>
> 
> Flush the cache so that the instructions written to the XOL area are
> visible.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

> 
> --- x/kernel/events/uprobes.c
> +++ x/kernel/events/uprobes.c
> @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
>  	vaddr = kmap_atomic(area->page);
>  	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
>  	kunmap_atomic(vaddr);
> +	/*
> +	 * We probably need flush_icache_user_range() but it needs vma.
> +	 * This should work on supported architectures too.
> +	 */
> +	flush_dcache_page(area->page);
> 
>  	return current->utask->xol_vaddr;
>  }

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Ananth N Mavinakayanahalli]

On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:
> On 10/29, Ananth N Mavinakayanahalli wrote:
> >
> > On Fri, Oct 26, 2012 at 06:39:51PM +0200, Oleg Nesterov wrote:
> > > >
> > > > > OTOH, I do not understand this stuff, everything is nop on x86. And
> > > > > when I look into Documentation/cachetlb.txt I am starting to think
> > > > > that may be this needs flush_icache_user_range instead?
> > > > >
> > > > > Rabin, Ananth could you clarify this?
> > > >
> > > > Yes. We need flush_icache_user_range(). Though for x86 its always been a
> > > > nop, one never knows if there is some Power4 or older machine out there
> > > > that is still being used. We are fine for Power5 and later.
> > >
> > > This is bad...
> > >
> > > flush_icache_user needs vma. perhaps just to check VM_EXEC...
> > >
> > > So let me repeat to be sure I really understand, do you confirm that
> > > _in general_ flush_dcache_page() is not enough?
> >
> > flush_dcache_page() on powerpc already checks for
> > CPU_FTR_COHERENT_ICACHE. So, yes, that is enough.
> 
> Thanks Ananth.
> 
> Still it is not clear to me if flush_dcache_page() would be always right
> if we add the new port.
> 
> OK. So I assume we need the fix and I am going to apply the patch below.
> 
> Ananth, Rabin, will you ack it (including the comment I affed) ?
> 
> Oleg.
> 
> ------------------------------------------------------------------------------
> [PATCH] uprobes: flush cache after xol write
> 
> From: Rabin Vincent <rabin@rab.in>
> 
> Flush the cache so that the instructions written to the XOL area are
> visible.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

> 
> --- x/kernel/events/uprobes.c
> +++ x/kernel/events/uprobes.c
> @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
>  	vaddr = kmap_atomic(area->page);
>  	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
>  	kunmap_atomic(vaddr);
> +	/*
> +	 * We probably need flush_icache_user_range() but it needs vma.
> +	 * This should work on supported architectures too.
> +	 */
> +	flush_dcache_page(area->page);
> 
>  	return current->utask->xol_vaddr;
>  }

[PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 11/04, Ananth N Mavinakayanahalli wrote:
>
> On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:
> >
> > [PATCH] uprobes: flush cache after xol write
> >
> > From: Rabin Vincent <rabin@rab.in>
> >
> > Flush the cache so that the instructions written to the XOL area are
> > visible.
> >
> > Signed-off-by: Rabin Vincent <rabin@rab.in>
>
> Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Thanks Ananth.

I assume that Srikar and Rabin agree, applied as 65b6ecc038

> >
> > --- x/kernel/events/uprobes.c
> > +++ x/kernel/events/uprobes.c
> > @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
> >  	vaddr = kmap_atomic(area->page);
> >  	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
> >  	kunmap_atomic(vaddr);
> > +	/*
> > +	 * We probably need flush_icache_user_range() but it needs vma.
> > +	 * This should work on supported architectures too.
> > +	 */
> > +	flush_dcache_page(area->page);
> >
> >  	return current->utask->xol_vaddr;
> >  }

Re: [PATCH 6/9] uprobes: flush cache after xol write

[Oleg Nesterov]

On 11/04, Ananth N Mavinakayanahalli wrote:
>
> On Sat, Nov 03, 2012 at 05:33:01PM +0100, Oleg Nesterov wrote:
> >
> > [PATCH] uprobes: flush cache after xol write
> >
> > From: Rabin Vincent <rabin@rab.in>
> >
> > Flush the cache so that the instructions written to the XOL area are
> > visible.
> >
> > Signed-off-by: Rabin Vincent <rabin@rab.in>
>
> Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Thanks Ananth.

I assume that Srikar and Rabin agree, applied as 65b6ecc038

> >
> > --- x/kernel/events/uprobes.c
> > +++ x/kernel/events/uprobes.c
> > @@ -1199,6 +1199,11 @@ static unsigned long xol_get_insn_slot(s
> >  	vaddr = kmap_atomic(area->page);
> >  	memcpy(vaddr + offset, uprobe->arch.insn, MAX_UINSN_BYTES);
> >  	kunmap_atomic(vaddr);
> > +	/*
> > +	 * We probably need flush_icache_user_range() but it needs vma.
> > +	 * This should work on supported architectures too.
> > +	 */
> > +	flush_dcache_page(area->page);
> >
> >  	return current->utask->xol_vaddr;
> >  }

[PATCH 7/9] uprobes: add arch write opcode hook

[Rabin Vincent]

Allow arches to write the opcode with a custom function.  ARM needs to
customize the swbp instruction depending on the condition code of the
instruction it replaces.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    3 +++
 kernel/events/uprobes.c |    8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index c3dc5de..35b9490 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -131,6 +131,9 @@ extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs)
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
 extern int __weak arch_uprobes_init(void);
+extern void __weak arch_uprobe_write_opcode(struct arch_uprobe *auprobe,
+					    void *vaddr,
+					    uprobe_opcode_t opcode);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 8c52f93..95ea618 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -203,6 +203,12 @@ bool __weak is_swbp_insn(uprobe_opcode_t *insn)
  * have fixed length instructions.
  */
 
+void __weak arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
+				     uprobe_opcode_t opcode)
+{
+	memcpy(vaddr, &opcode, UPROBE_SWBP_INSN_SIZE);
+}
+
 /*
  * write_opcode - write the opcode at a given virtual address.
  * @auprobe: arch breakpointing information.
@@ -242,7 +248,7 @@ retry:
 	vaddr_new = kmap_atomic(new_page);
 
 	memcpy(vaddr_new, vaddr_old, PAGE_SIZE);
-	memcpy(vaddr_new + (vaddr & ~PAGE_MASK), &opcode, UPROBE_SWBP_INSN_SIZE);
+	arch_uprobe_write_opcode(auprobe, vaddr_new + (vaddr & ~PAGE_MASK), opcode);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);
-- 
1.7.9.5

[PATCH 7/9] uprobes: add arch write opcode hook

[Rabin Vincent]

Allow arches to write the opcode with a custom function.  ARM needs to
customize the swbp instruction depending on the condition code of the
instruction it replaces.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/uprobes.h |    3 +++
 kernel/events/uprobes.c |    8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index c3dc5de..35b9490 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -131,6 +131,9 @@ extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs *regs)
 extern bool __weak arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void __weak arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr);
 extern int __weak arch_uprobes_init(void);
+extern void __weak arch_uprobe_write_opcode(struct arch_uprobe *auprobe,
+					    void *vaddr,
+					    uprobe_opcode_t opcode);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 8c52f93..95ea618 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -203,6 +203,12 @@ bool __weak is_swbp_insn(uprobe_opcode_t *insn)
  * have fixed length instructions.
  */
 
+void __weak arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
+				     uprobe_opcode_t opcode)
+{
+	memcpy(vaddr, &opcode, UPROBE_SWBP_INSN_SIZE);
+}
+
 /*
  * write_opcode - write the opcode at a given virtual address.
  * @auprobe: arch breakpointing information.
@@ -242,7 +248,7 @@ retry:
 	vaddr_new = kmap_atomic(new_page);
 
 	memcpy(vaddr_new, vaddr_old, PAGE_SIZE);
-	memcpy(vaddr_new + (vaddr & ~PAGE_MASK), &opcode, UPROBE_SWBP_INSN_SIZE);
+	arch_uprobe_write_opcode(auprobe, vaddr_new + (vaddr & ~PAGE_MASK), opcode);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);
-- 
1.7.9.5

[PATCH 8/9] ARM: support uprobe handling

[Rabin Vincent]

Extend the kprobes code to handle user-space probes.  Much of the code
can be reused so currently the ARM uprobes code reuses the kprobes
structures.  The decode tables are reused, with the modification that
for those instruction that require custom decoding for uprobes, a new
element is added in the table to specify a custom decoder function.

Thumb is not handled.

Cc: Jon Medhurst <tixy@yxit.co.uk>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/include/asm/kprobes.h   |   17 +---
 arch/arm/include/asm/probes.h    |   23 +++++
 arch/arm/kernel/kprobes-arm.c    |   27 +++++-
 arch/arm/kernel/kprobes-common.c |   63 ++++++++++----
 arch/arm/kernel/kprobes-test.c   |   12 ++-
 arch/arm/kernel/kprobes-thumb.c  |   31 ++++---
 arch/arm/kernel/kprobes.c        |    2 +-
 arch/arm/kernel/kprobes.h        |   36 +++++---
 arch/arm/kernel/uprobes-arm.c    |  178 ++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uprobes.h        |   23 +++++
 10 files changed, 349 insertions(+), 63 deletions(-)
 create mode 100644 arch/arm/include/asm/probes.h
 create mode 100644 arch/arm/kernel/uprobes-arm.c
 create mode 100644 arch/arm/kernel/uprobes.h

diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
index f82ec22..53b1a80 100644
--- a/arch/arm/include/asm/kprobes.h
+++ b/arch/arm/include/asm/kprobes.h
@@ -27,22 +27,7 @@
 #define flush_insn_slot(p)		do { } while (0)
 #define kretprobe_blacklist_size	0
 
-typedef u32 kprobe_opcode_t;
-
-struct kprobe;
-typedef void (kprobe_insn_handler_t)(struct kprobe *, struct pt_regs *);
-typedef unsigned long (kprobe_check_cc)(unsigned long);
-typedef void (kprobe_insn_singlestep_t)(struct kprobe *, struct pt_regs *);
-typedef void (kprobe_insn_fn_t)(void);
-
-/* Architecture specific copy of original instruction. */
-struct arch_specific_insn {
-	kprobe_opcode_t			*insn;
-	kprobe_insn_handler_t		*insn_handler;
-	kprobe_check_cc			*insn_check_cc;
-	kprobe_insn_singlestep_t	*insn_singlestep;
-	kprobe_insn_fn_t		*insn_fn;
-};
+#include <asm/probes.h>
 
 struct prev_kprobe {
 	struct kprobe *kp;
diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
new file mode 100644
index 0000000..df46994
--- /dev/null
+++ b/arch/arm/include/asm/probes.h
@@ -0,0 +1,23 @@
+#ifndef _ASM_PROBES_H
+#define _ASM_PROBES_H
+
+#ifdef CONFIG_KPROBES
+typedef u32 kprobe_opcode_t;
+
+struct kprobe;
+typedef void (kprobe_insn_handler_t)(struct kprobe *, struct pt_regs *);
+typedef unsigned long (kprobe_check_cc)(unsigned long);
+typedef void (kprobe_insn_singlestep_t)(struct kprobe *, struct pt_regs *);
+typedef void (kprobe_insn_fn_t)(void);
+
+/* Architecture specific copy of original instruction. */
+struct arch_specific_insn {
+	kprobe_opcode_t			*insn;
+	kprobe_insn_handler_t		*insn_handler;
+	kprobe_check_cc			*insn_check_cc;
+	kprobe_insn_singlestep_t	*insn_singlestep;
+	kprobe_insn_fn_t		*insn_fn;
+};
+#endif
+
+#endif
diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
index 8a30c89..d9cf0e2 100644
--- a/arch/arm/kernel/kprobes-arm.c
+++ b/arch/arm/kernel/kprobes-arm.c
@@ -62,6 +62,7 @@
 #include <linux/kprobes.h>
 #include <linux/module.h>
 
+#include "uprobes.h"
 #include "kprobes.h"
 
 #define sign_extend(x, signbit) ((x) | (0 - ((x) & (1 << (signbit)))))
@@ -545,31 +546,37 @@ static const union decode_item arm_cccc_000x_____1xx1_table[] = {
 
 	/* LDRD (register)	cccc 000x x0x0 xxxx xxxx xxxx 1101 xxxx */
 	/* STRD (register)	cccc 000x x0x0 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000d0, 0x000000d0, emulate_ldrdstrd,
 						 REGS(NOPCWB, NOPCX, 0, 0, NOPC)),
 
 	/* LDRD (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1101 xxxx */
 	/* STRD (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000d0, 0x004000d0, emulate_ldrdstrd,
 						 REGS(NOPCWB, NOPCX, 0, 0, 0)),
 
 	/* STRH (register)	cccc 000x x0x0 xxxx xxxx xxxx 1011 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000f0, 0x000000b0, emulate_str,
 						 REGS(NOPCWB, NOPC, 0, 0, NOPC)),
 
 	/* LDRH (register)	cccc 000x x0x1 xxxx xxxx xxxx 1011 xxxx */
 	/* LDRSB (register)	cccc 000x x0x1 xxxx xxxx xxxx 1101 xxxx */
 	/* LDRSH (register)	cccc 000x x0x1 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e500090, 0x00100090, emulate_ldr,
 						 REGS(NOPCWB, NOPC, 0, 0, NOPC)),
 
 	/* STRH (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1011 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000f0, 0x004000b0, emulate_str,
 						 REGS(NOPCWB, NOPC, 0, 0, 0)),
 
 	/* LDRH (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1011 xxxx */
 	/* LDRSB (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1101 xxxx */
 	/* LDRSH (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e500090, 0x00500090, emulate_ldr,
 						 REGS(NOPCWB, NOPC, 0, 0, 0)),
 
@@ -589,11 +596,13 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* TEQ (register)	cccc 0001 0011 xxxx xxxx xxxx xxx0 xxxx */
 	/* CMP (register)	cccc 0001 0101 xxxx xxxx xxxx xxx0 xxxx */
 	/* CMN (register)	cccc 0001 0111 xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900010, 0x01100000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, 0, 0, ANY)),
 
 	/* MOV (register)	cccc 0001 101x xxxx xxxx xxxx xxx0 xxxx */
 	/* MVN (register)	cccc 0001 111x xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00010, 0x01a00000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, 0, 0, ANY)),
 
@@ -607,6 +616,7 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* RSC (register)	cccc 0000 111x xxxx xxxx xxxx xxx0 xxxx */
 	/* ORR (register)	cccc 0001 100x xxxx xxxx xxxx xxx0 xxxx */
 	/* BIC (register)	cccc 0001 110x xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000010, 0x00000000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, 0, 0, ANY)),
 
@@ -614,11 +624,13 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* TEQ (reg-shift reg)	cccc 0001 0011 xxxx xxxx xxxx 0xx1 xxxx */
 	/* CMP (reg-shift reg)	cccc 0001 0101 xxxx xxxx xxxx 0xx1 xxxx */
 	/* CMN (reg-shift reg)	cccc 0001 0111 xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900090, 0x01100010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, NOPC, 0, ANY)),
 
 	/* MOV (reg-shift reg)	cccc 0001 101x xxxx xxxx xxxx 0xx1 xxxx */
 	/* MVN (reg-shift reg)	cccc 0001 111x xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00090, 0x01a00010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, NOPC, 0, ANY)),
 
@@ -632,6 +644,7 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* RSC (reg-shift reg)	cccc 0000 111x xxxx xxxx xxxx 0xx1 xxxx */
 	/* ORR (reg-shift reg)	cccc 0001 100x xxxx xxxx xxxx 0xx1 xxxx */
 	/* BIC (reg-shift reg)	cccc 0001 110x xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000090, 0x00000010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, NOPC, 0, ANY)),
 
@@ -666,11 +679,13 @@ static const union decode_item arm_cccc_001x_table[] = {
 	/* TEQ (immediate)	cccc 0011 0011 xxxx xxxx xxxx xxxx xxxx */
 	/* CMP (immediate)	cccc 0011 0101 xxxx xxxx xxxx xxxx xxxx */
 	/* CMN (immediate)	cccc 0011 0111 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900000, 0x03100000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, 0, 0, 0)),
 
 	/* MOV (immediate)	cccc 0011 101x xxxx xxxx xxxx xxxx xxxx */
 	/* MVN (immediate)	cccc 0011 111x xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00000, 0x03a00000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, 0, 0, 0)),
 
@@ -684,6 +699,7 @@ static const union decode_item arm_cccc_001x_table[] = {
 	/* RSC (immediate)	cccc 0010 111x xxxx xxxx xxxx xxxx xxxx */
 	/* ORR (immediate)	cccc 0011 100x xxxx xxxx xxxx xxxx xxxx */
 	/* BIC (immediate)	cccc 0011 110x xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000000, 0x02000000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, 0, 0, 0)),
 
@@ -850,21 +866,25 @@ static const union decode_item arm_cccc_01xx_table[] = {
 
 	/* STR (immediate)	cccc 010x x0x0 xxxx xxxx xxxx xxxx xxxx */
 	/* STRB (immediate)	cccc 010x x1x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e100000, 0x04000000, emulate_str,
 						 REGS(NOPCWB, ANY, 0, 0, 0)),
 
 	/* LDR (immediate)	cccc 010x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* LDRB (immediate)	cccc 010x x1x1 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_ldr)
 	DECODE_EMULATEX	(0x0e100000, 0x04100000, emulate_ldr,
 						 REGS(NOPCWB, ANY, 0, 0, 0)),
 
 	/* STR (register)	cccc 011x x0x0 xxxx xxxx xxxx xxxx xxxx */
 	/* STRB (register)	cccc 011x x1x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e100000, 0x06000000, emulate_str,
 						 REGS(NOPCWB, ANY, 0, 0, NOPC)),
 
 	/* LDR (register)	cccc 011x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* LDRB (register)	cccc 011x x1x1 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_ldr)
 	DECODE_EMULATEX	(0x0e100000, 0x06100000, emulate_ldr,
 						 REGS(NOPCWB, ANY, 0, 0, NOPC)),
 
@@ -876,6 +896,7 @@ static const union decode_item arm_cccc_100x_table[] = {
 
 	/* LDM			cccc 100x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* STM			cccc 100x x0x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (uprobe_decode_ldmstm)
 	DECODE_CUSTOM	(0x0e400000, 0x08000000, kprobe_decode_ldmstm),
 
 	/* STM (user registers)	cccc 100x x1x0 xxxx xxxx xxxx xxxx xxxx */
@@ -997,9 +1018,11 @@ static void __kprobes arm_singlestep(struct kprobe *p, struct pt_regs *regs)
  *   should also be very rare.
  */
 enum kprobe_insn __kprobes
-arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       bool uprobe)
 {
 	asi->insn_singlestep = arm_singlestep;
 	asi->insn_check_cc = kprobe_condition_checks[insn>>28];
-	return kprobe_decode_insn(insn, asi, kprobe_decode_arm_table, false);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_arm_table, false,
+				  uprobe);
 }
diff --git a/arch/arm/kernel/kprobes-common.c b/arch/arm/kernel/kprobes-common.c
index 18a7628..f811bfb 100644
--- a/arch/arm/kernel/kprobes-common.c
+++ b/arch/arm/kernel/kprobes-common.c
@@ -277,7 +277,8 @@ emulate_ldm_r3_15(struct kprobe *p, struct pt_regs *regs)
 }
 
 enum kprobe_insn __kprobes
-kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		     void *d)
 {
 	kprobe_insn_handler_t *handler = 0;
 	unsigned reglist = insn & 0xffff;
@@ -389,7 +390,7 @@ set_emulated_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
  * non-zero value, the corresponding nibble in pinsn is validated and modified
  * according to the type.
  */
-static bool __kprobes decode_regs(kprobe_opcode_t* pinsn, u32 regs)
+static bool __kprobes decode_regs(kprobe_opcode_t *pinsn, u32 regs, bool modify)
 {
 	kprobe_opcode_t insn = *pinsn;
 	kprobe_opcode_t mask = 0xf; /* Start at least significant nibble */
@@ -450,12 +451,16 @@ static bool __kprobes decode_regs(kprobe_opcode_t* pinsn, u32 regs)
 			break;
 		}
 
-		/* Replace value of nibble with new register number... */
-		insn &= ~mask;
-		insn |= new_bits & mask;
+		if (modify) {
+			/* Replace value of nibble with new register number */
+			insn &= ~mask;
+			insn |= new_bits & mask;
+		}
 	}
 
-	*pinsn = insn;
+	if (modify)
+		*pinsn = insn;
+
 	return true;
 
 reject:
@@ -468,7 +473,8 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 	[DECODE_TYPE_SIMULATE]	= sizeof(struct decode_simulate),
 	[DECODE_TYPE_EMULATE]	= sizeof(struct decode_emulate),
 	[DECODE_TYPE_OR]	= sizeof(struct decode_or),
-	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject)
+	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject),
+	[UDECODE_TYPE_NEXT]	= sizeof(struct decode_header)
 };
 
 /*
@@ -516,13 +522,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
  */
 int __kprobes
 kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
-				const union decode_item *table, bool thumb)
+				const union decode_item *table, bool thumb,
+				bool uprobe)
 {
+	enum kprobe_insn (*decoder)(kprobe_opcode_t,
+			  struct arch_specific_insn *, void *d) = NULL;
 	const struct decode_header *h = (struct decode_header *)table;
 	const struct decode_header *next;
 	bool matched = false;
 
-	insn = prepare_emulated_insn(insn, asi, thumb);
+	if (!uprobe)
+		insn = prepare_emulated_insn(insn, asi, thumb);
 
 	for (;; h = next) {
 		enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK;
@@ -534,13 +544,24 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 		next = (struct decode_header *)
 				((uintptr_t)h + decode_struct_sizes[type]);
 
-		if (!matched && (insn & h->mask.bits) != h->value.bits)
+		if (!uprobe && type == UDECODE_TYPE_NEXT)
 			continue;
 
-		if (!decode_regs(&insn, regs))
-			return INSN_REJECTED;
+		if (type != UDECODE_TYPE_NEXT) {
+			if (!matched &&
+			    (insn & h->mask.bits) != h->value.bits) {
+				decoder = NULL;
+				continue;
+			}
+
+			if (!decode_regs(&insn, regs, !uprobe))
+				return INSN_REJECTED;
+		}
 
 		switch (type) {
+		case UDECODE_TYPE_NEXT:
+			decoder = h->mask.decoder;
+			break;
 
 		case DECODE_TYPE_TABLE: {
 			struct decode_table *d = (struct decode_table *)h;
@@ -550,7 +571,11 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 
 		case DECODE_TYPE_CUSTOM: {
 			struct decode_custom *d = (struct decode_custom *)h;
-			return (*d->decoder.decoder)(insn, asi);
+
+			if (decoder)
+				return decoder(insn, asi, d);
+
+			return (*d->decoder.decoder)(insn, asi, d);
 		}
 
 		case DECODE_TYPE_SIMULATE: {
@@ -561,8 +586,14 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 
 		case DECODE_TYPE_EMULATE: {
 			struct decode_emulate *d = (struct decode_emulate *)h;
-			asi->insn_handler = d->handler.handler;
-			set_emulated_insn(insn, asi, thumb);
+
+			if (decoder)
+				return decoder(insn, asi, d);
+
+			if (!uprobe) {
+				asi->insn_handler = d->handler.handler;
+				set_emulated_insn(insn, asi, thumb);
+			}
 			return INSN_GOOD;
 		}
 
@@ -574,5 +605,5 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 		default:
 			return INSN_REJECTED;
 		}
-		}
 	}
+}
diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index 1862d8f..a2d4213 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -630,7 +630,8 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 	[DECODE_TYPE_SIMULATE]	= sizeof(struct decode_simulate),
 	[DECODE_TYPE_EMULATE]	= sizeof(struct decode_emulate),
 	[DECODE_TYPE_OR]	= sizeof(struct decode_or),
-	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject)
+	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject),
+	[UDECODE_TYPE_NEXT]	= sizeof(struct decode_header)
 };
 
 static int table_iter(const union decode_item *table,
@@ -646,9 +647,11 @@ static int table_iter(const union decode_item *table,
 		if (type == DECODE_TYPE_END)
 			return 0;
 
-		result = fn(h, args);
-		if (result)
-			return result;
+		if (type != UDECODE_TYPE_NEXT) {
+			result = fn(h, args);
+			if (result)
+				return result;
+		}
 
 		h = (struct decode_header *)
 			((uintptr_t)h + decode_struct_sizes[type]);
@@ -918,6 +921,7 @@ static void coverage_add(kprobe_opcode_t insn)
 			break;
 
 		case DECODE_TYPE_REJECT:
+		case UDECODE_TYPE_NEXT:
 		default:
 			return;
 		}
diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
index 6123daf..761aa55 100644
--- a/arch/arm/kernel/kprobes-thumb.c
+++ b/arch/arm/kernel/kprobes-thumb.c
@@ -83,7 +83,8 @@ t32_simulate_cond_branch(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t32_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t32_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       void *d)
 {
 	int cc = (insn >> 22) & 0xf;
 	asi->insn_check_cc = kprobe_condition_checks[cc];
@@ -158,9 +159,10 @@ t32_simulate_ldr_literal(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t32_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t32_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		  void *d)
 {
-	enum kprobe_insn ret = kprobe_decode_ldmstm(insn, asi);
+	enum kprobe_insn ret = kprobe_decode_ldmstm(insn, asi, d);
 
 	/* Fixup modified instruction to have halfwords in correct order...*/
 	insn = asi->insn[0];
@@ -1046,7 +1048,7 @@ t16_singlestep_it(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_it(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_it(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	asi->insn_singlestep = t16_singlestep_it;
 	return INSN_GOOD_NO_SLOT;
@@ -1063,7 +1065,8 @@ t16_simulate_cond_branch(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       void *d)
 {
 	int cc = (insn >> 8) & 0xf;
 	asi->insn_check_cc = kprobe_condition_checks[cc];
@@ -1149,7 +1152,7 @@ t16_emulate_hiregs(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_hiregs(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_hiregs(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	insn &= ~0x00ff;
 	insn |= 0x001; /* Set Rdn = R1 and Rm = R0 */
@@ -1175,7 +1178,7 @@ t16_emulate_push(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_push(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_push(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	/*
 	 * To simulate a PUSH we use a Thumb-2 "STMDB R9!, {registers}"
@@ -1225,7 +1228,7 @@ t16_emulate_pop_pc(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_pop(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_pop(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	/*
 	 * To simulate a POP we use a Thumb-2 "LDMDB R9!, {registers}"
@@ -1453,17 +1456,21 @@ static void __kprobes thumb32_singlestep(struct kprobe *p, struct pt_regs *regs)
 }
 
 enum kprobe_insn __kprobes
-thumb16_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+thumb16_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+			   bool uprobes)
 {
 	asi->insn_singlestep = thumb16_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
-	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb16_table, true);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb16_table, true,
+				  uprobes);
 }
 
 enum kprobe_insn __kprobes
-thumb32_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+thumb32_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+			   bool uprobes)
 {
 	asi->insn_singlestep = thumb32_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
-	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb32_table, true);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb32_table, true,
+				  uprobes);
 }
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 4dd41fc..1b2a324 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -80,7 +80,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	p->opcode = insn;
 	p->ainsn.insn = tmp_insn;
 
-	switch ((*decode_insn)(insn, &p->ainsn)) {
+	switch ((*decode_insn)(insn, &p->ainsn, false)) {
 	case INSN_REJECTED:	/* not supported */
 		return -EINVAL;
 
diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h
index 38945f7..442a161 100644
--- a/arch/arm/kernel/kprobes.h
+++ b/arch/arm/kernel/kprobes.h
@@ -35,20 +35,19 @@ enum kprobe_insn {
 };
 
 typedef enum kprobe_insn (kprobe_decode_insn_t)(kprobe_opcode_t,
-						struct arch_specific_insn *);
-
-#ifdef CONFIG_THUMB2_KERNEL
+						struct arch_specific_insn *,
+						bool uprobes);
 
 enum kprobe_insn thumb16_kprobe_decode_insn(kprobe_opcode_t,
-						struct arch_specific_insn *);
+					    struct arch_specific_insn *,
+					    bool uprobes);
 enum kprobe_insn thumb32_kprobe_decode_insn(kprobe_opcode_t,
-						struct arch_specific_insn *);
-
-#else /* !CONFIG_THUMB2_KERNEL */
+					    struct arch_specific_insn *,
+					    bool uprobes);
 
 enum kprobe_insn arm_kprobe_decode_insn(kprobe_opcode_t,
-					struct arch_specific_insn *);
-#endif
+					struct arch_specific_insn *,
+					bool uprobes);
 
 void __init arm_kprobe_decode_init(void);
 
@@ -165,7 +164,8 @@ void __kprobes kprobe_simulate_nop(struct kprobe *p, struct pt_regs *regs);
 void __kprobes kprobe_emulate_none(struct kprobe *p, struct pt_regs *regs);
 
 enum kprobe_insn __kprobes
-kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi);
+kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		     void *d);
 
 /*
  * Test if load/store instructions writeback the address register.
@@ -292,6 +292,7 @@ enum decode_type {
 	DECODE_TYPE_EMULATE,
 	DECODE_TYPE_OR,
 	DECODE_TYPE_REJECT,
+	UDECODE_TYPE_NEXT,
 	NUM_DECODE_TYPES /* Must be last enum */
 };
 
@@ -331,7 +332,9 @@ union decode_item {
 	u32			bits;
 	const union decode_item	*table;
 	kprobe_insn_handler_t	*handler;
-	kprobe_decode_insn_t	*decoder;
+	enum kprobe_insn (*decoder)(kprobe_opcode_t,
+				    struct arch_specific_insn *,
+				    void *d);
 };
 
 
@@ -396,6 +399,14 @@ struct decode_emulate {
 #define DECODE_EMULATE(_mask, _value, _handler)		\
 	DECODE_EMULATEX(_mask, _value, _handler, 0)
 
+#ifdef CONFIG_UPROBES
+#define UDECODE_NEXT(_decoder)				\
+	{.bits = UDECODE_TYPE_NEXT },			\
+	{.decoder = _decoder},				\
+	{.bits = 0 },
+#else
+#define UDECODE_NEXT(_decoder)
+#endif
 
 struct decode_or {
 	struct decode_header	header;
@@ -422,7 +433,8 @@ extern const union decode_item kprobe_decode_arm_table[];
 
 
 int kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
-			const union decode_item *table, bool thumb16);
+			const union decode_item *table, bool thumb16,
+			bool uprobe);
 
 
 #endif /* _ARM_KERNEL_KPROBES_H */
diff --git a/arch/arm/kernel/uprobes-arm.c b/arch/arm/kernel/uprobes-arm.c
new file mode 100644
index 0000000..8d7de10
--- /dev/null
+++ b/arch/arm/kernel/uprobes-arm.c
@@ -0,0 +1,178 @@
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+#include <linux/uprobes.h>
+#include <linux/module.h>
+
+#include "uprobes.h"
+#include "kprobes.h"
+
+static int uprobes_substitute_pc(kprobe_opcode_t *pinsn, u32 oregs)
+{
+	kprobe_opcode_t insn = *pinsn;
+	kprobe_opcode_t temp;
+	kprobe_opcode_t mask;
+	int freereg;
+	u32 free = 0xffff;
+	u32 regs;
+
+	for (regs = oregs; regs; regs >>= 4, insn >>= 4) {
+		if ((regs & 0xf) == REG_TYPE_NONE)
+			continue;
+
+		free &= ~(1 << (insn & 0xf));
+	}
+
+	/* No PC, no problem */
+	if (free & (1 << 15))
+		return 15;
+
+	if (!free)
+		return -1;
+
+	/*
+	 * fls instead of ffs ensures that for "ldrd r0, r1, [pc]" we would
+	 * pick LR instead of R1.
+	 */
+	freereg = free = fls(free) - 1;
+
+	temp = *pinsn;
+	insn = *pinsn;
+	regs = oregs;
+	mask = 0xf;
+
+	for (; regs; regs >>= 4, mask <<= 4, free <<= 4, temp >>= 4) {
+		if ((regs & 0xf) == REG_TYPE_NONE)
+			continue;
+
+		if ((temp & 0xf) != 15)
+			continue;
+
+		insn &= ~mask;
+		insn |= free & mask;
+	}
+
+	*pinsn = insn;
+	return freereg;
+}
+
+static void uprobe_set_pc(struct arch_uprobe *auprobe,
+			  struct arch_uprobe_task *autask,
+			  struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	autask->backup = regs->uregs[pcreg];
+	regs->uregs[pcreg] = regs->ARM_pc + 8;
+}
+
+static void uprobe_unset_pc(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs)
+{
+	/* PC will be taken care of by common code */
+	regs->uregs[auprobe->pcreg] = autask->backup;
+}
+
+static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe,
+			       struct arch_uprobe_task *autask,
+			       struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	alu_write_pc(regs->uregs[pcreg], regs);
+	regs->uregs[pcreg] = autask->backup;
+}
+
+static void uprobe_write_pc(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	load_write_pc(regs->uregs[pcreg], regs);
+	regs->uregs[pcreg] = autask->backup;
+}
+
+enum kprobe_insn
+decode_pc_ro(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, asi);
+	struct decode_emulate *decode = d;
+	u32 regs = decode->header.type_regs.bits >> DECODE_TYPE_BITS;
+	int reg;
+
+	reg = uprobes_substitute_pc(&auprobe->modinsn, regs);
+	if (reg == 15)
+		return INSN_GOOD;
+
+	if (reg == -1)
+		return INSN_REJECTED;
+
+	auprobe->pcreg = reg;
+	auprobe->prehandler = uprobe_set_pc;
+	auprobe->posthandler = uprobe_unset_pc;
+
+	return INSN_GOOD;
+}
+
+enum kprobe_insn
+decode_wb_pc(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+	     void *d, bool alu)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, asi);
+	enum kprobe_insn ret = decode_pc_ro(insn, asi, d);
+
+	if (((insn >> 12) & 0xf) == 15)
+		auprobe->posthandler = alu ? uprobe_aluwrite_pc
+					   : uprobe_write_pc;
+
+	return ret;
+}
+
+enum kprobe_insn
+decode_rd12rn16rm0rs8_rwflags(kprobe_opcode_t insn,
+			      struct arch_specific_insn *asi, void *d)
+{
+	return decode_wb_pc(insn, asi, d, true);
+}
+
+enum kprobe_insn
+decode_ldr(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
+{
+	return decode_wb_pc(insn, asi, d, false);
+}
+
+enum kprobe_insn
+uprobe_decode_ldmstm(kprobe_opcode_t insn,
+		     struct arch_specific_insn *asi, void *d)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe,
+						   asi);
+	unsigned reglist = insn & 0xffff;
+	int rn = (insn >> 16) & 0xf;
+	int lbit = insn & (1 << 20);
+	unsigned used = reglist | (1 << rn);
+
+	if (rn == 15)
+		return INSN_REJECTED;
+
+	if (!(used & (1 << 15)))
+		return INSN_GOOD;
+
+	if (used & (1 << 14))
+		return INSN_REJECTED;
+
+	/* Use LR instead of PC */
+	insn ^= 0xc000;
+
+	auprobe->pcreg = 14;
+	auprobe->modinsn = insn;
+
+	auprobe->prehandler = uprobe_set_pc;
+	if (lbit)
+		auprobe->posthandler = uprobe_write_pc;
+	else
+		auprobe->posthandler = uprobe_unset_pc;
+
+	return INSN_GOOD;
+}
diff --git a/arch/arm/kernel/uprobes.h b/arch/arm/kernel/uprobes.h
new file mode 100644
index 0000000..a1a4897
--- /dev/null
+++ b/arch/arm/kernel/uprobes.h
@@ -0,0 +1,23 @@
+#ifndef __ARM_KERNEL_UPROBES_H
+#define __ARM_KERNEL_UPROBES_H
+
+enum kprobe_insn uprobe_decode_ldmstm(kprobe_opcode_t insn,
+				      struct arch_specific_insn *asi,
+				      void *d);
+
+enum kprobe_insn decode_ldr(kprobe_opcode_t insn,
+			    struct arch_specific_insn *asi,
+			    void *d);
+
+enum kprobe_insn
+decode_rd12rn16rm0rs8_rwflags(kprobe_opcode_t insn,
+			      struct arch_specific_insn *asi, void *d);
+
+enum kprobe_insn
+decode_wb_pc(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+	     void *d, bool alu);
+
+enum kprobe_insn
+decode_pc_ro(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d);
+
+#endif
-- 
1.7.9.5

[PATCH 8/9] ARM: support uprobe handling

[Rabin Vincent]

Extend the kprobes code to handle user-space probes.  Much of the code
can be reused so currently the ARM uprobes code reuses the kprobes
structures.  The decode tables are reused, with the modification that
for those instruction that require custom decoding for uprobes, a new
element is added in the table to specify a custom decoder function.

Thumb is not handled.

Cc: Jon Medhurst <tixy@yxit.co.uk>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/include/asm/kprobes.h   |   17 +---
 arch/arm/include/asm/probes.h    |   23 +++++
 arch/arm/kernel/kprobes-arm.c    |   27 +++++-
 arch/arm/kernel/kprobes-common.c |   63 ++++++++++----
 arch/arm/kernel/kprobes-test.c   |   12 ++-
 arch/arm/kernel/kprobes-thumb.c  |   31 ++++---
 arch/arm/kernel/kprobes.c        |    2 +-
 arch/arm/kernel/kprobes.h        |   36 +++++---
 arch/arm/kernel/uprobes-arm.c    |  178 ++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/uprobes.h        |   23 +++++
 10 files changed, 349 insertions(+), 63 deletions(-)
 create mode 100644 arch/arm/include/asm/probes.h
 create mode 100644 arch/arm/kernel/uprobes-arm.c
 create mode 100644 arch/arm/kernel/uprobes.h

diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
index f82ec22..53b1a80 100644
--- a/arch/arm/include/asm/kprobes.h
+++ b/arch/arm/include/asm/kprobes.h
@@ -27,22 +27,7 @@
 #define flush_insn_slot(p)		do { } while (0)
 #define kretprobe_blacklist_size	0
 
-typedef u32 kprobe_opcode_t;
-
-struct kprobe;
-typedef void (kprobe_insn_handler_t)(struct kprobe *, struct pt_regs *);
-typedef unsigned long (kprobe_check_cc)(unsigned long);
-typedef void (kprobe_insn_singlestep_t)(struct kprobe *, struct pt_regs *);
-typedef void (kprobe_insn_fn_t)(void);
-
-/* Architecture specific copy of original instruction. */
-struct arch_specific_insn {
-	kprobe_opcode_t			*insn;
-	kprobe_insn_handler_t		*insn_handler;
-	kprobe_check_cc			*insn_check_cc;
-	kprobe_insn_singlestep_t	*insn_singlestep;
-	kprobe_insn_fn_t		*insn_fn;
-};
+#include <asm/probes.h>
 
 struct prev_kprobe {
 	struct kprobe *kp;
diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
new file mode 100644
index 0000000..df46994
--- /dev/null
+++ b/arch/arm/include/asm/probes.h
@@ -0,0 +1,23 @@
+#ifndef _ASM_PROBES_H
+#define _ASM_PROBES_H
+
+#ifdef CONFIG_KPROBES
+typedef u32 kprobe_opcode_t;
+
+struct kprobe;
+typedef void (kprobe_insn_handler_t)(struct kprobe *, struct pt_regs *);
+typedef unsigned long (kprobe_check_cc)(unsigned long);
+typedef void (kprobe_insn_singlestep_t)(struct kprobe *, struct pt_regs *);
+typedef void (kprobe_insn_fn_t)(void);
+
+/* Architecture specific copy of original instruction. */
+struct arch_specific_insn {
+	kprobe_opcode_t			*insn;
+	kprobe_insn_handler_t		*insn_handler;
+	kprobe_check_cc			*insn_check_cc;
+	kprobe_insn_singlestep_t	*insn_singlestep;
+	kprobe_insn_fn_t		*insn_fn;
+};
+#endif
+
+#endif
diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/kernel/kprobes-arm.c
index 8a30c89..d9cf0e2 100644
--- a/arch/arm/kernel/kprobes-arm.c
+++ b/arch/arm/kernel/kprobes-arm.c
@@ -62,6 +62,7 @@
 #include <linux/kprobes.h>
 #include <linux/module.h>
 
+#include "uprobes.h"
 #include "kprobes.h"
 
 #define sign_extend(x, signbit) ((x) | (0 - ((x) & (1 << (signbit)))))
@@ -545,31 +546,37 @@ static const union decode_item arm_cccc_000x_____1xx1_table[] = {
 
 	/* LDRD (register)	cccc 000x x0x0 xxxx xxxx xxxx 1101 xxxx */
 	/* STRD (register)	cccc 000x x0x0 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000d0, 0x000000d0, emulate_ldrdstrd,
 						 REGS(NOPCWB, NOPCX, 0, 0, NOPC)),
 
 	/* LDRD (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1101 xxxx */
 	/* STRD (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000d0, 0x004000d0, emulate_ldrdstrd,
 						 REGS(NOPCWB, NOPCX, 0, 0, 0)),
 
 	/* STRH (register)	cccc 000x x0x0 xxxx xxxx xxxx 1011 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000f0, 0x000000b0, emulate_str,
 						 REGS(NOPCWB, NOPC, 0, 0, NOPC)),
 
 	/* LDRH (register)	cccc 000x x0x1 xxxx xxxx xxxx 1011 xxxx */
 	/* LDRSB (register)	cccc 000x x0x1 xxxx xxxx xxxx 1101 xxxx */
 	/* LDRSH (register)	cccc 000x x0x1 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e500090, 0x00100090, emulate_ldr,
 						 REGS(NOPCWB, NOPC, 0, 0, NOPC)),
 
 	/* STRH (immediate)	cccc 000x x1x0 xxxx xxxx xxxx 1011 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e5000f0, 0x004000b0, emulate_str,
 						 REGS(NOPCWB, NOPC, 0, 0, 0)),
 
 	/* LDRH (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1011 xxxx */
 	/* LDRSB (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1101 xxxx */
 	/* LDRSH (immediate)	cccc 000x x1x1 xxxx xxxx xxxx 1111 xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e500090, 0x00500090, emulate_ldr,
 						 REGS(NOPCWB, NOPC, 0, 0, 0)),
 
@@ -589,11 +596,13 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* TEQ (register)	cccc 0001 0011 xxxx xxxx xxxx xxx0 xxxx */
 	/* CMP (register)	cccc 0001 0101 xxxx xxxx xxxx xxx0 xxxx */
 	/* CMN (register)	cccc 0001 0111 xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900010, 0x01100000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, 0, 0, ANY)),
 
 	/* MOV (register)	cccc 0001 101x xxxx xxxx xxxx xxx0 xxxx */
 	/* MVN (register)	cccc 0001 111x xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00010, 0x01a00000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, 0, 0, ANY)),
 
@@ -607,6 +616,7 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* RSC (register)	cccc 0000 111x xxxx xxxx xxxx xxx0 xxxx */
 	/* ORR (register)	cccc 0001 100x xxxx xxxx xxxx xxx0 xxxx */
 	/* BIC (register)	cccc 0001 110x xxxx xxxx xxxx xxx0 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000010, 0x00000000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, 0, 0, ANY)),
 
@@ -614,11 +624,13 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* TEQ (reg-shift reg)	cccc 0001 0011 xxxx xxxx xxxx 0xx1 xxxx */
 	/* CMP (reg-shift reg)	cccc 0001 0101 xxxx xxxx xxxx 0xx1 xxxx */
 	/* CMN (reg-shift reg)	cccc 0001 0111 xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900090, 0x01100010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, NOPC, 0, ANY)),
 
 	/* MOV (reg-shift reg)	cccc 0001 101x xxxx xxxx xxxx 0xx1 xxxx */
 	/* MVN (reg-shift reg)	cccc 0001 111x xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00090, 0x01a00010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, NOPC, 0, ANY)),
 
@@ -632,6 +644,7 @@ static const union decode_item arm_cccc_000x_table[] = {
 	/* RSC (reg-shift reg)	cccc 0000 111x xxxx xxxx xxxx 0xx1 xxxx */
 	/* ORR (reg-shift reg)	cccc 0001 100x xxxx xxxx xxxx 0xx1 xxxx */
 	/* BIC (reg-shift reg)	cccc 0001 110x xxxx xxxx xxxx 0xx1 xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000090, 0x00000010, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, NOPC, 0, ANY)),
 
@@ -666,11 +679,13 @@ static const union decode_item arm_cccc_001x_table[] = {
 	/* TEQ (immediate)	cccc 0011 0011 xxxx xxxx xxxx xxxx xxxx */
 	/* CMP (immediate)	cccc 0011 0101 xxxx xxxx xxxx xxxx xxxx */
 	/* CMN (immediate)	cccc 0011 0111 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0f900000, 0x03100000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, 0, 0, 0, 0)),
 
 	/* MOV (immediate)	cccc 0011 101x xxxx xxxx xxxx xxxx xxxx */
 	/* MVN (immediate)	cccc 0011 111x xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0fa00000, 0x03a00000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(0, ANY, 0, 0, 0)),
 
@@ -684,6 +699,7 @@ static const union decode_item arm_cccc_001x_table[] = {
 	/* RSC (immediate)	cccc 0010 111x xxxx xxxx xxxx xxxx xxxx */
 	/* ORR (immediate)	cccc 0011 100x xxxx xxxx xxxx xxxx xxxx */
 	/* BIC (immediate)	cccc 0011 110x xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_rd12rn16rm0rs8_rwflags)
 	DECODE_EMULATEX	(0x0e000000, 0x02000000, emulate_rd12rn16rm0rs8_rwflags,
 						 REGS(ANY, ANY, 0, 0, 0)),
 
@@ -850,21 +866,25 @@ static const union decode_item arm_cccc_01xx_table[] = {
 
 	/* STR (immediate)	cccc 010x x0x0 xxxx xxxx xxxx xxxx xxxx */
 	/* STRB (immediate)	cccc 010x x1x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e100000, 0x04000000, emulate_str,
 						 REGS(NOPCWB, ANY, 0, 0, 0)),
 
 	/* LDR (immediate)	cccc 010x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* LDRB (immediate)	cccc 010x x1x1 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_ldr)
 	DECODE_EMULATEX	(0x0e100000, 0x04100000, emulate_ldr,
 						 REGS(NOPCWB, ANY, 0, 0, 0)),
 
 	/* STR (register)	cccc 011x x0x0 xxxx xxxx xxxx xxxx xxxx */
 	/* STRB (register)	cccc 011x x1x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_pc_ro)
 	DECODE_EMULATEX	(0x0e100000, 0x06000000, emulate_str,
 						 REGS(NOPCWB, ANY, 0, 0, NOPC)),
 
 	/* LDR (register)	cccc 011x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* LDRB (register)	cccc 011x x1x1 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (decode_ldr)
 	DECODE_EMULATEX	(0x0e100000, 0x06100000, emulate_ldr,
 						 REGS(NOPCWB, ANY, 0, 0, NOPC)),
 
@@ -876,6 +896,7 @@ static const union decode_item arm_cccc_100x_table[] = {
 
 	/* LDM			cccc 100x x0x1 xxxx xxxx xxxx xxxx xxxx */
 	/* STM			cccc 100x x0x0 xxxx xxxx xxxx xxxx xxxx */
+	UDECODE_NEXT (uprobe_decode_ldmstm)
 	DECODE_CUSTOM	(0x0e400000, 0x08000000, kprobe_decode_ldmstm),
 
 	/* STM (user registers)	cccc 100x x1x0 xxxx xxxx xxxx xxxx xxxx */
@@ -997,9 +1018,11 @@ static void __kprobes arm_singlestep(struct kprobe *p, struct pt_regs *regs)
  *   should also be very rare.
  */
 enum kprobe_insn __kprobes
-arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       bool uprobe)
 {
 	asi->insn_singlestep = arm_singlestep;
 	asi->insn_check_cc = kprobe_condition_checks[insn>>28];
-	return kprobe_decode_insn(insn, asi, kprobe_decode_arm_table, false);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_arm_table, false,
+				  uprobe);
 }
diff --git a/arch/arm/kernel/kprobes-common.c b/arch/arm/kernel/kprobes-common.c
index 18a7628..f811bfb 100644
--- a/arch/arm/kernel/kprobes-common.c
+++ b/arch/arm/kernel/kprobes-common.c
@@ -277,7 +277,8 @@ emulate_ldm_r3_15(struct kprobe *p, struct pt_regs *regs)
 }
 
 enum kprobe_insn __kprobes
-kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		     void *d)
 {
 	kprobe_insn_handler_t *handler = 0;
 	unsigned reglist = insn & 0xffff;
@@ -389,7 +390,7 @@ set_emulated_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
  * non-zero value, the corresponding nibble in pinsn is validated and modified
  * according to the type.
  */
-static bool __kprobes decode_regs(kprobe_opcode_t* pinsn, u32 regs)
+static bool __kprobes decode_regs(kprobe_opcode_t *pinsn, u32 regs, bool modify)
 {
 	kprobe_opcode_t insn = *pinsn;
 	kprobe_opcode_t mask = 0xf; /* Start at least significant nibble */
@@ -450,12 +451,16 @@ static bool __kprobes decode_regs(kprobe_opcode_t* pinsn, u32 regs)
 			break;
 		}
 
-		/* Replace value of nibble with new register number... */
-		insn &= ~mask;
-		insn |= new_bits & mask;
+		if (modify) {
+			/* Replace value of nibble with new register number */
+			insn &= ~mask;
+			insn |= new_bits & mask;
+		}
 	}
 
-	*pinsn = insn;
+	if (modify)
+		*pinsn = insn;
+
 	return true;
 
 reject:
@@ -468,7 +473,8 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 	[DECODE_TYPE_SIMULATE]	= sizeof(struct decode_simulate),
 	[DECODE_TYPE_EMULATE]	= sizeof(struct decode_emulate),
 	[DECODE_TYPE_OR]	= sizeof(struct decode_or),
-	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject)
+	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject),
+	[UDECODE_TYPE_NEXT]	= sizeof(struct decode_header)
 };
 
 /*
@@ -516,13 +522,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
  */
 int __kprobes
 kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
-				const union decode_item *table, bool thumb)
+				const union decode_item *table, bool thumb,
+				bool uprobe)
 {
+	enum kprobe_insn (*decoder)(kprobe_opcode_t,
+			  struct arch_specific_insn *, void *d) = NULL;
 	const struct decode_header *h = (struct decode_header *)table;
 	const struct decode_header *next;
 	bool matched = false;
 
-	insn = prepare_emulated_insn(insn, asi, thumb);
+	if (!uprobe)
+		insn = prepare_emulated_insn(insn, asi, thumb);
 
 	for (;; h = next) {
 		enum decode_type type = h->type_regs.bits & DECODE_TYPE_MASK;
@@ -534,13 +544,24 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 		next = (struct decode_header *)
 				((uintptr_t)h + decode_struct_sizes[type]);
 
-		if (!matched && (insn & h->mask.bits) != h->value.bits)
+		if (!uprobe && type == UDECODE_TYPE_NEXT)
 			continue;
 
-		if (!decode_regs(&insn, regs))
-			return INSN_REJECTED;
+		if (type != UDECODE_TYPE_NEXT) {
+			if (!matched &&
+			    (insn & h->mask.bits) != h->value.bits) {
+				decoder = NULL;
+				continue;
+			}
+
+			if (!decode_regs(&insn, regs, !uprobe))
+				return INSN_REJECTED;
+		}
 
 		switch (type) {
+		case UDECODE_TYPE_NEXT:
+			decoder = h->mask.decoder;
+			break;
 
 		case DECODE_TYPE_TABLE: {
 			struct decode_table *d = (struct decode_table *)h;
@@ -550,7 +571,11 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 
 		case DECODE_TYPE_CUSTOM: {
 			struct decode_custom *d = (struct decode_custom *)h;
-			return (*d->decoder.decoder)(insn, asi);
+
+			if (decoder)
+				return decoder(insn, asi, d);
+
+			return (*d->decoder.decoder)(insn, asi, d);
 		}
 
 		case DECODE_TYPE_SIMULATE: {
@@ -561,8 +586,14 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 
 		case DECODE_TYPE_EMULATE: {
 			struct decode_emulate *d = (struct decode_emulate *)h;
-			asi->insn_handler = d->handler.handler;
-			set_emulated_insn(insn, asi, thumb);
+
+			if (decoder)
+				return decoder(insn, asi, d);
+
+			if (!uprobe) {
+				asi->insn_handler = d->handler.handler;
+				set_emulated_insn(insn, asi, thumb);
+			}
 			return INSN_GOOD;
 		}
 
@@ -574,5 +605,5 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 		default:
 			return INSN_REJECTED;
 		}
-		}
 	}
+}
diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index 1862d8f..a2d4213 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -630,7 +630,8 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 	[DECODE_TYPE_SIMULATE]	= sizeof(struct decode_simulate),
 	[DECODE_TYPE_EMULATE]	= sizeof(struct decode_emulate),
 	[DECODE_TYPE_OR]	= sizeof(struct decode_or),
-	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject)
+	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject),
+	[UDECODE_TYPE_NEXT]	= sizeof(struct decode_header)
 };
 
 static int table_iter(const union decode_item *table,
@@ -646,9 +647,11 @@ static int table_iter(const union decode_item *table,
 		if (type == DECODE_TYPE_END)
 			return 0;
 
-		result = fn(h, args);
-		if (result)
-			return result;
+		if (type != UDECODE_TYPE_NEXT) {
+			result = fn(h, args);
+			if (result)
+				return result;
+		}
 
 		h = (struct decode_header *)
 			((uintptr_t)h + decode_struct_sizes[type]);
@@ -918,6 +921,7 @@ static void coverage_add(kprobe_opcode_t insn)
 			break;
 
 		case DECODE_TYPE_REJECT:
+		case UDECODE_TYPE_NEXT:
 		default:
 			return;
 		}
diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/kernel/kprobes-thumb.c
index 6123daf..761aa55 100644
--- a/arch/arm/kernel/kprobes-thumb.c
+++ b/arch/arm/kernel/kprobes-thumb.c
@@ -83,7 +83,8 @@ t32_simulate_cond_branch(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t32_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t32_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       void *d)
 {
 	int cc = (insn >> 22) & 0xf;
 	asi->insn_check_cc = kprobe_condition_checks[cc];
@@ -158,9 +159,10 @@ t32_simulate_ldr_literal(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t32_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t32_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		  void *d)
 {
-	enum kprobe_insn ret = kprobe_decode_ldmstm(insn, asi);
+	enum kprobe_insn ret = kprobe_decode_ldmstm(insn, asi, d);
 
 	/* Fixup modified instruction to have halfwords in correct order...*/
 	insn = asi->insn[0];
@@ -1046,7 +1048,7 @@ t16_singlestep_it(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_it(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_it(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	asi->insn_singlestep = t16_singlestep_it;
 	return INSN_GOOD_NO_SLOT;
@@ -1063,7 +1065,8 @@ t16_simulate_cond_branch(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_cond_branch(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		       void *d)
 {
 	int cc = (insn >> 8) & 0xf;
 	asi->insn_check_cc = kprobe_condition_checks[cc];
@@ -1149,7 +1152,7 @@ t16_emulate_hiregs(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_hiregs(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_hiregs(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	insn &= ~0x00ff;
 	insn |= 0x001; /* Set Rdn = R1 and Rm = R0 */
@@ -1175,7 +1178,7 @@ t16_emulate_push(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_push(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_push(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	/*
 	 * To simulate a PUSH we use a Thumb-2 "STMDB R9!, {registers}"
@@ -1225,7 +1228,7 @@ t16_emulate_pop_pc(struct kprobe *p, struct pt_regs *regs)
 }
 
 static enum kprobe_insn __kprobes
-t16_decode_pop(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+t16_decode_pop(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
 {
 	/*
 	 * To simulate a POP we use a Thumb-2 "LDMDB R9!, {registers}"
@@ -1453,17 +1456,21 @@ static void __kprobes thumb32_singlestep(struct kprobe *p, struct pt_regs *regs)
 }
 
 enum kprobe_insn __kprobes
-thumb16_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+thumb16_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+			   bool uprobes)
 {
 	asi->insn_singlestep = thumb16_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
-	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb16_table, true);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb16_table, true,
+				  uprobes);
 }
 
 enum kprobe_insn __kprobes
-thumb32_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+thumb32_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+			   bool uprobes)
 {
 	asi->insn_singlestep = thumb32_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
-	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb32_table, true);
+	return kprobe_decode_insn(insn, asi, kprobe_decode_thumb32_table, true,
+				  uprobes);
 }
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
index 4dd41fc..1b2a324 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/kernel/kprobes.c
@@ -80,7 +80,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	p->opcode = insn;
 	p->ainsn.insn = tmp_insn;
 
-	switch ((*decode_insn)(insn, &p->ainsn)) {
+	switch ((*decode_insn)(insn, &p->ainsn, false)) {
 	case INSN_REJECTED:	/* not supported */
 		return -EINVAL;
 
diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/kernel/kprobes.h
index 38945f7..442a161 100644
--- a/arch/arm/kernel/kprobes.h
+++ b/arch/arm/kernel/kprobes.h
@@ -35,20 +35,19 @@ enum kprobe_insn {
 };
 
 typedef enum kprobe_insn (kprobe_decode_insn_t)(kprobe_opcode_t,
-						struct arch_specific_insn *);
-
-#ifdef CONFIG_THUMB2_KERNEL
+						struct arch_specific_insn *,
+						bool uprobes);
 
 enum kprobe_insn thumb16_kprobe_decode_insn(kprobe_opcode_t,
-						struct arch_specific_insn *);
+					    struct arch_specific_insn *,
+					    bool uprobes);
 enum kprobe_insn thumb32_kprobe_decode_insn(kprobe_opcode_t,
-						struct arch_specific_insn *);
-
-#else /* !CONFIG_THUMB2_KERNEL */
+					    struct arch_specific_insn *,
+					    bool uprobes);
 
 enum kprobe_insn arm_kprobe_decode_insn(kprobe_opcode_t,
-					struct arch_specific_insn *);
-#endif
+					struct arch_specific_insn *,
+					bool uprobes);
 
 void __init arm_kprobe_decode_init(void);
 
@@ -165,7 +164,8 @@ void __kprobes kprobe_simulate_nop(struct kprobe *p, struct pt_regs *regs);
 void __kprobes kprobe_emulate_none(struct kprobe *p, struct pt_regs *regs);
 
 enum kprobe_insn __kprobes
-kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi);
+kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+		     void *d);
 
 /*
  * Test if load/store instructions writeback the address register.
@@ -292,6 +292,7 @@ enum decode_type {
 	DECODE_TYPE_EMULATE,
 	DECODE_TYPE_OR,
 	DECODE_TYPE_REJECT,
+	UDECODE_TYPE_NEXT,
 	NUM_DECODE_TYPES /* Must be last enum */
 };
 
@@ -331,7 +332,9 @@ union decode_item {
 	u32			bits;
 	const union decode_item	*table;
 	kprobe_insn_handler_t	*handler;
-	kprobe_decode_insn_t	*decoder;
+	enum kprobe_insn (*decoder)(kprobe_opcode_t,
+				    struct arch_specific_insn *,
+				    void *d);
 };
 
 
@@ -396,6 +399,14 @@ struct decode_emulate {
 #define DECODE_EMULATE(_mask, _value, _handler)		\
 	DECODE_EMULATEX(_mask, _value, _handler, 0)
 
+#ifdef CONFIG_UPROBES
+#define UDECODE_NEXT(_decoder)				\
+	{.bits = UDECODE_TYPE_NEXT },			\
+	{.decoder = _decoder},				\
+	{.bits = 0 },
+#else
+#define UDECODE_NEXT(_decoder)
+#endif
 
 struct decode_or {
 	struct decode_header	header;
@@ -422,7 +433,8 @@ extern const union decode_item kprobe_decode_arm_table[];
 
 
 int kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
-			const union decode_item *table, bool thumb16);
+			const union decode_item *table, bool thumb16,
+			bool uprobe);
 
 
 #endif /* _ARM_KERNEL_KPROBES_H */
diff --git a/arch/arm/kernel/uprobes-arm.c b/arch/arm/kernel/uprobes-arm.c
new file mode 100644
index 0000000..8d7de10
--- /dev/null
+++ b/arch/arm/kernel/uprobes-arm.c
@@ -0,0 +1,178 @@
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+#include <linux/uprobes.h>
+#include <linux/module.h>
+
+#include "uprobes.h"
+#include "kprobes.h"
+
+static int uprobes_substitute_pc(kprobe_opcode_t *pinsn, u32 oregs)
+{
+	kprobe_opcode_t insn = *pinsn;
+	kprobe_opcode_t temp;
+	kprobe_opcode_t mask;
+	int freereg;
+	u32 free = 0xffff;
+	u32 regs;
+
+	for (regs = oregs; regs; regs >>= 4, insn >>= 4) {
+		if ((regs & 0xf) == REG_TYPE_NONE)
+			continue;
+
+		free &= ~(1 << (insn & 0xf));
+	}
+
+	/* No PC, no problem */
+	if (free & (1 << 15))
+		return 15;
+
+	if (!free)
+		return -1;
+
+	/*
+	 * fls instead of ffs ensures that for "ldrd r0, r1, [pc]" we would
+	 * pick LR instead of R1.
+	 */
+	freereg = free = fls(free) - 1;
+
+	temp = *pinsn;
+	insn = *pinsn;
+	regs = oregs;
+	mask = 0xf;
+
+	for (; regs; regs >>= 4, mask <<= 4, free <<= 4, temp >>= 4) {
+		if ((regs & 0xf) == REG_TYPE_NONE)
+			continue;
+
+		if ((temp & 0xf) != 15)
+			continue;
+
+		insn &= ~mask;
+		insn |= free & mask;
+	}
+
+	*pinsn = insn;
+	return freereg;
+}
+
+static void uprobe_set_pc(struct arch_uprobe *auprobe,
+			  struct arch_uprobe_task *autask,
+			  struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	autask->backup = regs->uregs[pcreg];
+	regs->uregs[pcreg] = regs->ARM_pc + 8;
+}
+
+static void uprobe_unset_pc(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs)
+{
+	/* PC will be taken care of by common code */
+	regs->uregs[auprobe->pcreg] = autask->backup;
+}
+
+static void uprobe_aluwrite_pc(struct arch_uprobe *auprobe,
+			       struct arch_uprobe_task *autask,
+			       struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	alu_write_pc(regs->uregs[pcreg], regs);
+	regs->uregs[pcreg] = autask->backup;
+}
+
+static void uprobe_write_pc(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs)
+{
+	u32 pcreg = auprobe->pcreg;
+
+	load_write_pc(regs->uregs[pcreg], regs);
+	regs->uregs[pcreg] = autask->backup;
+}
+
+enum kprobe_insn
+decode_pc_ro(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, asi);
+	struct decode_emulate *decode = d;
+	u32 regs = decode->header.type_regs.bits >> DECODE_TYPE_BITS;
+	int reg;
+
+	reg = uprobes_substitute_pc(&auprobe->modinsn, regs);
+	if (reg == 15)
+		return INSN_GOOD;
+
+	if (reg == -1)
+		return INSN_REJECTED;
+
+	auprobe->pcreg = reg;
+	auprobe->prehandler = uprobe_set_pc;
+	auprobe->posthandler = uprobe_unset_pc;
+
+	return INSN_GOOD;
+}
+
+enum kprobe_insn
+decode_wb_pc(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+	     void *d, bool alu)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe, asi);
+	enum kprobe_insn ret = decode_pc_ro(insn, asi, d);
+
+	if (((insn >> 12) & 0xf) == 15)
+		auprobe->posthandler = alu ? uprobe_aluwrite_pc
+					   : uprobe_write_pc;
+
+	return ret;
+}
+
+enum kprobe_insn
+decode_rd12rn16rm0rs8_rwflags(kprobe_opcode_t insn,
+			      struct arch_specific_insn *asi, void *d)
+{
+	return decode_wb_pc(insn, asi, d, true);
+}
+
+enum kprobe_insn
+decode_ldr(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d)
+{
+	return decode_wb_pc(insn, asi, d, false);
+}
+
+enum kprobe_insn
+uprobe_decode_ldmstm(kprobe_opcode_t insn,
+		     struct arch_specific_insn *asi, void *d)
+{
+	struct arch_uprobe *auprobe = container_of(asi, struct arch_uprobe,
+						   asi);
+	unsigned reglist = insn & 0xffff;
+	int rn = (insn >> 16) & 0xf;
+	int lbit = insn & (1 << 20);
+	unsigned used = reglist | (1 << rn);
+
+	if (rn == 15)
+		return INSN_REJECTED;
+
+	if (!(used & (1 << 15)))
+		return INSN_GOOD;
+
+	if (used & (1 << 14))
+		return INSN_REJECTED;
+
+	/* Use LR instead of PC */
+	insn ^= 0xc000;
+
+	auprobe->pcreg = 14;
+	auprobe->modinsn = insn;
+
+	auprobe->prehandler = uprobe_set_pc;
+	if (lbit)
+		auprobe->posthandler = uprobe_write_pc;
+	else
+		auprobe->posthandler = uprobe_unset_pc;
+
+	return INSN_GOOD;
+}
diff --git a/arch/arm/kernel/uprobes.h b/arch/arm/kernel/uprobes.h
new file mode 100644
index 0000000..a1a4897
--- /dev/null
+++ b/arch/arm/kernel/uprobes.h
@@ -0,0 +1,23 @@
+#ifndef __ARM_KERNEL_UPROBES_H
+#define __ARM_KERNEL_UPROBES_H
+
+enum kprobe_insn uprobe_decode_ldmstm(kprobe_opcode_t insn,
+				      struct arch_specific_insn *asi,
+				      void *d);
+
+enum kprobe_insn decode_ldr(kprobe_opcode_t insn,
+			    struct arch_specific_insn *asi,
+			    void *d);
+
+enum kprobe_insn
+decode_rd12rn16rm0rs8_rwflags(kprobe_opcode_t insn,
+			      struct arch_specific_insn *asi, void *d);
+
+enum kprobe_insn
+decode_wb_pc(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+	     void *d, bool alu);
+
+enum kprobe_insn
+decode_pc_ro(kprobe_opcode_t insn, struct arch_specific_insn *asi, void *d);
+
+#endif
-- 
1.7.9.5

[PATCH 8/9] ARM: support uprobe handling

[Russell King - ARM Linux]

On Sun, Oct 14, 2012 at 09:23:12PM +0200, Rabin Vincent wrote:
> Extend the kprobes code to handle user-space probes.  Much of the code
> can be reused so currently the ARM uprobes code reuses the kprobes
> structures.  The decode tables are reused, with the modification that
> for those instruction that require custom decoding for uprobes, a new
> element is added in the table to specify a custom decoder function.

How are accesses to userspace handled by the kprobes code?  Please point
me to where these accesses are performed.

Re: [PATCH 8/9] ARM: support uprobe handling

[Russell King - ARM Linux]

On Sun, Oct 14, 2012 at 09:23:12PM +0200, Rabin Vincent wrote:
> Extend the kprobes code to handle user-space probes.  Much of the code
> can be reused so currently the ARM uprobes code reuses the kprobes
> structures.  The decode tables are reused, with the modification that
> for those instruction that require custom decoding for uprobes, a new
> element is added in the table to specify a custom decoder function.

How are accesses to userspace handled by the kprobes code?  Please point
me to where these accesses are performed.

[PATCH 8/9] ARM: support uprobe handling

[Rabin Vincent]

2012/11/4 Russell King - ARM Linux <linux@arm.linux.org.uk>
> On Sun, Oct 14, 2012 at 09:23:12PM +0200, Rabin Vincent wrote:
> > Extend the kprobes code to handle user-space probes.  Much of the code
> > can be reused so currently the ARM uprobes code reuses the kprobes
> > structures.  The decode tables are reused, with the modification that
> > for those instruction that require custom decoding for uprobes, a new
> > element is added in the table to specify a custom decoder function.
>
> How are accesses to userspace handled by the kprobes code?  Please point
> me to where these accesses are performed.

If you mean the accesses where we write the probe opcode, this is done
in write_opcode() in kernel/events/uprobes.c

If you instead mean the accesses which load/store instructions would
perform, this is done from userspace itself, by placing the modified
instruction in an executable area which is mapped to userspace, and
modifying the PC to point there.  See xol_get_insn_slot() in
kernel/events/uprobes.c

Re: [PATCH 8/9] ARM: support uprobe handling

[Rabin Vincent]

2012/11/4 Russell King - ARM Linux <linux@arm.linux.org.uk>
> On Sun, Oct 14, 2012 at 09:23:12PM +0200, Rabin Vincent wrote:
> > Extend the kprobes code to handle user-space probes.  Much of the code
> > can be reused so currently the ARM uprobes code reuses the kprobes
> > structures.  The decode tables are reused, with the modification that
> > for those instruction that require custom decoding for uprobes, a new
> > element is added in the table to specify a custom decoder function.
>
> How are accesses to userspace handled by the kprobes code?  Please point
> me to where these accesses are performed.

If you mean the accesses where we write the probe opcode, this is done
in write_opcode() in kernel/events/uprobes.c

If you instead mean the accesses which load/store instructions would
perform, this is done from userspace itself, by placing the modified
instruction in an executable area which is mapped to userspace, and
modifying the PC to point there.  See xol_get_insn_slot() in
kernel/events/uprobes.c

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

Add basic uprobes support for ARM.

perf probe --exec and SystemTap's userspace probing work.  The ARM
kprobes test code has also been run in a userspace harness to test the
uprobe instruction decoding.

Caveats:

 - Thumb is not supported
 - XOL abort/trap handling is not implemented

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/Kconfig                   |    4 +
 arch/arm/include/asm/ptrace.h      |    6 ++
 arch/arm/include/asm/thread_info.h |    5 +-
 arch/arm/include/asm/uprobes.h     |   34 +++++++
 arch/arm/kernel/Makefile           |    1 +
 arch/arm/kernel/signal.c           |    4 +
 arch/arm/kernel/uprobes.c          |  191 ++++++++++++++++++++++++++++++++++++
 7 files changed, 244 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/uprobes.h
 create mode 100644 arch/arm/kernel/uprobes.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 272c3a1..2191b61d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -168,6 +168,10 @@ config ZONE_DMA
 config NEED_DMA_MAP_STATE
        def_bool y
 
+config ARCH_SUPPORTS_UPROBES
+	depends on KPROBES
+	def_bool y
+
 config ARCH_HAS_DMA_SET_COHERENT_MASK
 	bool
 
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 142d6ae..297936a 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -197,6 +197,12 @@ static inline long regs_return_value(struct pt_regs *regs)
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
 
+static inline void instruction_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	instruction_pointer(regs) = val;
+}
+
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8477b4c..7bedaee 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -148,6 +148,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_SIGPENDING		0
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
+#define TIF_UPROBE		7
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
@@ -160,6 +161,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
+#define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
@@ -172,7 +174,8 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 /*
  * Change these and you break ASM code in entry-common.S
  */
-#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_RESUME)
+#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
+				 _TIF_NOTIFY_RESUME | _TIF_UPROBE)
 
 #endif /* __KERNEL__ */
 #endif /* __ASM_ARM_THREAD_INFO_H */
diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h
new file mode 100644
index 0000000..fa4b81e
--- /dev/null
+++ b/arch/arm/include/asm/uprobes.h
@@ -0,0 +1,34 @@
+#ifndef _ASM_UPROBES_H
+#define _ASM_UPROBES_H
+
+#include <asm/probes.h>
+
+typedef u32 uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES		4
+#define UPROBE_XOL_SLOT_BYTES	64
+
+#define UPROBE_SWBP_INSN	0x07f001f9
+#define UPROBE_SS_INSN		0x07f001fa
+#define UPROBE_SWBP_INSN_SIZE	4
+
+struct arch_uprobe_task {
+	u32 backup;
+};
+
+struct arch_uprobe {
+	u8 insn[MAX_UINSN_BYTES];
+	uprobe_opcode_t modinsn;
+	uprobe_opcode_t bpinsn;
+	bool simulate;
+	u32 pcreg;
+	void (*prehandler)(struct arch_uprobe *auprobe,
+			   struct arch_uprobe_task *autask,
+			   struct pt_regs *regs);
+	void (*posthandler)(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs);
+	struct arch_specific_insn asi;
+};
+
+#endif
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5bbec7b..a39f634 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
+obj-$(CONFIG_UPROBES)		+= uprobes.o uprobes-arm.o kprobes-arm.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
 ifdef CONFIG_THUMB2_KERNEL
 obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 56f72d2..3b1e88e 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -12,6 +12,7 @@
 #include <linux/personality.h>
 #include <linux/uaccess.h>
 #include <linux/tracehook.h>
+#include <linux/uprobes.h>
 
 #include <asm/elf.h>
 #include <asm/cacheflush.h>
@@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 					return restart;
 				}
 				syscall = 0;
+			} else if (thread_flags & _TIF_UPROBE) {
+				clear_thread_flag(TIF_UPROBE);
+				uprobe_notify_resume(regs);
 			} else {
 				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
new file mode 100644
index 0000000..f25a4af
--- /dev/null
+++ b/arch/arm/kernel/uprobes.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (C) 2012 Rabin Vincent <rabin@rab.in>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/uprobes.h>
+#include <linux/notifier.h>
+#include <linux/kprobes.h>
+
+#include <asm/opcodes.h>
+#include <asm/traps.h>
+
+#include "kprobes.h"
+
+bool is_swbp_insn(uprobe_opcode_t *insn)
+{
+	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;
+}
+
+bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) {
+		regs->ARM_pc += 4;
+		return true;
+	}
+
+	return false;
+}
+
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct kprobe kp;
+
+	if (!auprobe->simulate)
+		return false;
+
+	kp.addr = (void *) regs->ARM_pc;
+	kp.opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn);
+	kp.ainsn.insn_handler = auprobe->asi.insn_handler;
+
+	auprobe->asi.insn_singlestep(&kp, regs);
+
+	return true;
+}
+
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+			     unsigned long addr)
+{
+	unsigned int insn;
+	unsigned int bpinsn;
+	enum kprobe_insn ret;
+
+	/* Thumb not yet support */
+	if (addr & 0x3)
+		return -EINVAL;
+
+	insn = __mem_to_opcode_arm(*(unsigned int *)auprobe->insn);
+	auprobe->modinsn = insn;
+
+	ret = arm_kprobe_decode_insn(insn, &auprobe->asi, true);
+	switch (ret) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		break;
+
+	case INSN_GOOD:
+	default:
+		break;
+	}
+
+	bpinsn = UPROBE_SWBP_INSN;
+	if (insn >= 0xe0000000)
+		bpinsn |= 0xe0000000;  /* Unconditional instruction */
+	else
+		bpinsn |= insn & 0xf0000000;  /* Copy condition from insn */
+
+	auprobe->bpinsn = bpinsn;
+
+	return 0;
+}
+
+void arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
+			      uprobe_opcode_t opcode)
+{
+	unsigned long *addr = vaddr;
+
+	if (opcode == UPROBE_SWBP_INSN)
+		opcode = __opcode_to_mem_arm(auprobe->bpinsn);
+
+	*addr = opcode;
+}
+
+void arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
+{
+	unsigned long *addr = vaddr;
+
+	addr[0] = __opcode_to_mem_arm(auprobe->modinsn);
+	addr[1] = __opcode_to_mem_arm(0xe0000000 | UPROBE_SS_INSN);
+}
+
+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	if (auprobe->prehandler)
+		auprobe->prehandler(auprobe, &utask->autask, regs);
+
+	regs->ARM_pc = utask->xol_vaddr;
+
+	return 0;
+}
+
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	regs->ARM_pc = utask->vaddr + 4;
+
+	if (auprobe->posthandler)
+		auprobe->posthandler(auprobe, &utask->autask, regs);
+
+	return 0;
+}
+
+bool arch_uprobe_xol_was_trapped(struct task_struct *t)
+{
+	/* TODO: implement */
+	return false;
+}
+
+void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	/* TODO: implement */
+}
+
+int arch_uprobe_exception_notify(struct notifier_block *self,
+				 unsigned long val, void *data)
+{
+	return NOTIFY_DONE;
+}
+
+static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
+		uprobe_pre_sstep_notifier(regs);
+	else
+		uprobe_post_sstep_notifier(regs);
+	local_irq_restore(flags);
+
+	return 0;
+}
+
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+	return instruction_pointer(regs);
+}
+
+static struct undef_hook uprobes_arm_break_hook = {
+	.instr_mask	= 0x0fffffff,
+	.instr_val	= UPROBE_SWBP_INSN,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= USR_MODE,
+	.fn		= uprobe_trap_handler,
+};
+
+static struct undef_hook uprobes_arm_ss_hook = {
+	.instr_mask	= 0x0fffffff,
+	.instr_val	= UPROBE_SS_INSN,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= USR_MODE,
+	.fn		= uprobe_trap_handler,
+};
+
+int arch_uprobes_init(void)
+{
+	register_undef_hook(&uprobes_arm_break_hook);
+	register_undef_hook(&uprobes_arm_ss_hook);
+
+	return 0;
+}
-- 
1.7.9.5

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

Add basic uprobes support for ARM.

perf probe --exec and SystemTap's userspace probing work.  The ARM
kprobes test code has also been run in a userspace harness to test the
uprobe instruction decoding.

Caveats:

 - Thumb is not supported
 - XOL abort/trap handling is not implemented

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/Kconfig                   |    4 +
 arch/arm/include/asm/ptrace.h      |    6 ++
 arch/arm/include/asm/thread_info.h |    5 +-
 arch/arm/include/asm/uprobes.h     |   34 +++++++
 arch/arm/kernel/Makefile           |    1 +
 arch/arm/kernel/signal.c           |    4 +
 arch/arm/kernel/uprobes.c          |  191 ++++++++++++++++++++++++++++++++++++
 7 files changed, 244 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/uprobes.h
 create mode 100644 arch/arm/kernel/uprobes.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 272c3a1..2191b61d 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -168,6 +168,10 @@ config ZONE_DMA
 config NEED_DMA_MAP_STATE
        def_bool y
 
+config ARCH_SUPPORTS_UPROBES
+	depends on KPROBES
+	def_bool y
+
 config ARCH_HAS_DMA_SET_COHERENT_MASK
 	bool
 
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index 142d6ae..297936a 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -197,6 +197,12 @@ static inline long regs_return_value(struct pt_regs *regs)
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
 
+static inline void instruction_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	instruction_pointer(regs) = val;
+}
+
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 8477b4c..7bedaee 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -148,6 +148,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define TIF_SIGPENDING		0
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
+#define TIF_UPROBE		7
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
 #define TIF_SYSCALL_TRACEPOINT	10
@@ -160,6 +161,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
+#define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
@@ -172,7 +174,8 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
 /*
  * Change these and you break ASM code in entry-common.S
  */
-#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_RESUME)
+#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
+				 _TIF_NOTIFY_RESUME | _TIF_UPROBE)
 
 #endif /* __KERNEL__ */
 #endif /* __ASM_ARM_THREAD_INFO_H */
diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h
new file mode 100644
index 0000000..fa4b81e
--- /dev/null
+++ b/arch/arm/include/asm/uprobes.h
@@ -0,0 +1,34 @@
+#ifndef _ASM_UPROBES_H
+#define _ASM_UPROBES_H
+
+#include <asm/probes.h>
+
+typedef u32 uprobe_opcode_t;
+
+#define MAX_UINSN_BYTES		4
+#define UPROBE_XOL_SLOT_BYTES	64
+
+#define UPROBE_SWBP_INSN	0x07f001f9
+#define UPROBE_SS_INSN		0x07f001fa
+#define UPROBE_SWBP_INSN_SIZE	4
+
+struct arch_uprobe_task {
+	u32 backup;
+};
+
+struct arch_uprobe {
+	u8 insn[MAX_UINSN_BYTES];
+	uprobe_opcode_t modinsn;
+	uprobe_opcode_t bpinsn;
+	bool simulate;
+	u32 pcreg;
+	void (*prehandler)(struct arch_uprobe *auprobe,
+			   struct arch_uprobe_task *autask,
+			   struct pt_regs *regs);
+	void (*posthandler)(struct arch_uprobe *auprobe,
+			    struct arch_uprobe_task *autask,
+			    struct pt_regs *regs);
+	struct arch_specific_insn asi;
+};
+
+#endif
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 5bbec7b..a39f634 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
+obj-$(CONFIG_UPROBES)		+= uprobes.o uprobes-arm.o kprobes-arm.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
 ifdef CONFIG_THUMB2_KERNEL
 obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 56f72d2..3b1e88e 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -12,6 +12,7 @@
 #include <linux/personality.h>
 #include <linux/uaccess.h>
 #include <linux/tracehook.h>
+#include <linux/uprobes.h>
 
 #include <asm/elf.h>
 #include <asm/cacheflush.h>
@@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 					return restart;
 				}
 				syscall = 0;
+			} else if (thread_flags & _TIF_UPROBE) {
+				clear_thread_flag(TIF_UPROBE);
+				uprobe_notify_resume(regs);
 			} else {
 				clear_thread_flag(TIF_NOTIFY_RESUME);
 				tracehook_notify_resume(regs);
diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
new file mode 100644
index 0000000..f25a4af
--- /dev/null
+++ b/arch/arm/kernel/uprobes.c
@@ -0,0 +1,191 @@
+/*
+ * Copyright (C) 2012 Rabin Vincent <rabin@rab.in>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/uprobes.h>
+#include <linux/notifier.h>
+#include <linux/kprobes.h>
+
+#include <asm/opcodes.h>
+#include <asm/traps.h>
+
+#include "kprobes.h"
+
+bool is_swbp_insn(uprobe_opcode_t *insn)
+{
+	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;
+}
+
+bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) {
+		regs->ARM_pc += 4;
+		return true;
+	}
+
+	return false;
+}
+
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct kprobe kp;
+
+	if (!auprobe->simulate)
+		return false;
+
+	kp.addr = (void *) regs->ARM_pc;
+	kp.opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn);
+	kp.ainsn.insn_handler = auprobe->asi.insn_handler;
+
+	auprobe->asi.insn_singlestep(&kp, regs);
+
+	return true;
+}
+
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+			     unsigned long addr)
+{
+	unsigned int insn;
+	unsigned int bpinsn;
+	enum kprobe_insn ret;
+
+	/* Thumb not yet support */
+	if (addr & 0x3)
+		return -EINVAL;
+
+	insn = __mem_to_opcode_arm(*(unsigned int *)auprobe->insn);
+	auprobe->modinsn = insn;
+
+	ret = arm_kprobe_decode_insn(insn, &auprobe->asi, true);
+	switch (ret) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		break;
+
+	case INSN_GOOD:
+	default:
+		break;
+	}
+
+	bpinsn = UPROBE_SWBP_INSN;
+	if (insn >= 0xe0000000)
+		bpinsn |= 0xe0000000;  /* Unconditional instruction */
+	else
+		bpinsn |= insn & 0xf0000000;  /* Copy condition from insn */
+
+	auprobe->bpinsn = bpinsn;
+
+	return 0;
+}
+
+void arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
+			      uprobe_opcode_t opcode)
+{
+	unsigned long *addr = vaddr;
+
+	if (opcode == UPROBE_SWBP_INSN)
+		opcode = __opcode_to_mem_arm(auprobe->bpinsn);
+
+	*addr = opcode;
+}
+
+void arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
+{
+	unsigned long *addr = vaddr;
+
+	addr[0] = __opcode_to_mem_arm(auprobe->modinsn);
+	addr[1] = __opcode_to_mem_arm(0xe0000000 | UPROBE_SS_INSN);
+}
+
+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	if (auprobe->prehandler)
+		auprobe->prehandler(auprobe, &utask->autask, regs);
+
+	regs->ARM_pc = utask->xol_vaddr;
+
+	return 0;
+}
+
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	regs->ARM_pc = utask->vaddr + 4;
+
+	if (auprobe->posthandler)
+		auprobe->posthandler(auprobe, &utask->autask, regs);
+
+	return 0;
+}
+
+bool arch_uprobe_xol_was_trapped(struct task_struct *t)
+{
+	/* TODO: implement */
+	return false;
+}
+
+void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	/* TODO: implement */
+}
+
+int arch_uprobe_exception_notify(struct notifier_block *self,
+				 unsigned long val, void *data)
+{
+	return NOTIFY_DONE;
+}
+
+static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
+		uprobe_pre_sstep_notifier(regs);
+	else
+		uprobe_post_sstep_notifier(regs);
+	local_irq_restore(flags);
+
+	return 0;
+}
+
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+	return instruction_pointer(regs);
+}
+
+static struct undef_hook uprobes_arm_break_hook = {
+	.instr_mask	= 0x0fffffff,
+	.instr_val	= UPROBE_SWBP_INSN,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= USR_MODE,
+	.fn		= uprobe_trap_handler,
+};
+
+static struct undef_hook uprobes_arm_ss_hook = {
+	.instr_mask	= 0x0fffffff,
+	.instr_val	= UPROBE_SS_INSN,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= USR_MODE,
+	.fn		= uprobe_trap_handler,
+};
+
+int arch_uprobes_init(void)
+{
+	register_undef_hook(&uprobes_arm_break_hook);
+	register_undef_hook(&uprobes_arm_ss_hook);
+
+	return 0;
+}
-- 
1.7.9.5

[PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> Add basic uprobes support for ARM.
> 
> perf probe --exec and SystemTap's userspace probing work.  The ARM
> kprobes test code has also been run in a userspace harness to test the
> uprobe instruction decoding.

The assumption that the target code is ARM appears to be buried all over
the place.

Certainly this code as currently written must depend on !THUMB2_KERNEL.

However, there's an underlying problem here which we'd need to solve.
The kprobes code can take advantage of the fact that the kernel is all
ARM or (almost) all Thumb code.  So there is no support for kprobes
supporting ARM and Thumb at the same time.

With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
and Linaro building Thumb-2 userspaces, it may be an increasingly common
configuration independently of whether the kernel is built in Thumb-2
or not.

Furthermode, there is no such thing as a pure Thumb-2 system in practice:
PLTs are always ARM, for example.  For uprobes to work well in userspace,
both should be supported together.

I only skimmed the patches fairly quickly, so apologies if you do in fact
handle this -- but I don't see any evidence of it right now.

Cheers
---Dave

> 
> Caveats:
> 
>  - Thumb is not supported
>  - XOL abort/trap handling is not implemented
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/Kconfig                   |    4 +
>  arch/arm/include/asm/ptrace.h      |    6 ++
>  arch/arm/include/asm/thread_info.h |    5 +-
>  arch/arm/include/asm/uprobes.h     |   34 +++++++
>  arch/arm/kernel/Makefile           |    1 +
>  arch/arm/kernel/signal.c           |    4 +
>  arch/arm/kernel/uprobes.c          |  191 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 244 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/uprobes.h
>  create mode 100644 arch/arm/kernel/uprobes.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 272c3a1..2191b61d 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -168,6 +168,10 @@ config ZONE_DMA
>  config NEED_DMA_MAP_STATE
>         def_bool y
>  
> +config ARCH_SUPPORTS_UPROBES
> +	depends on KPROBES
> +	def_bool y
> +
>  config ARCH_HAS_DMA_SET_COHERENT_MASK
>  	bool
>  
> diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
> index 142d6ae..297936a 100644
> --- a/arch/arm/include/asm/ptrace.h
> +++ b/arch/arm/include/asm/ptrace.h
> @@ -197,6 +197,12 @@ static inline long regs_return_value(struct pt_regs *regs)
>  
>  #define instruction_pointer(regs)	(regs)->ARM_pc
>  
> +static inline void instruction_pointer_set(struct pt_regs *regs,
> +					   unsigned long val)
> +{
> +	instruction_pointer(regs) = val;
> +}
> +
>  #ifdef CONFIG_SMP
>  extern unsigned long profile_pc(struct pt_regs *regs);
>  #else
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index 8477b4c..7bedaee 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -148,6 +148,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  #define TIF_SIGPENDING		0
>  #define TIF_NEED_RESCHED	1
>  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> +#define TIF_UPROBE		7
>  #define TIF_SYSCALL_TRACE	8
>  #define TIF_SYSCALL_AUDIT	9
>  #define TIF_SYSCALL_TRACEPOINT	10
> @@ -160,6 +161,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
>  #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
>  #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
> +#define _TIF_UPROBE		(1 << TIF_UPROBE)
>  #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
>  #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
>  #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
> @@ -172,7 +174,8 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  /*
>   * Change these and you break ASM code in entry-common.S
>   */
> -#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_RESUME)
> +#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
> +				 _TIF_NOTIFY_RESUME | _TIF_UPROBE)
>  
>  #endif /* __KERNEL__ */
>  #endif /* __ASM_ARM_THREAD_INFO_H */
> diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h
> new file mode 100644
> index 0000000..fa4b81e
> --- /dev/null
> +++ b/arch/arm/include/asm/uprobes.h
> @@ -0,0 +1,34 @@
> +#ifndef _ASM_UPROBES_H
> +#define _ASM_UPROBES_H
> +
> +#include <asm/probes.h>
> +
> +typedef u32 uprobe_opcode_t;
> +
> +#define MAX_UINSN_BYTES		4
> +#define UPROBE_XOL_SLOT_BYTES	64
> +
> +#define UPROBE_SWBP_INSN	0x07f001f9
> +#define UPROBE_SS_INSN		0x07f001fa
> +#define UPROBE_SWBP_INSN_SIZE	4
> +
> +struct arch_uprobe_task {
> +	u32 backup;
> +};
> +
> +struct arch_uprobe {
> +	u8 insn[MAX_UINSN_BYTES];
> +	uprobe_opcode_t modinsn;
> +	uprobe_opcode_t bpinsn;
> +	bool simulate;
> +	u32 pcreg;
> +	void (*prehandler)(struct arch_uprobe *auprobe,
> +			   struct arch_uprobe_task *autask,
> +			   struct pt_regs *regs);
> +	void (*posthandler)(struct arch_uprobe *auprobe,
> +			    struct arch_uprobe_task *autask,
> +			    struct pt_regs *regs);
> +	struct arch_specific_insn asi;
> +};
> +
> +#endif
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 5bbec7b..a39f634 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
>  obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
>  obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
> +obj-$(CONFIG_UPROBES)		+= uprobes.o uprobes-arm.o kprobes-arm.o
>  obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
>  ifdef CONFIG_THUMB2_KERNEL
>  obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 56f72d2..3b1e88e 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -12,6 +12,7 @@
>  #include <linux/personality.h>
>  #include <linux/uaccess.h>
>  #include <linux/tracehook.h>
> +#include <linux/uprobes.h>
>  
>  #include <asm/elf.h>
>  #include <asm/cacheflush.h>
> @@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
>  					return restart;
>  				}
>  				syscall = 0;
> +			} else if (thread_flags & _TIF_UPROBE) {
> +				clear_thread_flag(TIF_UPROBE);
> +				uprobe_notify_resume(regs);
>  			} else {
>  				clear_thread_flag(TIF_NOTIFY_RESUME);
>  				tracehook_notify_resume(regs);
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> new file mode 100644
> index 0000000..f25a4af
> --- /dev/null
> +++ b/arch/arm/kernel/uprobes.c
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright (C) 2012 Rabin Vincent <rabin@rab.in>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/uprobes.h>
> +#include <linux/notifier.h>
> +#include <linux/kprobes.h>
> +
> +#include <asm/opcodes.h>
> +#include <asm/traps.h>
> +
> +#include "kprobes.h"
> +
> +bool is_swbp_insn(uprobe_opcode_t *insn)
> +{
> +	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;
> +}
> +
> +bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) {
> +		regs->ARM_pc += 4;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct kprobe kp;
> +
> +	if (!auprobe->simulate)
> +		return false;
> +
> +	kp.addr = (void *) regs->ARM_pc;
> +	kp.opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn);
> +	kp.ainsn.insn_handler = auprobe->asi.insn_handler;
> +
> +	auprobe->asi.insn_singlestep(&kp, regs);
> +
> +	return true;
> +}
> +
> +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> +			     unsigned long addr)
> +{
> +	unsigned int insn;
> +	unsigned int bpinsn;
> +	enum kprobe_insn ret;
> +
> +	/* Thumb not yet support */
> +	if (addr & 0x3)
> +		return -EINVAL;
> +
> +	insn = __mem_to_opcode_arm(*(unsigned int *)auprobe->insn);
> +	auprobe->modinsn = insn;
> +
> +	ret = arm_kprobe_decode_insn(insn, &auprobe->asi, true);
> +	switch (ret) {
> +	case INSN_REJECTED:
> +		return -EINVAL;
> +
> +	case INSN_GOOD_NO_SLOT:
> +		auprobe->simulate = true;
> +		break;
> +
> +	case INSN_GOOD:
> +	default:
> +		break;
> +	}
> +
> +	bpinsn = UPROBE_SWBP_INSN;
> +	if (insn >= 0xe0000000)
> +		bpinsn |= 0xe0000000;  /* Unconditional instruction */
> +	else
> +		bpinsn |= insn & 0xf0000000;  /* Copy condition from insn */
> +
> +	auprobe->bpinsn = bpinsn;
> +
> +	return 0;
> +}
> +
> +void arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
> +			      uprobe_opcode_t opcode)
> +{
> +	unsigned long *addr = vaddr;
> +
> +	if (opcode == UPROBE_SWBP_INSN)
> +		opcode = __opcode_to_mem_arm(auprobe->bpinsn);
> +
> +	*addr = opcode;
> +}
> +
> +void arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
> +{
> +	unsigned long *addr = vaddr;
> +
> +	addr[0] = __opcode_to_mem_arm(auprobe->modinsn);
> +	addr[1] = __opcode_to_mem_arm(0xe0000000 | UPROBE_SS_INSN);
> +}
> +
> +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	if (auprobe->prehandler)
> +		auprobe->prehandler(auprobe, &utask->autask, regs);
> +
> +	regs->ARM_pc = utask->xol_vaddr;
> +
> +	return 0;
> +}
> +
> +int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	regs->ARM_pc = utask->vaddr + 4;
> +
> +	if (auprobe->posthandler)
> +		auprobe->posthandler(auprobe, &utask->autask, regs);
> +
> +	return 0;
> +}
> +
> +bool arch_uprobe_xol_was_trapped(struct task_struct *t)
> +{
> +	/* TODO: implement */
> +	return false;
> +}
> +
> +void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	/* TODO: implement */
> +}
> +
> +int arch_uprobe_exception_notify(struct notifier_block *self,
> +				 unsigned long val, void *data)
> +{
> +	return NOTIFY_DONE;
> +}
> +
> +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
> +		uprobe_pre_sstep_notifier(regs);
> +	else
> +		uprobe_post_sstep_notifier(regs);
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
> +{
> +	return instruction_pointer(regs);
> +}
> +
> +static struct undef_hook uprobes_arm_break_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SWBP_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +static struct undef_hook uprobes_arm_ss_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SS_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +int arch_uprobes_init(void)
> +{
> +	register_undef_hook(&uprobes_arm_break_hook);
> +	register_undef_hook(&uprobes_arm_ss_hook);
> +
> +	return 0;
> +}
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> Add basic uprobes support for ARM.
> 
> perf probe --exec and SystemTap's userspace probing work.  The ARM
> kprobes test code has also been run in a userspace harness to test the
> uprobe instruction decoding.

The assumption that the target code is ARM appears to be buried all over
the place.

Certainly this code as currently written must depend on !THUMB2_KERNEL.

However, there's an underlying problem here which we'd need to solve.
The kprobes code can take advantage of the fact that the kernel is all
ARM or (almost) all Thumb code.  So there is no support for kprobes
supporting ARM and Thumb at the same time.

With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
and Linaro building Thumb-2 userspaces, it may be an increasingly common
configuration independently of whether the kernel is built in Thumb-2
or not.

Furthermode, there is no such thing as a pure Thumb-2 system in practice:
PLTs are always ARM, for example.  For uprobes to work well in userspace,
both should be supported together.

I only skimmed the patches fairly quickly, so apologies if you do in fact
handle this -- but I don't see any evidence of it right now.

Cheers
---Dave

> 
> Caveats:
> 
>  - Thumb is not supported
>  - XOL abort/trap handling is not implemented
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/Kconfig                   |    4 +
>  arch/arm/include/asm/ptrace.h      |    6 ++
>  arch/arm/include/asm/thread_info.h |    5 +-
>  arch/arm/include/asm/uprobes.h     |   34 +++++++
>  arch/arm/kernel/Makefile           |    1 +
>  arch/arm/kernel/signal.c           |    4 +
>  arch/arm/kernel/uprobes.c          |  191 ++++++++++++++++++++++++++++++++++++
>  7 files changed, 244 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/uprobes.h
>  create mode 100644 arch/arm/kernel/uprobes.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 272c3a1..2191b61d 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -168,6 +168,10 @@ config ZONE_DMA
>  config NEED_DMA_MAP_STATE
>         def_bool y
>  
> +config ARCH_SUPPORTS_UPROBES
> +	depends on KPROBES
> +	def_bool y
> +
>  config ARCH_HAS_DMA_SET_COHERENT_MASK
>  	bool
>  
> diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
> index 142d6ae..297936a 100644
> --- a/arch/arm/include/asm/ptrace.h
> +++ b/arch/arm/include/asm/ptrace.h
> @@ -197,6 +197,12 @@ static inline long regs_return_value(struct pt_regs *regs)
>  
>  #define instruction_pointer(regs)	(regs)->ARM_pc
>  
> +static inline void instruction_pointer_set(struct pt_regs *regs,
> +					   unsigned long val)
> +{
> +	instruction_pointer(regs) = val;
> +}
> +
>  #ifdef CONFIG_SMP
>  extern unsigned long profile_pc(struct pt_regs *regs);
>  #else
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index 8477b4c..7bedaee 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -148,6 +148,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  #define TIF_SIGPENDING		0
>  #define TIF_NEED_RESCHED	1
>  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> +#define TIF_UPROBE		7
>  #define TIF_SYSCALL_TRACE	8
>  #define TIF_SYSCALL_AUDIT	9
>  #define TIF_SYSCALL_TRACEPOINT	10
> @@ -160,6 +161,7 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
>  #define _TIF_NEED_RESCHED	(1 << TIF_NEED_RESCHED)
>  #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
> +#define _TIF_UPROBE		(1 << TIF_UPROBE)
>  #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
>  #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
>  #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
> @@ -172,7 +174,8 @@ extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>  /*
>   * Change these and you break ASM code in entry-common.S
>   */
> -#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | _TIF_NOTIFY_RESUME)
> +#define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
> +				 _TIF_NOTIFY_RESUME | _TIF_UPROBE)
>  
>  #endif /* __KERNEL__ */
>  #endif /* __ASM_ARM_THREAD_INFO_H */
> diff --git a/arch/arm/include/asm/uprobes.h b/arch/arm/include/asm/uprobes.h
> new file mode 100644
> index 0000000..fa4b81e
> --- /dev/null
> +++ b/arch/arm/include/asm/uprobes.h
> @@ -0,0 +1,34 @@
> +#ifndef _ASM_UPROBES_H
> +#define _ASM_UPROBES_H
> +
> +#include <asm/probes.h>
> +
> +typedef u32 uprobe_opcode_t;
> +
> +#define MAX_UINSN_BYTES		4
> +#define UPROBE_XOL_SLOT_BYTES	64
> +
> +#define UPROBE_SWBP_INSN	0x07f001f9
> +#define UPROBE_SS_INSN		0x07f001fa
> +#define UPROBE_SWBP_INSN_SIZE	4
> +
> +struct arch_uprobe_task {
> +	u32 backup;
> +};
> +
> +struct arch_uprobe {
> +	u8 insn[MAX_UINSN_BYTES];
> +	uprobe_opcode_t modinsn;
> +	uprobe_opcode_t bpinsn;
> +	bool simulate;
> +	u32 pcreg;
> +	void (*prehandler)(struct arch_uprobe *auprobe,
> +			   struct arch_uprobe_task *autask,
> +			   struct pt_regs *regs);
> +	void (*posthandler)(struct arch_uprobe *auprobe,
> +			    struct arch_uprobe_task *autask,
> +			    struct pt_regs *regs);
> +	struct arch_specific_insn asi;
> +};
> +
> +#endif
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 5bbec7b..a39f634 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
>  obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
>  obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
>  obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
> +obj-$(CONFIG_UPROBES)		+= uprobes.o uprobes-arm.o kprobes-arm.o
>  obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-common.o patch.o
>  ifdef CONFIG_THUMB2_KERNEL
>  obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 56f72d2..3b1e88e 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -12,6 +12,7 @@
>  #include <linux/personality.h>
>  #include <linux/uaccess.h>
>  #include <linux/tracehook.h>
> +#include <linux/uprobes.h>
>  
>  #include <asm/elf.h>
>  #include <asm/cacheflush.h>
> @@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
>  					return restart;
>  				}
>  				syscall = 0;
> +			} else if (thread_flags & _TIF_UPROBE) {
> +				clear_thread_flag(TIF_UPROBE);
> +				uprobe_notify_resume(regs);
>  			} else {
>  				clear_thread_flag(TIF_NOTIFY_RESUME);
>  				tracehook_notify_resume(regs);
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> new file mode 100644
> index 0000000..f25a4af
> --- /dev/null
> +++ b/arch/arm/kernel/uprobes.c
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright (C) 2012 Rabin Vincent <rabin@rab.in>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/uprobes.h>
> +#include <linux/notifier.h>
> +#include <linux/kprobes.h>
> +
> +#include <asm/opcodes.h>
> +#include <asm/traps.h>
> +
> +#include "kprobes.h"
> +
> +bool is_swbp_insn(uprobe_opcode_t *insn)
> +{
> +	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;
> +}
> +
> +bool arch_uprobe_ignore(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	if (!auprobe->asi.insn_check_cc(regs->ARM_cpsr)) {
> +		regs->ARM_pc += 4;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct kprobe kp;
> +
> +	if (!auprobe->simulate)
> +		return false;
> +
> +	kp.addr = (void *) regs->ARM_pc;
> +	kp.opcode = __mem_to_opcode_arm(*(unsigned int *) auprobe->insn);
> +	kp.ainsn.insn_handler = auprobe->asi.insn_handler;
> +
> +	auprobe->asi.insn_singlestep(&kp, regs);
> +
> +	return true;
> +}
> +
> +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> +			     unsigned long addr)
> +{
> +	unsigned int insn;
> +	unsigned int bpinsn;
> +	enum kprobe_insn ret;
> +
> +	/* Thumb not yet support */
> +	if (addr & 0x3)
> +		return -EINVAL;
> +
> +	insn = __mem_to_opcode_arm(*(unsigned int *)auprobe->insn);
> +	auprobe->modinsn = insn;
> +
> +	ret = arm_kprobe_decode_insn(insn, &auprobe->asi, true);
> +	switch (ret) {
> +	case INSN_REJECTED:
> +		return -EINVAL;
> +
> +	case INSN_GOOD_NO_SLOT:
> +		auprobe->simulate = true;
> +		break;
> +
> +	case INSN_GOOD:
> +	default:
> +		break;
> +	}
> +
> +	bpinsn = UPROBE_SWBP_INSN;
> +	if (insn >= 0xe0000000)
> +		bpinsn |= 0xe0000000;  /* Unconditional instruction */
> +	else
> +		bpinsn |= insn & 0xf0000000;  /* Copy condition from insn */
> +
> +	auprobe->bpinsn = bpinsn;
> +
> +	return 0;
> +}
> +
> +void arch_uprobe_write_opcode(struct arch_uprobe *auprobe, void *vaddr,
> +			      uprobe_opcode_t opcode)
> +{
> +	unsigned long *addr = vaddr;
> +
> +	if (opcode == UPROBE_SWBP_INSN)
> +		opcode = __opcode_to_mem_arm(auprobe->bpinsn);
> +
> +	*addr = opcode;
> +}
> +
> +void arch_uprobe_xol_copy(struct arch_uprobe *auprobe, void *vaddr)
> +{
> +	unsigned long *addr = vaddr;
> +
> +	addr[0] = __opcode_to_mem_arm(auprobe->modinsn);
> +	addr[1] = __opcode_to_mem_arm(0xe0000000 | UPROBE_SS_INSN);
> +}
> +
> +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	if (auprobe->prehandler)
> +		auprobe->prehandler(auprobe, &utask->autask, regs);
> +
> +	regs->ARM_pc = utask->xol_vaddr;
> +
> +	return 0;
> +}
> +
> +int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	regs->ARM_pc = utask->vaddr + 4;
> +
> +	if (auprobe->posthandler)
> +		auprobe->posthandler(auprobe, &utask->autask, regs);
> +
> +	return 0;
> +}
> +
> +bool arch_uprobe_xol_was_trapped(struct task_struct *t)
> +{
> +	/* TODO: implement */
> +	return false;
> +}
> +
> +void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	/* TODO: implement */
> +}
> +
> +int arch_uprobe_exception_notify(struct notifier_block *self,
> +				 unsigned long val, void *data)
> +{
> +	return NOTIFY_DONE;
> +}
> +
> +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
> +		uprobe_pre_sstep_notifier(regs);
> +	else
> +		uprobe_post_sstep_notifier(regs);
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
> +{
> +	return instruction_pointer(regs);
> +}
> +
> +static struct undef_hook uprobes_arm_break_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SWBP_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +static struct undef_hook uprobes_arm_ss_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SS_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +int arch_uprobes_init(void)
> +{
> +	register_undef_hook(&uprobes_arm_break_hook);
> +	register_undef_hook(&uprobes_arm_ss_hook);
> +
> +	return 0;
> +}
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

2012/10/15 Dave Martin <dave.martin@linaro.org>:
> On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
>> Add basic uprobes support for ARM.
>>
>> perf probe --exec and SystemTap's userspace probing work.  The ARM
>> kprobes test code has also been run in a userspace harness to test the
>> uprobe instruction decoding.
>
> The assumption that the target code is ARM appears to be buried all over
> the place.

Right, as stated:

>> Caveats:
>>  - Thumb is not supported

> Certainly this code as currently written must depend on !THUMB2_KERNEL.

Why?  It currently works for ARM userspace even if the kernel is
Thumb-2.

> However, there's an underlying problem here which we'd need to solve.
> The kprobes code can take advantage of the fact that the kernel is all
> ARM or (almost) all Thumb code.  So there is no support for kprobes
> supporting ARM and Thumb at the same time.
>
> With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
> and Linaro building Thumb-2 userspaces, it may be an increasingly common
> configuration independently of whether the kernel is built in Thumb-2
> or not.
>
> Furthermode, there is no such thing as a pure Thumb-2 system in practice:
> PLTs are always ARM, for example.  For uprobes to work well in userspace,
> both should be supported together.

Right.  I don't think it's difficult to support both of them together,
my thought was just to have userspace tell us if they want to probe a
Thumb instruction via the usual 0th-bit set convention, and then take it
from there.  I didn't do the Thumb handling currently because I didn't
really look into modifying the kprobes Thumb instruction decoding and
the IT handling for uprobes.

Re: [PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

2012/10/15 Dave Martin <dave.martin@linaro.org>:
> On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
>> Add basic uprobes support for ARM.
>>
>> perf probe --exec and SystemTap's userspace probing work.  The ARM
>> kprobes test code has also been run in a userspace harness to test the
>> uprobe instruction decoding.
>
> The assumption that the target code is ARM appears to be buried all over
> the place.

Right, as stated:

>> Caveats:
>>  - Thumb is not supported

> Certainly this code as currently written must depend on !THUMB2_KERNEL.

Why?  It currently works for ARM userspace even if the kernel is
Thumb-2.

> However, there's an underlying problem here which we'd need to solve.
> The kprobes code can take advantage of the fact that the kernel is all
> ARM or (almost) all Thumb code.  So there is no support for kprobes
> supporting ARM and Thumb at the same time.
>
> With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
> and Linaro building Thumb-2 userspaces, it may be an increasingly common
> configuration independently of whether the kernel is built in Thumb-2
> or not.
>
> Furthermode, there is no such thing as a pure Thumb-2 system in practice:
> PLTs are always ARM, for example.  For uprobes to work well in userspace,
> both should be supported together.

Right.  I don't think it's difficult to support both of them together,
my thought was just to have userspace tell us if they want to probe a
Thumb instruction via the usual 0th-bit set convention, and then take it
from there.  I didn't do the Thumb handling currently because I didn't
really look into modifying the kprobes Thumb instruction decoding and
the IT handling for uprobes.

[PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> 2012/10/15 Dave Martin <dave.martin@linaro.org>:
> > On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> >> Add basic uprobes support for ARM.
> >>
> >> perf probe --exec and SystemTap's userspace probing work.  The ARM
> >> kprobes test code has also been run in a userspace harness to test the
> >> uprobe instruction decoding.
> >
> > The assumption that the target code is ARM appears to be buried all over
> > the place.
> 
> Right, as stated:
> 
> >> Caveats:
> >>  - Thumb is not supported
> 
> > Certainly this code as currently written must depend on !THUMB2_KERNEL.
> 
> Why?  It currently works for ARM userspace even if the kernel is
> Thumb-2.

My bad, I misread what was happening in the Makefile changes.

My concern is about whether we can build the ARM and Thumb-2 kprobes
code into the same kernel.  If so, no problem, but I believe this is
not a tested configuration for kprobes itself.

If you've not already done so, it should be possible to test this
by adding CONFIG_THUMB2_KERNEL=y to your config, providing your
hardware is Thumb-2 capable.

> > However, there's an underlying problem here which we'd need to solve.
> > The kprobes code can take advantage of the fact that the kernel is all
> > ARM or (almost) all Thumb code.  So there is no support for kprobes
> > supporting ARM and Thumb at the same time.
> >
> > With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
> > and Linaro building Thumb-2 userspaces, it may be an increasingly common
> > configuration independently of whether the kernel is built in Thumb-2
> > or not.
> >
> > Furthermode, there is no such thing as a pure Thumb-2 system in practice:
> > PLTs are always ARM, for example.  For uprobes to work well in userspace,
> > both should be supported together.
> 
> Right.  I don't think it's difficult to support both of them together,
> my thought was just to have userspace tell us if they want to probe a
> Thumb instruction via the usual 0th-bit set convention, and then take it
> from there.  I didn't do the Thumb handling currently because I didn't
> really look into modifying the kprobes Thumb instruction decoding and
> the IT handling for uprobes.

Having looked at the code a little more closely, it looks more closely
aligned with this goal than I thought at first.

I agree with your suggestion to require the caller to specify explicitly
whether they want to insert an ARM or Thumb probe (indeed, I think
there's no other way to do it, since guessing the target instruction
set can't be done reliably).  The code for setting a probe can then
switch trivially on that low bit.

Additional undef_hooks would be needed for the Thumb case, but this
looks relatively straightforward, as you say.


General question which I'm not sure I understand yet: is is possible
to combine uprobes/kprobes decode more completely?  It's not obvious
to me whether the uprobes-specific decoding only relates to features
which architecturally execute differently in user mode versus
privileged mode.  Some explanation somewhere could be helpful.

Cheers
---Dave

Re: [PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> 2012/10/15 Dave Martin <dave.martin@linaro.org>:
> > On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> >> Add basic uprobes support for ARM.
> >>
> >> perf probe --exec and SystemTap's userspace probing work.  The ARM
> >> kprobes test code has also been run in a userspace harness to test the
> >> uprobe instruction decoding.
> >
> > The assumption that the target code is ARM appears to be buried all over
> > the place.
> 
> Right, as stated:
> 
> >> Caveats:
> >>  - Thumb is not supported
> 
> > Certainly this code as currently written must depend on !THUMB2_KERNEL.
> 
> Why?  It currently works for ARM userspace even if the kernel is
> Thumb-2.

My bad, I misread what was happening in the Makefile changes.

My concern is about whether we can build the ARM and Thumb-2 kprobes
code into the same kernel.  If so, no problem, but I believe this is
not a tested configuration for kprobes itself.

If you've not already done so, it should be possible to test this
by adding CONFIG_THUMB2_KERNEL=y to your config, providing your
hardware is Thumb-2 capable.

> > However, there's an underlying problem here which we'd need to solve.
> > The kprobes code can take advantage of the fact that the kernel is all
> > ARM or (almost) all Thumb code.  So there is no support for kprobes
> > supporting ARM and Thumb at the same time.
> >
> > With userspace, we don't have this luxury.  With Debian armhf, Ubuntu
> > and Linaro building Thumb-2 userspaces, it may be an increasingly common
> > configuration independently of whether the kernel is built in Thumb-2
> > or not.
> >
> > Furthermode, there is no such thing as a pure Thumb-2 system in practice:
> > PLTs are always ARM, for example.  For uprobes to work well in userspace,
> > both should be supported together.
> 
> Right.  I don't think it's difficult to support both of them together,
> my thought was just to have userspace tell us if they want to probe a
> Thumb instruction via the usual 0th-bit set convention, and then take it
> from there.  I didn't do the Thumb handling currently because I didn't
> really look into modifying the kprobes Thumb instruction decoding and
> the IT handling for uprobes.

Having looked at the code a little more closely, it looks more closely
aligned with this goal than I thought at first.

I agree with your suggestion to require the caller to specify explicitly
whether they want to insert an ARM or Thumb probe (indeed, I think
there's no other way to do it, since guessing the target instruction
set can't be done reliably).  The code for setting a probe can then
switch trivially on that low bit.

Additional undef_hooks would be needed for the Thumb case, but this
looks relatively straightforward, as you say.


General question which I'm not sure I understand yet: is is possible
to combine uprobes/kprobes decode more completely?  It's not obvious
to me whether the uprobes-specific decoding only relates to features
which architecturally execute differently in user mode versus
privileged mode.  Some explanation somewhere could be helpful.

Cheers
---Dave

[PATCH 9/9] ARM: add uprobes support

[Jon Medhurst (Tixy)]

On Mon, 2012-10-15 at 18:44 +0100, Dave Martin wrote:
> On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> > 2012/10/15 Dave Martin <dave.martin@linaro.org>:
> > > On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> > >> Add basic uprobes support for ARM.
> > >>
> > >> perf probe --exec and SystemTap's userspace probing work.  The ARM
> > >> kprobes test code has also been run in a userspace harness to test the
> > >> uprobe instruction decoding.
> > >
> > > The assumption that the target code is ARM appears to be buried all over
> > > the place.
> > 
> > Right, as stated:
> > 
> > >> Caveats:
> > >>  - Thumb is not supported
> > 
> > > Certainly this code as currently written must depend on !THUMB2_KERNEL.
> > 
> > Why?  It currently works for ARM userspace even if the kernel is
> > Thumb-2.
> 
> My bad, I misread what was happening in the Makefile changes.
> 
> My concern is about whether we can build the ARM and Thumb-2 kprobes
> code into the same kernel.  If so, no problem, but I believe this is
> not a tested configuration for kprobes itself.

When reworking kprobes I originally started by having ARM instruction
support in Thumb kernels (with all test cases working) and, if I
remember correct, this got dropped because we had difficulty in coming
up with a robust way of specifying whether a probe pointer was in Thumb
code or not. (Different methods of getting pointers to Thumb code didn't
always set bit 0 so we 'solved' this by ignoring bit 0 and assuming all
code in Thumb kernels was Thumb.)

<snip>
> General question which I'm not sure I understand yet: is is possible
> to combine uprobes/kprobes decode more completely?  It's not obvious
> to me whether the uprobes-specific decoding only relates to features
> which architecturally execute differently in user mode versus
> privileged mode.  Some explanation somewhere could be helpful.

I just been looking at the decoding changes in patch 8 and had similar
thoughts. The patch as it stands looks rather bolted on the side and
makes the resulting code rather messy. My initial thoughts are that
either:

a) uprobes is similar enough to kprobes that the existing code can be
morphed into something that cleanly supports both, or

b) the similarities aren't close enough and that we should factor out
the similarities into a more generalised decoding base, which the
{u,k}probe code can then build on.

c) some mix of a) and b)

I can't help but think of the various calls over the past year or so for
a general ARM/Thumb instruction decoding framework (the last one only a
few weeks ago on the linux-arm-kernel list). Perhaps b) would be a small
step towards that.

I hope to find some time to understand the uprobe patches in more
detail, so I can try and come up with some sensible suggestions on a
cleaner solution; because I feel that as they stand they aren't really
suitable for inclusion in the kernel.

Rabin, what tree/commit are your patches based on? (They don't seem to
apply cleanly to 3.6 or 3.7-rc1.) I want to apply them locally so I can
use my favourite visualisation tool and to play with them.

Thanks

-- 
Tixy

Re: [PATCH 9/9] ARM: add uprobes support

[Jon Medhurst (Tixy)]

On Mon, 2012-10-15 at 18:44 +0100, Dave Martin wrote:
> On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> > 2012/10/15 Dave Martin <dave.martin@linaro.org>:
> > > On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> > >> Add basic uprobes support for ARM.
> > >>
> > >> perf probe --exec and SystemTap's userspace probing work.  The ARM
> > >> kprobes test code has also been run in a userspace harness to test the
> > >> uprobe instruction decoding.
> > >
> > > The assumption that the target code is ARM appears to be buried all over
> > > the place.
> > 
> > Right, as stated:
> > 
> > >> Caveats:
> > >>  - Thumb is not supported
> > 
> > > Certainly this code as currently written must depend on !THUMB2_KERNEL.
> > 
> > Why?  It currently works for ARM userspace even if the kernel is
> > Thumb-2.
> 
> My bad, I misread what was happening in the Makefile changes.
> 
> My concern is about whether we can build the ARM and Thumb-2 kprobes
> code into the same kernel.  If so, no problem, but I believe this is
> not a tested configuration for kprobes itself.

When reworking kprobes I originally started by having ARM instruction
support in Thumb kernels (with all test cases working) and, if I
remember correct, this got dropped because we had difficulty in coming
up with a robust way of specifying whether a probe pointer was in Thumb
code or not. (Different methods of getting pointers to Thumb code didn't
always set bit 0 so we 'solved' this by ignoring bit 0 and assuming all
code in Thumb kernels was Thumb.)

<snip>
> General question which I'm not sure I understand yet: is is possible
> to combine uprobes/kprobes decode more completely?  It's not obvious
> to me whether the uprobes-specific decoding only relates to features
> which architecturally execute differently in user mode versus
> privileged mode.  Some explanation somewhere could be helpful.

I just been looking at the decoding changes in patch 8 and had similar
thoughts. The patch as it stands looks rather bolted on the side and
makes the resulting code rather messy. My initial thoughts are that
either:

a) uprobes is similar enough to kprobes that the existing code can be
morphed into something that cleanly supports both, or

b) the similarities aren't close enough and that we should factor out
the similarities into a more generalised decoding base, which the
{u,k}probe code can then build on.

c) some mix of a) and b)

I can't help but think of the various calls over the past year or so for
a general ARM/Thumb instruction decoding framework (the last one only a
few weeks ago on the linux-arm-kernel list). Perhaps b) would be a small
step towards that.

I hope to find some time to understand the uprobe patches in more
detail, so I can try and come up with some sensible suggestions on a
cleaner solution; because I feel that as they stand they aren't really
suitable for inclusion in the kernel.

Rabin, what tree/commit are your patches based on? (They don't seem to
apply cleanly to 3.6 or 3.7-rc1.) I want to apply them locally so I can
use my favourite visualisation tool and to play with them.

Thanks

-- 
Tixy

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Wed, Oct 17, 2012 at 03:50:48PM +0100, Jon Medhurst (Tixy) wrote:
> I just been looking at the decoding changes in patch 8 and had similar
> thoughts. The patch as it stands looks rather bolted on the side and
> makes the resulting code rather messy.

I agree.

> a) uprobes is similar enough to kprobes that the existing code can be
> morphed into something that cleanly supports both, or
> 
> b) the similarities aren't close enough and that we should factor out
> the similarities into a more generalised decoding base, which the
> {u,k}probe code can then build on.
> 
> c) some mix of a) and b)
> 
> I can't help but think of the various calls over the past year or so for
> a general ARM/Thumb instruction decoding framework (the last one only a
> few weeks ago on the linux-arm-kernel list). Perhaps b) would be a small
> step towards that.
> 
> I hope to find some time to understand the uprobe patches in more
> detail, so I can try and come up with some sensible suggestions on a
> cleaner solution; because I feel that as they stand they aren't really
> suitable for inclusion in the kernel.

I contemplated sending the decoding patch with [RFC] but finally went
with [PATCH] since they mostly mean the same thing :-).

Suggestions welcome.  For one thing, the creation of a fake struct
kprobe from within the uprobes and the dependency on kprobes because of
that is not very nice, we probably need a "struct probe" of some sort
perhaps.

> Rabin, what tree/commit are your patches based on? (They don't seem to
> apply cleanly to 3.6 or 3.7-rc1.) I want to apply them locally so I can
> use my favourite visualisation tool and to play with them.

The patches are based on next-20121012.  The uprobes core is seeing
quite a few changes in linux-next so the series will probably not apply
on later linux-next trees.

Re: [PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Wed, Oct 17, 2012 at 03:50:48PM +0100, Jon Medhurst (Tixy) wrote:
> I just been looking at the decoding changes in patch 8 and had similar
> thoughts. The patch as it stands looks rather bolted on the side and
> makes the resulting code rather messy.

I agree.

> a) uprobes is similar enough to kprobes that the existing code can be
> morphed into something that cleanly supports both, or
> 
> b) the similarities aren't close enough and that we should factor out
> the similarities into a more generalised decoding base, which the
> {u,k}probe code can then build on.
> 
> c) some mix of a) and b)
> 
> I can't help but think of the various calls over the past year or so for
> a general ARM/Thumb instruction decoding framework (the last one only a
> few weeks ago on the linux-arm-kernel list). Perhaps b) would be a small
> step towards that.
> 
> I hope to find some time to understand the uprobe patches in more
> detail, so I can try and come up with some sensible suggestions on a
> cleaner solution; because I feel that as they stand they aren't really
> suitable for inclusion in the kernel.

I contemplated sending the decoding patch with [RFC] but finally went
with [PATCH] since they mostly mean the same thing :-).

Suggestions welcome.  For one thing, the creation of a fake struct
kprobe from within the uprobes and the dependency on kprobes because of
that is not very nice, we probably need a "struct probe" of some sort
perhaps.

> Rabin, what tree/commit are your patches based on? (They don't seem to
> apply cleanly to 3.6 or 3.7-rc1.) I want to apply them locally so I can
> use my favourite visualisation tool and to play with them.

The patches are based on next-20121012.  The uprobes core is seeing
quite a few changes in linux-next so the series will probably not apply
on later linux-next trees.

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Mon, Oct 15, 2012 at 06:44:50PM +0100, Dave Martin wrote:
> On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> > Why?  It currently works for ARM userspace even if the kernel is
> > Thumb-2.
> 
> My bad, I misread what was happening in the Makefile changes.
> 
> My concern is about whether we can build the ARM and Thumb-2 kprobes
> code into the same kernel.  If so, no problem, but I believe this is
> not a tested configuration for kprobes itself.
> 
> If you've not already done so, it should be possible to test this
> by adding CONFIG_THUMB2_KERNEL=y to your config, providing your
> hardware is Thumb-2 capable.

I've tested this before.  With a Thumb-2 kernel, both the kprobes test
(Thumb) and the uprobes test (ARM) run fine.

> General question which I'm not sure I understand yet: is is possible
> to combine uprobes/kprobes decode more completely?  It's not obvious
> to me whether the uprobes-specific decoding only relates to features
> which architecturally execute differently in user mode versus
> privileged mode.  Some explanation somewhere could be helpful.

What we change is not the decoding itself but the handling of the
instructions:

  (1) Load and stores are executed from the xol area by user space so
  the instructions need to be rewritten when they touch the PC.  Kprobes
  code rewrites the instructions directly and executes them or in some
  cases simulates them.

  (2) All other non-simulated instructions are also executed from the
  XOL area in userspace.  Because of this, the ALU instructions which
  use the PC also need to be rewritten to not use the PC.  Perhaps we
  can actually get rid of this and just execute these instruction from
  kernel space like it is done for uprobes.

  So the uprobes code is uses the decoding tables just to know if the
  instruction is using the PC or not, but if we make the ALU
  instructions execute from kernel space we could actually use the
  emulate_*() functions like kprobes does.

Re: [PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Mon, Oct 15, 2012 at 06:44:50PM +0100, Dave Martin wrote:
> On Mon, Oct 15, 2012 at 01:44:55PM +0200, Rabin Vincent wrote:
> > Why?  It currently works for ARM userspace even if the kernel is
> > Thumb-2.
> 
> My bad, I misread what was happening in the Makefile changes.
> 
> My concern is about whether we can build the ARM and Thumb-2 kprobes
> code into the same kernel.  If so, no problem, but I believe this is
> not a tested configuration for kprobes itself.
> 
> If you've not already done so, it should be possible to test this
> by adding CONFIG_THUMB2_KERNEL=y to your config, providing your
> hardware is Thumb-2 capable.

I've tested this before.  With a Thumb-2 kernel, both the kprobes test
(Thumb) and the uprobes test (ARM) run fine.

> General question which I'm not sure I understand yet: is is possible
> to combine uprobes/kprobes decode more completely?  It's not obvious
> to me whether the uprobes-specific decoding only relates to features
> which architecturally execute differently in user mode versus
> privileged mode.  Some explanation somewhere could be helpful.

What we change is not the decoding itself but the handling of the
instructions:

  (1) Load and stores are executed from the xol area by user space so
  the instructions need to be rewritten when they touch the PC.  Kprobes
  code rewrites the instructions directly and executes them or in some
  cases simulates them.

  (2) All other non-simulated instructions are also executed from the
  XOL area in userspace.  Because of this, the ALU instructions which
  use the PC also need to be rewritten to not use the PC.  Perhaps we
  can actually get rid of this and just execute these instruction from
  kernel space like it is done for uprobes.

  So the uprobes code is uses the decoding tables just to know if the
  instruction is using the PC or not, but if we make the ALU
  instructions execute from kernel space we could actually use the
  emulate_*() functions like kprobes does.

[PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> Add basic uprobes support for ARM.
> 
> perf probe --exec and SystemTap's userspace probing work.  The ARM
> kprobes test code has also been run in a userspace harness to test the
> uprobe instruction decoding.
> 
> Caveats:
> 
>  - Thumb is not supported
>  - XOL abort/trap handling is not implemented

[...]

> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> new file mode 100644
> index 0000000..f25a4af
> --- /dev/null
> +++ b/arch/arm/kernel/uprobes.c

[...]

> +bool is_swbp_insn(uprobe_opcode_t *insn)
> +{
> +	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;

You should take care not to match any instruction whose top bits are
0xF0000000.  That could be some completely different instruction.

[...]

> +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)

Is the check unnecessary here?  I think the same comparison will
happen as a result of evaluating the associated undef_hook.

However, as above you must still check for and reject cases where
(instr & 0xF0000000) == 0xF0000000.

[...]

> +static struct undef_hook uprobes_arm_break_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SWBP_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +static struct undef_hook uprobes_arm_ss_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SS_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};

Re: [PATCH 9/9] ARM: add uprobes support

[Dave Martin]

On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> Add basic uprobes support for ARM.
> 
> perf probe --exec and SystemTap's userspace probing work.  The ARM
> kprobes test code has also been run in a userspace harness to test the
> uprobe instruction decoding.
> 
> Caveats:
> 
>  - Thumb is not supported
>  - XOL abort/trap handling is not implemented

[...]

> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> new file mode 100644
> index 0000000..f25a4af
> --- /dev/null
> +++ b/arch/arm/kernel/uprobes.c

[...]

> +bool is_swbp_insn(uprobe_opcode_t *insn)
> +{
> +	return (__mem_to_opcode_arm(*insn) & 0x0fffffff) == UPROBE_SWBP_INSN;

You should take care not to match any instruction whose top bits are
0xF0000000.  That could be some completely different instruction.

[...]

> +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)

Is the check unnecessary here?  I think the same comparison will
happen as a result of evaluating the associated undef_hook.

However, as above you must still check for and reject cases where
(instr & 0xF0000000) == 0xF0000000.

[...]

> +static struct undef_hook uprobes_arm_break_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SWBP_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};
> +
> +static struct undef_hook uprobes_arm_ss_hook = {
> +	.instr_mask	= 0x0fffffff,
> +	.instr_val	= UPROBE_SS_INSN,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= USR_MODE,
> +	.fn		= uprobe_trap_handler,
> +};

[PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Mon, Oct 15, 2012 at 06:31:47PM +0100, Dave Martin wrote:
> On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> > +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
> 
> Is the check unnecessary here?  I think the same comparison will
> happen as a result of evaluating the associated undef_hook.

The check is there because this uprobe_trap_handler() is registered for
two different undefined instructions: UPROBE_SWBP_INSN (the one which is
used to insert the probe) and UPROBE_SS_INSN (the one placed in the XOL
area for simulating single-stepping).

Re: [PATCH 9/9] ARM: add uprobes support

[Rabin Vincent]

On Mon, Oct 15, 2012 at 06:31:47PM +0100, Dave Martin wrote:
> On Sun, Oct 14, 2012 at 09:23:13PM +0200, Rabin Vincent wrote:
> > +static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
> > +{
> > +	unsigned long flags;
> > +
> > +	local_irq_save(flags);
> > +	if ((instr & 0x0fffffff) == UPROBE_SWBP_INSN)
> 
> Is the check unnecessary here?  I think the same comparison will
> happen as a result of evaluating the associated undef_hook.

The check is there because this uprobe_trap_handler() is registered for
two different undefined instructions: UPROBE_SWBP_INSN (the one which is
used to insert the probe) and UPROBE_SS_INSN (the one placed in the XOL
area for simulating single-stepping).

[PATCH 9/9] ARM: add uprobes support

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> @@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
>  					return restart;
>  				}
>  				syscall = 0;
> +			} else if (thread_flags & _TIF_UPROBE) {
> +				clear_thread_flag(TIF_UPROBE);
> +				uprobe_notify_resume(regs);
>  			} else {
>  				clear_thread_flag(TIF_NOTIFY_RESUME);
>  				tracehook_notify_resume(regs);

This doesn't look right. do_signal() can modify instruction pointer
after we hit the breakpoint. IOW, uprobe_notify_resume() should be
called before do_signal().

Oleg.

Re: [PATCH 9/9] ARM: add uprobes support

[Oleg Nesterov]

On 10/14, Rabin Vincent wrote:
>
> @@ -655,6 +656,9 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
>  					return restart;
>  				}
>  				syscall = 0;
> +			} else if (thread_flags & _TIF_UPROBE) {
> +				clear_thread_flag(TIF_UPROBE);
> +				uprobe_notify_resume(regs);
>  			} else {
>  				clear_thread_flag(TIF_NOTIFY_RESUME);
>  				tracehook_notify_resume(regs);

This doesn't look right. do_signal() can modify instruction pointer
after we hit the breakpoint. IOW, uprobe_notify_resume() should be
called before do_signal().

Oleg.

[PATCH 1/9] uprobes: move function declarations out of arch

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:05]:

> It seems odd to keep the function declarations in the arch header where
> they will need to be copy/pasted verbatim across arches.  Move them to
> the common header.
> 



> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/x86/include/asm/uprobes.h |    6 ------
>  include/linux/uprobes.h        |    8 ++++++++
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 

You need to take care of the powerpc port
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=8b7b80b9ebb46dd88fbb94e918297295cf312b59

Also as Oleg pointed out, He has already posted changes on top of -tip.
So it probably makes sense to redo these patches on top of those
patches.

-- 
Thanks and Regards
Srikar

Re: [PATCH 1/9] uprobes: move function declarations out of arch

[Srikar Dronamraju]

* Rabin Vincent <rabin@rab.in> [2012-10-14 21:23:05]:

> It seems odd to keep the function declarations in the arch header where
> they will need to be copy/pasted verbatim across arches.  Move them to
> the common header.
> 



> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/x86/include/asm/uprobes.h |    6 ------
>  include/linux/uprobes.h        |    8 ++++++++
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 

You need to take care of the powerpc port
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=8b7b80b9ebb46dd88fbb94e918297295cf312b59

Also as Oleg pointed out, He has already posted changes on top of -tip.
So it probably makes sense to redo these patches on top of those
patches.

-- 
Thanks and Regards
Srikar

[PATCH 1/9] uprobes: move function declarations out of arch

[Rabin Vincent]

2012/10/15 Srikar Dronamraju <srikar@linux.vnet.ibm.com>:
> You need to take care of the powerpc port
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=8b7b80b9ebb46dd88fbb94e918297295cf312b59
>
> Also as Oleg pointed out, He has already posted changes on top of -tip.
> So it probably makes sense to redo these patches on top of those
> patches.

OK, will look into it.

Re: [PATCH 1/9] uprobes: move function declarations out of arch

[Rabin Vincent]

2012/10/15 Srikar Dronamraju <srikar@linux.vnet.ibm.com>:
> You need to take care of the powerpc port
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=8b7b80b9ebb46dd88fbb94e918297295cf312b59
>
> Also as Oleg pointed out, He has already posted changes on top of -tip.
> So it probably makes sense to redo these patches on top of those
> patches.

OK, will look into it.