tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

marking kern_assert(9) as __dead, and recursive panics



I would like to declare kern_assert() as __dead, so that static analysers can understand that code after a failed KASSERT is never executed.

However, kern_assert returns without panicing if panicstr != NULL (that is, if a panic has already occurred), so gcc will not allow it to be declared __dead.

kern_assert's predecessor, __assert(), used to be declared with __attribute__((__noreturn__)), but that was removed in revision 1.70 of sys/lib/libkern/libkern.h and revision 1.9 of sys/lib/libkern/__assert.c. A similar change was made to assert_sleepable() in kern_lock.c at the same time. The relevant commits were these:

2007-07-29 11:45  ad
        * src/sys/kern/kern_lock.c (1.117):
        Be more forgiving if panicstr != NULL.

2007-07-29 11:46  ad
        * src/sys/lib/libkern/: __assert.c (1.9), libkern.h (1.70):
        Disable kernel assertions if panicstr != NULL.

2007-08-03 13:06  ad
        * src/sys/lib/libkern/__assert.c (1.10):
        Do the panicstr check only if _KERNEL.


I propose to to do the following:

* Keep the panicstr test in assert_sleepable() in sys/kern/kern_lock.c,
  so it continues to degenerates to a no-op after a panic.  I am not
  sure whether this behaviour is necessary, but I see no reason to
  change the status quo here.

* Remove the panicstr test from kern_assert() in
  sys/lib/libkern/kern_assert.c, so that KASSERT, KASSERTMSG and
  friends do not degenerate to no-ops after a panic.

  I don't know a reason for making all kernel asserts degenerate
  to no-ops, but I imagine that it might have been a workaround
  for problems with recursive panics, and I propose to address
  recursive panics directly (see below).

  I can also imagine that there are particular kernel asserts
  that need to degenerate to no-ops after a panic, and I suggest
  explicitly rewriting them in terms of (panicstr != NULL ||
  <other tests>).  I have not attempted to identify such asserts.

* Change the declaration of kern_assert() in sys/lib/libkern/libkern.h to

      void kern_assert(const char *, ...) __dead __printflike(1, 2);

  so that static analysers will know that it never returns.

* Change db_panic() in ddb/db_panic.c so that a panic while the system
  is already in DDB will always re-enter DDB, regardless of the
  value of the db_onpanic kernel variable (which corresponds to the
  ddb.onpanic sysctl variable).

* Change db_command_loop() in ddb/db_command.c so that a panic while
  executing the commands stored in the db_cmd_on_enter kernel variable
  (which corresponds to the ddb/commandonenter sysctl variable) will
  not cause an infinite loop.

The end result should be that, after a panic has occurred, an assertion failure will be treated like any other recursive panic, and either drop into the debugger, or halt or reboot immediately, without syncing the disks.

I append a patch to do all that (except for asserts that might need to be changed to explicitly test panicstr).

--apb (Alan Barrett)


Index: sys/ddb/db_command.c
===================================================================
--- sys/ddb/db_command.c        6 Jan 2013 04:17:27 -0000       1.142
+++ sys/ddb/db_command.c        10 Feb 2013 15:37:12 -0000
@@ -535,6 +535,7 @@ db_command_loop(void)
 {
        label_t db_jmpbuf;
        label_t *savejmp;
+       static bool db_processing_cmd_on_enter = false;
/*
         * Initialize 'prev' and 'next' to dot.
@@ -554,7 +555,13 @@ db_command_loop(void)
        (void) setjmp(&db_jmpbuf);
/* Execute default ddb start commands */
-       db_execute_commandlist(db_cmd_on_enter);
+       if (db_processing_cmd_on_enter) {
+               db_printf("Fault during db_cmd_on_enter; aborting...\n");
+       } else {
+               db_processing_cmd_on_enter = true;
+               db_execute_commandlist(db_cmd_on_enter);
+       }
+       db_processing_cmd_on_enter = false;
(void) setjmp(&db_jmpbuf);
        while (!db_cmd_loop_done) {
Index: sys/ddb/db_panic.c
===================================================================
--- sys/ddb/db_panic.c  10 Feb 2013 11:04:20 -0000      1.1
+++ sys/ddb/db_panic.c  10 Feb 2013 15:37:12 -0000
@@ -40,9 +40,22 @@ __KERNEL_RCSID(0, "$NetBSD: db_panic.c,v
 void db_panic(void)
 {
- if (db_onpanic == 1)
-               Debugger();
-       else if (db_onpanic >= 0) {
+       /*
+        * Do what db_onpanic requires.
+        * Valid settings are documented in ddb(4):
+        * -1: No traceback, do not enter debugger.
+        *  0: Print traceback, do not enter debugger.
+        *  1: No traceback, enter debugger.
+        *  2: Print traceback, enter debugger.
+        * other positive values: documented to enter debugger;
+        *     in this implementation, they are treated the same as 2.
+        * other negative values: documented not to enter debugger;
+        *     in this implementation, they are treated the same as -1.
+        *
+        * Also, if DDB was already active (db_recover != 0), then
+        * re-enter the debugger regardless of the value of db_onpanic.
+        */
+       if (db_onpanic >= 0 && db_onpanic != 1) {
                static int intrace = 0;
if (intrace == 0) {
@@ -57,8 +70,8 @@ void db_panic(void)
                        intrace = 0;
                } else
                        printf("Faulted in mid-traceback; aborting...");
-               if (db_onpanic == 2)
-                       Debugger();
        }
+       if (db_onpanic >= 1 || db_recover != 0)
+               Debugger();
        return;
 }
Index: sys/lib/libkern/kern_assert.c
===================================================================
--- sys/lib/libkern/kern_assert.c       29 Sep 2011 20:50:09 -0000      1.2
+++ sys/lib/libkern/kern_assert.c       10 Feb 2013 15:37:12 -0000
@@ -42,10 +42,6 @@ void
 kern_assert(const char *fmt, ...)
 {
        va_list ap;
-#ifdef _KERNEL
-       if (panicstr != NULL)
-               return;
-#endif
        va_start(ap, fmt);
        vpanic(fmt, ap);
        va_end(ap);
Index: sys/lib/libkern/libkern.h
===================================================================
--- sys/lib/libkern/libkern.h   30 Aug 2012 12:16:49 -0000      1.106
+++ sys/lib/libkern/libkern.h   10 Feb 2013 15:37:12 -0000
@@ -300,8 +300,7 @@ int  ffs(int);
 #define        ffs(x)          __builtin_ffs(x)
 #endif
-void kern_assert(const char *, ...)
-    __attribute__((__format__(__printf__, 1, 2)));
+void   kern_assert(const char *, ...) __dead __printflike(1,2);
 unsigned int
        bcdtobin(unsigned int);
 unsigned int


Home | Main Index | Thread Index | Old Index