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 <diand...@chromium.org>
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
>
