Date: Wed, 17 Sep 2008 21:42:00 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Cc: rik@freebsd.org, hackers@freebsd.org Subject: Re: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c Message-ID: <gaH7cu1zt0Bw5e8Q2XfgrlKSRyY@iXA9ZWPrtc2I2BMzBXoToMd7YdQ> In-Reply-To: <bb4a86c70809170956x36234893he8894a49127b6b6e@mail.gmail.com> References: <20080917161633.9E2F717101@shadow.codelabs.ru> <bb4a86c70809170956x36234893he8894a49127b6b6e@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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 <rea-fbsd@codelabs.ru> 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 = 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 ---
--
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (FreeBSD)
iEYEARECAAYFAkjRQWgACgkQthUKNsbL7Yjv4QCfeZCRAyyrp4ax4CU3mtP8oX24
lxgAn2WLb9S3ZFQUJK5qiaeUUYAa4yPG
=zuGF
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?gaH7cu1zt0Bw5e8Q2XfgrlKSRyY>