From owner-freebsd-bugs@FreeBSD.ORG Wed Oct 28 03:10:02 2009 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 358EA1065670 for ; Wed, 28 Oct 2009 03:10: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 E79A28FC0A for ; Wed, 28 Oct 2009 03:10:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id n9S3A10q062710 for ; Wed, 28 Oct 2009 03:10:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id n9S3A1G3062709; Wed, 28 Oct 2009 03:10:01 GMT (envelope-from gnats) Resent-Date: Wed, 28 Oct 2009 03:10:01 GMT Resent-Message-Id: <200910280310.n9S3A1G3062709@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Ben Kaduk Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id AE2101065696 for ; Wed, 28 Oct 2009 03:04:09 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (www.freebsd.org [IPv6:2001:4f8:fff6::21]) by mx1.freebsd.org (Postfix) with ESMTP id 9B8748FC16 for ; Wed, 28 Oct 2009 03:04:09 +0000 (UTC) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.14.3/8.14.3) with ESMTP id n9S3493C000921 for ; Wed, 28 Oct 2009 03:04:09 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.14.3/8.14.3/Submit) id n9S348OM000919; Wed, 28 Oct 2009 03:04:08 GMT (envelope-from nobody) Message-Id: <200910280304.n9S348OM000919@www.freebsd.org> Date: Wed, 28 Oct 2009 03:04:08 GMT From: Ben Kaduk To: freebsd-gnats-submit@FreeBSD.org X-Send-Pr-Version: www-3.1 Cc: Subject: kern/140036: lock order reversal with iwn0_com_lock and iwn0 softc lock X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Oct 2009 03:10:02 -0000 >Number: 140036 >Category: kern >Synopsis: lock order reversal with iwn0_com_lock and iwn0 softc lock >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Oct 28 03:10:01 UTC 2009 >Closed-Date: >Last-Modified: >Originator: Ben Kaduk >Release: 9-curent >Organization: MIT SIPB >Environment: FreeBSD hysteresis.mit.edu 9.0-CURRENT FreeBSD 9.0-CURRENT #1: Mon Oct 26 00:21: 52 EDT 2009 kaduk@hysteresis.mit.edu:/usr/obj/usr/src/sys/GENERIC amd64 >Description: With a recent -current (with the new iwn driver that has support for 5000-series cards, many thanks!), I get a lock order reversal on boot: 1st 0xffffff800033d018 iwn0_com_lock (iwn0_com_lock) @ /usr/src/sys/net80211/ie ee80211_proto.c:1082 2nd 0xffffff8000309010 iwn0 (network driver) @ /usr/src/sys/modules/iwn/../../d ev/iwn/if_iwn.c:3358 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2a _witness_debugger() at _witness_debugger+0x2e witness_checkorder() at witness_checkorder+0x81e _mtx_lock_flags() at _mtx_lock_flags+0x78 iwn_wme_update() at iwn_wme_update+0xa0 ieee80211_wme_updateparams_locked() at ieee80211_wme_updateparams_locked+0xef ieee80211_wme_updateparams() at ieee80211_wme_updateparams+0x52 ieee80211_wme_initparams() at ieee80211_wme_initparams+0x1d9 ieee80211_sta_join1() at ieee80211_sta_join1+0xb0 sta_pick_bss() at sta_pick_bss+0xb2 scan_task() at scan_task+0x594 taskqueue_run() at taskqueue_run+0x91 taskqueue_thread_loop() at taskqueue_thread_loop+0x3f fork_exit() at fork_exit+0x12a fork_trampoline() at fork_trampoline+0xe --- trap 0, rip = 0, rsp = 0xffffff803a983d30, rbp = 0 --- It looks like this is the same one as reported in http://lists.freebsd.org/piper mail/freebsd-current/2009-May/007528.html but hasn't made its way onto http://sources.zabbadoz.net/freebsd/lor.html >How-To-Repeat: Boot a system with iwn hardware and the driver enabled >Fix: The first lock is the ieee80211_com lock, and the second is the iwn softc lock. I note that the softc lock is obtained in iwn_wme_update, which is in the call t race after ieee80211_wme_updateparams, which obtains the ic lock. In particular , a quick check seems to indicate that iwn_wme_update is only ever called from i eee80211_wme_updateparams_locked, so it seems that the ic lock should provide se rialization, provided that it is not dropped elsewhere in ieee80211_wme_updatepa rams_locked. It seems that the only function (other than wme->wme_update) calle d from ieee80211_wme_updateparams_locked is ieee80211_beacon_notify(), which in turn calls vap->iv_update_beacon(). However, iwn does not modify vap->iv_update_beacon from the default null_update_ beacon(), which is an empty function. Thus, it would seem that the ic lock is a ctually sufficient protection in this case. However, locking is not exactly my area of expertise, so I'm not entirely sure w hether it is better to rely on the ic lock for serialization, or to drop it and acquire the softc lock for the call to iwn_cmd() in iwn_wme_update(). I've been running my system with the former approach for about an hour, and it seems okay with a few ssh sessions and firefox open. With that version of th e code, I no longer see this LOR, though I do see a different one in iwn that wa sn't there before: 1st 0xffffff800033d018 iwn0_com_lock (iwn0_com_lock) @ /usr/src/sys/modules/iwn /../../dev/iwn/if_iwn.c:1735 2nd 0xffffff8000309010 iwn0 (network driver) @ /usr/src/sys/modules/iwn/../../d ev/iwn/if_iwn.c:3003 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2a _witness_debugger() at _witness_debugger+0x2e witness_checkorder() at witness_checkorder+0x81e _mtx_lock_flags() at _mtx_lock_flags+0x78 iwn_raw_xmit() at iwn_raw_xmit+0x88 ieee80211_send_mgmt() at ieee80211_send_mgmt+0x4d5 sta_newstate() at sta_newstate+0x43a ieee80211_newstate_cb() at ieee80211_newstate_cb+0xac taskqueue_run() at taskqueue_run+0x91 taskqueue_thread_loop() at taskqueue_thread_loop+0x3f fork_exit() at fork_exit+0x12a fork_trampoline() at fork_trampoline+0xe --- trap 0, rip = 0, rsp = 0xffffff803a983d30, rbp = 0 --- This, however, can be eliminated by dropping the ic lock around the portion of i wn_raw_xmit() that grabs the softc lock. I'm now running with that version, and it is LOR-free and doesn't seem to have had any problems during the ten minutes I've been running it. For what it's worth, the latter approach (drop the ic lock to pick up the softc lock) does not seem to eliminate the original LOR, so it seems that the former a pproach actually is better. As such, I believe that the attached patch is correct. ( http://stuff.mit.edu/afs/sipb.mit.edu/user/kaduk/freebsd/patches/if_iwn.c.diff.2009.10.27 ) Patch attached with submission follows: --- if_iwn.c.orig 2009-10-27 21:11:54.000000000 -0400 +++ if_iwn.c 2009-10-27 22:59:29.000000000 -0400 @@ -3000,6 +3000,7 @@ return ENETDOWN; } + IEEE80211_UNLOCK(ic); IWN_LOCK(sc); if (params == NULL) txq = &sc->txq[M_WME_GETAC(m)]; @@ -3025,6 +3026,7 @@ ifp->if_oerrors++; } IWN_UNLOCK(sc); + IEEE80211_LOCK(ic); return error; } @@ -3355,9 +3357,7 @@ cmd.ac[i].txoplimit = htole16(IWN_TXOP_TO_US(wmep->wmep_txopLimit)); } - IWN_LOCK(sc); (void) iwn_cmd(sc, IWN_CMD_EDCA_PARAMS, &cmd, sizeof cmd, 1 /*async*/); - IWN_UNLOCK(sc); return 0; #undef IWN_TXOP_TO_US #undef IWN_EXP2 >Release-Note: >Audit-Trail: >Unformatted: