Ever since commit 5516fd7b92a7 ("debug: prevent entering debug mode on
panic/exception.") (yes, years ago) my kgdb workflow has been broken.
On Chrome OS we have 'kernel.panic = -1' in
'/etc/sysctl.d/00-sysctl.conf'. That means that when userspace starts
up it will tell the kernel "please reboot on panic". ...and so when I
get a panic then the system reboots instead of letting me debug it.
While I could go in an change the 'sysctl.conf' and I could go in and
hack the kernel myself, these things are inconvenient. I either need
to keep a private kernel patch or or remember to edit a file every
time I install an updated version of Chrome OS. What is convenient
(for me) is to have a CONFIG option that makes kgdb override the panic
request. This is because the Chrome OS build system makes it very
easy for me to add some extra CONFIG "fragments" to my debug kernels.
Hopefully having this extra config option is OK and useful to others
who would also prefer to make sure that kgdb is always entered on a
panic no matter what userspace might request.
Signed-off-by: Douglas Anderson <[email protected]>
---
kernel/debug/debug_core.c | 5 +++--
lib/Kconfig.kgdb | 10 ++++++++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 65c0f1363788..d4a38543fcdd 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -703,7 +703,8 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs)
* reboot on panic. We don't want to get stuck waiting for input
* on such systems, especially if its "just" an oops.
*/
- if (signo != SIGTRAP && panic_timeout)
+ if (!IS_ENABLED(CONFIG_KGDB_ALWAYS_ENTER_ON_PANIC) &&
+ signo != SIGTRAP && panic_timeout)
return 1;
memset(ks, 0, sizeof(struct kgdb_state));
@@ -843,7 +844,7 @@ static int kgdb_panic_event(struct notifier_block *self,
* panic_timeout indicates the system should automatically
* reboot on panic.
*/
- if (panic_timeout)
+ if (!IS_ENABLED(CONFIG_KGDB_ALWAYS_ENTER_ON_PANIC) && panic_timeout)
return NOTIFY_DONE;
if (dbg_kdb_mode)
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index ab4ff0eea776..f12c6e1394c6 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -67,6 +67,16 @@ config KGDB_LOW_LEVEL_TRAP
exception handler which will allow kgdb to step through a
notify handler.
+config KGDB_ALWAYS_ENTER_ON_PANIC
+ bool "KGDB: Enter kgdb on panic even if reboot specified"
+ default n
+ help
+ If kgdb is enabled and the system is configured to reboot on
+ panic then there's a question of whether we should drop into
+ kgdb on panic or whether we should reboot on panic. If you
+ say yes here then we'll enter kgdb. If you say no here then
+ we'll reboot.
+
config KGDB_KDB
bool "KGDB_KDB: include kdb frontend for kgdb"
default n
--
2.20.0.rc2.403.gdbc3b29805-goog
On Sun, Dec 09, 2018 at 06:36:49PM -0800, Douglas Anderson wrote:
> Ever since commit 5516fd7b92a7 ("debug: prevent entering debug mode on
> panic/exception.") (yes, years ago) my kgdb workflow has been broken.
> On Chrome OS we have 'kernel.panic = -1' in
> '/etc/sysctl.d/00-sysctl.conf'. That means that when userspace starts
> up it will tell the kernel "please reboot on panic". ...and so when I
> get a panic then the system reboots instead of letting me debug it.
>
> While I could go in an change the 'sysctl.conf' and I could go in and
> hack the kernel myself, these things are inconvenient. I either need
> to keep a private kernel patch or or remember to edit a file every
> time I install an updated version of Chrome OS. What is convenient
> (for me) is to have a CONFIG option that makes kgdb override the panic
> request. This is because the Chrome OS build system makes it very
> easy for me to add some extra CONFIG "fragments" to my debug kernels.
>
> Hopefully having this extra config option is OK and useful to others
> who would also prefer to make sure that kgdb is always entered on a
> panic no matter what userspace might request.
>
> Signed-off-by: Douglas Anderson <[email protected]>
Sorry to be late with this review. I forgot to search for "debug:" when
I was checking for missed patches earlier.
Mind you... one of the reasons I deferred review when you first sent it
in was that my gut reaction was "I don't like it" so I decided to wait
until I could offer a head reaction instead.
Ultimately I'm not sure this should be solved within kgdb. Perhaps best
phrased as: is the problem that kgdb *misinterprets* panic_timeout or is
the problem that Doug wants to *override* panic_timeout?
I think the answer to this question is the later meaning I'm interested
in what happens if you introduce a CONFIG_PANIC_TIMEOUT_FORCE (c.f.
CONFIG_CMDLINE_FORCE) to prevent the userspace changing the
panic_timeout (either by avoiding registering the panic sysctl or, if
that is a huge ABI problem attaching it to a different variable).
TBH I'm not sure if such a patch would be accepted but I think it makes
more semantic sense!
(there is a small review comment below but the above is more important)
> ---
>
> kernel/debug/debug_core.c | 5 +++--
> lib/Kconfig.kgdb | 10 ++++++++++
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 65c0f1363788..d4a38543fcdd 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -703,7 +703,8 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs)
> * reboot on panic. We don't want to get stuck waiting for input
> * on such systems, especially if its "just" an oops.
> */
> - if (signo != SIGTRAP && panic_timeout)
> + if (!IS_ENABLED(CONFIG_KGDB_ALWAYS_ENTER_ON_PANIC) &&
> + signo != SIGTRAP && panic_timeout)
This code path is called via notify_die() rather than a panic().
Daniel.
> return 1;
>
> memset(ks, 0, sizeof(struct kgdb_state));
> @@ -843,7 +844,7 @@ static int kgdb_panic_event(struct notifier_block *self,
> * panic_timeout indicates the system should automatically
> * reboot on panic.
> */
> - if (panic_timeout)
> + if (!IS_ENABLED(CONFIG_KGDB_ALWAYS_ENTER_ON_PANIC) && panic_timeout)
> return NOTIFY_DONE;
>
> if (dbg_kdb_mode)
> diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
> index ab4ff0eea776..f12c6e1394c6 100644
> --- a/lib/Kconfig.kgdb
> +++ b/lib/Kconfig.kgdb
> @@ -67,6 +67,16 @@ config KGDB_LOW_LEVEL_TRAP
> exception handler which will allow kgdb to step through a
> notify handler.
>
> +config KGDB_ALWAYS_ENTER_ON_PANIC
> + bool "KGDB: Enter kgdb on panic even if reboot specified"
> + default n
> + help
> + If kgdb is enabled and the system is configured to reboot on
> + panic then there's a question of whether we should drop into
> + kgdb on panic or whether we should reboot on panic. If you
> + say yes here then we'll enter kgdb. If you say no here then
> + we'll reboot.
> +
> config KGDB_KDB
> bool "KGDB_KDB: include kdb frontend for kgdb"
> default n
> --
> 2.20.0.rc2.403.gdbc3b29805-goog
>
Hi,
On Tue, Dec 18, 2018 at 9:05 AM Daniel Thompson
<[email protected]> wrote:
>
> On Sun, Dec 09, 2018 at 06:36:49PM -0800, Douglas Anderson wrote:
> > Ever since commit 5516fd7b92a7 ("debug: prevent entering debug mode on
> > panic/exception.") (yes, years ago) my kgdb workflow has been broken.
> > On Chrome OS we have 'kernel.panic = -1' in
> > '/etc/sysctl.d/00-sysctl.conf'. That means that when userspace starts
> > up it will tell the kernel "please reboot on panic". ...and so when I
> > get a panic then the system reboots instead of letting me debug it.
> >
> > While I could go in an change the 'sysctl.conf' and I could go in and
> > hack the kernel myself, these things are inconvenient. I either need
> > to keep a private kernel patch or or remember to edit a file every
> > time I install an updated version of Chrome OS. What is convenient
> > (for me) is to have a CONFIG option that makes kgdb override the panic
> > request. This is because the Chrome OS build system makes it very
> > easy for me to add some extra CONFIG "fragments" to my debug kernels.
> >
> > Hopefully having this extra config option is OK and useful to others
> > who would also prefer to make sure that kgdb is always entered on a
> > panic no matter what userspace might request.
> >
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> Sorry to be late with this review. I forgot to search for "debug:" when
> I was checking for missed patches earlier.
>
> Mind you... one of the reasons I deferred review when you first sent it
> in was that my gut reaction was "I don't like it" so I decided to wait
> until I could offer a head reaction instead.
>
> Ultimately I'm not sure this should be solved within kgdb. Perhaps best
> phrased as: is the problem that kgdb *misinterprets* panic_timeout or is
> the problem that Doug wants to *override* panic_timeout?
>
> I think the answer to this question is the later meaning I'm interested
> in what happens if you introduce a CONFIG_PANIC_TIMEOUT_FORCE (c.f.
> CONFIG_CMDLINE_FORCE) to prevent the userspace changing the
> panic_timeout (either by avoiding registering the panic sysctl or, if
> that is a huge ABI problem attaching it to a different variable).
>
> TBH I'm not sure if such a patch would be accepted but I think it makes
> more semantic sense!
>
> (there is a small review comment below but the above is more important)
Thanks for the review. Yeah, it definitely was a bit of a
questionable patch but I figured I'd throw it out there to see what
folks thought. I think we should just drop it. I talked with Brian
about this offline and we agree that it actually should be OK to just
drop the setting from '00-sysctl.conf'. I have my patch at
crrev.com/c/1382879 and it looks like folks are happy with it. :-)
-Doug