[PATCH 1/4] arm64: debug: unmask PSTATE.D earlier

[PATCH 1/4] arm64: debug: unmask PSTATE.D earlier

[Will Deacon]

Clearing PSTATE.D is one of the requirements for generating a debug
exception. The arm64 booting protocol requires that PSTATE.D is set,
since many of the debug registers (for example, the hw_breakpoint
registers) are UNKNOWN out of reset and could potentially generate
spurious, fatal debug exceptions in early boot code if PSTATE.D was
clear. Once the debug registers have been safely initialised, PSTATE.D
is cleared, however this is currently broken for two reasons:

(1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
    CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
    runs after SMP (and the scheduler) have been initialised, there is
    no guarantee that it is actually running on the boot CPU. In this
    case, the boot CPU is left with PSTATE.D set and is not capable of
    generating debug exceptions.

(2) In a preemptible kernel, we may explicitly schedule on the IRQ
    return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
    thread, then we may schedule the kthread_init thread, run the
    postcore_initcall to clear PSTATE.D and then context switch back
    to the idle thread before returning from the IRQ. The exception
    return path will then restore PSTATE.D from the stack, and set it
    again.

This patch fixes the problem by moving the clearing of PSTATE.D earlier
to proc.S. This has the desirable effect of clearing it in one place for
all CPUs, long before we have to worry about the scheduler or any
exception handling. We ensure that the previous reset of MDSCR_EL1 has
completed before unmasking the exception, so that any spurious
exceptions resulting from UNKNOWN debug registers are not generated.

Without this patch applied, the kprobes selftests have been seen to fail
under KVM, where we end up attempting to step the OOL instruction buffer
with PSTATE.D set and therefore fail to complete the step.

Cc: <stable@vger.kernel.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/debug-monitors.c | 1 -
 arch/arm64/kernel/smp.c            | 1 -
 arch/arm64/mm/proc.S               | 2 ++
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 2fbc1b99e8fb..7a907e401c98 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -156,7 +156,6 @@ static int debug_monitors_init(void)
 	/* Clear the OS lock. */
 	on_each_cpu(clear_os_lock, NULL, 1);
 	isb();
-	local_dbg_enable();
 
 	/* Register hotplug handler. */
 	__register_cpu_notifier(&os_lock_nb);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 62ff3c0622e2..c0a772560ab7 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -267,7 +267,6 @@ asmlinkage void secondary_start_kernel(void)
 	set_cpu_online(cpu, true);
 	complete(&cpu_running);
 
-	local_dbg_enable();
 	local_irq_enable();
 	local_async_enable();
 
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index c4317879b938..5bb61de23201 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -180,6 +180,8 @@ ENTRY(__cpu_setup)
 	msr	cpacr_el1, x0			// Enable FP/ASIMD
 	mov	x0, #1 << 12			// Reset mdscr_el1 and disable
 	msr	mdscr_el1, x0			// access to the DCC from EL0
+	isb					// Unmask debug exceptions now,
+	enable_dbg				// since this is per-cpu
 	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
 	/*
 	 * Memory region attributes for LPAE:
-- 
2.1.4

[PATCH 2/4] arm64: debug: remove redundant spsr manipulation

[Will Deacon]

There is no need to explicitly clear the SS bit immediately before
setting it unconditionally.

Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/debug-monitors.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 7a907e401c98..91fff48d0f57 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -170,22 +170,13 @@ postcore_initcall(debug_monitors_init);
  */
 static void set_regs_spsr_ss(struct pt_regs *regs)
 {
-	unsigned long spsr;
-
-	spsr = regs->pstate;
-	spsr &= ~DBG_SPSR_SS;
-	spsr |= DBG_SPSR_SS;
-	regs->pstate = spsr;
+	regs->pstate |= DBG_SPSR_SS;
 }
 NOKPROBE_SYMBOL(set_regs_spsr_ss);
 
 static void clear_regs_spsr_ss(struct pt_regs *regs)
 {
-	unsigned long spsr;
-
-	spsr = regs->pstate;
-	spsr &= ~DBG_SPSR_SS;
-	regs->pstate = spsr;
+	regs->pstate &= ~DBG_SPSR_SS;
 }
 NOKPROBE_SYMBOL(clear_regs_spsr_ss);
 
-- 
2.1.4

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Will Deacon]

Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
exception and we'll likely walk off into random data structures. This
should never happen, but when it does, it's a PITA to debug. Add a
WARN_ON to shout if we realise this is about to take place.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/probes/kprobes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 9c70e8812ea9..c89811d1e294 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -254,6 +254,8 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 
 		if (kcb->kprobe_status == KPROBE_REENTER)
 			spsr_set_debug_flag(regs, 0);
