[BUGFIX PATCH 0/3] kprobes/arm: Improve kprobes implementation on arm

[BUGFIX PATCH 0/3] kprobes/arm: Improve kprobes implementation on arm

[Masami Hiramatsu]

Hi,

Here are patches which improve kprobe on arm implementation.
This includes some improves ported from x86 for multiple
kretprobes on same function and recursing kprobes on FIQ
(NMI) path. Also, I've fixed a bug(?) on recursing path.

- [1/3]: Port an improvement (and fix) for recursing kprobe
         on single-stepping by probing FIQ/NMI context.
- [2/3]: Skip single-stepping (and counting nmissed) if
         the recursing kprobe was hit on a conditional
         instruction which should not be executed.
- [3/3]: Fix to show correct return address with multiple
         kretprobe events on same function.

David, I think arm64 also has some conditinal instructions
which should be skipped to handle it and single stepping
if the condition is false. Or, user will see the probe
events even when the instruction is not executed.

Thank you,

---

Masami Hiramatsu (3):
      kprobes/arm: Allow to handle reentered kprobe on single-stepping
      kprobes/arm: Skip single-stepping in recursing path if possible
      kprobes/arm: Fix the return address of multiple kretprobes


 arch/arm/probes/kprobes/core.c |   49 +++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 11 deletions(-)

--
Masami Hiramatsu

[BUGFIX PATCH 1/3] kprobes/arm: Allow to handle reentered kprobe on single-stepping

[Masami Hiramatsu]

