Hi Stefan On Mon, Dec 14, 2020 at 03:13:12PM +0100, Stefan Saecherl wrote: > 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 <[email protected]> > Signed-off-by: Lorena Kretzschmar <[email protected]> > Signed-off-by: Stefan Saecherl <[email protected]>
I took this to be a gap in the kgdbtest suite so I added a couple of tests that cover this area. Before this patch they failed now they pass (at least they do for ARCH=x86). I don't see any new failures either, so: Tested-by: Daniel Thompson <[email protected]> Daniel. > --- > 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
