svn commit: r332860 - head/sys/kern

[Bruce Evans]

On Sat, 21 Apr 2018, Jonathan T. Looney wrote:

> Log:
>  When running with INVARIANTS, the kernel contains extra checks.  However,
>  these assumptions may not hold true once we've panic'd. Therefore, the
>  checks hold less value after a panic.  Additionally, if one of the checks
>  fails while we are already panic'd, this creates a double-panic which can
>  interfere with debugging the original panic.
>
>  Therefore, this commit allows an administrator to suppress a response to
>  KASSERT checks after a panic by setting a tunable/sysctl.  The
>  tunable/sysctl (debug.kassert.suppress_in_panic) defaults to being
>  enabled.
> ...
> Modified: head/sys/kern/kern_shutdown.c
> ==============================================================================
> --- head/sys/kern/kern_shutdown.c	Sat Apr 21 15:15:47 2018	(r332859)
> +++ head/sys/kern/kern_shutdown.c	Sat Apr 21 17:05:00 2018	(r332860)
> @@ -642,6 +642,7 @@ static int kassert_do_log = 1;
> static int kassert_log_pps_limit = 4;
> static int kassert_log_mute_at = 0;
> static int kassert_log_panic_at = 0;
> +static int kassert_suppress_in_panic = 1;
> static int kassert_warnings = 0;
>
> SYSCTL_NODE(_debug, OID_AUTO, kassert, CTLFLAG_RW, NULL, "kassert options");
> @@ -676,6 +677,10 @@ SYSCTL_INT(_debug_kassert, OID_AUTO, log_pps_limit, CT
> SYSCTL_INT(_debug_kassert, OID_AUTO, log_mute_at, CTLFLAG_RWTUN,
>     &kassert_log_mute_at, 0, "max number of KASSERTS to log");
>
> +SYSCTL_INT(_debug_kassert, OID_AUTO, suppress_in_panic, CTLFLAG_RWTUN,
> +    &kassert_suppress_in_panic, 0,
> +    "KASSERTs will be suppressed while handling a panic");
> +
> static int kassert_sysctl_kassert(SYSCTL_HANDLER_ARGS);
>
> SYSCTL_PROC(_debug_kassert, OID_AUTO, kassert,
> @@ -707,6 +712,10 @@ kassert_panic(const char *fmt, ...)
> {
> 	static char buf[256];
> 	va_list ap;
> +
> +	/* If we already panic'd, don't create a double-fault. */
> +	if (panicstr != NULL && kassert_suppress_in_panic)
> +		return;
>
> 	va_start(ap, fmt);
> 	(void)vsnprintf(buf, sizeof(buf), fmt, ap);

panic() can't return, but I see that KASSERT() has already been broken
to use kassert_panic() which does return in some cases including this
new one.

KASSERT(9) is still documented to call panic(), and none of the options
to break it including this new one, or kassert_panic() itself are
documented in KASSERT(9) or in any other section 9 man page.

Lots of code was and still is written under the assumption that KASSERT()
works as documented.  E.g., after a null pointer check using KASSERT(),
the pointer is normally used and the page fault for this should cause a
panic anyway.  This is actually a feature -- it works around KASSERT()
breaking restartability of the fault.  Other cases are not so good.  There
man be a cascade of KASSERT() failures, and the new option prevents even
printing a message about even one of them.  If you are lucky then you get
a clean null pointer recursive panic.

The only example in KASSERT(9) is to give the worst use of it (for breaking
clean null pointer panics when KASSERT() works as documented, but now doesn't
even break them when KASSERT() returns).

Bruce