Date: Tue, 23 Sep 2008 12:28:24 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> 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 Message-ID: <byCZZ2L%2BP/vIU5hBTMuURReOshU@p9DJkXKp%2Br2jequ3/WCPe%2BJakGc> In-Reply-To: <bb4a86c70809190920l29f40e3cja19f6795e19b02b@mail.gmail.com> References: <20080917161633.9E2F717101@shadow.codelabs.ru> <bb4a86c70809170956x36234893he8894a49127b6b6e@mail.gmail.com> <gaH7cu1zt0Bw5e8Q2XfgrlKSRyY@iXA9ZWPrtc2I2BMzBXoToMd7YdQ> <bb4a86c70809171052t5e961675n139b2848e6addd0@mail.gmail.com> <OBTLESaczqaewZBU9qdf0FWZRHE@7/OI9n2WwGs0JPogavdPqCpU9P8> <OukumQcOIGlPdbI1QmdweY8cFsk@IW4pzQfcswHj%2B/3imThIGjvxQdw> <bb4a86c70809190920l29f40e3cja19f6795e19b02b@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?byCZZ2L%2BP/vIU5hBTMuURReOshU>