* [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints
@ 2015-08-25 22:02 Douglas Anderson
2015-08-25 22:26 ` Stephen Boyd
2015-08-26 0:32 ` Kees Cook
0 siblings, 2 replies; 3+ messages in thread
From: Douglas Anderson @ 2015-08-25 22:02 UTC (permalink / raw)
To: Kees Cook, Nicolas Pitre
Cc: Aapo Vienamo, Stephen Boyd, Douglas Anderson, linux,
masami.hiramatsu.pt, wangnan0, linux-arm-kernel, linux-kernel
In (23a4e40 arm: kgdb: Handle read-only text / modules) we moved to
using patch_text() to set breakpoints so that we could handle the case
when we had CONFIG_DEBUG_RODATA. That patch used patch_text().
Unfortunately, patch_text() assumes that we're not in atomic context
when it runs since it needs to grab a mutex and also wait for other
CPUs to stop (which it does with a completion).
This would result in a stack crawl if you had
CONFIG_DEBUG_ATOMIC_SLEEP and tried to set a breakpoint in kgdb. The
crawl looked something like:
BUG: scheduling while atomic: swapper/0/0/0x00010007
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.2.0-rc7-00133-geb63b34 #1073
Hardware name: Rockchip (Device Tree)
(unwind_backtrace) from [<c00133d4>] (show_stack+0x20/0x24)
(show_stack) from [<c05400e8>] (dump_stack+0x84/0xb8)
(dump_stack) from [<c004913c>] (__schedule_bug+0x54/0x6c)
(__schedule_bug) from [<c054065c>] (__schedule+0x80/0x668)
(__schedule) from [<c0540cfc>] (schedule+0xb8/0xd4)
(schedule) from [<c0543a3c>] (schedule_timeout+0x2c/0x234)
(schedule_timeout) from [<c05417c0>] (wait_for_common+0xf4/0x188)
(wait_for_common) from [<c0541874>] (wait_for_completion+0x20/0x24)
(wait_for_completion) from [<c00a0104>] (__stop_cpus+0x58/0x70)
(__stop_cpus) from [<c00a0580>] (stop_cpus+0x3c/0x54)
(stop_cpus) from [<c00a06c4>] (__stop_machine+0xcc/0xe8)
(__stop_machine) from [<c00a0714>] (stop_machine+0x34/0x44)
(stop_machine) from [<c00173e8>] (patch_text+0x28/0x34)
(patch_text) from [<c001733c>] (kgdb_arch_set_breakpoint+0x40/0x4c)
(kgdb_arch_set_breakpoint) from [<c00a0d68>] (kgdb_validate_break_address+0x2c/0x60)
(kgdb_validate_break_address) from [<c00a0e90>] (dbg_set_sw_break+0x1c/0xdc)
(dbg_set_sw_break) from [<c00a2e88>] (gdb_serial_stub+0x9c4/0xba4)
(gdb_serial_stub) from [<c00a11cc>] (kgdb_cpu_enter+0x1f8/0x60c)
(kgdb_cpu_enter) from [<c00a18cc>] (kgdb_handle_exception+0x19c/0x1d0)
(kgdb_handle_exception) from [<c0016f7c>] (kgdb_compiled_brk_fn+0x30/0x3c)
(kgdb_compiled_brk_fn) from [<c00091a4>] (do_undefinstr+0x1a4/0x20c)
(do_undefinstr) from [<c001400c>] (__und_svc_finish+0x0/0x34)
It turns out that when we're in kgdb all the CPUs are stopped anyway
so there's no reason we should be calling patch_text(). We can
instead directly call __patch_text() which assumes that CPUs have
already been stopped.
Fixes: 23a4e4050ba9 ("arm: kgdb: Handle read-only text / modules")
Reported-by: Aapo Vienamo <avienamo@nvidia.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
arch/arm/kernel/kgdb.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
index a6ad93c..fd9eefc 100644
--- a/arch/arm/kernel/kgdb.c
+++ b/arch/arm/kernel/kgdb.c
@@ -259,15 +259,17 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
if (err)
return err;
- patch_text((void *)bpt->bpt_addr,
- *(unsigned int *)arch_kgdb_ops.gdb_bpt_instr);
+ /* Machine is already stopped, so we can use __patch_text() directly */
+ __patch_text((void *)bpt->bpt_addr,
+ *(unsigned int *)arch_kgdb_ops.gdb_bpt_instr);
return err;
}
int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
{
- patch_text((void *)bpt->bpt_addr, *(unsigned int *)bpt->saved_instr);
+ /* Machine is already stopped, so we can use __patch_text() directly */
+ __patch_text((void *)bpt->bpt_addr, *(unsigned int *)bpt->saved_instr);
return 0;
}
--
2.5.0.457.gab17608
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints
2015-08-25 22:02 [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints Douglas Anderson
@ 2015-08-25 22:26 ` Stephen Boyd
2015-08-26 0:32 ` Kees Cook
1 sibling, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2015-08-25 22:26 UTC (permalink / raw)
To: Douglas Anderson
Cc: Kees Cook, Nicolas Pitre, Aapo Vienamo, linux,
masami.hiramatsu.pt, wangnan0, linux-arm-kernel, linux-kernel
On 08/25, Douglas Anderson wrote:
> In (23a4e40 arm: kgdb: Handle read-only text / modules) we moved to
> using patch_text() to set breakpoints so that we could handle the case
> when we had CONFIG_DEBUG_RODATA. That patch used patch_text().
> Unfortunately, patch_text() assumes that we're not in atomic context
> when it runs since it needs to grab a mutex and also wait for other
> CPUs to stop (which it does with a completion).
>
> This would result in a stack crawl if you had
> CONFIG_DEBUG_ATOMIC_SLEEP and tried to set a breakpoint in kgdb. The
> crawl looked something like:
>
> BUG: scheduling while atomic: swapper/0/0/0x00010007
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.2.0-rc7-00133-geb63b34 #1073
> Hardware name: Rockchip (Device Tree)
> (unwind_backtrace) from [<c00133d4>] (show_stack+0x20/0x24)
> (show_stack) from [<c05400e8>] (dump_stack+0x84/0xb8)
> (dump_stack) from [<c004913c>] (__schedule_bug+0x54/0x6c)
> (__schedule_bug) from [<c054065c>] (__schedule+0x80/0x668)
> (__schedule) from [<c0540cfc>] (schedule+0xb8/0xd4)
> (schedule) from [<c0543a3c>] (schedule_timeout+0x2c/0x234)
> (schedule_timeout) from [<c05417c0>] (wait_for_common+0xf4/0x188)
> (wait_for_common) from [<c0541874>] (wait_for_completion+0x20/0x24)
> (wait_for_completion) from [<c00a0104>] (__stop_cpus+0x58/0x70)
> (__stop_cpus) from [<c00a0580>] (stop_cpus+0x3c/0x54)
> (stop_cpus) from [<c00a06c4>] (__stop_machine+0xcc/0xe8)
> (__stop_machine) from [<c00a0714>] (stop_machine+0x34/0x44)
> (stop_machine) from [<c00173e8>] (patch_text+0x28/0x34)
> (patch_text) from [<c001733c>] (kgdb_arch_set_breakpoint+0x40/0x4c)
> (kgdb_arch_set_breakpoint) from [<c00a0d68>] (kgdb_validate_break_address+0x2c/0x60)
> (kgdb_validate_break_address) from [<c00a0e90>] (dbg_set_sw_break+0x1c/0xdc)
> (dbg_set_sw_break) from [<c00a2e88>] (gdb_serial_stub+0x9c4/0xba4)
> (gdb_serial_stub) from [<c00a11cc>] (kgdb_cpu_enter+0x1f8/0x60c)
> (kgdb_cpu_enter) from [<c00a18cc>] (kgdb_handle_exception+0x19c/0x1d0)
> (kgdb_handle_exception) from [<c0016f7c>] (kgdb_compiled_brk_fn+0x30/0x3c)
> (kgdb_compiled_brk_fn) from [<c00091a4>] (do_undefinstr+0x1a4/0x20c)
> (do_undefinstr) from [<c001400c>] (__und_svc_finish+0x0/0x34)
>
> It turns out that when we're in kgdb all the CPUs are stopped anyway
> so there's no reason we should be calling patch_text(). We can
> instead directly call __patch_text() which assumes that CPUs have
> already been stopped.
>
> Fixes: 23a4e4050ba9 ("arm: kgdb: Handle read-only text / modules")
> Reported-by: Aapo Vienamo <avienamo@nvidia.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints
2015-08-25 22:02 [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints Douglas Anderson
2015-08-25 22:26 ` Stephen Boyd
@ 2015-08-26 0:32 ` Kees Cook
1 sibling, 0 replies; 3+ messages in thread
From: Kees Cook @ 2015-08-26 0:32 UTC (permalink / raw)
To: Douglas Anderson
Cc: Nicolas Pitre, Aapo Vienamo, Stephen Boyd,
Russell King - ARM Linux, Masami Hiramatsu, wangnan0,
linux-arm-kernel@lists.infradead.org, LKML
On Tue, Aug 25, 2015 at 3:02 PM, Douglas Anderson <dianders@chromium.org> wrote:
> In (23a4e40 arm: kgdb: Handle read-only text / modules) we moved to
> using patch_text() to set breakpoints so that we could handle the case
> when we had CONFIG_DEBUG_RODATA. That patch used patch_text().
> Unfortunately, patch_text() assumes that we're not in atomic context
> when it runs since it needs to grab a mutex and also wait for other
> CPUs to stop (which it does with a completion).
>
> This would result in a stack crawl if you had
> CONFIG_DEBUG_ATOMIC_SLEEP and tried to set a breakpoint in kgdb. The
> crawl looked something like:
>
> BUG: scheduling while atomic: swapper/0/0/0x00010007
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.2.0-rc7-00133-geb63b34 #1073
> Hardware name: Rockchip (Device Tree)
> (unwind_backtrace) from [<c00133d4>] (show_stack+0x20/0x24)
> (show_stack) from [<c05400e8>] (dump_stack+0x84/0xb8)
> (dump_stack) from [<c004913c>] (__schedule_bug+0x54/0x6c)
> (__schedule_bug) from [<c054065c>] (__schedule+0x80/0x668)
> (__schedule) from [<c0540cfc>] (schedule+0xb8/0xd4)
> (schedule) from [<c0543a3c>] (schedule_timeout+0x2c/0x234)
> (schedule_timeout) from [<c05417c0>] (wait_for_common+0xf4/0x188)
> (wait_for_common) from [<c0541874>] (wait_for_completion+0x20/0x24)
> (wait_for_completion) from [<c00a0104>] (__stop_cpus+0x58/0x70)
> (__stop_cpus) from [<c00a0580>] (stop_cpus+0x3c/0x54)
> (stop_cpus) from [<c00a06c4>] (__stop_machine+0xcc/0xe8)
> (__stop_machine) from [<c00a0714>] (stop_machine+0x34/0x44)
> (stop_machine) from [<c00173e8>] (patch_text+0x28/0x34)
> (patch_text) from [<c001733c>] (kgdb_arch_set_breakpoint+0x40/0x4c)
> (kgdb_arch_set_breakpoint) from [<c00a0d68>] (kgdb_validate_break_address+0x2c/0x60)
> (kgdb_validate_break_address) from [<c00a0e90>] (dbg_set_sw_break+0x1c/0xdc)
> (dbg_set_sw_break) from [<c00a2e88>] (gdb_serial_stub+0x9c4/0xba4)
> (gdb_serial_stub) from [<c00a11cc>] (kgdb_cpu_enter+0x1f8/0x60c)
> (kgdb_cpu_enter) from [<c00a18cc>] (kgdb_handle_exception+0x19c/0x1d0)
> (kgdb_handle_exception) from [<c0016f7c>] (kgdb_compiled_brk_fn+0x30/0x3c)
> (kgdb_compiled_brk_fn) from [<c00091a4>] (do_undefinstr+0x1a4/0x20c)
> (do_undefinstr) from [<c001400c>] (__und_svc_finish+0x0/0x34)
>
> It turns out that when we're in kgdb all the CPUs are stopped anyway
> so there's no reason we should be calling patch_text(). We can
> instead directly call __patch_text() which assumes that CPUs have
> already been stopped.
>
> Fixes: 23a4e4050ba9 ("arm: kgdb: Handle read-only text / modules")
> Reported-by: Aapo Vienamo <avienamo@nvidia.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> arch/arm/kernel/kgdb.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
> index a6ad93c..fd9eefc 100644
> --- a/arch/arm/kernel/kgdb.c
> +++ b/arch/arm/kernel/kgdb.c
> @@ -259,15 +259,17 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> if (err)
> return err;
>
> - patch_text((void *)bpt->bpt_addr,
> - *(unsigned int *)arch_kgdb_ops.gdb_bpt_instr);
> + /* Machine is already stopped, so we can use __patch_text() directly */
> + __patch_text((void *)bpt->bpt_addr,
> + *(unsigned int *)arch_kgdb_ops.gdb_bpt_instr);
>
> return err;
> }
>
> int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> {
> - patch_text((void *)bpt->bpt_addr, *(unsigned int *)bpt->saved_instr);
> + /* Machine is already stopped, so we can use __patch_text() directly */
> + __patch_text((void *)bpt->bpt_addr, *(unsigned int *)bpt->saved_instr);
>
> return 0;
> }
> --
> 2.5.0.457.gab17608
>
Ah, yes, much nicer. :) Thanks for chasing this!
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-08-26 0:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-25 22:02 [PATCH] arm: kgdb: Don't try to stop the machine when setting breakpoints Douglas Anderson
2015-08-25 22:26 ` Stephen Boyd
2015-08-26 0:32 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox