Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1613721694-16418-1-git-send-email-sumit.garg@linaro.org>
In-Reply-To: <1613721694-16418-1-git-send-email-sumit.garg@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 22 Feb 2021 15:58:04 -0800
Message-ID: <CAD=FV=X1hsFf08J5JNifzFGw=1ikmXj2mp6K=KMOAzCYDWKZUw@mail.gmail.com>
Subject: Re: [PATCH] kernel: debug: Handle breakpoints in kernel .init.text section
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jason Wessel <jason.wessel@windriver.com>,
        Peter Zijlstra <peterz@infradead.org>, stefan.saecherl@fau.de,
        qy15sije@cip.cs.fau.de, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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