The problem is that breakpoints that are set early (e.g. via kgdbwait) cannot be deleted after boot completed (to be precise after mark_rodata_ro ran).
When setting a breakpoint early there are executable pages that are writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint succeeds and the breakpoint is saved as type BP_BREAKPOINT. Later in the boot write access to these pages is restricted. So when removing the breakpoint the copy_to_kernel_nofault call in kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which can work around nonwriteability. One thing to consider when doing this is that code can go away during boot (e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case gracefully by just having copy_to_kernel_nofault fail but if one then calls text_poke_kgdb the system dies due to the BUG_ON we moved out of __text_poke. To avoid this __text_poke now returns an error in case of a nonpresent code page and the error is handled at call site. Checkpatch complains about two uses of BUG_ON but the new code should not trigger BUG_ON in cases where the old didn't. Co-developed-by: Lorena Kretzschmar <qy15s...@cip.cs.fau.de> Signed-off-by: Lorena Kretzschmar <qy15s...@cip.cs.fau.de> Signed-off-by: Stefan Saecherl <stefan.saech...@fau.de> --- arch/x86/kernel/alternative.c | 16 +++++++---- arch/x86/kernel/kgdb.c | 54 ++++++++++++++++++++++++----------- 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 2400ad62f330..0f145d837885 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode, size_t len) if (cross_page_boundary) pages[1] = virt_to_page(addr + PAGE_SIZE); } - /* - * If something went wrong, crash and burn since recovery paths are not - * implemented. - */ - BUG_ON(!pages[0] || (cross_page_boundary && !pages[1])); + + if (!pages[0] || (cross_page_boundary && !pages[1])) + return ERR_PTR(-EFAULT); /* * Map the page without the global bit, as TLB flushing is done with @@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t len) { lockdep_assert_held(&text_mutex); - return __text_poke(addr, opcode, len); + addr = __text_poke(addr, opcode, len); + /* + * If something went wrong, crash and burn since recovery paths are not + * implemented. + */ + BUG_ON(IS_ERR(addr)); + return addr; } /** diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c index ff7878df96b4..e98c9c43db7c 100644 --- a/arch/x86/kernel/kgdb.c +++ b/arch/x86/kernel/kgdb.c @@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip) int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) { int err; + void *addr; bpt->type = BP_BREAKPOINT; err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr, @@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) */ if (mutex_is_locked(&text_mutex)) return -EBUSY; - text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr, - BREAK_INSTR_SIZE); + + addr = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr, + BREAK_INSTR_SIZE); + /* This should never trigger because the above call to copy_from_kernel_nofault + * already succeeded. + */ + BUG_ON(IS_ERR(addr)); + bpt->type = BP_POKE_BREAKPOINT; return 0; @@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt) { - if (bpt->type != BP_POKE_BREAKPOINT) - goto knl_write; - /* - * It is safe to call text_poke_kgdb() because normal kernel execution - * is stopped on all cores, so long as the text_mutex is not locked. - */ - if (mutex_is_locked(&text_mutex)) - goto knl_write; - text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr, - BREAK_INSTR_SIZE); - return 0; + void *addr; + int err; -knl_write: - return copy_to_kernel_nofault((char *)bpt->bpt_addr, - (char *)bpt->saved_instr, BREAK_INSTR_SIZE); + if (bpt->type == BP_POKE_BREAKPOINT) { + if (mutex_is_locked(&text_mutex)) { + err = copy_to_kernel_nofault((char *)bpt->bpt_addr, + (char *)bpt->saved_instr, + BREAK_INSTR_SIZE); + } else { + /* + * It is safe to call text_poke_kgdb() because normal kernel execution + * is stopped on all cores, so long as the text_mutex is not locked. + */ + addr = text_poke_kgdb((void *)bpt->bpt_addr, + bpt->saved_instr, + BREAK_INSTR_SIZE); + err = PTR_ERR_OR_ZERO(addr); + } + } else { + err = copy_to_kernel_nofault((char *)bpt->bpt_addr, + (char *)bpt->saved_instr, + BREAK_INSTR_SIZE); + if (err == -EFAULT && !mutex_is_locked(&text_mutex)) { + addr = text_poke_kgdb((void *)bpt->bpt_addr, + bpt->saved_instr, + BREAK_INSTR_SIZE); + err = PTR_ERR_OR_ZERO(addr); + } + } + return err; } const struct kgdb_arch arch_kgdb_ops = { -- 2.20.1