This is arm port of commit 6a5022a56ac3 ("kprobes/x86: Allow to
handle reentered kprobe on single-stepping")

Since the FIQ handlers can interrupt in the single stepping
(or preparing the single stepping, do_debug etc.), we should
consider a kprobe is hit in the NMI handler. Even in that
case, the kprobe is allowed to be reentered as same as the
kprobes hit in kprobe handlers
(KPROBE_HIT_ACTIVE or KPROBE_HIT_SSDONE).

The real issue will happen when a kprobe hit while another
reentered kprobe is processing (KPROBE_REENTER), because
we already consumed a saved-area for the previous kprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/probes/kprobes/core.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index a4ec240..264fedb 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -270,6 +270,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 			switch (kcb->kprobe_status) {
 			case KPROBE_HIT_ACTIVE:
 			case KPROBE_HIT_SSDONE:
+			case KPROBE_HIT_SS:
 				/* A pre- or post-handler probe got us here. */
 				kprobes_inc_nmissed_count(p);
 				save_previous_kprobe(kcb);
@@ -278,6 +279,11 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				singlestep(p, regs, kcb);
 				restore_previous_kprobe(kcb);
 				break;
+			case KPROBE_REENTER:
+				/* A nested probe was hit in FIQ, it is a BUG */
+				pr_warn("Unrecoverable kprobe detected at %p.\n",
+					p->addr);
+				/* fall through */
 			default:
 				/* impossible cases */
 				BUG();

Re: [BUGFIX PATCH 1/3] kprobes/arm: Allow to handle reentered kprobe on single-stepping

[Jon Medhurst (Tixy)]

On Tue, 2017-02-14 at 00:03 +0900, Masami Hiramatsu wrote:
> This is arm port of commit 6a5022a56ac3 ("kprobes/x86: Allow to
> handle reentered kprobe on single-stepping")
> 
> Since the FIQ handlers can interrupt in the single stepping
> (or preparing the single stepping, do_debug etc.), we should
> consider a kprobe is hit in the NMI handler. Even in that
> case, the kprobe is allowed to be reentered as same as the
> kprobes hit in kprobe handlers
> (KPROBE_HIT_ACTIVE or KPROBE_HIT_SSDONE).
> 
> The real issue will happen when a kprobe hit while another

Could to with 'is' being inserted above  ^^^
(I know this is a copy of the x86 commit message)

> reentered kprobe is processing (KPROBE_REENTER), because
> we already consumed a saved-area for the previous kprobe.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---

Acked-by: Jon Medhurst <tixy@linaro.org>

>  arch/arm/probes/kprobes/core.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index a4ec240..264fedb 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -270,6 +270,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
>  			switch (kcb->kprobe_status) {
>  			case KPROBE_HIT_ACTIVE:
>  			case KPROBE_HIT_SSDONE:
> +			case KPROBE_HIT_SS:
>  				/* A pre- or post-handler probe got us here. */
>  				kprobes_inc_nmissed_count(p);
>  				save_previous_kprobe(kcb);
> @@ -278,6 +279,11 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
>  				singlestep(p, regs, kcb);
>  				restore_previous_kprobe(kcb);
>  				break;
> +			case KPROBE_REENTER:
> +				/* A nested probe was hit in FIQ, it is a BUG */
> +				pr_warn("Unrecoverable kprobe detected at %p.\n",
> +					p->addr);
> +				/* fall through */
>  			default:
>  				/* impossible cases */
>  				BUG();
> 

Re: [BUGFIX PATCH 1/3] kprobes/arm: Allow to handle reentered kprobe on single-stepping

[Masami Hiramatsu]

On Tue, 14 Feb 2017 10:01:43 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Tue, 2017-02-14 at 00:03 +0900, Masami Hiramatsu wrote:
> > This is arm port of commit 6a5022a56ac3 ("kprobes/x86: Allow to
> > handle reentered kprobe on single-stepping")
> > 
> > Since the FIQ handlers can interrupt in the single stepping
> > (or preparing the single stepping, do_debug etc.), we should
> > consider a kprobe is hit in the NMI handler. Even in that
> > case, the kprobe is allowed to be reentered as same as the
> > kprobes hit in kprobe handlers
> > (KPROBE_HIT_ACTIVE or KPROBE_HIT_SSDONE).
> > 
> > The real issue will happen when a kprobe hit while another
> 
> Could to with 'is' being inserted above  ^^^
> (I know this is a copy of the x86 commit message)

Ah! yes, it should be corrected. 
> 
> > reentered kprobe is processing (KPROBE_REENTER), because
> > we already consumed a saved-area for the previous kprobe.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> 
> Acked-by: Jon Medhurst <tixy@linaro.org>

Thank you!

> 
> >  arch/arm/probes/kprobes/core.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> > index a4ec240..264fedb 100644
> > --- a/arch/arm/probes/kprobes/core.c
> > +++ b/arch/arm/probes/kprobes/core.c
> > @@ -270,6 +270,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
> >  			switch (kcb->kprobe_status) {
> >  			case KPROBE_HIT_ACTIVE:
> >  			case KPROBE_HIT_SSDONE:
> > +			case KPROBE_HIT_SS:
> >  				/* A pre- or post-handler probe got us here. */
> >  				kprobes_inc_nmissed_count(p);
> >  				save_previous_kprobe(kcb);
> > @@ -278,6 +279,11 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
> >  				singlestep(p, regs, kcb);
> >  				restore_previous_kprobe(kcb);
> >  				break;
> > +			case KPROBE_REENTER:
> > +				/* A nested probe was hit in FIQ, it is a BUG */
> > +				pr_warn("Unrecoverable kprobe detected at %p.\n",
> > +					p->addr);
> > +				/* fall through */
> >  			default:
> >  				/* impossible cases */
> >  				BUG();
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[BUGFIX PATCH 2/3] kprobes/arm: Skip single-stepping in recursing path if possible

[Masami Hiramatsu]

Kprobes/arm skips single-stepping (moreover handling the event)
if the conditional instruction must not be executed. This
also apply the rule when we hit the recursing kprobe, so
that kprobe does not count nmissed up in that case.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/probes/kprobes/core.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 264fedb..84989ae 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -265,7 +265,15 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 #endif
 
 	if (p) {
-		if (cur) {
+		if (!p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
+			/*
+			 * Probe hit but conditional execution check failed,
+			 * so just skip the instruction and continue as if
+			 * nothing had happened.
+			 * In this case, we can skip recursing check too.
+			 */
+			singlestep_skip(p, regs);
+		} else if (cur) {
 			/* Kprobe is pending, so we're recursing. */
 			switch (kcb->kprobe_status) {
 			case KPROBE_HIT_ACTIVE:
@@ -288,7 +296,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				/* impossible cases */
 				BUG();
 			}
-		} else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
+		} else {
 			/* Probe hit and conditional execution check ok. */
 			set_current_kprobe(p);
 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
@@ -309,13 +317,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				}
 				reset_current_kprobe();
 			}
-		} else {
-			/*
-			 * Probe hit but conditional execution check failed,
-			 * so just skip the instruction and continue as if
-			 * nothing had happened.
-			 */
-			singlestep_skip(p, regs);
 		}
 	} else if (cur) {
 		/* We probably hit a jprobe.  Call its break handler. */

Re: [BUGFIX PATCH 2/3] kprobes/arm: Skip single-stepping in recursing path if possible

[Jon Medhurst (Tixy)]

On Tue, 2017-02-14 at 00:04 +0900, Masami Hiramatsu wrote:
> Kprobes/arm skips single-stepping (moreover handling the event)
> if the conditional instruction must not be executed. This
> also apply the rule when we hit the recursing kprobe, so
> that kprobe does not count nmissed up in that case.

Perhaps that last sentence would read better if written something like:

"This also applies that rule when we hit a recursing kprobe, so that the
nmissed count isn't incremented in that case."


> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Acked-by: Jon Medhurst <tixy@linaro.org>

> ---
>  arch/arm/probes/kprobes/core.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 264fedb..84989ae 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -265,7 +265,15 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
>  #endif
>  
>  	if (p) {
> -		if (cur) {
> +		if (!p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
> +			/*
> +			 * Probe hit but conditional execution check failed,
> +			 * so just skip the instruction and continue as if
> +			 * nothing had happened.
> +			 * In this case, we can skip recursing check too.
> +			 */
> +			singlestep_skip(p, regs);
> +		} else if (cur) {
>  			/* Kprobe is pending, so we're recursing. */
>  			switch (kcb->kprobe_status) {
>  			case KPROBE_HIT_ACTIVE:
> @@ -288,7 +296,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
>  				/* impossible cases */
>  				BUG();
>  			}
> -		} else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
> +		} else {
>  			/* Probe hit and conditional execution check ok. */
>  			set_current_kprobe(p);
>  			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> @@ -309,13 +317,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
>  				}
>  				reset_current_kprobe();
>  			}
> -		} else {
> -			/*
> -			 * Probe hit but conditional execution check failed,
> -			 * so just skip the instruction and continue as if
> -			 * nothing had happened.
> -			 */
> -			singlestep_skip(p, regs);
>  		}
>  	} else if (cur) {
>  		/* We probably hit a jprobe.  Call its break handler. */
> 

Re: [BUGFIX PATCH 2/3] kprobes/arm: Skip single-stepping in recursing path if possible

[Masami Hiramatsu]

On Tue, 14 Feb 2017 10:07:17 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Tue, 2017-02-14 at 00:04 +0900, Masami Hiramatsu wrote:
> > Kprobes/arm skips single-stepping (moreover handling the event)
> > if the conditional instruction must not be executed. This
> > also apply the rule when we hit the recursing kprobe, so
> > that kprobe does not count nmissed up in that case.
> 
> Perhaps that last sentence would read better if written something like:
> 
> "This also applies that rule when we hit a recursing kprobe, so that the
> nmissed count isn't incremented in that case."

OK, Thanks!

> 
> 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Acked-by: Jon Medhurst <tixy@linaro.org>
> 
> > ---
> >  arch/arm/probes/kprobes/core.c |   19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> > index 264fedb..84989ae 100644
> > --- a/arch/arm/probes/kprobes/core.c
> > +++ b/arch/arm/probes/kprobes/core.c
> > @@ -265,7 +265,15 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
> >  #endif
> >  
> >  	if (p) {
> > -		if (cur) {
> > +		if (!p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
> > +			/*
> > +			 * Probe hit but conditional execution check failed,
> > +			 * so just skip the instruction and continue as if
> > +			 * nothing had happened.
> > +			 * In this case, we can skip recursing check too.
> > +			 */
> > +			singlestep_skip(p, regs);
> > +		} else if (cur) {
> >  			/* Kprobe is pending, so we're recursing. */
> >  			switch (kcb->kprobe_status) {
> >  			case KPROBE_HIT_ACTIVE:
> > @@ -288,7 +296,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
> >  				/* impossible cases */
> >  				BUG();
> >  			}
> > -		} else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
> > +		} else {
> >  			/* Probe hit and conditional execution check ok. */
> >  			set_current_kprobe(p);
> >  			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > @@ -309,13 +317,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
> >  				}
> >  				reset_current_kprobe();
> >  			}
> > -		} else {
> > -			/*
> > -			 * Probe hit but conditional execution check failed,
> > -			 * so just skip the instruction and continue as if
> > -			 * nothing had happened.
> > -			 */
> > -			singlestep_skip(p, regs);
> >  		}
> >  	} else if (cur) {
> >  		/* We probably hit a jprobe.  Call its break handler. */
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Masami Hiramatsu]

This is arm port of commit 737480a0d525 ("kprobes/x86:
Fix the return address of multiple kretprobes").

Fix the return address of subsequent kretprobes when multiple
kretprobes are set on the same function.

For example:

  # cd /sys/kernel/debug/tracing
  # echo "r:event1 sys_symlink" > kprobe_events
  # echo "r:event2 sys_symlink" >> kprobe_events
  # echo 1 > events/kprobes/enable
  # ln -s /tmp/foo /tmp/bar

 (without this patch)

  # cat trace | grep -v ^#
              ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
              ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)

 (with this patch)

  # cat trace | grep -v ^#
              ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
              ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: KUMANO Syuhei <kumano.prog@gmail.com>
---
 arch/arm/probes/kprobes/core.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 84989ae..023800a 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 	struct hlist_node *tmp;
 	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
+	kprobe_opcode_t *correct_ret_addr = NULL;
 
 	INIT_HLIST_HEAD(&empty_rp);
 	kretprobe_hash_lock(current, &head, &flags);
@@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 			/* another task is sharing our hash bucket */
 			continue;
 
+		orig_ret_address = (unsigned long)ri->ret_addr;
+
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+
+	kretprobe_assert(ri, orig_ret_address, trampoline_address);
+
+	correct_ret_addr = ri->ret_addr;
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
 		if (ri->rp && ri->rp->handler) {
 			__this_cpu_write(current_kprobe, &ri->rp->kp);
 			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
+			ri->ret_addr = correct_ret_addr;
 			ri->rp->handler(ri, regs);
 			__this_cpu_write(current_kprobe, NULL);
 		}
 
-		orig_ret_address = (unsigned long)ri->ret_addr;
 		recycle_rp_inst(ri, &empty_rp);
 
 		if (orig_ret_address != trampoline_address)
@@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 			break;
 	}
 
-	kretprobe_assert(ri, orig_ret_address, trampoline_address);
 	kretprobe_hash_unlock(current, &flags);
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Jon Medhurst (Tixy)]

