Hi,
On Fri, Feb 19, 2021 at 12:03 AM Sumit Garg <sumit.g...@linaro.org> wrote:
>
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
>
> In order to keep track of .init.text section breakpoints, add another
> breakpoint state as BP_ACTIVE_INIT and don't try to free these
> breakpoints once the system is in running state.
>
> To be clear there is still a very small window between call to
> free_initmem() and system_state = SYSTEM_RUNNING which can lead to
> removal of freed .init.text section breakpoints but I think we can live
> with that.
I know kdb / kgdb tries to keep out of the way of the rest of the
system and so there's a bias to just try to infer the state of the
rest of the system, but this feels like a halfway solution when really
a cleaner solution really wouldn't intrude much on the main kernel.
It seems like it's at least worth asking if we can just add a call
like kgdb_drop_init_breakpoints() into main.c. Then we don't have to
try to guess the state...
> Suggested-by: Peter Zijlstra <pet...@infradead.org>
> Signed-off-by: Sumit Garg <sumit.g...@linaro.org>
> ---
> include/linux/kgdb.h | 3 ++-
> kernel/debug/debug_core.c | 17 +++++++++++++----
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 0d6cf64..57b8885 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -71,7 +71,8 @@ enum kgdb_bpstate {
> BP_UNDEFINED = 0,
> BP_REMOVED,
> BP_SET,
> - BP_ACTIVE
> + BP_ACTIVE_INIT,
> + BP_ACTIVE,
> };
>
> struct kgdb_bkpt {
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index af6e8b4f..229dd11 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -324,7 +324,11 @@ int dbg_activate_sw_breakpoints(void)
> }
>
> kgdb_flush_swbreak_addr(kgdb_break[i].bpt_addr);
> - kgdb_break[i].state = BP_ACTIVE;
> + if (system_state >= SYSTEM_RUNNING ||
> + !init_section_contains((void *)kgdb_break[i].bpt_addr, 0))
I haven't searched through all the code, but is there any chance that
this could trigger incorrectly? After we free the init memory could
it be re-allocated to something that would contain code that would
execute in kernel context and now we'd be unable to set breakpoints in
that area?
> + kgdb_break[i].state = BP_ACTIVE;
> + else
> + kgdb_break[i].state = BP_ACTIVE_INIT;
I don't really see what the "BP_ACTIVE_INIT" state gets you. Why not
just leave it as "BP_ACTIVE" and put all the logic fully in
dbg_deactivate_sw_breakpoints()?
...or, if we can inject a call in main.c we can do a one time delete
of all "init" breakpoints and get rid of all this logic? Heck, even
if we can't get called by "main.c", we still only need to do a
one-time drop of all init breakpoints the first time we drop into the
debugger after they are freed, right?
Oh shoot. I just realized another problem. What about hardware
breakpoints? You still need to call "remove" on them once after init
memory is freed, right?
-Doug
