kern/140036: lock order reversal with iwn0_com_lock and iwn0 softc lock

Wed Oct 28 03:10:02 UTC 2009

>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 at 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:
