Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section
From: Sumit Garg
Date: Wed Feb 24 2021 - 00:30:18 EST
On 2021年2月23日 at 18:24, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> wrote:
>
>
On Tue, Feb 23, 2021 at 02:33:50PM +0530, Sumit Garg wrote:
>
> Thanks Doug for your comments.
>
>
>
> On 2021年2月23日 at 05:28, Doug Anderson <dianders@xxxxxxxxxxxx> 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.
>
Ah, I see. Thanks for further clarification. Will get rid of
BP_ACTIVE_INIT state.
-Sumit
>
>
Daniel.