On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> This is arm port of commit 737480a0d525 ("kprobes/x86:
> Fix the return address of multiple kretprobes").
> 
> Fix the return address of subsequent kretprobes when multiple
> kretprobes are set on the same function.
> 
> For example:
> 
>   # cd /sys/kernel/debug/tracing
>   # echo "r:event1 sys_symlink" > kprobe_events
>   # echo "r:event2 sys_symlink" >> kprobe_events
>   # echo 1 > events/kprobes/enable
>   # ln -s /tmp/foo /tmp/bar
> 
>  (without this patch)
> 
>   # cat trace | grep -v ^#
>               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
>               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
>  (with this patch)
> 
>   # cat trace | grep -v ^#
>               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
>               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> ---

I don't fully understand this function, but I've checked that the ARM
version now matches the x86 version (apart from the x86 specific
register fixup and some comments). So, FWIW

Acked-by: Jon Medhurst <tixy@linaro.org>

I ran the before and after test case in the commit log on ARM and
verified the result is correct. I also tried running the ARM kprobe
tests with these 3 fixes but the tests fail. However, they also fail
without any of these changes, so I'll investigate that further...
  
>  arch/arm/probes/kprobes/core.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 84989ae..023800a 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  	struct hlist_node *tmp;
>  	unsigned long flags, orig_ret_address = 0;
>  	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
> +	kprobe_opcode_t *correct_ret_addr = NULL;
>  
>  	INIT_HLIST_HEAD(&empty_rp);
>  	kretprobe_hash_lock(current, &head, &flags);
> @@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			/* another task is sharing our hash bucket */
>  			continue;
>  
> +		orig_ret_address = (unsigned long)ri->ret_addr;
> +
> +		if (orig_ret_address != trampoline_address)
> +			/*
> +			 * This is the real return address. Any other
> +			 * instances associated with this task are for
> +			 * other calls deeper on the call stack
> +			 */
> +			break;
> +	}
> +
> +	kretprobe_assert(ri, orig_ret_address, trampoline_address);
> +
> +	correct_ret_addr = ri->ret_addr;
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task != current)
> +			/* another task is sharing our hash bucket */
> +			continue;
> +
> +		orig_ret_address = (unsigned long)ri->ret_addr;
>  		if (ri->rp && ri->rp->handler) {
>  			__this_cpu_write(current_kprobe, &ri->rp->kp);
>  			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
> +			ri->ret_addr = correct_ret_addr;
>  			ri->rp->handler(ri, regs);
>  			__this_cpu_write(current_kprobe, NULL);
>  		}
>  
> -		orig_ret_address = (unsigned long)ri->ret_addr;
>  		recycle_rp_inst(ri, &empty_rp);
>  
>  		if (orig_ret_address != trampoline_address)
> @@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
>  			break;
>  	}
>  
> -	kretprobe_assert(ri, orig_ret_address, trampoline_address);
>  	kretprobe_hash_unlock(current, &flags);
>  
>  	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> 

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Jon Medhurst (Tixy)]

On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > Fix the return address of multiple kretprobes").
> > 
> > Fix the return address of subsequent kretprobes when multiple
> > kretprobes are set on the same function.
> > 
> > For example:
> > 
> >   # cd /sys/kernel/debug/tracing
> >   # echo "r:event1 sys_symlink" > kprobe_events
> >   # echo "r:event2 sys_symlink" >> kprobe_events
> >   # echo 1 > events/kprobes/enable
> >   # ln -s /tmp/foo /tmp/bar
> > 
> >  (without this patch)
> > 
> >   # cat trace | grep -v ^#
> >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > 
> >  (with this patch)
> > 
> >   # cat trace | grep -v ^#
> >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > ---
> 
> I don't fully understand this function, but I've checked that the ARM
> version now matches the x86 version (apart from the x86 specific
> register fixup and some comments). So, FWIW
> 
> Acked-by: Jon Medhurst <tixy@linaro.org>
> 
> I ran the before and after test case in the commit log on ARM and
> verified the result is correct. I also tried running the ARM kprobe
> tests with these 3 fixes but the tests fail. However, they also fail
> without any of these changes, so I'll investigate that further...

Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")

This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
that on 4.10-rc4, with the three patches in this series, then the ARM
kprobes tests pass OK.

I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
a year, so can probably wait a little longer).

