Date: Wed, 17 Sep 2008 20:16:33 +0400 (MSD) From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: FreeBSD-gnats-submit@freebsd.org Subject: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c Message-ID: <20080917161633.9E2F717101@shadow.codelabs.ru> Resent-Message-ID: <200809171620.m8HGK2lj093583@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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 ---
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080917161633.9E2F717101>