* [PATCH] arm64: Fix early single-stepping
@ 2020-10-26 17:29 Jean-Philippe Brucker
2020-10-26 17:38 ` Will Deacon
2020-10-27 10:13 ` Masami Hiramatsu
0 siblings, 2 replies; 16+ messages in thread
From: Jean-Philippe Brucker @ 2020-10-26 17:29 UTC (permalink / raw)
To: catalin.marinas, will
Cc: Jean-Philippe Brucker, dianders, linux-arm-kernel, mhiramat
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>
---
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] arm64: Fix early single-stepping 2020-10-26 17:29 [PATCH] arm64: Fix early single-stepping Jean-Philippe Brucker @ 2020-10-26 17:38 ` Will Deacon 2020-10-27 0:48 ` Masami Hiramatsu 2020-10-27 10:13 ` Masami Hiramatsu 1 sibling, 1 reply; 16+ messages in thread From: Will Deacon @ 2020-10-26 17:38 UTC (permalink / raw) To: Jean-Philippe Brucker Cc: catalin.marinas, dianders, linux-arm-kernel, mhiramat On Mon, Oct 26, 2020 at 06:29:09PM +0100, Jean-Philippe Brucker 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. Hmm, does this mean we end up setting MDSCR_EL1.KDE before we've reset the hardware breakpoint/watchpoint registers? Why do we need kprobes so early? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-26 17:38 ` Will Deacon @ 2020-10-27 0:48 ` Masami Hiramatsu 0 siblings, 0 replies; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-27 0:48 UTC (permalink / raw) To: Will Deacon Cc: Jean-Philippe Brucker, mhiramat, dianders, linux-arm-kernel, catalin.marinas On Mon, 26 Oct 2020 17:38:37 +0000 Will Deacon <will@kernel.org> wrote: > On Mon, Oct 26, 2020 at 06:29:09PM +0100, Jean-Philippe Brucker 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. Oops, thanks for the fix! I missed it. > Hmm, does this mean we end up setting MDSCR_EL1.KDE before we've reset the > hardware breakpoint/watchpoint registers? Why do we need kprobes so early? This is for boot-time tracing. To enable kprobes events in core_initcall(), we need to enable kprobes itself in early_initcall().(or early_initcall_sync()) With this, we can trace postcore functions with kprobes(it includes some platform initializations), which is making boot-time ftrace more useful. For example, we can trace function-calls in specific code area as I posted an example; https://lore.kernel.org/linux-doc/159887792384.1330989.5993224243767476896.stgit@devnote2/ So this expands the feature to the earlier stages. Thank you, -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-26 17:29 [PATCH] arm64: Fix early single-stepping Jean-Philippe Brucker 2020-10-26 17:38 ` Will Deacon @ 2020-10-27 10:13 ` Masami Hiramatsu 2020-10-27 10:42 ` Masami Hiramatsu 1 sibling, 1 reply; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-27 10:13 UTC (permalink / raw) To: Jean-Philippe Brucker Cc: catalin.marinas, will, dianders, linux-arm-kernel, mhiramat 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-27 10:13 ` Masami Hiramatsu @ 2020-10-27 10:42 ` Masami Hiramatsu 2020-10-27 11:59 ` Jean-Philippe Brucker 0 siblings, 1 reply; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-27 10:42 UTC (permalink / raw) To: Masami Hiramatsu Cc: Jean-Philippe Brucker, will, dianders, linux-arm-kernel, catalin.marinas On Tue, 27 Oct 2020 19:13:18 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > 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. Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) Anyway this works for me too. Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> Tested-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you! -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-27 10:42 ` Masami Hiramatsu @ 2020-10-27 11:59 ` Jean-Philippe Brucker 2020-10-27 12:33 ` Will Deacon 0 siblings, 1 reply; 16+ messages in thread From: Jean-Philippe Brucker @ 2020-10-27 11:59 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: catalin.marinas, will, dianders, linux-arm-kernel On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > On Tue, 27 Oct 2020 19:13:18 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > 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. > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > Anyway this works for me too. No worries :) Although now I've been wondering whether it would be better to just disable the OS lock lazily, on the first call to enable_debug_monitors(). It might add a tiny performance penalty but would avoid this problem reappearing if one of the debugger needs to start even earlier in the future. > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Tested-by: Masami Hiramatsu <mhiramat@kernel.org> Thanks! Jean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-27 11:59 ` Jean-Philippe Brucker @ 2020-10-27 12:33 ` Will Deacon 2020-10-27 13:49 ` Masami Hiramatsu 0 siblings, 1 reply; 16+ messages in thread From: Will Deacon @ 2020-10-27 12:33 UTC (permalink / raw) To: Jean-Philippe Brucker Cc: catalin.marinas, Masami Hiramatsu, linux-arm-kernel, dianders On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > On Tue, 27 Oct 2020 19:13:18 +0900 > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > 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. > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > Anyway this works for me too. > > No worries :) Although now I've been wondering whether it would be better > to just disable the OS lock lazily, on the first call to > enable_debug_monitors(). It might add a tiny performance penalty but would > avoid this problem reappearing if one of the debugger needs to start even > earlier in the future. I'm still uneasy about enabling KDE with the watchpoint registers in an unknown state, so I think this needs more work. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-27 12:33 ` Will Deacon @ 2020-10-27 13:49 ` Masami Hiramatsu 2020-10-28 8:28 ` Jean-Philippe Brucker 0 siblings, 1 reply; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-27 13:49 UTC (permalink / raw) To: Will Deacon Cc: Jean-Philippe Brucker, dianders, Masami Hiramatsu, linux-arm-kernel, catalin.marinas On Tue, 27 Oct 2020 12:33:18 +0000 Will Deacon <will@kernel.org> wrote: > On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > > On Tue, 27 Oct 2020 19:13:18 +0900 > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > 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. > > > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > > Anyway this works for me too. > > > > No worries :) Although now I've been wondering whether it would be better > > to just disable the OS lock lazily, on the first call to > > enable_debug_monitors(). It might add a tiny performance penalty but would > > avoid this problem reappearing if one of the debugger needs to start even > > earlier in the future. > > I'm still uneasy about enabling KDE with the watchpoint registers in an > unknown state, so I think this needs more work. Hmm, how we reset it in the early stage? reset watchpoint registers first? Thanks, > > Will -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-27 13:49 ` Masami Hiramatsu @ 2020-10-28 8:28 ` Jean-Philippe Brucker 2020-10-28 8:36 ` Will Deacon 0 siblings, 1 reply; 16+ messages in thread From: Jean-Philippe Brucker @ 2020-10-28 8:28 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: catalin.marinas, Will Deacon, dianders, linux-arm-kernel On Tue, Oct 27, 2020 at 10:49:22PM +0900, Masami Hiramatsu wrote: > On Tue, 27 Oct 2020 12:33:18 +0000 > Will Deacon <will@kernel.org> wrote: > > > On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > > > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > > > On Tue, 27 Oct 2020 19:13:18 +0900 > > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > 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. > > > > > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > > > Anyway this works for me too. > > > > > > No worries :) Although now I've been wondering whether it would be better > > > to just disable the OS lock lazily, on the first call to > > > enable_debug_monitors(). It might add a tiny performance penalty but would > > > avoid this problem reappearing if one of the debugger needs to start even > > > earlier in the future. > > > > I'm still uneasy about enabling KDE with the watchpoint registers in an > > unknown state, so I think this needs more work. > > Hmm, how we reset it in the early stage? reset watchpoint registers first? Yes, I think so. Same order problem as the OS lock, they need to be reset before enable_debug_monitors(). On CPU0 that would be before early_initcall and for secondaries the hotplug notifier needs to be installed earlier as well. I'll send a v2. Thanks, Jean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-28 8:28 ` Jean-Philippe Brucker @ 2020-10-28 8:36 ` Will Deacon 2020-10-28 9:07 ` Masami Hiramatsu 2020-11-25 16:09 ` Masami Hiramatsu 0 siblings, 2 replies; 16+ messages in thread From: Will Deacon @ 2020-10-28 8:36 UTC (permalink / raw) To: Jean-Philippe Brucker Cc: catalin.marinas, Masami Hiramatsu, linux-arm-kernel, dianders On Wed, Oct 28, 2020 at 09:28:20AM +0100, Jean-Philippe Brucker wrote: > On Tue, Oct 27, 2020 at 10:49:22PM +0900, Masami Hiramatsu wrote: > > On Tue, 27 Oct 2020 12:33:18 +0000 > > Will Deacon <will@kernel.org> wrote: > > > > > On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > > > > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > > > > On Tue, 27 Oct 2020 19:13:18 +0900 > > > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > > > 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. > > > > > > > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > > > > Anyway this works for me too. > > > > > > > > No worries :) Although now I've been wondering whether it would be better > > > > to just disable the OS lock lazily, on the first call to > > > > enable_debug_monitors(). It might add a tiny performance penalty but would > > > > avoid this problem reappearing if one of the debugger needs to start even > > > > earlier in the future. > > > > > > I'm still uneasy about enabling KDE with the watchpoint registers in an > > > unknown state, so I think this needs more work. > > > > Hmm, how we reset it in the early stage? reset watchpoint registers first? > > Yes, I think so. Same order problem as the OS lock, they need to be reset > before enable_debug_monitors(). On CPU0 that would be before > early_initcall and for secondaries the hotplug notifier needs to be > installed earlier as well. I'll send a v2. Cheers. An alternative (which I think would be better in the long run anyway) would be to avoid using hardware step in kprobes and instead rely on a BRK instruction to trap after running the trampoline. Then we just need to ensure that the BRK handlers are up and running in time (which might require an early handler, like we have for KASAN). Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-28 8:36 ` Will Deacon @ 2020-10-28 9:07 ` Masami Hiramatsu 2020-10-28 9:48 ` Jean-Philippe Brucker 2020-11-25 16:09 ` Masami Hiramatsu 1 sibling, 1 reply; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-28 9:07 UTC (permalink / raw) To: Will Deacon Cc: Jean-Philippe Brucker, dianders, Masami Hiramatsu, linux-arm-kernel, catalin.marinas On Wed, 28 Oct 2020 08:36:44 +0000 Will Deacon <will@kernel.org> wrote: > On Wed, Oct 28, 2020 at 09:28:20AM +0100, Jean-Philippe Brucker wrote: > > On Tue, Oct 27, 2020 at 10:49:22PM +0900, Masami Hiramatsu wrote: > > > On Tue, 27 Oct 2020 12:33:18 +0000 > > > Will Deacon <will@kernel.org> wrote: > > > > > > > On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > > > > > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > > > > > On Tue, 27 Oct 2020 19:13:18 +0900 > > > > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > > > > > 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. > > > > > > > > > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > > > > > Anyway this works for me too. > > > > > > > > > > No worries :) Although now I've been wondering whether it would be better > > > > > to just disable the OS lock lazily, on the first call to > > > > > enable_debug_monitors(). It might add a tiny performance penalty but would > > > > > avoid this problem reappearing if one of the debugger needs to start even > > > > > earlier in the future. > > > > > > > > I'm still uneasy about enabling KDE with the watchpoint registers in an > > > > unknown state, so I think this needs more work. > > > > > > Hmm, how we reset it in the early stage? reset watchpoint registers first? > > > > Yes, I think so. Same order problem as the OS lock, they need to be reset > > before enable_debug_monitors(). On CPU0 that would be before > > early_initcall and for secondaries the hotplug notifier needs to be > > installed earlier as well. I'll send a v2. > > Cheers. An alternative (which I think would be better in the long run > anyway) would be to avoid using hardware step in kprobes and instead rely > on a BRK instruction to trap after running the trampoline. But how we trap the instruction which can change pc? (like br?) Are all those instruction emulated now? Thank you, > Then we just need to ensure that the BRK handlers are up and running in > time (which might require an early handler, like we have for KASAN). > > Will -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-28 9:07 ` Masami Hiramatsu @ 2020-10-28 9:48 ` Jean-Philippe Brucker 2020-10-28 12:21 ` Masami Hiramatsu 0 siblings, 1 reply; 16+ messages in thread From: Jean-Philippe Brucker @ 2020-10-28 9:48 UTC (permalink / raw) To: Masami Hiramatsu; +Cc: catalin.marinas, Will Deacon, dianders, linux-arm-kernel On Wed, Oct 28, 2020 at 06:07:31PM +0900, Masami Hiramatsu wrote: > > > Yes, I think so. Same order problem as the OS lock, they need to be reset > > > before enable_debug_monitors(). On CPU0 that would be before > > > early_initcall and for secondaries the hotplug notifier needs to be > > > installed earlier as well. I'll send a v2. > > > > Cheers. An alternative (which I think would be better in the long run > > anyway) would be to avoid using hardware step in kprobes and instead rely > > on a BRK instruction to trap after running the trampoline. > > But how we trap the instruction which can change pc? (like br?) > Are all those instruction emulated now? According to aarch64_insn_is_steppable() anything that changes the PC is emulated. I'm also checking whether there is a change of behavior with synchronous exceptions taken while single-stepping (page faults). Thanks, Jean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-28 9:48 ` Jean-Philippe Brucker @ 2020-10-28 12:21 ` Masami Hiramatsu 0 siblings, 0 replies; 16+ messages in thread From: Masami Hiramatsu @ 2020-10-28 12:21 UTC (permalink / raw) To: Jean-Philippe Brucker Cc: catalin.marinas, Will Deacon, dianders, linux-arm-kernel On Wed, 28 Oct 2020 10:48:27 +0100 Jean-Philippe Brucker <jean-philippe@linaro.org> wrote: > On Wed, Oct 28, 2020 at 06:07:31PM +0900, Masami Hiramatsu wrote: > > > > Yes, I think so. Same order problem as the OS lock, they need to be reset > > > > before enable_debug_monitors(). On CPU0 that would be before > > > > early_initcall and for secondaries the hotplug notifier needs to be > > > > installed earlier as well. I'll send a v2. > > > > > > Cheers. An alternative (which I think would be better in the long run > > > anyway) would be to avoid using hardware step in kprobes and instead rely > > > on a BRK instruction to trap after running the trampoline. > > > > But how we trap the instruction which can change pc? (like br?) > > Are all those instruction emulated now? > > According to aarch64_insn_is_steppable() anything that changes the PC is > emulated. OK, that sounds good. Then we can put the BRK right after the copied instruction. > I'm also checking whether there is a change of behavior with > synchronous exceptions taken while single-stepping (page faults). Thanks! From the kprobe_fault_handler(), it seems the page faults handled before the single-stepping exception and the fault handler disables single steping explicitly. So if we use BRK, that code will not be needed. Thank you, -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-10-28 8:36 ` Will Deacon 2020-10-28 9:07 ` Masami Hiramatsu @ 2020-11-25 16:09 ` Masami Hiramatsu 2020-11-25 16:11 ` Will Deacon 1 sibling, 1 reply; 16+ messages in thread From: Masami Hiramatsu @ 2020-11-25 16:09 UTC (permalink / raw) To: Will Deacon Cc: Jean-Philippe Brucker, catalin.marinas, dianders, Steven Rostedt, Masami Hiramatsu, linux-arm-kernel Hi Will, On Wed, 28 Oct 2020 08:36:44 +0000 Will Deacon <will@kernel.org> wrote: > On Wed, Oct 28, 2020 at 09:28:20AM +0100, Jean-Philippe Brucker wrote: > > On Tue, Oct 27, 2020 at 10:49:22PM +0900, Masami Hiramatsu wrote: > > > On Tue, 27 Oct 2020 12:33:18 +0000 > > > Will Deacon <will@kernel.org> wrote: > > > > > > > On Tue, Oct 27, 2020 at 12:59:09PM +0100, Jean-Philippe Brucker wrote: > > > > > On Tue, Oct 27, 2020 at 07:42:58PM +0900, Masami Hiramatsu wrote: > > > > > > On Tue, 27 Oct 2020 19:13:18 +0900 > > > > > > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > > > > > > > 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. > > > > > > > > > > > > Oops, it was my mistake. I missed to boot up with Xen. (so I find another bug...) > > > > > > Anyway this works for me too. > > > > > > > > > > No worries :) Although now I've been wondering whether it would be better > > > > > to just disable the OS lock lazily, on the first call to > > > > > enable_debug_monitors(). It might add a tiny performance penalty but would > > > > > avoid this problem reappearing if one of the debugger needs to start even > > > > > earlier in the future. > > > > > > > > I'm still uneasy about enabling KDE with the watchpoint registers in an > > > > unknown state, so I think this needs more work. > > > > > > Hmm, how we reset it in the early stage? reset watchpoint registers first? > > > > Yes, I think so. Same order problem as the OS lock, they need to be reset > > before enable_debug_monitors(). On CPU0 that would be before > > early_initcall and for secondaries the hotplug notifier needs to be > > installed earlier as well. I'll send a v2. > > Cheers. An alternative (which I think would be better in the long run > anyway) would be to avoid using hardware step in kprobes and instead rely > on a BRK instruction to trap after running the trampoline. We started working on using the BRK instead of hardware step in kprobes in other threads. However, there still be a bug in the kernel. I would like to fix or at least mitigate this issue until this is released (since it's a bug) Would you think we can push the BRK only kprobes until it or in stable kernel? Or, we should add a mitigation patch for this bug? For the mitigation, I think we can introduce a kconfig flag which indicates the arch doesn't support early kprobes, in that case we defer the kprobe and boot-time trace later stage. This flag will be removed after we introduce the BRK-only kprobes. Thank you, -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-11-25 16:09 ` Masami Hiramatsu @ 2020-11-25 16:11 ` Will Deacon 2020-11-25 16:18 ` Masami Hiramatsu 0 siblings, 1 reply; 16+ messages in thread From: Will Deacon @ 2020-11-25 16:11 UTC (permalink / raw) To: Masami Hiramatsu Cc: Jean-Philippe Brucker, Steven Rostedt, dianders, linux-arm-kernel, catalin.marinas Hi Masami, On Thu, Nov 26, 2020 at 01:09:06AM +0900, Masami Hiramatsu wrote: > On Wed, 28 Oct 2020 08:36:44 +0000 > Will Deacon <will@kernel.org> wrote: > > Cheers. An alternative (which I think would be better in the long run > > anyway) would be to avoid using hardware step in kprobes and instead rely > > on a BRK instruction to trap after running the trampoline. > > We started working on using the BRK instead of hardware step in kprobes > in other threads. However, there still be a bug in the kernel. > I would like to fix or at least mitigate this issue until this is released > (since it's a bug) > > Would you think we can push the BRK only kprobes until it or in stable kernel? > Or, we should add a mitigation patch for this bug? > For the mitigation, I think we can introduce a kconfig flag which indicates > the arch doesn't support early kprobes, in that case we defer the kprobe and > boot-time trace later stage. This flag will be removed after we introduce the > BRK-only kprobes. The BRK stuff is merged upstream: http://git.kernel.org/linus/7ee31a3aa8f49 Are you saying that this isn't sufficient to fix the problem? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm64: Fix early single-stepping 2020-11-25 16:11 ` Will Deacon @ 2020-11-25 16:18 ` Masami Hiramatsu 0 siblings, 0 replies; 16+ messages in thread From: Masami Hiramatsu @ 2020-11-25 16:18 UTC (permalink / raw) To: Will Deacon Cc: Jean-Philippe Brucker, Steven Rostedt, dianders, linux-arm-kernel, catalin.marinas On Wed, 25 Nov 2020 16:11:34 +0000 Will Deacon <will@kernel.org> wrote: > Hi Masami, > > On Thu, Nov 26, 2020 at 01:09:06AM +0900, Masami Hiramatsu wrote: > > On Wed, 28 Oct 2020 08:36:44 +0000 > > Will Deacon <will@kernel.org> wrote: > > > Cheers. An alternative (which I think would be better in the long run > > > anyway) would be to avoid using hardware step in kprobes and instead rely > > > on a BRK instruction to trap after running the trampoline. > > > > We started working on using the BRK instead of hardware step in kprobes > > in other threads. However, there still be a bug in the kernel. > > I would like to fix or at least mitigate this issue until this is released > > (since it's a bug) > > > > Would you think we can push the BRK only kprobes until it or in stable kernel? > > Or, we should add a mitigation patch for this bug? > > For the mitigation, I think we can introduce a kconfig flag which indicates > > the arch doesn't support early kprobes, in that case we defer the kprobe and > > boot-time trace later stage. This flag will be removed after we introduce the > > BRK-only kprobes. > > The BRK stuff is merged upstream: > > http://git.kernel.org/linus/7ee31a3aa8f49 > > Are you saying that this isn't sufficient to fix the problem? Oops, No, it should be enough. I thought we were still in discussion on the other thread... Anyway, thank you for merging! -- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-11-25 16:19 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-26 17:29 [PATCH] arm64: Fix early single-stepping Jean-Philippe Brucker 2020-10-26 17:38 ` Will Deacon 2020-10-27 0:48 ` Masami Hiramatsu 2020-10-27 10:13 ` Masami Hiramatsu 2020-10-27 10:42 ` Masami Hiramatsu 2020-10-27 11:59 ` Jean-Philippe Brucker 2020-10-27 12:33 ` Will Deacon 2020-10-27 13:49 ` Masami Hiramatsu 2020-10-28 8:28 ` Jean-Philippe Brucker 2020-10-28 8:36 ` Will Deacon 2020-10-28 9:07 ` Masami Hiramatsu 2020-10-28 9:48 ` Jean-Philippe Brucker 2020-10-28 12:21 ` Masami Hiramatsu 2020-11-25 16:09 ` Masami Hiramatsu 2020-11-25 16:11 ` Will Deacon 2020-11-25 16:18 ` Masami Hiramatsu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.