Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580958 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-15 01:30 +0100
    [BUGFIX PATCH V2 3/3] kprobes/arm: Fix the return address of multiple kretprobes Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-15 01:40 +0100
    [BUGFIX PATCH V2 2/3] kprobes/arm: Skip single-stepping in recursing path if possible Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-15 01:40 +0100
    Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes  implementation on arm "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2017-02-17 12:10 +0100
      Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes  implementation on arm Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-17 22:50 +0100
        Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes  implementation on arm "Jon Medhurst (Tixy)" <tixy@linaro.org> - 2017-02-22 17:00 +0100
          Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes  implementation on arm Masami Hiramatsu <mhiramat@kernel.org> - 2017-02-23 00:30 +0100

#1580958 — [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

Hi,

Here is the 2nd version of the patches which improve kprobe
on arm implementation (a kind of bugfix). Version 1 is here;

https://lkml.org/lkml/2017/2/13/538

In this version I didn't update the code, just update the
patch description according to Tixy's comment and add his Ack.

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

[toc] | [next] | [standalone]

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

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>
Acked-by: Jon Medhurst <tixy@linaro.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) {

[toc] | [prev] | [next] | [standalone]

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

Kprobes/arm skips single-stepping (moreover handling the event)
if the conditional instruction must not be executed. 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. */

[toc] | [prev] | [next] | [standalone]

#1583329 — Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

On Wed, 2017-02-15 at 09:27 +0900, Masami Hiramatsu wrote:
> Hi,
> 
> Here is the 2nd version of the patches which improve kprobe
> on arm implementation (a kind of bugfix). Version 1 is here;
> 
> https://lkml.org/lkml/2017/2/13/538
> 
> In this version I didn't update the code, just update the
> patch description according to Tixy's comment and add his Ack.
> 
> 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
> 

Thanks for doing these. Am I correct in assuming we don't need to
consider these fixes urgent or critical? Only the first looks like it
could be serious, and the x86 fix for that is 3 years old and ARM has
gone without it all this time. So I'm guessing it's fine to wait for the
normal development process and deal with it after the about to open
merge window is completed?

If so, I propose that I put the patches in a branch for Russell to pull
later (unless he pipes up with objections or says otherwise). Meantime
I'll investigate the kprobes test failures I see (which actually looks
like cache/TLB issues and not test code problems after all).

BTW, I added the ARM kernel list to the CC. I spotted you didn't add it
to you patch postings, which means people interested in ARM (other than
Russell) wouldn't have seen them.

Thanks

-- 
Tixy

[toc] | [prev] | [next] | [standalone]

#1583756 — Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

On Fri, 17 Feb 2017 11:01:10 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Wed, 2017-02-15 at 09:27 +0900, Masami Hiramatsu wrote:
> > Hi,
> > 
> > Here is the 2nd version of the patches which improve kprobe
> > on arm implementation (a kind of bugfix). Version 1 is here;
> > 
> > https://lkml.org/lkml/2017/2/13/538
> > 
> > In this version I didn't update the code, just update the
> > patch description according to Tixy's comment and add his Ack.
> > 
> > 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
> > 
> 
> Thanks for doing these. Am I correct in assuming we don't need to
> consider these fixes urgent or critical? Only the first looks like it
> could be serious, and the x86 fix for that is 3 years old and ARM has
> gone without it all this time. So I'm guessing it's fine to wait for the
> normal development process and deal with it after the about to open
> merge window is completed?

Agreed. I'm not sure how frequently FIQ is used in ARM, but anyway
it happens only when root user intensively uses kprobes on FIQ handlers.

> If so, I propose that I put the patches in a branch for Russell to pull
> later (unless he pipes up with objections or says otherwise). Meantime
> I'll investigate the kprobes test failures I see (which actually looks
> like cache/TLB issues and not test code problems after all).

OK, btw, I couldn't reproduce the kprobes test failure with
CONFIG_DEBUG_RODATA=y on qemu...

> 
> BTW, I added the ARM kernel list to the CC. I spotted you didn't add it
> to you patch postings, which means people interested in ARM (other than
> Russell) wouldn't have seen them.

Ah, I forgot that, Thank you!

> 
> Thanks
> 
> -- 
> Tixy


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1586276 — Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

On Sat, 2017-02-18 at 06:43 +0900, Masami Hiramatsu wrote:
> On Fri, 17 Feb 2017 11:01:10 +0000
> "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
[...]
> > Meantime
> > I'll investigate the kprobes test failures I see (which actually looks
> > like cache/TLB issues and not test code problems after all).
> 
> OK, btw, I couldn't reproduce the kprobes test failure with
> CONFIG_DEBUG_RODATA=y on qemu...

That's because it's a problem with caches which qemu doesn't emulate.
I worked out the cause and posted a patch...
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489517.html

-- 
Tixy

[toc] | [prev] | [next] | [standalone]

#1586550 — Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

On Wed, 22 Feb 2017 15:58:49 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Sat, 2017-02-18 at 06:43 +0900, Masami Hiramatsu wrote:
> > On Fri, 17 Feb 2017 11:01:10 +0000
> > "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> [...]
> > > Meantime
> > > I'll investigate the kprobes test failures I see (which actually looks
> > > like cache/TLB issues and not test code problems after all).
> > 
> > OK, btw, I couldn't reproduce the kprobes test failure with
> > CONFIG_DEBUG_RODATA=y on qemu...
> 
> That's because it's a problem with caches which qemu doesn't emulate.
> I worked out the cause and posted a patch...
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489517.html

OK, I see.

Thanks! 

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web