Messages in this thread | | | Date | Thu, 20 Nov 2014 09:34:40 +0000 | From | Daniel Thompson <> | Subject | Re: [RFC] debug: add parameters to prevent entering debug mode on errors |
| |
On 20/11/14 08:18, Kiran Raparthy wrote: > Hi Daniel, > > On 18 November 2014 22:43, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> On 18/11/14 12:08, Kiran Kumar Raparthy wrote: >>> From: Colin Cross <ccross@android.com> >>> >>> debug: add parameters to prevent entering debug mode on errors >>> >>> On non-developer devices kgdb prevents CONFIG_PANIC_TIMEOUT from rebooting the >>> device after a panic. Add module parameters debug_core.break_on_exception and >>> debug_core.break_on_panic to allow skipping debug on panics and exceptions >>> respectively. Both default to true to preserve existing behavior. >> >> I am a little unsure about break_on_panic. >> >> It ought to be possible for kgdb/kdb to honour CONFIG_PANIC_TIMEOUT by >> tracking how long it takes for the user to attach a debugger (or to run >> the first kdb command after the panic). As it happens the timeout value >> is already an exported kernel symbol so all the info it there for us to >> use... >> >> Doing so would save us imposing further configuration burden on the user >> (although it would be a good deal more code). >> >> Note that I can't think of an automatic way to handle break_on_exception >> so I'm less worried about that one. > Alright,so it it okay if we have this mechanism limited to "skip debug > on exceptions"? > please let me know if i have misunderstood your point.
Spliting it up would certainly stop a review comment from needlessly interfering with good stuff being delivered. That's always a good thing.
To be clear though, providing the user a way to prevent kgdb from preventing the machine from rebooting after panic seems to me to be a useful feature. It is simply that I think the existing panic_timeout value could be used to realize it.
>>> + return 1; >>> + >>> memset(ks, 0, sizeof(struct kgdb_state)); >>> ks->cpu = raw_smp_processor_id(); >>> ks->ex_vector = evector; >>> @@ -821,6 +830,9 @@ static int kgdb_panic_event(struct notifier_block *self, >>> unsigned long val, >>> void *data) >>> { >>> + if (!break_on_panic) >>> + return NOTIFY_DONE;
How about simply:
if (panic_timeout) return NOTIFY_DONE;
(plus a nice comment explaining why)
This doesn't implement a timeout and so does not prevent a physically present user from exploiting kgdb. Nevertheless its an accurate interpretation of what the user told us to do and leaves the door open to adding a timeout in the future.
Actually it might be a good idea to use panic_timeout to control trap-on-oops as well! If the user wants the machine to reboot itself on panic they certainly don't want it to hang during an oops.
if (panic_timeout) return NOTIFY_DONE;
>>> + >>> if (dbg_kdb_mode) >>> kdb_prinf("PANIC: %s\n", (char *)data); >>> kgdb_breakpoint(); >>> >>
| |