Date: Wed, 22 Oct 2003 12:47:47 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Luigi Rizzo <rizzo@icir.org> Cc: hackers@freebsd.org Subject: Re: suspect spl*() code in syscons.c Message-ID: <20031022121648.A20430@gamplex.bde.org> In-Reply-To: <20031021154738.A9175@xorpc.icir.org> References: <20031021154738.A9175@xorpc.icir.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 21 Oct 2003, Luigi Rizzo wrote:
> both -current and -stable have the following snippet of code in
> sys/dev/syscons/syscons.c:scclose():
>
> {
> ...
> int s;
>
> if (SC_VTY(dev) != SC_CONSOLECTL) {
> ...
> s = spltty();
> ...
> }
> spltty();
> (*linesw[tp->t_line].l_close)(tp, flag);
> ttyclose(tp);
> spl0();
> return(0);
> }
>
> Note that the omitted code never does any spl*() call, nor it
> uses the saved value anymore. Also, i am a bit suspicious about the
> spltty()/spl0() sequence.
>
> Can someone explain if this code is correct ?
> (I have Bcc-ed the committers involved in writing this code,
> maybe they know the answer).
The initial ipl state is "known" to be that given by spl0(), so there is
no need to save the initial state to restore it using splx(); spl0()
sufficies.
However, when the omitted code neglects to restore the state, the initial
state is not that given by spl0() when the omitted code is executed.
Then using spl0() instead of splx() apparently-accidentally fixes the bug.
The scope of the spltty() seems to be a bit large but I don't know what
it should be exactly.
Ideally, the whole device close should be atomic (and as a prerequisite,
non-interruptible), but that is not possible for all console drivers
although it may be possible for syscons. I have noticed the following
incompletely handled races in the corresponding code for the sio driver:
1. Last-close may block, mainly in l_close. Quoting the above code again:
spltty();
(*linesw[tp->t_line].l_close)(tp, flag);
ttyclose(tp);
spl0();
So despite holding spltty() here, we may block (mainly waiting for output
to drain). Perhaps we have to wait for output to drain even for
synchcronous consoles like syscons, since their output may be blocked by
something like scroll lock for syscons (I don't know where this block
actually occurs). While we are blocked, we have to drop the ipl so we
may be affected by signals. But generally signals aren't a problem here.
More the reverse -- you can have a process waiting forever in exit() for
output to drain and want to kill it but can't because the process has
committed to exiting and doesn't accept any more signals.
2. The vfs permits (non-first) opens to succeed while last-close is blocked.
For sio, this is useful behaviour. You can use it to issue ioctls to
unblock last-closes that would otherwise be blocked until the next reboot.
However, the driver doesn't really understand this so it may make a mess
of its state when the last-close completes despite the device ending up
open at the file level.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20031022121648.A20430>