Message-ID: <CAFA6WYO0PkbpXUJp9jayb71LsydpnHfLFyc21bHotC1dLJ7Dhg@mail.gmail.com>
Date:   Tue, 23 Feb 2021 14:33:50 +0530
From:   Sumit Garg <sumit.garg@...aro.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     kgdb-bugreport@...ts.sourceforge.net,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        Peter Zijlstra <peterz@...radead.org>, stefan.saecherl@....de,
        qy15sije@....cs.fau.de, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section

Thanks Doug for your comments.

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

Powered by blists - more mailing lists