+		else
+			WARN_ON(regs->pstate & PSR_D_BIT);
 
 		/* IRQs and single stepping do not mix well. */
 		kprobes_save_local_irqflag(kcb, regs);
-- 
2.1.4

[PATCH 4/4] arm64: debug: remove unused local_dbg_{enable, disable} macros

[Will Deacon]

The debug enable/disable macros are not used anywhere in the kernel, so
remove them from irqflags.h

Reported-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/irqflags.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 11cc941bd107..8c581281fa12 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -110,8 +110,5 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
 		: : "r" (flags) : "memory");				\
 	} while (0)
 
-#define local_dbg_enable()	asm("msr	daifclr, #8" : : : "memory")
-#define local_dbg_disable()	asm("msr	daifset, #8" : : : "memory")
-
 #endif
 #endif
-- 
2.1.4

[PATCH 1/4] arm64: debug: unmask PSTATE.D earlier

[Catalin Marinas]

On Tue, Jul 19, 2016 at 03:07:37PM +0100, Will Deacon wrote:
> Clearing PSTATE.D is one of the requirements for generating a debug
> exception. The arm64 booting protocol requires that PSTATE.D is set,
> since many of the debug registers (for example, the hw_breakpoint
> registers) are UNKNOWN out of reset and could potentially generate
> spurious, fatal debug exceptions in early boot code if PSTATE.D was
> clear. Once the debug registers have been safely initialised, PSTATE.D
> is cleared, however this is currently broken for two reasons:
> 
> (1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
>     CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
>     runs after SMP (and the scheduler) have been initialised, there is
>     no guarantee that it is actually running on the boot CPU. In this
>     case, the boot CPU is left with PSTATE.D set and is not capable of
>     generating debug exceptions.
> 
> (2) In a preemptible kernel, we may explicitly schedule on the IRQ
>     return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
>     thread, then we may schedule the kthread_init thread, run the
>     postcore_initcall to clear PSTATE.D and then context switch back
>     to the idle thread before returning from the IRQ. The exception
>     return path will then restore PSTATE.D from the stack, and set it
>     again.
> 
> This patch fixes the problem by moving the clearing of PSTATE.D earlier
> to proc.S. This has the desirable effect of clearing it in one place for
> all CPUs, long before we have to worry about the scheduler or any
> exception handling. We ensure that the previous reset of MDSCR_EL1 has
> completed before unmasking the exception, so that any spurious
> exceptions resulting from UNKNOWN debug registers are not generated.
> 
> Without this patch applied, the kprobes selftests have been seen to fail
> under KVM, where we end up attempting to step the OOL instruction buffer
> with PSTATE.D set and therefore fail to complete the step.
> 
> Cc: <stable@vger.kernel.org>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

For the record (and for the whole series):

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>

I'll queue them for 4.8.

Thanks.

-- 
Catalin

[PATCH 2/4] arm64: debug: remove redundant spsr manipulation

[Mark Rutland]

On Tue, Jul 19, 2016 at 03:07:38PM +0100, Will Deacon wrote:
> There is no need to explicitly clear the SS bit immediately before
> setting it unconditionally.
> 
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/debug-monitors.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 7a907e401c98..91fff48d0f57 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -170,22 +170,13 @@ postcore_initcall(debug_monitors_init);
>   */
>  static void set_regs_spsr_ss(struct pt_regs *regs)
>  {
> -	unsigned long spsr;
> -
> -	spsr = regs->pstate;
> -	spsr &= ~DBG_SPSR_SS;
> -	spsr |= DBG_SPSR_SS;
> -	regs->pstate = spsr;
> +	regs->pstate |= DBG_SPSR_SS;
>  }
>  NOKPROBE_SYMBOL(set_regs_spsr_ss);
>  
>  static void clear_regs_spsr_ss(struct pt_regs *regs)
>  {
> -	unsigned long spsr;
> -
> -	spsr = regs->pstate;
> -	spsr &= ~DBG_SPSR_SS;
> -	regs->pstate = spsr;
> +	regs->pstate &= ~DBG_SPSR_SS;
>  }
>  NOKPROBE_SYMBOL(clear_regs_spsr_ss);
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Mark Rutland]

