Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section

[Daniel Thompson <daniel.thompson@linaro.org>]

On Tue, Feb 23, 2021 at 02:33:50PM +0530, Sumit Garg wrote:
> Thanks Doug for your comments.
> 
> On Tue, 23 Feb 2021 at 05:28, Doug Anderson <dianders@chromium.org> wrote:
> > > 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...

Just for the record, +1. This would be a better approach.


> Sounds reasonable, will post RFC for this. I think we should call such
> function as kgdb_free_init_mem() in similar way as:
> - kprobe_free_init_mem()
> - ftrace_free_init_mem()

As is matching the names...


> @@ -378,8 +382,13 @@ int dbg_deactivate_sw_breakpoints(void)
>         int i;
> 
>         for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> -               if (kgdb_break[i].state != BP_ACTIVE)
> +               if (kgdb_break[i].state < BP_ACTIVE_INIT)
> +                       continue;
> +               if (system_state >= SYSTEM_RUNNING &&
> +                   kgdb_break[i].state == BP_ACTIVE_INIT) {
> +                       kgdb_break[i].state = BP_UNDEFINED;
>                         continue;
> +               }
>                 error = kgdb_arch_remove_breakpoint(&kgdb_break[i]);
>                 if (error) {
>                         pr_info("BP remove failed: %lx\n",
> 
> >
> > > +                       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()?
> 
> Please see my response above.
>
> [which was]
> > "BP_ACTIVE_INIT" state is added specifically to handle this scenario
> > as to keep track of breakpoints that actually belong to the .init.text
> > section. And we should be able to again set breakpoints after free
> > since below change in this patch would mark them as "BP_UNDEFINED":

This answer does not say whether the BP_ACTIVE_INIT state needs to be
per-breakpoint state or whether we can infer it from the global state.

Changing the state of breakpoints in .init is a one-shot activity
whether it is triggered explicitly (e.g. kgdb_free_init_mem) or implicitly
(run the first time dbg_deactivate_sw_breakpoints is called with the system
state >= running).

As Doug has suggested it is quite possible to unify all the logic to
handle .init within a single function by running that function when the
state changes globally.


Daniel.