-- 
Tixy

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Masami Hiramatsu]

On Tue, 14 Feb 2017 13:47:07 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > Fix the return address of multiple kretprobes").
> > > 
> > > Fix the return address of subsequent kretprobes when multiple
> > > kretprobes are set on the same function.
> > > 
> > > For example:
> > > 
> > >   # cd /sys/kernel/debug/tracing
> > >   # echo "r:event1 sys_symlink" > kprobe_events
> > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > >   # echo 1 > events/kprobes/enable
> > >   # ln -s /tmp/foo /tmp/bar
> > > 
> > >  (without this patch)
> > > 
> > >   # cat trace | grep -v ^#
> > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > 
> > >  (with this patch)
> > > 
> > >   # cat trace | grep -v ^#
> > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > 
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > ---
> > 
> > I don't fully understand this function, but I've checked that the ARM
> > version now matches the x86 version (apart from the x86 specific
> > register fixup and some comments). So, FWIW
> > 
> > Acked-by: Jon Medhurst <tixy@linaro.org>
> > 
> > I ran the before and after test case in the commit log on ARM and
> > verified the result is correct. I also tried running the ARM kprobe
> > tests with these 3 fixes but the tests fail. However, they also fail
> > without any of these changes, so I'll investigate that further...
> 
> Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> 
> This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> that on 4.10-rc4, with the three patches in this series, then the ARM
> kprobes tests pass OK.
> 
> I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> a year, so can probably wait a little longer).

Hmm, I'm running arm kernel on qemu, which maybe the reason why
the test case passed in my environment, since my kconfig also sets
CONFIG_DEBUG_RODATA=y.

BTW, would you see that any kprobe_events didn't work with
CONFIG_DEBUG_RODATA=y? (what the failure messages were?)

Thank you,