On Tue, Jul 19, 2016 at 03:07:39PM +0100, Will Deacon wrote:
> Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> exception and we'll likely walk off into random data structures. This
> should never happen, but when it does, it's a PITA to debug. Add a
> WARN_ON to shout if we realise this is about to take place.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/probes/kprobes.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 9c70e8812ea9..c89811d1e294 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -254,6 +254,8 @@ static void __kprobes setup_singlestep(struct kprobe *p,
>  
>  		if (kcb->kprobe_status == KPROBE_REENTER)
>  			spsr_set_debug_flag(regs, 0);
> +		else
> +			WARN_ON(regs->pstate & PSR_D_BIT);
>  
>  		/* IRQs and single stepping do not mix well. */
>  		kprobes_save_local_irqflag(kcb, regs);
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 4/4] arm64: debug: remove unused local_dbg_{enable, disable} macros

[Mark Rutland]

On Tue, Jul 19, 2016 at 03:07:40PM +0100, Will Deacon wrote:
> The debug enable/disable macros are not used anywhere in the kernel, so
> remove them from irqflags.h
> 
> Reported-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/irqflags.h | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 11cc941bd107..8c581281fa12 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -110,8 +110,5 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
>  		: : "r" (flags) : "memory");				\
>  	} while (0)
>  
> -#define local_dbg_enable()	asm("msr	daifclr, #8" : : : "memory")
> -#define local_dbg_disable()	asm("msr	daifset, #8" : : : "memory")
> -
>  #endif
>  #endif
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Pratyush Anand]

Hi Will,

Its already in torvalds/linux.git: master now. I have some related
queries, so thought to discuss it here.

On Tue, Jul 19, 2016 at 7:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> exception and we'll likely walk off into random data structures. This
> should never happen, but when it does, it's a PITA to debug. Add a

But it happens in many know scenarios, like:

1) We are executing a WARN_ON(), which will call `BRK  BUG_BRK_IMM`.
It prints warning messages through breakpoint handler. Now, suppose we
have a kprobe instrumented at a print function branch, say
print_worker_info(), we will land into
kprobe_handler()->setup_singlestep() with D-bit set. In this case if
we do not clear it, then we receive undefined exception before we
could get single step exception.

2) Similarly, if we instrument kprobe at uprobe_breakpoint_handler()
(code not yet in upstream),  we land into similar situation which
leads to infinite "Unexpected kernel single-step exception at EL1".

So, why can't we clear PSR_D_BIT in setup_singlestep unconditionally?
I found that both of the above issue is resolved by doing that.

~Pratyush

> WARN_ON to shout if we realise this is about to take place.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 9c70e8812ea9..c89811d1e294 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -254,6 +254,8 @@ static void __kprobes setup_singlestep(struct kprobe *p,
>
>                 if (kcb->kprobe_status == KPROBE_REENTER)
>                         spsr_set_debug_flag(regs, 0);
> +               else
> +                       WARN_ON(regs->pstate & PSR_D_BIT);
>
>                 /* IRQs and single stepping do not mix well. */
>                 kprobes_save_local_irqflag(kcb, regs);
> --
> 2.1.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Will Deacon]

On Wed, Aug 03, 2016 at 05:52:55PM +0530, Pratyush Anand wrote:
> Hi Will,
> 
> Its already in torvalds/linux.git: master now. I have some related
> queries, so thought to discuss it here.
> 
> On Tue, Jul 19, 2016 at 7:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> > Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> > exception and we'll likely walk off into random data structures. This
> > should never happen, but when it does, it's a PITA to debug. Add a
> 
> But it happens in many know scenarios, like:
> 
> 1) We are executing a WARN_ON(), which will call `BRK  BUG_BRK_IMM`.
> It prints warning messages through breakpoint handler. Now, suppose we
> have a kprobe instrumented at a print function branch, say
> print_worker_info(), we will land into
> kprobe_handler()->setup_singlestep() with D-bit set. In this case if
> we do not clear it, then we receive undefined exception before we
> could get single step exception.
> 
> 2) Similarly, if we instrument kprobe at uprobe_breakpoint_handler()
> (code not yet in upstream),  we land into similar situation which
> leads to infinite "Unexpected kernel single-step exception at EL1".
> 
> So, why can't we clear PSR_D_BIT in setup_singlestep unconditionally?
> I found that both of the above issue is resolved by doing that.

