Date: Sat, 2 Oct 2010 21:28:52 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Don Lewis <truckman@freebsd.org> Cc: arch@freebsd.org Subject: Re: "process slock" vs. "scrlock" lock order Message-ID: <20101002190453.K11563@besplex.bde.org> In-Reply-To: <201010020741.o927f7FJ056708@gw.catspoiler.org> References: <201010020741.o927f7FJ056708@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101002190453.K11563>