Re: [PATCH v3 0/5] ARM64: Add kernel probes(Kprobes) support

[Steve Capper <steve.capper@linaro.org>]

On Tue, Dec 09, 2014 at 09:27:18AM -0500, David Long wrote:
> On 12/09/14 08:33, Steve Capper wrote:
> >On Thu, Dec 04, 2014 at 08:53:03PM +0900, Masami Hiramatsu wrote:

[...]

> >
> >Not sure if this is helpful, but the following also caused a crash for
> >me:
> >
> >echo "p:trace_event_buffer_lock_reserve trace_event_buffer_lock_reserve" > /sys/kernel/debug/tracing/kprobe_events
> >echo "p:memcpy memcpy" >> /sys/kernel/debug/tracing/kprobe_events
> >echo 1 > /sys/kernel/debug/tracing/events/kprobes/enable
> >
> >[immediate crash]
> >
> >The crash point for me is in the arm64 ASID allocator, it again looks
> >like the interrupts are in an unexpected state.
> >(check_and_switch_context goes down the irqs disabled code path, I
> >think incorrectly).
> >
> >This occurred for me both with and without the proposed irq saving fix.
> >
> >I will do some more digging.
> >
> 
> Thanks, more information is good.
> 

Hi,

Some good news, I think I've fixed the problem I've been experiencing.

Basically, I've torn out all the interrupt save/restore and have
narrowed the scope to just sandwich the instruction single-step. This
simplifies a lot of logic, and I've now been able to perf record a
kprobe on memcpy (and the trace_event_buffer_lock_reserve + memcpy
test) without any issues on a Juno platform.

I may have been somewhat over-zealous with the chainsaw, so please do
put this fix through its paces.

Cheers,
--
Steve


>From d3f4d80ce19bec71bd03209beb2fbfd8084d6543 Mon Sep 17 00:00:00 2001
From: Steve Capper <steve.capper@linaro.org>
Date: Mon, 1 Dec 2014 11:30:10 +0000
Subject: [PATCH] Fix the interrupt handling for kprobes

---
 arch/arm64/kernel/kprobes.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/kernel/kprobes.c b/arch/arm64/kernel/kprobes.c
index be7c330..d39d826 100644
--- a/arch/arm64/kernel/kprobes.c
+++ b/arch/arm64/kernel/kprobes.c
@@ -229,10 +229,6 @@ skip_singlestep_missed(struct kprobe_ctlblk *kcb, struct pt_regs *regs)
 {
 	/* set return addr to next pc to continue */
 	instruction_pointer(regs) += sizeof(kprobe_opcode_t);
-
-	if (kcb->kprobe_status != KPROBE_REENTER)
-		kprobes_restore_local_irqflag(regs);
-
 }
 
 static void __kprobes setup_singlestep(struct kprobe *p,
@@ -259,7 +255,7 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 			spsr_set_debug_flag(regs, 0);
 
 		/* IRQs and single stepping do not mix well. */
-		local_irq_disable();
+		kprobes_save_local_irqflag(regs);
 		kernel_enable_single_step(regs);
 		instruction_pointer(regs) = slot;
 	} else	{
@@ -326,7 +322,6 @@ post_kprobe_handler(struct kprobe_ctlblk *kcb, struct pt_regs *regs)
 	}
 
 	reset_current_kprobe();
-	kprobes_restore_local_irqflag(regs);
 }
 
 int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
@@ -380,8 +375,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr)
 			return 1;
 
 		break;
-	default:
-		break;
 	}
 	return 0;
 }
@@ -446,7 +439,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 		 * handling of this interrupt is appropriate.
 		 * Return back to original instruction, and continue.
 		 */
-		kprobes_restore_local_irqflag(regs);
 		return;
 	} else if (cur) {
 		/* We probably hit a jprobe.  Call its break handler. */
@@ -459,7 +451,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 		/* breakpoint is removed, now in a race
 		 * Return back to original instruction & continue.
 		 */
-		kprobes_restore_local_irqflag(regs);
 	}
 }
 
@@ -485,6 +476,7 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
 	retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
 
 	if (retval == DBG_HOOK_HANDLED) {
+		kprobes_restore_local_irqflag(regs);
 		kernel_disable_single_step();
 
 		if (kcb->kprobe_status == KPROBE_REENTER)
@@ -499,7 +491,6 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
 static int __kprobes
 kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
 {
-	kprobes_save_local_irqflag(regs);
 	kprobe_handler(regs);
 	return DBG_HOOK_HANDLED;
 }
@@ -563,7 +554,6 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 		memcpy((void *)stack_addr, kcb->jprobes_stack,
 		       MIN_STACK_SIZE(stack_addr));
 		preempt_enable_no_resched();
-		kprobes_restore_local_irqflag(regs);
 		return 1;
 	}
 	return 0;
@@ -655,8 +645,6 @@ trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
 		kfree(ri);
 	}
 
-	kprobes_restore_local_irqflag(regs);
-
 	/* return 1 so that post handlers not called */
 	return 1;
 }
-- 
1.9.3