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 <diand...@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.
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
