From owner-freebsd-hackers@FreeBSD.ORG Tue Oct 21 19:49:32 2003 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E4AD616A4B3 for ; Tue, 21 Oct 2003 19:49:32 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3F3A643FD7 for ; Tue, 21 Oct 2003 19:49:29 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id MAA11982; Wed, 22 Oct 2003 12:49:09 +1000 Date: Wed, 22 Oct 2003 12:47:47 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Luigi Rizzo In-Reply-To: <20031021154738.A9175@xorpc.icir.org> Message-ID: <20031022121648.A20430@gamplex.bde.org> References: <20031021154738.A9175@xorpc.icir.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: hackers@freebsd.org Subject: Re: suspect spl*() code in syscons.c X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Oct 2003 02:49:33 -0000 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