[v3,05/11] arm64: Add call_break_hook() to early_brk64() for early kgdb

Commit Message

Doug Anderson April 28, 2020, 9:13 p.m. UTC

In order to make early kgdb work properly we need early_brk64() to be
able to call into it.  This is as easy as adding a call into
call_break_hook() just like we do later in the normal brk_handler().

Once we do this we can let kgdb know that it can break into the
debugger a little earlier (specifically when parsing early_param's).

NOTE: without this patch it turns out that arm64 can't do breakpoints
even at dbg_late_init(), so if we decide something about this patch is
wrong we might need to move dbg_late_init() a little later.

Signed-off-by: Douglas Anderson <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Will Deacon <[e-mail]>
Reviewed-by: Greg Kroah-Hartman <[e-mail]>
Reviewed-by: Daniel Thompson <[e-mail]>
---

Changes in v3:
- Change boolean weak function to KConfig.

Changes in v2: None

 arch/arm64/Kconfig                      | 1 +
 arch/arm64/include/asm/debug-monitors.h | 2 ++
 arch/arm64/kernel/debug-monitors.c      | 2 +-
 arch/arm64/kernel/traps.c               | 3 +++
 4 files changed, 7 insertions(+), 1 deletion(-)

Comments

Will Deacon May 11, 2020, 2:59 p.m. UTC | #1

Hi Doug,

On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 48222a4760c2..59c353dfc8e9 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -297,7 +297,7 @@ void unregister_kernel_break_hook(struct break_hook *hook)
>  	unregister_debug_hook(&hook->node);
>  }
>  
> -static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> +int call_break_hook(struct pt_regs *regs, unsigned int esr)
>  {
>  	struct break_hook *hook;
>  	struct list_head *list;
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index cf402be5c573..a8173f0c1774 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
>  	if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
>  		return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
>  #endif
> +	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> +		return 0;

I think this just means we're not running debug_traps_init() early enough,
and actually the KASAN early handler is unnecessary too.

If we call debug_traps_init() directly from setup_arch() and drop the
arch_initcall(), can we then drop early_brk64 entirely?

Will

Doug Anderson May 11, 2020, 10:45 p.m. UTC | #2

Hi,

On Mon, May 11, 2020 at 7:59 AM Will Deacon <[e-mail]> wrote:
>
> Hi Doug,
>
> On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > index 48222a4760c2..59c353dfc8e9 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -297,7 +297,7 @@ void unregister_kernel_break_hook(struct break_hook *hook)
> >       unregister_debug_hook(&hook->node);
> >  }
> >
> > -static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> > +int call_break_hook(struct pt_regs *regs, unsigned int esr)
> >  {
> >       struct break_hook *hook;
> >       struct list_head *list;
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index cf402be5c573..a8173f0c1774 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> >       if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> >               return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> >  #endif
> > +     if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> > +             return 0;
>
> I think this just means we're not running debug_traps_init() early enough,
> and actually the KASAN early handler is unnecessary too.
>
> If we call debug_traps_init() directly from setup_arch() and drop the
> arch_initcall(), can we then drop early_brk64 entirely?

It seems to work in my testing.  ...but the worry I have is the
comment right before trap_init().  It says:

/* This registration must happen early, before debug_traps_init(). */

By moving debug_traps_init() early we're violating that comment.  Do I
just remove that comment, or was there a good reason for it?  ...or am
I reading it wrong and I should have read it as if it said:

/* NOTE: this registration happens early, before debug_traps_init(). */

...then removing it is fine.  Maybe that's right?

I coded this up and put it on the Chrome OS gerrit at
<https://crrev.com/c/2195061>.  I'm happy to post this on the list as
a loner patch to replace this one or spin the whole series depending
on what people want.


-Doug

Will Deacon May 12, 2020, 7:35 a.m. UTC | #3

On Mon, May 11, 2020 at 03:45:02PM -0700, Doug Anderson wrote:
> On Mon, May 11, 2020 at 7:59 AM Will Deacon <[e-mail]> wrote:
> > On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > > index 48222a4760c2..59c353dfc8e9 100644
> > > --- a/arch/arm64/kernel/debug-monitors.c
> > > +++ b/arch/arm64/kernel/debug-monitors.c
> > > @@ -297,7 +297,7 @@ void unregister_kernel_break_hook(struct break_hook *hook)
> > >       unregister_debug_hook(&hook->node);
> > >  }
> > >
> > > -static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> > > +int call_break_hook(struct pt_regs *regs, unsigned int esr)
> > >  {
> > >       struct break_hook *hook;
> > >       struct list_head *list;
> > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > index cf402be5c573..a8173f0c1774 100644
> > > --- a/arch/arm64/kernel/traps.c
> > > +++ b/arch/arm64/kernel/traps.c
> > > @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> > >       if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> > >               return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> > >  #endif
> > > +     if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > +             return 0;
> >
> > I think this just means we're not running debug_traps_init() early enough,
> > and actually the KASAN early handler is unnecessary too.
> >
> > If we call debug_traps_init() directly from setup_arch() and drop the
> > arch_initcall(), can we then drop early_brk64 entirely?
> 
> It seems to work in my testing.  ...but the worry I have is the
> comment right before trap_init().  It says:
> 
> /* This registration must happen early, before debug_traps_init(). */

I /think/ the reason for this is because debug_traps_init() replaces the
BRK vector, so if that runs before the break hooks have been registered
for e.g. BUG() then BUG() won't work during that window. Hmm, so dropping
early_brk64 is problematic after all. Damn.

Is trap_init() early enough for you? If so, we could call debug_traps_init()
from traps_init() after registering the break hooks.

Will

Doug Anderson May 12, 2020, 3:27 p.m. UTC | #4

Hi,

On Tue, May 12, 2020 at 12:36 AM Will Deacon <[e-mail]> wrote:
>
> On Mon, May 11, 2020 at 03:45:02PM -0700, Doug Anderson wrote:
> > On Mon, May 11, 2020 at 7:59 AM Will Deacon <[e-mail]> wrote:
> > > On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> > > > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > > > index 48222a4760c2..59c353dfc8e9 100644
> > > > --- a/arch/arm64/kernel/debug-monitors.c
> > > > +++ b/arch/arm64/kernel/debug-monitors.c
> > > > @@ -297,7 +297,7 @@ void unregister_kernel_break_hook(struct break_hook *hook)
> > > >       unregister_debug_hook(&hook->node);
> > > >  }
> > > >
> > > > -static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> > > > +int call_break_hook(struct pt_regs *regs, unsigned int esr)
> > > >  {
> > > >       struct break_hook *hook;
> > > >       struct list_head *list;
> > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > index cf402be5c573..a8173f0c1774 100644
> > > > --- a/arch/arm64/kernel/traps.c
> > > > +++ b/arch/arm64/kernel/traps.c
> > > > @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> > > >       if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> > > >               return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > >  #endif
> > > > +     if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > > +             return 0;
> > >
> > > I think this just means we're not running debug_traps_init() early enough,
> > > and actually the KASAN early handler is unnecessary too.
> > >
> > > If we call debug_traps_init() directly from setup_arch() and drop the
> > > arch_initcall(), can we then drop early_brk64 entirely?
> >
> > It seems to work in my testing.  ...but the worry I have is the
> > comment right before trap_init().  It says:
> >
> > /* This registration must happen early, before debug_traps_init(). */
>
> I /think/ the reason for this is because debug_traps_init() replaces the
> BRK vector, so if that runs before the break hooks have been registered
> for e.g. BUG() then BUG() won't work during that window. Hmm, so dropping
> early_brk64 is problematic after all. Damn.
>
> Is trap_init() early enough for you? If so, we could call debug_traps_init()
> from traps_init() after registering the break hooks.

"Early enough" is a subjective term, of course.  The earlier we can
init, the earlier we can drop into the debugger.  ...but, of course,
everyone thinks their feature is the most important and should be
first, so let's see...

Certainly if we waited until trap_init() it wouldn't be early enough
to set "ARCH_HAS_EARLY_DEBUG".  Setting that means that debugging is
ready when early params are parsed and those happen at the start of
setup_arch().  The call to trap_init() happens a bit later.

If we decide that we just don't care about getting
"ARCH_HAS_EARLY_DEBUG" to work then the earliest we'll be able to
break into the debugger (via kgdbwait) is dbg_late_init().  That
_does_ happen after trap_init() so your solution would work.

As a person who spends most of his time in driver land, it wouldn't be
the end of the world to wait for dbg_late_init().  That's still much
earlier than most code I'd ever debug.  ...and, bonus points is that
if we hit a crash any time after earlyparams we _will_ still drop into
the debugger.  It's only breakpoints that won't be available until
dbg_late_init().


tl;dr:

* If we care about "kgdbwait" and breakpoints working as early as
possible then we need my patch.

* If we are OK w/ a slightly later "kgdbwait" then I think we can move
debug_traps_init() to trap_init() and get rid of the early version.


Please let me know which way you'd like to proceed.

-Doug

Will Deacon May 13, 2020, 6:17 a.m. UTC | #5

Hey Doug,

On Tue, May 12, 2020 at 08:27:50AM -0700, Doug Anderson wrote:
> On Tue, May 12, 2020 at 12:36 AM Will Deacon <[e-mail]> wrote:
> > On Mon, May 11, 2020 at 03:45:02PM -0700, Doug Anderson wrote:
> > > On Mon, May 11, 2020 at 7:59 AM Will Deacon <[e-mail]> wrote:
> > > > On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > > index cf402be5c573..a8173f0c1774 100644
> > > > > --- a/arch/arm64/kernel/traps.c
> > > > > +++ b/arch/arm64/kernel/traps.c
> > > > > @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> > > > >       if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> > > > >               return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > > >  #endif
> > > > > +     if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > > > +             return 0;
> > > >
> > > > I think this just means we're not running debug_traps_init() early enough,
> > > > and actually the KASAN early handler is unnecessary too.
> > > >
> > > > If we call debug_traps_init() directly from setup_arch() and drop the
> > > > arch_initcall(), can we then drop early_brk64 entirely?
> > >
> > > It seems to work in my testing.  ...but the worry I have is the
> > > comment right before trap_init().  It says:
> > >
> > > /* This registration must happen early, before debug_traps_init(). */
> >
> > I /think/ the reason for this is because debug_traps_init() replaces the
> > BRK vector, so if that runs before the break hooks have been registered
> > for e.g. BUG() then BUG() won't work during that window. Hmm, so dropping
> > early_brk64 is problematic after all. Damn.
> >
> > Is trap_init() early enough for you? If so, we could call debug_traps_init()
> > from traps_init() after registering the break hooks.
> 
> "Early enough" is a subjective term, of course.  The earlier we can
> init, the earlier we can drop into the debugger.  ...but, of course,
> everyone thinks their feature is the most important and should be
> first, so let's see...
> 
> Certainly if we waited until trap_init() it wouldn't be early enough
> to set "ARCH_HAS_EARLY_DEBUG".  Setting that means that debugging is
> ready when early params are parsed and those happen at the start of
> setup_arch().  The call to trap_init() happens a bit later.
> 
> If we decide that we just don't care about getting
> "ARCH_HAS_EARLY_DEBUG" to work then the earliest we'll be able to
> break into the debugger (via kgdbwait) is dbg_late_init().  That
> _does_ happen after trap_init() so your solution would work.
> 
> As a person who spends most of his time in driver land, it wouldn't be
> the end of the world to wait for dbg_late_init().  That's still much
> earlier than most code I'd ever debug.  ...and, bonus points is that
> if we hit a crash any time after earlyparams we _will_ still drop into
> the debugger.  It's only breakpoints that won't be available until
> dbg_late_init().
> 
> 
> tl;dr:
> 
> * If we care about "kgdbwait" and breakpoints working as early as
> possible then we need my patch.
> 
> * If we are OK w/ a slightly later "kgdbwait" then I think we can move
> debug_traps_init() to trap_init() and get rid of the early version.
> 
> 
> Please let me know which way you'd like to proceed.

Let's go with the trap_init() approach for now, and we can revisit it later
if somebody has a compelling reason to initialise things earlier. However,
I don't think you can remove early_brk64(), as it's needed for BUG() to
work correctly.

Will

Doug Anderson May 13, 2020, 11:08 p.m. UTC | #6

Hi,

On Tue, May 12, 2020 at 11:17 PM Will Deacon <[e-mail]> wrote:
>
> Hey Doug,
>
> On Tue, May 12, 2020 at 08:27:50AM -0700, Doug Anderson wrote:
> > On Tue, May 12, 2020 at 12:36 AM Will Deacon <[e-mail]> wrote:
> > > On Mon, May 11, 2020 at 03:45:02PM -0700, Doug Anderson wrote:
> > > > On Mon, May 11, 2020 at 7:59 AM Will Deacon <[e-mail]> wrote:
> > > > > On Tue, Apr 28, 2020 at 02:13:45PM -0700, Douglas Anderson wrote:
> > > > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > > > index cf402be5c573..a8173f0c1774 100644
> > > > > > --- a/arch/arm64/kernel/traps.c
> > > > > > +++ b/arch/arm64/kernel/traps.c
> > > > > > @@ -1044,6 +1044,9 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> > > > > >       if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
> > > > > >               return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> > > > > >  #endif
> > > > > > +     if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > > > > +             return 0;
> > > > >
> > > > > I think this just means we're not running debug_traps_init() early enough,
> > > > > and actually the KASAN early handler is unnecessary too.
> > > > >
> > > > > If we call debug_traps_init() directly from setup_arch() and drop the
> > > > > arch_initcall(), can we then drop early_brk64 entirely?
> > > >
> > > > It seems to work in my testing.  ...but the worry I have is the
> > > > comment right before trap_init().  It says:
> > > >
> > > > /* This registration must happen early, before debug_traps_init(). */
> > >
> > > I /think/ the reason for this is because debug_traps_init() replaces the
> > > BRK vector, so if that runs before the break hooks have been registered
> > > for e.g. BUG() then BUG() won't work during that window. Hmm, so dropping
> > > early_brk64 is problematic after all. Damn.
> > >
> > > Is trap_init() early enough for you? If so, we could call debug_traps_init()
> > > from traps_init() after registering the break hooks.
> >
> > "Early enough" is a subjective term, of course.  The earlier we can
> > init, the earlier we can drop into the debugger.  ...but, of course,
> > everyone thinks their feature is the most important and should be
> > first, so let's see...
> >
> > Certainly if we waited until trap_init() it wouldn't be early enough
> > to set "ARCH_HAS_EARLY_DEBUG".  Setting that means that debugging is
> > ready when early params are parsed and those happen at the start of
> > setup_arch().  The call to trap_init() happens a bit later.
> >
> > If we decide that we just don't care about getting
> > "ARCH_HAS_EARLY_DEBUG" to work then the earliest we'll be able to
> > break into the debugger (via kgdbwait) is dbg_late_init().  That
> > _does_ happen after trap_init() so your solution would work.
> >
> > As a person who spends most of his time in driver land, it wouldn't be
> > the end of the world to wait for dbg_late_init().  That's still much
> > earlier than most code I'd ever debug.  ...and, bonus points is that
> > if we hit a crash any time after earlyparams we _will_ still drop into
> > the debugger.  It's only breakpoints that won't be available until
> > dbg_late_init().
> >
> >
> > tl;dr:
> >
> > * If we care about "kgdbwait" and breakpoints working as early as
> > possible then we need my patch.
> >
> > * If we are OK w/ a slightly later "kgdbwait" then I think we can move
> > debug_traps_init() to trap_init() and get rid of the early version.
> >
> >
> > Please let me know which way you'd like to proceed.
>
> Let's go with the trap_init() approach for now, and we can revisit it later
> if somebody has a compelling reason to initialise things earlier. However,
> I don't think you can remove early_brk64(), as it's needed for BUG() to
> work correctly.

Posted at:

https://lore.kernel.org/r/20200513160501.1.I0b5edf030cc6ebef6ab4829f8867cdaea42485d8@changeid

I'll also reply to the v4 version of this patch to point at it.

-Doug

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 40fb05d96c60..08a736175d2d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -13,6 +13,7 @@  config ARM64
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_DMA_PREP_COHERENT
 	select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
+	select ARCH_HAS_EARLY_DEBUG
 	select ARCH_HAS_FAST_MULTIPLIER
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 7619f473155f..2d82a0314d29 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -97,6 +97,8 @@  void unregister_user_break_hook(struct break_hook *hook);
 void register_kernel_break_hook(struct break_hook *hook);
 void unregister_kernel_break_hook(struct break_hook *hook);
 
+int call_break_hook(struct pt_regs *regs, unsigned int esr);
+
 u8 debug_monitors_arch(void);
 
 enum dbg_active_el {
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 48222a4760c2..59c353dfc8e9 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -297,7 +297,7 @@  void unregister_kernel_break_hook(struct break_hook *hook)
 	unregister_debug_hook(&hook->node);
 }
 
-static int call_break_hook(struct pt_regs *regs, unsigned int esr)
+int call_break_hook(struct pt_regs *regs, unsigned int esr)
 {
 	struct break_hook *hook;
 	struct list_head *list;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index cf402be5c573..a8173f0c1774 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1044,6 +1044,9 @@  int __init early_brk64(unsigned long addr, unsigned int esr,
 	if ((comment & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
 		return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
+	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
 	return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
 }