Date: Wed, 29 Nov 2017 20:52:04 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alexey Dokuchaev <danfe@freebsd.org> Cc: Edward Tomasz Napierala <trasz@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r326314 - in head/sys: ddb kern sys Message-ID: <20171129200224.A930@besplex.bde.org> In-Reply-To: <20171129065701.GB71839@FreeBSD.org> References: <201711281253.vASCrtlB071488@repo.freebsd.org> <20171129030035.W2283@besplex.bde.org> <20171129065701.GB71839@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 29 Nov 2017, Alexey Dokuchaev wrote:
> On Wed, Nov 29, 2017 at 04:04:49AM +1100, Bruce Evans wrote:
>> I use the following fix of disabling the spam in all cases (unless reenabled
>> in all cases using ddb): [...]
>
> Since someone is hacking on subr_kdb.c, may I ask them to commit attached
> small patch? I want to do some work in that area, and these style issues
Mergeing external diffs is actually harder for me since I have too many
local changes that aren't quite ready to commit.
X Index: kern/subr_kdb.c
X ===================================================================
X --- kern/subr_kdb.c (revision 326310)
X +++ kern/subr_kdb.c (working copy)
X @@ -224,7 +224,7 @@
X return (0);
X }
X
X -void
X +static void
X kdb_panic(const char *msg)
X {
X
X @@ -232,7 +232,7 @@
X panic("%s", msg);
X }
X
X -void
X +static void
X kdb_reboot(void)
X {
X
These changes are problematic. They don't compile, since the functions
are still declared as implicit-extern in <sys/kdb.h>. More seriously,
if these functions can be static because they are only called from
kdb_alternate_break_internal(), then all calls to them are broken since
"alternate" "break"s are broken (more broken than their name). They
are done from interrupt handlers where it is invalid to call almost any
function for the "fast" interrupt handler case (serial console drivers)
and invalid to call most functions in the normal interrupt handler case
(syscons and vt).
Calling kdb_reboot() from an interrupt handler is most invalid. It
calls shutdown_nice(), which needs normal thread context. In the usual
case where initproc != NULL, shutdown_nice() begins with PROC_LOCK(initproc).
This deadlocks if the lock is already held, which is possible iff the
interrupt handler is "fast". Normal interrupt handlers might work since
ithreads have close enough to normal context. However, they might hold
an unrelated spinlock so acquiring a sleep lock might cause a LOR and a
warning for this, and should cause a warning even if no LOR.
Calling kdb_panic() from an interrupt handler has a chance of working
since panic() ignores locking. Races might occur instead. Console input
on the device handling the interrupt might be broken during panic. It
is required to work in ddb, but this requires delicate locking and special
methods while kdb is active and is not done for panics. Panicing from
within ddb is similarly broken.
"alternate" "break"s also open some minor security holes in syscons keyboard
enables.
X @@ -362,7 +362,6 @@
X * is selected or the current debugger does not support backtraces, this
X * function silently returns.
X */
X -
X void
X kdb_backtrace(void)
X {
X ...
The style fixes are correct.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171129200224.A930>