I think that will work, but the advantage of the WARN_ON is that it can
highlight cases where kprobes have been placed on the debug exception
path, which is generally a Bad Idea as it can result in infinite recursion
loops.

I know that __kprobes is supposed to deal with this, but in reality that's
all a best guess and looks to be incomplete. If we can do a better job
of annotating the debug exception path, I'd be up for unconditional
clearing of PSR_D_BIT in the target when returning.

Will

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Pratyush Anand]

Hi Will,

Thanks for the reply.

On 09/08/2016:01:48:32 PM, Will Deacon wrote:
> On Wed, Aug 03, 2016 at 05:52:55PM +0530, Pratyush Anand wrote:
> > Hi Will,
> > 
> > Its already in torvalds/linux.git: master now. I have some related
> > queries, so thought to discuss it here.
> > 
> > On Tue, Jul 19, 2016 at 7:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> > > exception and we'll likely walk off into random data structures. This
> > > should never happen, but when it does, it's a PITA to debug. Add a
> > 
> > But it happens in many know scenarios, like:
> > 
> > 1) We are executing a WARN_ON(), which will call `BRK  BUG_BRK_IMM`.
> > It prints warning messages through breakpoint handler. Now, suppose we
> > have a kprobe instrumented at a print function branch, say
> > print_worker_info(), we will land into
> > kprobe_handler()->setup_singlestep() with D-bit set. In this case if
> > we do not clear it, then we receive undefined exception before we
> > could get single step exception.
> > 
> > 2) Similarly, if we instrument kprobe at uprobe_breakpoint_handler()
> > (code not yet in upstream),  we land into similar situation which
> > leads to infinite "Unexpected kernel single-step exception at EL1".
> > 
> > So, why can't we clear PSR_D_BIT in setup_singlestep unconditionally?
> > I found that both of the above issue is resolved by doing that.
> 
> I think that will work, but the advantage of the WARN_ON is that it can
> highlight cases where kprobes have been placed on the debug exception
> path, which is generally a Bad Idea as it can result in infinite recursion
> loops.

It might result in infinite recursion if we place kprobe at a function which is
called from kprobe breakpoint/single step handler. However, it should still be
OK if kprobe is placed in other debug exception path.
Other arches like x86 allows that, so I think we will have to support as well.
> 
> I know that __kprobes is supposed to deal with this, but in reality that's
> all a best guess and looks to be incomplete. If we can do a better job
> of annotating the debug exception path, I'd be up for unconditional
> clearing of PSR_D_BIT in the target when returning.

OK, so I will send a patch for review with proper comment logs for debug
exception path.

~Pratyush

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Masami Hiramatsu]

On Wed, 10 Aug 2016 13:31:14 +0530
Pratyush Anand <panand@redhat.com> wrote:

> Hi Will,
> 
> Thanks for the reply.
> 
> On 09/08/2016:01:48:32 PM, Will Deacon wrote:
> > On Wed, Aug 03, 2016 at 05:52:55PM +0530, Pratyush Anand wrote:
> > > Hi Will,
> > > 
> > > Its already in torvalds/linux.git: master now. I have some related
> > > queries, so thought to discuss it here.
> > > 
> > > On Tue, Jul 19, 2016 at 7:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > > Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> > > > exception and we'll likely walk off into random data structures. This
> > > > should never happen, but when it does, it's a PITA to debug. Add a
> > > 
> > > But it happens in many know scenarios, like:
> > > 
> > > 1) We are executing a WARN_ON(), which will call `BRK  BUG_BRK_IMM`.
> > > It prints warning messages through breakpoint handler. Now, suppose we
> > > have a kprobe instrumented at a print function branch, say
> > > print_worker_info(), we will land into
> > > kprobe_handler()->setup_singlestep() with D-bit set. In this case if
> > > we do not clear it, then we receive undefined exception before we
> > > could get single step exception.

