Re: [PATCH] arm64: Fix early single-stepping

[Masami Hiramatsu <mhiramat@kernel.org>]

On Mon, 26 Oct 2020 18:29:09 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> To use debug features such as single-step, the OS lock must be unlocked
> in the debug registers. Currently this is done in postcore_initcall
> which is now too late.
> 
> Commit 36dadef23fcc ("kprobes: Init kprobes in early_initcall") enabled
> using kprobes from early_initcall, when OS lock is still locked. So when
> kprobe attempts to single-step a patched instruction, instead of
> trapping, execution continues until it throws an undef exception:
> 
> [    0.064233] Kprobe smoke test: started
> [    0.151133] ------------[ cut here ]------------
> [    0.151458] kernel BUG at arch/arm64/kernel/traps.c:406!
> [    0.151812] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>               ...
> [    0.162689] Call trace:
> [    0.163014]  do_undefinstr+0x1d4/0x1f4
> [    0.163336]  el1_sync_handler+0xbc/0x140
> [    0.163839]  el1_sync+0x80/0x100
> [    0.164154]  0xffffffc01001d004
> [    0.164527]  init_kprobes+0x13c/0x154
> [    0.164968]  do_one_initcall+0x54/0x2e0
> [    0.165322]  kernel_init_freeable+0xf4/0x258
> [    0.165783]  kernel_init+0x20/0x12c
> [    0.166117]  ret_from_fork+0x10/0x30
> [    0.166595] Code: 97ffff53 a9425bf5 17ffff9b f9001bf7 (d4210000)
> [    0.167084] ---[ end trace 36778fdf576e9a79 ]---
> 
> To fix this, unlock the OS lock as early as possible. Do it in
> traps_init() for CPU0, since KGDB wants to use single-step from that
> point on according to commit b322c65f8ca3 ("arm64: Call
> debug_traps_init() from trap_init() to help early kgdb").
> For secondary CPUs, setup the CPU hotplug handler at early_initcall.
> 
> Fixes: 36dadef23fcc ("kprobes: Init kprobes in early_initcall")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Hi Jean,

How have you confirmed this fixes the issue?
On my environment, this doesn't fix the issue.
We need to call debug_monitors_init() before calling init_kprobe(). 
But since both are early_initcall, it doesn't guarantee that the
debug_monitors_init() is called before init_kprobes().

I think there are 2 ways to fix this (in addition to this patch),
- split kprobe selftest from init_kprobe() and call the selftest
  in core_initcall()
- call clear_os_lock(0) from arch_init_kprobes()

Thank you,

> ---
>  arch/arm64/kernel/debug-monitors.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 75a423c3336a..80f082021234 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -135,7 +135,7 @@ static int __init debug_monitors_init(void)
>  				 "arm64/debug_monitors:starting",
>  				 clear_os_lock, NULL);
>  }
> -postcore_initcall(debug_monitors_init);
> +early_initcall(debug_monitors_init);
>  
>  /*
>   * Single step API and exception handling.
> @@ -380,6 +380,7 @@ NOKPROBE_SYMBOL(aarch32_break_handler);
>  
>  void __init debug_traps_init(void)
>  {
> +	clear_os_lock(0);
>  	hook_debug_fault_code(DBG_ESR_EVT_HWSS, single_step_handler, SIGTRAP,
>  			      TRAP_TRACE, "single-step handler");
>  	hook_debug_fault_code(DBG_ESR_EVT_BRK, brk_handler, SIGTRAP,
> -- 
> 2.29.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel