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

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

[Sumit Garg]

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.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Sumit Garg <sumit.garg@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))
+			kgdb_break[i].state = BP_ACTIVE;
+		else
+			kgdb_break[i].state = BP_ACTIVE_INIT;
 	}
 	return ret;
 }
@@ -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",
@@ -425,7 +434,7 @@ int kgdb_has_hit_break(unsigned long addr)
 	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 &&
 		    kgdb_break[i].bpt_addr == addr)
 			return 1;
 	}
@@ -439,7 +448,7 @@ int dbg_remove_all_break(void)
 
 	/* Clear memory breakpoints. */
 	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
-		if (kgdb_break[i].state != BP_ACTIVE)
+		if (kgdb_break[i].state < BP_ACTIVE_INIT)
 			goto setundefined;
 		error = kgdb_arch_remove_breakpoint(&kgdb_break[i]);
 		if (error)
-- 
2.7.4

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

[Doug Anderson]

Hi,

On Fri, Feb 19, 2021 at 12:03 AM Sumit Garg <sumit.garg@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 <peterz@infradead.org>
> Signed-off-by: Sumit Garg <sumit.garg@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

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

[Sumit Garg]

Thanks Doug for your comments.

On Tue, 23 Feb 2021 at 05:28, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Feb 19, 2021 at 12:03 AM Sumit Garg <sumit.garg@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...
>

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()

>
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Sumit Garg <sumit.garg@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?
>

"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":

@@ -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.

>
> ...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?

Yes and this is what we are trying to achieve via changes to
dbg_deactivate_sw_breakpoints() that will drop all the init
breakpoints the first time we drop into the debugger after they are
freed.

>
> 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?

Since the hw breakpoints are implemented in an arch specific manner,
so I would expect following arch specific callbacks to correctly
handle .init.text section hw breakpoints:
- arch_kgdb_ops.disable_hw_break()
- arch_kgdb_ops.correct_hw_break()

in a similar manner as this patch does for sw breakpoints. And since
they are only implemented for x86 currently for which I don't have a
development machine, so I will leave that for others to fix.

-Sumit

>
> -Doug

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

[Daniel Thompson]

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.

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

[Sumit Garg]

On Tue, 23 Feb 2021 at 18:24, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> 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.
>

Ah, I see. Thanks for further clarification. Will get rid of
BP_ACTIVE_INIT state.

-Sumit

>
> Daniel.