From owner-freebsd-current@FreeBSD.ORG Tue Sep 23 08:42:46 2008 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 31DAE1065673; Tue, 23 Sep 2008 08:42:46 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 962CD8FC17; Tue, 23 Sep 2008 08:42:45 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Sender; b=P3LTUo9H242nTXu8ivcyT3VXf8SXNvjdN4HBx3Wl0WuUCsWPurW1qjZYoEzpcpqcMh2vzRRasA8X+vVHANRH96U2GvtNMlur1LicizRQRL6Jav+J/upwB6HNFxkkFWHjoidPJJZxmbXi8WXw8D1Kmkk5uU9mKIcGX5oDMgspU64=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.177.25]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1Ki3Fy-0000JO-40; Tue, 23 Sep 2008 12:28:26 +0400 Date: Tue, 23 Sep 2008 12:28:24 +0400 From: Eygene Ryabinkin To: Maksim Yevmenkin Message-ID: References: <20080917161633.9E2F717101@shadow.codelabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Nx8xdmI2KD3LNVVP" Content-Disposition: inline In-Reply-To: Sender: rea-fbsd@codelabs.ru Cc: rik@freebsd.org, ed@freebsd.org, current@freebsd.org, bug-followup@freebsd.org Subject: Re: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Sep 2008 08:42:46 -0000 --Nx8xdmI2KD3LNVVP Content-Type: multipart/mixed; boundary="Vy6UCbb9EK60RK4A" Content-Disposition: inline --Vy6UCbb9EK60RK4A Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Maksim, good day. Fri, Sep 19, 2008 at 09:20:41AM -0700, Maksim Yevmenkin wrote: > > Tried my initial patch on some 7.0-PRERELEASE -- it locks keyboard when > > geli asks for the password. Had not much time to dig it out, will try > > to do it as soon as I can. Substituting KBDMUX_LOCK/UNLOCK with Giant > > locking helps even on this FreeBSD version. > > > > More testing needed, may be there are some other issues that aren't > > revealing themselves... >=20 > did you have a chance to do some testing? i tried substituting > KBDMUX_LOCK/UNLOCK with Giant locking here locally and played with a > couple of keyboards under X and console. no apparent issues or witness > complains. Sorry for being a bit slow, but I have good news: another race was found and patched. Now it is in the syscons code: high-level procedures are racing with sckbdevent(): on my (magical ;))) notebook scgetc() from syscons.c got the right key, but on return to sccngetch() the code was substituted with 0x100 (NOKEY) and scancode was effectively throwed out. So what I had seen was not the keyboard lock, but just the visual effect of it. Since syscons.c has some splx()/spltty() calls still hanging around and comments are saying that these are to protect from the sckbdevent() and scrn_timer(). I had wrapped these procedures with Giant operations. There is one suspicious function, scstart(): I had not touched it, but may be it should also be protected with some kind of lock. The attached patch does this. I did some limited testing for it: still continuing to do it on all available systems. > would it be ok for me to commit this? >=20 > --- kbdmux.c.orig 2008-07-29 14:21:20.000000000 -0700 > +++ kbdmux.c 2008-09-19 09:02:54.000000000 -0700 > @@ -104,9 +104,11 @@ >=20 > #define KBDMUX_LOCK_DESTROY(s) >=20 > -#define KBDMUX_LOCK(s) > +#define KBDMUX_LOCK(s) \ > + mtx_lock(&Giant) >=20 > -#define KBDMUX_UNLOCK(s) > +#define KBDMUX_UNLOCK(s) \ > + mtx_unlock(&Giant) >=20 > #define KBDMUX_LOCK_ASSERT(s, w) Yes, I think it will be fine -- I have no issues with this patch. Although now I am testing the new patch together with my old one. Will try to roll this patch on some systems too. Thanks! --=20 Eygene _ ___ _.--. # \`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard / ' ` , __.--' # to read the on-line manual =20 )/' _/ \ `-_, / # while single-stepping the kernel. `-'" `"\_ ,_.-;_.-\_ ', fsc/as # _.-'_./ {_.' ; / # -- FreeBSD Developers handbook=20 {_.-``-' {_/ # --Vy6UCbb9EK60RK4A Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="syscons-read-race.patch" Content-Transfer-Encoding: quoted-printable Avoids races of high-level syscons code with the sckbdevent handler. --- sys/dev/syscons/syscons.c.orig 2008-09-23 11:46:45.000000000 +0400 +++ sys/dev/syscons/syscons.c 2008-09-23 12:16:32.000000000 +0400 @@ -1569,7 +1569,9 @@ scp->ts =3D save; =20 s =3D spltty(); /* block sckbdevent and scrn_timer */ + mtx_lock(&Giant); sccnupdate(scp); + mtx_unlock(&Giant); splx(s); } =20 @@ -1590,6 +1592,7 @@ int s =3D spltty(); /* block sckbdevent and scrn_timer while we poll */ int c; =20 + mtx_lock(&Giant); /* assert(sc_console !=3D NULL) */ =20 /*=20 @@ -1601,11 +1604,13 @@ sccnupdate(scp); =20 if (fkeycp < fkey.len) { + mtx_unlock(&Giant); splx(s); return fkey.str[fkeycp++]; } =20 if (scp->sc->kbd =3D=3D NULL) { + mtx_unlock(&Giant); splx(s); return -1; } @@ -1628,6 +1633,7 @@ scp->kbd_mode =3D cur_mode; kbd_ioctl(scp->sc->kbd, KDSKBMODE, (caddr_t)&scp->kbd_mode); kbd_disable(scp->sc->kbd); + mtx_unlock(&Giant); splx(s); =20 switch (KEYFLAGS(c)) { --Vy6UCbb9EK60RK4A-- --Nx8xdmI2KD3LNVVP Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (FreeBSD) iEYEARECAAYFAkjYqKgACgkQthUKNsbL7Yh1KQCeJ6p2maQenUEwO8SQs4rpA1Y0 bjkAn2NmJVhuu1lcSMiOaf9tydIJxRv/ =/vb6 -----END PGP SIGNATURE----- --Nx8xdmI2KD3LNVVP--