If the D-bit means debug trap flag for single-stepping, we need to store
the flag in kprobe_ctlblk so that we can restore the previous state
in post_kprobe handler.
And also, if we found the kprobes in such state, we should skip it so that
not involving any other functions, and if possible disable it forcibly if
it is really dangerous.

> > > 
> > > 2) Similarly, if we instrument kprobe at uprobe_breakpoint_handler()
> > > (code not yet in upstream),  we land into similar situation which
> > > leads to infinite "Unexpected kernel single-step exception at EL1".

It should be marked by NOKPROBE_SYMBOL.

> > > 
> > > So, why can't we clear PSR_D_BIT in setup_singlestep unconditionally?
> > > I found that both of the above issue is resolved by doing that.
> > 
> > I think that will work, but the advantage of the WARN_ON is that it can
> > highlight cases where kprobes have been placed on the debug exception
> > path, which is generally a Bad Idea as it can result in infinite recursion
> > loops.
> 
> It might result in infinite recursion if we place kprobe at a function which is
> called from kprobe breakpoint/single step handler.

For those functions we have to mark as blacklist by NOPROBE_SYMBOL() macro.

> However, it should still be
> OK if kprobe is placed in other debug exception path.

It depends on the architecture kprobe implementation, if we can not separate
the debug exception path to the kprobe at very last part of the path, we
can not probe that. So I recommend you to check it is kprobe or not at first.

> Other arches like x86 allows that, so I think we will have to support as well.

Yes, actually on x86, we hook kprobes directly in do_int3.

> > 
> > I know that __kprobes is supposed to deal with this, but in reality that's
> > all a best guess and looks to be incomplete. If we can do a better job
> > of annotating the debug exception path, I'd be up for unconditional
> > clearing of PSR_D_BIT in the target when returning.
> 
> OK, so I will send a patch for review with proper comment logs for debug
> exception path.

Could you also test it by using ftrace kprobe-trace interface and if you find
any place where can cause infinit recursion, please put the function in blacklist.

Thank you,


> 
> ~Pratyush


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Pratyush Anand]

On 10/08/2016:09:04:01 PM, Masami Hiramatsu wrote:
> On Wed, 10 Aug 2016 13:31:14 +0530
> Pratyush Anand <panand@redhat.com> wrote:
> 
> > Hi Will,
> > 
> > Thanks for the reply.
> > 
> > On 09/08/2016:01:48:32 PM, Will Deacon wrote:
> > > On Wed, Aug 03, 2016 at 05:52:55PM +0530, Pratyush Anand wrote:
> > > > Hi Will,
> > > > 
> > > > Its already in torvalds/linux.git: master now. I have some related
> > > > queries, so thought to discuss it here.
> > > > 
> > > > On Tue, Jul 19, 2016 at 7:37 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > > > Stepping with PSTATE.D=1 is bad news. The step won't generate a debug
> > > > > exception and we'll likely walk off into random data structures. This
> > > > > should never happen, but when it does, it's a PITA to debug. Add a
> > > > 
> > > > But it happens in many know scenarios, like:
> > > > 
> > > > 1) We are executing a WARN_ON(), which will call `BRK  BUG_BRK_IMM`.
> > > > It prints warning messages through breakpoint handler. Now, suppose we
> > > > have a kprobe instrumented at a print function branch, say
> > > > print_worker_info(), we will land into
> > > > kprobe_handler()->setup_singlestep() with D-bit set. In this case if
> > > > we do not clear it, then we receive undefined exception before we
> > > > could get single step exception.
> 
> If the D-bit means debug trap flag for single-stepping,

Well, this is what my understanding is about D-bit. Will/Catalin can correct if
something is wrong.

PSTATE.D is debug exception mask bit. It is set whenever we enter into an
exception mode. When it is set then Watchpoint, Breakpoint, and Software Step
exceptions are masked. However, software Breakpoint Instruction exceptions can
never be masked. Therefore, if we ever execute a BRK instruction, irrespective
of D-bit setting, we will be receiving a corresponding  breakpoint exception.
For example:
- We are executing kprobe pre/post handler, and kprobe has been inserted in one
  of the instruction of a function called by handler. So, it executes BRK
  instruction and we land into the case of KPROBE_REENTER. (This case is already
  handled by current code)