> 
> -- 
> Tixy
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Jon Medhurst (Tixy)]

On Wed, 2017-02-15 at 01:01 +0900, Masami Hiramatsu wrote:
> On Tue, 14 Feb 2017 13:47:07 +0000
> "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> 
> > On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > > Fix the return address of multiple kretprobes").
> > > > 
> > > > Fix the return address of subsequent kretprobes when multiple
> > > > kretprobes are set on the same function.
> > > > 
> > > > For example:
> > > > 
> > > >   # cd /sys/kernel/debug/tracing
> > > >   # echo "r:event1 sys_symlink" > kprobe_events
> > > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > > >   # echo 1 > events/kprobes/enable
> > > >   # ln -s /tmp/foo /tmp/bar
> > > > 
> > > >  (without this patch)
> > > > 
> > > >   # cat trace | grep -v ^#
> > > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > 
> > > >  (with this patch)
> > > > 
> > > >   # cat trace | grep -v ^#
> > > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > 
> > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > > ---
> > > 
> > > I don't fully understand this function, but I've checked that the ARM
> > > version now matches the x86 version (apart from the x86 specific
> > > register fixup and some comments). So, FWIW
> > > 
> > > Acked-by: Jon Medhurst <tixy@linaro.org>
> > > 
> > > I ran the before and after test case in the commit log on ARM and
> > > verified the result is correct. I also tried running the ARM kprobe
> > > tests with these 3 fixes but the tests fail. However, they also fail
> > > without any of these changes, so I'll investigate that further...
> > 
> > Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> > ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> > 
> > This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> > that on 4.10-rc4, with the three patches in this series, then the ARM
> > kprobes tests pass OK.
> > 
> > I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> > a year, so can probably wait a little longer).
> 
> Hmm, I'm running arm kernel on qemu, which maybe the reason why
> the test case passed in my environment, since my kconfig also sets
> CONFIG_DEBUG_RODATA=y.
> 
> BTW, would you see that any kprobe_events didn't work with
> CONFIG_DEBUG_RODATA=y? (what the failure messages were?)

The tests I'm running are the ARM specific tests that are enabled by
CONFIG_ARM_KPROBES_TEST=y. I'm running the tests on real multicore ARM
hardware (Versatile Express with a TC2 CoreTile)

For me, sometimes the first test gave:

    Beginning kprobe tests...
    Probe ARM code
        kprobe
    FAIL: test regs not OK

Other times, for the specific instruction emulation tests they return

   FAIL: test_before_handler not run

Not sure how much of the diagnostic appear without setting the tests to
be verbose, which I do with:

  sed -e 's/VERBOSE 0/VERBOSE 1/' -i arch/arm/probes/kprobes/test-core.h

Whilst writing a reply, I looked at the test code in
arch/arm/probes/kprobes/test-core.c (which I wrote some years ago) and
there is possibly a clue staring at us in the comments at the top of the
file...

 *
 * The above would expand to assembler looking something like:
 *
 *	@ TESTCASE_START
 *	bl	__kprobes_test_case_start
 *	.pushsection .rodata
 *	"10:
 *	.ascii "mov r0, r7"	@ text title for test case
 *	.byte	0
 *	.popsection
 *	@ start of inline data...
 *	.word	10b		@ pointer to title in .rodata
section

