"process slock" vs. "scrlock" lock order

[Bruce Evans]

On Sat, 2 Oct 2010, Don Lewis wrote:

> The hard coded lock order list in subr_witness.c has "scrlock" listed
> before "process slock".  This causes a lock order reversal when
> calcru1(), which requires "process slock" to be held, calls printf() to
> report unexpected runtime problems.  The call to printf() eventually
> gets into the console code which locks "scrlock".

Console drivers are not permitted to use any normal locks, since they
are required to work when called from any instruction boundary via a
trace trap into ddb.  Syscons has lots of state, so it is difficult
for it to be reentrant enough to be a console driver.  It barely tries,
but mostly works anyway.  It used to use the axed cndbctl() call to
try harder.  This told it when ddb was entered and exited, so that it
could do things like save its state on ddb entry and restore it on ddb
exit.  In practice, it did little more than stop the screen saver and
switch to vty0 on ddb entry and set a private variable to indicate
that it was in ddb mode instead of peeking at db_active.  Then it used
this local variable in a few places to avoid a few dangerous things.
It still uses this variable to decide what to do, but this variable
is now never initialized (except statically to 0).  Replacing tests
of this variable by tests of kdb_active would unbreak a few things
and lose mainly the vty switch relative to the old version.

"scrlock" seems to be the only lock in syscons internals (except it
is giant locked), and it is already guarded by a kdb_active test (and
that is the only kdb_active test in syscons internals), so it mostly
doesn't cause problems for calls from ddb, just like the old cndbctl()
tests.  This part of it was cloned from sio where it is less incorrect
since the corresponding lock is made MTX_QUIET iff any sio devices is
a console.   (This is still wrong, since sio's lock should be a normal
one and any console lock a separate non-normal one.  Among other
problems, it makes sio's lock too quiet.)  syscons's lock is missing
the MTX_QUIET, but this lock is not a normal one (it is only used for
console output) so it can become more correct.  OTOH, its limited use
makes it useless for locking syscons generally.  It is only used to
prevents corruption of data structures (and garbled output) by multople
concurrent calls into the console driver.  It doesn't prevent corruption
from a console call concurrent with a (Giant-locked and maybe tty-locked)
user call.  sio's needs the corresponding locking only to reduce
garbling of output, since it console calls are reentrant enough to to
avoid corrupting any software state and most hardware state.

> This normally isn't
> noticed because both of these are spin locks, and hardly anyone uses
> WITNESS without disabling the checking of spinlocks with
> WITNESS_SKIPSPIN.  If spin lock checking is not disabled, the result is
> a silent reset because witness catches the LOR, which recurses into
> printf(), which ends up causing a panic in cnputs().
>
> One obvious fix would be to move "scrlock" to a later spot in the list,
> but I suspect the same problem could occur with the "sio" or "uart"
> locks if a serial console is being used.  It might not be possible to
> fix them the same way because there might be cases where they are in the
> input path and get locked before "process slock" or other spin locks
> that can be held when calling printf().

I think sio isn't affected, since it uses MTX_QUIET (though maybe it needs
MTX_NOWITNESS too -- one or both of those should "work" by breaking
witnessing in much the same way as WITNESS_SKIPSPIN).  uart is missing
the MTX_QUIET, and uses a too-normal lock for the console.

uart has locking for the whole of cngetc() too (except it drops the
look to wait), while sio has only reentrancy for cngetc().  Both are
useless for serialization, since cngetc() hasn't actually been a getc
function since ~2001 (?) when the multiple console changes broke input.
It is now cncheckc() misnamed.  The multiple console code polls each
console for input in turn, even when there is only 1 active console,
and this involves dropping locks so interrupts tend to eat your input.

> Another fix for this particular case would be to rearrange the code in
> calcru1() so that the calls to printf() occur after ruxp->rux_* are
> updated and where I assume it would be safe to temporarily drop "process
> slock" for the duration of the printf() calls.

printf() is supposed to be callable from almost anywhere (just not quite
at any instruction boundary unless in ddb mode).

There is related broken locking in cnputs().  This uses a non-normal
mutex for serialization.  The mutex is MTX_NOWITNESS and MTX_QUIET,
but there are no kdb_active tests before using it, and it us not bogusly
MTX_RECURSE, so it can deadlock in some cases (all cases with ddb output?)
when cnputs() is debugged.  I use better serialization of output involving
a similar (but less normal) lock over single printfs (callers wanting
to ensure non-garbled output must put it all together).  Deadlock is
avoided by ignoring the lock after trying for it for 1 second.  Console
drivers still need lower-level locking to protect their data structures.

The Giant locking in syscons seems bogus now that there is tty locking.
In the syscons directory, it is only done explicitly in sckbdevent(),
which calls tty_rint*() which needs tty locking but there is none
visible (maybe an upper layer of the interrupt handler does it, or
Giant locking of everything is enough).

"scrlock" causes problems with tty locking too.  syscons.c has only a
single explicit tty_lock() call, and that one is under "#if 0" together
with some scroll lock handling since "scrlock" causes a more detectable
LOR relative to tty_lock.  This is in sc_cngetc().  The LOR detection
has exposed the larger bug that a console driver is calling an upper
tty layer.  In old versions, the call was directly to scstart() except
for a check of the upper layer's open flag.  This was unsafe too (it
called up to the tty layer).  Add full tty locking to syscons and you
would probably find its console routines can't go anywhere without
hitting the tty lock, so when a console routine is called with the tty
lock held, it should deadlock or panic.  Giant locking was too feeble
to detect such problems, and before Giant I thought syscons was missing
lots of spl locking (which needed to be splhigh() to defend against
reentry for printf from an interrupt handler, leaving only the problem
of reentry for printf from a trap handler).

Bruce