- We are executing uprobe handler or any other BRK handler such as in WARN_ON,
  and we trace that path using kprobe.So, we enter into kprobe breakpoint
  handler,from another BRK handler.(This case is not being handled currently)

In all such cases kprobe breakpoint exception will be raised when we were
already in debug exception mode.
SPSR's D bit (bit 9) shows the value of PSTATE.D immediately before the
exception was taken.So, in above example cases we would find it set in kprobe
breakpoint handler.
Single step exception will always be followed by a kprobe breakpoint exception.
However, it will only be raised gracefully if we clear D bit while returning
from breakpoint exception. If D bit is set then, it results into undefined
exception and when it's handler enables dbg then single step exception is
generated, however it will never be handled(because address does not match and
therefore treated as unexpected).

> we need to store
> the flag in kprobe_ctlblk so that we can restore the previous state
> in post_kprobe handler.

I do not think that we need to save it, because irrespective of whether we clear
it in breakpoint handler, it will be found set again in kprobe single step
handler. So, while returning from kprobe single step handler, regs->pstate.D bit
will always be set.

> And also, if we found the kprobes in such state, we should skip it so that
> not involving any other functions, and if possible disable it forcibly if
> it is really dangerous.
> 
> > > > 
> > > > 2) Similarly, if we instrument kprobe at uprobe_breakpoint_handler()
> > > > (code not yet in upstream),  we land into similar situation which
> > > > leads to infinite "Unexpected kernel single-step exception at EL1".
> 
> It should be marked by NOKPROBE_SYMBOL.

I think, if we clear pstate.D in setup_singlestep unconditionally, then we do
not need to blacklist it. x86 allows to trace uprobe handler, so I think we can
do it for ARM64 as well.

> 
> > > > 
> > > > So, why can't we clear PSR_D_BIT in setup_singlestep unconditionally?
> > > > I found that both of the above issue is resolved by doing that.
> > > 
> > > I think that will work, but the advantage of the WARN_ON is that it can
> > > highlight cases where kprobes have been placed on the debug exception
> > > path, which is generally a Bad Idea as it can result in infinite recursion
> > > loops.
> > 
> > It might result in infinite recursion if we place kprobe at a function which is
> > called from kprobe breakpoint/single step handler.
> 
> For those functions we have to mark as blacklist by NOPROBE_SYMBOL() macro.

Yes, agreed.

> 
> > However, it should still be
> > OK if kprobe is placed in other debug exception path.
> 
> It depends on the architecture kprobe implementation, if we can not separate
> the debug exception path to the kprobe at very last part of the path, we
> can not probe that. So I recommend you to check it is kprobe or not at first.

When we are in setup_singlestep() then we are sure that its a kprobe breakpoint.
> 
> > Other arches like x86 allows that, so I think we will have to support as well.
> 
> Yes, actually on x86, we hook kprobes directly in do_int3.
> 
> > > 
> > > I know that __kprobes is supposed to deal with this, but in reality that's
> > > all a best guess and looks to be incomplete. If we can do a better job
> > > of annotating the debug exception path, I'd be up for unconditional
> > > clearing of PSR_D_BIT in the target when returning.
> > 
> > OK, so I will send a patch for review with proper comment logs for debug
> > exception path.
> 
> Could you also test it by using ftrace kprobe-trace interface and if you find
> any place where can cause infinit recursion, please put the function in blacklist.

OK, Will do.

Thanks for your suggestion and review of the proposal. It was helpful.

~Pratyush

[PATCH 3/4] arm64: kprobes: WARN if attempting to step with PSTATE.D=1

[Pratyush Anand]

On 12/08/2016:06:16:56 PM, Pratyush Anand wrote:
> On 10/08/2016:09:04:01 PM, Masami Hiramatsu wrote:
> > Could you also test it by using ftrace kprobe-trace interface and if you find
> > any place where can cause infinit recursion, please put the function in blacklist.
> 
> OK, Will do.

Sorry, I do not think that we can test with kprobe over ftrace in arm64. It is
yet to be supported.

~Pratyush