From owner-freebsd-bugs@FreeBSD.ORG Wed Sep 17 16:20:02 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D420B1065678; Wed, 17 Sep 2008 16:20:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id AFA318FC1E; Wed, 17 Sep 2008 16:20:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m8HGK2ea093584; Wed, 17 Sep 2008 16:20:02 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m8HGK2lj093583; Wed, 17 Sep 2008 16:20:02 GMT (envelope-from gnats) Resent-Date: Wed, 17 Sep 2008 16:20:02 GMT Resent-Message-Id: <200809171620.m8HGK2lj093583@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@freebsd.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Cc: ed@freebsd.org, emax@freebsd.org Resent-Reply-To: FreeBSD-gnats-submit@freebsd.org, Eygene Ryabinkin Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9E352106569D for ; Wed, 17 Sep 2008 16:16:36 +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 51AA98FC12 for ; Wed, 17 Sep 2008 16:16:36 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from shadow.codelabs.ru (shadow.codelabs.ru [144.206.177.8]) by 0.mx.codelabs.ru with esmtps (TLSv1:CAMELLIA256-SHA:256) id 1Kfzhi-000Dp5-Be for FreeBSD-gnats-submit@freebsd.org; Wed, 17 Sep 2008 20:16:34 +0400 Received: by shadow.localdomain (Postfix, from userid 1001) id 9E2F717101; Wed, 17 Sep 2008 20:16:33 +0400 (MSD) Message-Id: <20080917161633.9E2F717101@shadow.codelabs.ru> Date: Wed, 17 Sep 2008 20:16:33 +0400 (MSD) From: Eygene Ryabinkin To: FreeBSD-gnats-submit@freebsd.org X-Send-Pr-Version: 3.113 X-GNATS-Notify: ed@freebsd.org, emax@freebsd.org Cc: Subject: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Eygene Ryabinkin List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Sep 2008 16:20:02 -0000 >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: