From owner-freebsd-hackers@FreeBSD.ORG Wed Sep 17 17:53:23 2008 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CD32B1065678 for ; Wed, 17 Sep 2008 17:53:23 +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 5DFBB8FC25 for ; Wed, 17 Sep 2008 17:53:23 +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=jv5Ngv61QTpn4A+fFp+XQUP48Aw5JXYxU9Y1ouaYZTaGx/Ax4kt6QbEd+rbD+6ybBxYCW8SDwVEdPexwF4lXgD1lavTJc2YzfYp7cKYQ/YjyelTsDw5NWvH7sEMYp3yYpOGBybfR4lAKZ4EEudHLOeFDFwrl5cWFf06DWCaiXi8=; Received: from phoenix.codelabs.ru (ppp91-78-117-241.pppoe.mtu-net.ru [91.78.117.241]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1Kg12P-000K1S-V5; Wed, 17 Sep 2008 21:42:02 +0400 Date: Wed, 17 Sep 2008 21:42:00 +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="MGYHOYXEY6WxJCY8" Content-Disposition: inline In-Reply-To: Sender: rea-fbsd@codelabs.ru X-Mailman-Approved-At: Wed, 17 Sep 2008 19:57:19 +0000 Cc: rik@freebsd.org, hackers@freebsd.org Subject: Re: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Sep 2008 17:53:23 -0000 --MGYHOYXEY6WxJCY8 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Maxim, good day. Cc'ing this discuission to hackers@ -- I was just going to write the separate letter on this topic to the list. Wed, Sep 17, 2008 at 09:56:14AM -0700, Maksim Yevmenkin wrote: > have you tried to simply set KBDMUX_LOCK/UNLOCK() to > mtx_lock/unlock(&Giant) ? Since kbdmux callout is initialized as non-MPSAFE, this will result in double locking the Giant (at least I see it from the code). I am not sure that this is very good -- had not yet verified that Giant is recursive. Can try it tomorrow. Since you had written the code and #if 0'ed the locking part, may I ask, why? Are there any known issues or it was just not very good to introduce locking at that time (rev. 1.1, 3 years ago)? Thanks! > On Wed, Sep 17, 2008 at 9:16 AM, Eygene Ryabinkin = wrote: > >>Number: 127446 > >>Category: kern > >>Synopsis: [patch] fix race in sys/dev/kbdmux/kbdmux.c > >>Confidential: no > >>Severity: critical > >>Priority: high > >>Responsible: freebsd-bugs > >>State: open > >>Quarter: > >>Keywords: > >>Date-Required: > >>Class: sw-bug > >>Submitter-Id: current-users > >>Arrival-Date: Wed Sep 17 16:20:02 UTC 2008 > >>Closed-Date: > >>Last-Modified: > >>Originator: Eygene Ryabinkin > >>Release: FreeBSD 7.1-PRERELEASE amd64 > >>Organization: > > Code Labs > >>Environment: > > > > System: FreeBSD XXX 7.1-PRERELEASE FreeBSD 7.1-PRERELEASE #55: Wed Sep = 17 19:57:25 MSD 2008 root@XXX:/usr/src/sys/amd64/compile/XXX amd64 > > > > CVSupped system yesterday late at the evening (aroung 17:00 UTC). > > > >>Description: > > > > Since kbdmux(4) is not MPSAFE now, its interrupt routines are running > > under Giant. But there is kbdmux_read_char() routine that runs unlocked > > and can race with the interrupt handler. When there is no input data > > in the keyboard queue and kbdmux(4) is in the POLLING mode, routine will > > try to poll each mux member for the scancode. And if user presses the > > key at that time, KBDMUX_READ_CHAR() can race with the interrupt handler > > kbdmux_kbd_event() since we don't lock polling loop. > > > >>How-To-Repeat: > > > > I see this on my laptop: sometimes during boot, when system asks me for > > the password of geli(8)-encrypted volume, it doubles the symbols or even > > panics. I don't see this on the other systems, so perhaps my laptop is > > just so lucky ;)) > > > > But one can try to enable echoing of the input symbols during the geli's > > bootup password dialog (setting g_eli_visible_passphrase to 1 > > unconditionally) and maybe symbols will be doubled. I see this issue > > only during boot-up phase, but I feel that this is due to the fact that > > for the rest of the system's operation only interrupt handler is > > working, at least I see it from the debug printf()s. > > > >>Fix: > > > > The following patch fixes the things for me. It just acquires Giant for > > the time of polling. I did a limited testing at my systems and there > > were no signs of regressions yet. > > > > Seems like in the properly locked situation (with non-dummy KBDMUX_LOCK > > and KBDMUX_UNLOCK) this issue will vanish, so I had conditionalized > > Giant grabbing. > > > > --- kbdmux-read-race.patch begins here --- > > --- sys/dev/kbdmux/kbdmux.c.orig 2008-09-17 10:41:00.000000000 += 0400 > > +++ sys/dev/kbdmux/kbdmux.c 2008-09-17 19:52:00.000000000 +0400 > > @@ -79,6 +79,10 @@ > > */ > > > > #if 0 /* not yet */ > > +#define KBDMUX_LOCK_POLLER(dummy) > > + > > +#define KBDMUX_UNLOCK_POLLER(dummy) > > + > > #define KBDMUX_LOCK_DECL_GLOBAL \ > > struct mtx ks_lock > > #define KBDMUX_LOCK_INIT(s) \ > > @@ -98,6 +102,10 @@ > > #define KBDMUX_QUEUE_INTR(s) \ > > taskqueue_enqueue(taskqueue_swi_giant, &(s)->ks_task) > > #else > > +#define KBDMUX_LOCK_POLLER(dummy) \ > > + mtx_lock(&Giant) > > +#define KBDMUX_UNLOCK_POLLER(dummy) \ > > + mtx_unlock(&Giant) > > #define KBDMUX_LOCK_DECL_GLOBAL > > > > #define KBDMUX_LOCK_INIT(s) > > @@ -661,6 +669,14 @@ > > if (state->ks_flags & POLLING) { > > kbdmux_kbd_t *k; > > > > + /* > > + * Grab Giant: we don't want to race with > > + * the keyboard interrupt handler. And this > > + * can happen, because when a key will be > > + * pressed, our READ_CHAR will be competing > > + * with the kbdmux_kbd_event()'s one. > > + */ > > + KBDMUX_LOCK_POLLER(); > > SLIST_FOREACH(k, &state->ks_kbds, next) { > > while (KBDMUX_CHECK_CHAR(k->kbd)) { > > scancode =3D KBDMUX_READ_CHAR(k-= >kbd, 0); > > @@ -674,6 +690,7 @@ > > putc(scancode, &state->ks_inq); > > } > > } > > + KBDMUX_UNLOCK_POLLER(); > > > > if (state->ks_inq.c_cc > 0) > > goto next_code; > > --- kbdmux-read-race.patch ends here --- --=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 {_.-``-' {_/ # --MGYHOYXEY6WxJCY8 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (FreeBSD) iEYEARECAAYFAkjRQWgACgkQthUKNsbL7Yjv4QCfeZCRAyyrp4ax4CU3mtP8oX24 lxgAn2WLb9S3ZFQUJK5qiaeUUYAa4yPG =zuGF -----END PGP SIGNATURE----- --MGYHOYXEY6WxJCY8--