Note the ".pushsection .rodata" (though I don't see an immediate obvious
reason why that would cause a problem. It certainly seems likely that
the problem is with the ARM test code rather than actual kprobe
implementation itself.

Like I said, this issue has been there for a year or more, so I wasn't
planning on spending time on it for a few more days yet whilst I get on
with other urgent matters.

-- 
Tixy


Basically, m

Re: [BUGFIX PATCH 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Masami Hiramatsu]

On Tue, 14 Feb 2017 16:39:50 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Wed, 2017-02-15 at 01:01 +0900, Masami Hiramatsu wrote:
> > On Tue, 14 Feb 2017 13:47:07 +0000
> > "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> > 
> > > On Tue, 2017-02-14 at 10:32 +0000, Jon Medhurst (Tixy) wrote:
> > > > On Tue, 2017-02-14 at 00:05 +0900, Masami Hiramatsu wrote:
> > > > > This is arm port of commit 737480a0d525 ("kprobes/x86:
> > > > > Fix the return address of multiple kretprobes").
> > > > > 
> > > > > Fix the return address of subsequent kretprobes when multiple
> > > > > kretprobes are set on the same function.
> > > > > 
> > > > > For example:
> > > > > 
> > > > >   # cd /sys/kernel/debug/tracing
> > > > >   # echo "r:event1 sys_symlink" > kprobe_events
> > > > >   # echo "r:event2 sys_symlink" >> kprobe_events
> > > > >   # echo 1 > events/kprobes/enable
> > > > >   # ln -s /tmp/foo /tmp/bar
> > > > > 
> > > > >  (without this patch)
> > > > > 
> > > > >   # cat trace | grep -v ^#
> > > > >               ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
> > > > >               ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > > 
> > > > >  (with this patch)
> > > > > 
> > > > >   # cat trace | grep -v ^#
> > > > >               ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > >               ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
> > > > > 
> > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > > > Cc: KUMANO Syuhei <kumano.prog@gmail.com>
> > > > > ---
> > > > 
> > > > I don't fully understand this function, but I've checked that the ARM
> > > > version now matches the x86 version (apart from the x86 specific
> > > > register fixup and some comments). So, FWIW
> > > > 
> > > > Acked-by: Jon Medhurst <tixy@linaro.org>
> > > > 
> > > > I ran the before and after test case in the commit log on ARM and
> > > > verified the result is correct. I also tried running the ARM kprobe
> > > > tests with these 3 fixes but the tests fail. However, they also fail
> > > > without any of these changes, so I'll investigate that further...
> > > 
> > > Bisecting the issue led me back to Linux 4.5 and commit 25362dc496ed
> > > ("ARM: 8501/1: mm: flip priority of CONFIG_DEBUG_RODATA")
> > > 
> > > This sets CONFIG_DEBUG_RODATA to be enabled by default. If I disable
> > > that on 4.10-rc4, with the three patches in this series, then the ARM
> > > kprobes tests pass OK.
> > > 
> > > I'll stick the DEBUG_RODATA issue on my todo list (it's been around for
> > > a year, so can probably wait a little longer).
> > 
> > Hmm, I'm running arm kernel on qemu, which maybe the reason why
> > the test case passed in my environment, since my kconfig also sets
> > CONFIG_DEBUG_RODATA=y.
> > 
> > BTW, would you see that any kprobe_events didn't work with
> > CONFIG_DEBUG_RODATA=y? (what the failure messages were?)
> 
> The tests I'm running are the ARM specific tests that are enabled by
> CONFIG_ARM_KPROBES_TEST=y. I'm running the tests on real multicore ARM
> hardware (Versatile Express with a TC2 CoreTile)

Ah, I didn't enabled it. I'll also try to run it.

> 
> For me, sometimes the first test gave:
> 
>     Beginning kprobe tests...
>     Probe ARM code
>         kprobe
>     FAIL: test regs not OK
> 
> Other times, for the specific instruction emulation tests they return
> 
>    FAIL: test_before_handler not run
> 
> Not sure how much of the diagnostic appear without setting the tests to
> be verbose, which I do with:
> 
>   sed -e 's/VERBOSE 0/VERBOSE 1/' -i arch/arm/probes/kprobes/test-core.h
> 
> Whilst writing a reply, I looked at the test code in
> arch/arm/probes/kprobes/test-core.c (which I wrote some years ago) and
> there is possibly a clue staring at us in the comments at the top of the
> file...
> 
>  *
>  * The above would expand to assembler looking something like:
>  *
>  *	@ TESTCASE_START
>  *	bl	__kprobes_test_case_start
>  *	.pushsection .rodata
>  *	"10:
>  *	.ascii "mov r0, r7"	@ text title for test case
>  *	.byte	0
>  *	.popsection
>  *	@ start of inline data...
>  *	.word	10b		@ pointer to title in .rodata
> section
> 
> Note the ".pushsection .rodata" (though I don't see an immediate obvious
> reason why that would cause a problem. It certainly seems likely that
> the problem is with the ARM test code rather than actual kprobe
> implementation itself.

OK, then I'll just update the series according your comment.

> 
> Like I said, this issue has been there for a year or more, so I wasn't
> planning on spending time on it for a few more days yet whilst I get on
> with other urgent matters.

OK, I'll also investigate it.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>