From owner-freebsd-usb@FreeBSD.ORG Mon Jul 19 04:21:27 2010 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3B6311065679 for ; Mon, 19 Jul 2010 04:21:27 +0000 (UTC) (envelope-from moonlightakkiy@yahoo.ca) Received: from web51808.mail.re2.yahoo.com (web51808.mail.re2.yahoo.com [206.190.38.239]) by mx1.freebsd.org (Postfix) with SMTP id E04278FC14 for ; Mon, 19 Jul 2010 04:21:26 +0000 (UTC) Received: (qmail 51271 invoked by uid 60001); 19 Jul 2010 04:21:26 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.ca; s=s1024; t=1279513285; bh=6j9k4UHlI9S+hxqsY2J3t8MWo+nmyzK4jTgu/RDgmTI=; h=Message-ID:X-YMail-OSG:Received:X-Mailer:References:Date:From:Subject:To:Cc:In-Reply-To:MIME-Version:Content-Type; b=jYptPIFJWFGhuZg4Px/avyjBTXkCdKUb8+xL2c+siKyBmu4AN+vcTogYfhOIrvKhyE5GDDg3hXT1FMoRYhT9lKqeKnKC+vJs6+VABUn9Zw/MIiR5he6EkcIcObjlb5J/bXyiEyX0GtOwu1UjV9kD46jIH0ePqZEY9m5jZwr/1Kw= DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=yahoo.ca; h=Message-ID:X-YMail-OSG:Received:X-Mailer:References:Date:From:Subject:To:Cc:In-Reply-To:MIME-Version:Content-Type; b=n5MJ/ipkFXo+vnOXjUeoVFePC4hZahnPRUdAYJoEsUei5t0NqWLV6DKk0QEt6Tq3N1ur++zIZnd8F1ypnnh1yIXCPrmcgg1EvxIfN/FteI09bluhH9uUXVEhSSfn/x2SD7g6Co1Jtxx0ht0hP6zx5qTzS2dVC0APhbUVRohlhKs=; Message-ID: <964771.49833.qm@web51808.mail.re2.yahoo.com> X-YMail-OSG: TeLcbeoVM1lSLqYZqrgciQYdh6MzK8ZiIkhDj62W5siPmf1 ye6W8CGWHSm5fCAAJPAPuQMgTHtkJXffEHWfC1DHCcp2xjI5VMH2M_Jx.g7R WGTQ6ZFLZ.hNzE7mNevi8p3vNEG5KUeDu4RiNOxSl.sDGInlFU_ARywfnE0s _3ZcoWpIADWFFybYaLD3JVa9zLAJFt4PIoZwyqivyayvpPD4MZxf16PqZoz5 Xrfevz3AMavWI5HsSdOe4xdWsuQOCCOl9eHrIZbLlDjV7MYELOYt52nUwsmy 3Og6oqe4TJmAVgQsoSsFSG5CmY8KeeWZcTUZz5diiqVBh9reijQiEHiaDP2b AGgz2w8w- Received: from [173.183.132.20] by web51808.mail.re2.yahoo.com via HTTP; Sun, 18 Jul 2010 21:21:25 PDT X-Mailer: YahooMailRC/420.4 YahooMailWebService/0.8.104.276605 References: <201007141511.46190.hselasky@c2i.net> Date: Sun, 18 Jul 2010 21:21:25 -0700 (PDT) From: PseudoCylon To: Hans Petter Selasky In-Reply-To: <201007141511.46190.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sam Leffler , freebsd-current@freebsd.org, freebsd-usb@freebsd.org Subject: Re: [panic] Race in IEEE802.11 layer towards device drivers X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jul 2010 04:21:27 -0000 [NB] Obviously, I didn't click "reply ALL" last time, so here are missing part > > > > -if(vap->iv_opmode == IEEE80211_M_HOSTAP){ > > > > > > > > -RUN_LOCK(sc); > > > > +if (vap->iv_opmode == IEEE80211_M_HOSTAP) > > > > > > > > sc->cmdq_key_set = RUN_CMDQ_GO; > > > > > > > > -RUN_UNLOCK(sc); > > > > -} > > > > > > > > > > > > Why are you removing these locks? > > > > It is simple assignment, it must be atomic. > > Not necessarily. If you don't put lock statements around it or use the > volatile keyword, the compiler can re-organize the execution order. I will > have a look at it. > > > > > > Another question: > > > i = RUN_CMDQ_GET(&sc->cmdq_store); > > > DPRINTF("cmdq_store=%d\n", i); > > > > > > sc->cmdq[i].func = run_update_beacon_cb; > > > sc->cmdq[i].arg0 = vap; > > > > > > Why is this code and similar places not enclosed with mutexes? > > > > First, I couldn't use a lock in key_delete() because of LoR. So, I use > > atomic instead. RUN_CMDQ_GET is atomic_fetch_add(). Whatever executes that > > code gets unique place (cmdq[i]) to write, so there shouldn't be any race. > > > > Then out of order execution happened. Specially, when key_set() overtakes > > key_delete(), encryption fails. So, all deferred processes are called back > > via run_cmdq_cb() to maintain the order. Because cmdq functions are first > > written for key_delete() where lock causes LoR. So, lock isn't needed. > > > > run_cmdq_cb() uses lock. But it is for calling callback functions locked. > > So that, functions just call another function locked, like > > ##_callback() > > { > > LOCK(); > > ##_locked(); > > UNLOCK(); > > } > > won't be needed. > > If the run_cmdq_cb() is running at the same time which you are queuing > elements, then I note that you set .func before .arg0. The ##_callback() code > only checks if .func has been set. Actually the "i" increment should be after > that you filled out the data, and then you see that you cannot use atomic. I > think the most simple solution is to add another mutex, sc->sc_cmdq_mtx, which > protects the queue and it's associated data. Also, what do you do if the queue > wraps around? You should have a mechanism to prevent that, because you then > might start executing commands in random order? > Here is a patch (patch against P4 if_run.c rev 14 and if_runvar.h rev 8) -- begin patch -- diff --git a/dev/usb/wlan/if_run.c b/dev/usb/wlan/if_run.c index 8c96534..c988ad4 100644 --- a/dev/usb/wlan/if_run.c +++ b/dev/usb/wlan/if_run.c @@ -90,12 +90,6 @@ SYSCTL_INT(_hw_usb_run, OID_AUTO, debug, CTLFLAG_RW, &run_debug, 0, #define IEEE80211_HAS_ADDR4(wh) \ (((wh)->i_fc[1] & IEEE80211_FC1_DIR_MASK) == IEEE80211_FC1_DIR_DSTODS) -/* - * Because of LOR in run_key_delete(), use atomic instead. - * '& RUN_CMDQ_MASQ' is to loop cmdq[]. - */ -#define RUN_CMDQ_GET(c)(atomic_fetchadd_32((c), 1) & RUN_CMDQ_MASQ) - static const struct usb_device_id run_devs[] = { { USB_VP(USB_VENDOR_ABOCOM,USB_PRODUCT_ABOCOM_RT2770) }, { USB_VP(USB_VENDOR_ABOCOM,USB_PRODUCT_ABOCOM_RT2870) }, @@ -554,6 +548,8 @@ run_attach(device_t self) mtx_init(&sc->sc_mtx, device_get_nameunit(sc->sc_dev), MTX_NETWORK_LOCK, MTX_DEF); +mtx_init(&sc->sc_cmdq_mtx, device_get_nameunit(sc->sc_dev), + MTX_NETWORK_LOCK, MTX_DEF); iface_index = RT2860_IFACE_INDEX; @@ -737,6 +733,7 @@ run_detach(device_t self) } mtx_destroy(&sc->sc_mtx); +mtx_destroy(&sc->sc_cmdq_mtx); return (0); } @@ -830,9 +827,6 @@ run_vap_create(struct ieee80211com *ic, if(sc->rvp_cnt++ == 0) ic->ic_opmode = opmode; -if(opmode == IEEE80211_M_HOSTAP) -sc->cmdq_run = RUN_CMDQ_GO; - DPRINTF("rvp_id=%d bmap=%x rvp_cnt=%d\n", rvp->rvp_id, sc->rvp_bmap, sc->rvp_cnt); @@ -889,27 +883,31 @@ run_cmdq_cb(void *arg, int pending) struct run_softc *sc = arg; uint8_t i; -/* call cmdq[].func locked */ -RUN_LOCK(sc); -for(i = sc->cmdq_exec; sc->cmdq[i].func && pending; - i = sc->cmdq_exec, pending--){ +RUN_CMDQ_LOCK(sc); +for (i = sc->cmdq_exec; sc->cmdq[i].func; i = sc->cmdq_exec) { DPRINTFN(6, "cmdq_exec=%d pending=%d\n", i, pending); -if(sc->cmdq_run == RUN_CMDQ_GO){ +if (sc->cmdq_run == RUN_CMDQ_GO || + (sc->cmdq_key_set == RUN_CMDQ_GO && + sc->cmdq[i].func == run_key_set_cb)) { +RUN_CMDQ_UNLOCK(sc); +RUN_LOCK(sc); /* * If arg0 is NULL, callback func needs more * than one arg. So, pass ptr to cmdq struct. */ -if(sc->cmdq[i].arg0) +if (sc->cmdq[i].arg0) sc->cmdq[i].func(sc->cmdq[i].arg0); else sc->cmdq[i].func(&sc->cmdq[i]); +RUN_UNLOCK(sc); +RUN_CMDQ_LOCK(sc); } sc->cmdq[i].arg0 = NULL; sc->cmdq[i].func = NULL; sc->cmdq_exec++; sc->cmdq_exec &= RUN_CMDQ_MASQ; } -RUN_UNLOCK(sc); +RUN_CMDQ_UNLOCK(sc); } static void @@ -1771,6 +1769,19 @@ run_newstate(struct ieee80211vap *vap, enum ieee80211_state nstate, int arg) case IEEE80211_S_INIT: restart_ratectl = 1; +/* + * When hostapd has set a key, don't clear it. + * But, when the device is being brought down, clear it. + */ +if (sc->cmdq_key_set != RUN_CMDQ_GO || + ostate == IEEE80211_S_RUN) { +/* clear shared key table */ +run_set_region_4(sc, + RT2860_SKEY(rvp->rvp_id, 0), 0, 4 * 32); +/* clear shared key mode */ +run_set_region_4(sc, RT2860_SKEY_MODE_0_7, 0, 4); +} + if (ostate != IEEE80211_S_RUN) break; @@ -1922,12 +1933,24 @@ run_wme_update(struct ieee80211com *ic) struct run_softc *sc = ic->ic_ifp->if_softc; /* sometime called wothout lock */ -if(mtx_owned(&ic->ic_comlock.mtx)){ -uint32_t i = RUN_CMDQ_GET(&sc->cmdq_store); +if (mtx_owned(&ic->ic_comlock.mtx)) { +RUN_CMDQ_LOCK(sc); +uint8_t i = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", i); + +if (sc->cmdq[i].func != NULL) { +DPRINTF("cmdq is full\n"); +RUN_CMDQ_UNLOCK(sc); +return (-1); +} + sc->cmdq[i].func = run_wme_update_cb; sc->cmdq[i].arg0 = ic; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; ieee80211_runtask(ic, &sc->cmdq_task); + +RUN_CMDQ_UNLOCK(sc); return (0); } @@ -2085,28 +2108,38 @@ run_key_set(struct ieee80211vap *vap, struct ieee80211_key *k, { struct ieee80211com *ic = vap->iv_ic; struct run_softc *sc = ic->ic_ifp->if_softc; -uint32_t i; +uint8_t i; -i = RUN_CMDQ_GET(&sc->cmdq_store); +RUN_CMDQ_LOCK(sc); + +i = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", i); + +if (sc->cmdq[i].func != NULL) { +DPRINTF("cmdq is full\n"); +RUN_CMDQ_UNLOCK(sc); +return (0); +} + sc->cmdq[i].func = run_key_set_cb; sc->cmdq[i].arg0 = NULL; sc->cmdq[i].arg1 = vap; sc->cmdq[i].k = k; IEEE80211_ADDR_COPY(sc->cmdq[i].mac, mac); +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; ieee80211_runtask(ic, &sc->cmdq_task); /* * To make sure key will be set when hostapd * calls iv_key_set() before if_init(). */ -if(vap->iv_opmode == IEEE80211_M_HOSTAP){ -RUN_LOCK(sc); +if (vap->iv_opmode == IEEE80211_M_HOSTAP) sc->cmdq_key_set = RUN_CMDQ_GO; -RUN_UNLOCK(sc); -} -return(1); +RUN_CMDQ_UNLOCK(sc); + +return (1); } /* @@ -2154,16 +2187,23 @@ run_key_delete(struct ieee80211vap *vap, struct ieee80211_key *k) struct ieee80211com *ic = vap->iv_ic; struct run_softc *sc = ic->ic_ifp->if_softc; struct ieee80211_key *k0; -uint32_t i; +uint8_t i; /* * When called back, key might be gone. So, make a copy * of some values need to delete keys before deferring. - * But, because of LOR with node lock, cannot use lock here. - * So, use atomic instead. */ -i = RUN_CMDQ_GET(&sc->cmdq_store); +RUN_CMDQ_LOCK(sc); + +i = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", i); + +if (sc->cmdq[i].func != NULL) { +DPRINTF("cmdq is full\n"); +RUN_CMDQ_UNLOCK(sc); +return (0); +} + sc->cmdq[i].func = run_key_delete_cb; sc->cmdq[i].arg0 = NULL; sc->cmdq[i].arg1 = sc; @@ -2172,9 +2212,13 @@ run_key_delete(struct ieee80211vap *vap, struct ieee80211_key *k) k0->wk_keyix = k->wk_keyix; /* matching wcid was written to wk_pad in run_key_set() */ k0->wk_pad = k->wk_pad; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; ieee80211_runtask(ic, &sc->cmdq_task); -return (1);/* return fake success */ +RUN_CMDQ_UNLOCK(sc); + +return (1); } static void @@ -2362,19 +2406,31 @@ run_newassoc(struct ieee80211_node *ni, int isnew) } /* only interested in true associations */ -if (isnew && ni->ni_associd != 0){ - +if (isnew && ni->ni_associd != 0) { /* - * This function could is called though timeout function. + * This function could be called though timeout function. * Need to defer. */ -uint32_t cnt = RUN_CMDQ_GET(&sc->cmdq_store); +RUN_CMDQ_LOCK(sc); + +uint8_t cnt = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", cnt); + +if (sc->cmdq[cnt].func != NULL) { +DPRINTF("cmdq is full\n"); +RUN_CMDQ_UNLOCK(sc); +return; +} + sc->cmdq[cnt].func = run_newassoc_cb; sc->cmdq[cnt].arg0 = NULL; sc->cmdq[cnt].arg1 = ni; sc->cmdq[cnt].wcid = wcid; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; ieee80211_runtask(ic, &sc->cmdq_task); + +RUN_CMDQ_UNLOCK(sc); } DPRINTF("new assoc isnew=%d associd=%x addr=%s\n", @@ -2805,11 +2861,20 @@ tr_setup: if (error != USB_ERR_CANCELLED) { if (error == USB_ERR_TIMEOUT) { device_printf(sc->sc_dev, "device timeout\n"); -uint32_t i = RUN_CMDQ_GET(&sc->cmdq_store); + +RUN_CMDQ_LOCK(sc); +uint8_t i = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", i); -sc->cmdq[i].func = run_usb_timeout_cb; -sc->cmdq[i].arg0 = vap; -ieee80211_runtask(ic, &sc->cmdq_task); +if (sc->cmdq[i].func != NULL) +DPRINTF("cmdq is full\n"); +else { +sc->cmdq[i].func = run_usb_timeout_cb; +sc->cmdq[i].arg0 = vap; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; +ieee80211_runtask(ic, &sc->cmdq_task); +} +RUN_CMDQ_UNLOCK(sc); } /* @@ -3067,11 +3132,24 @@ run_tx(struct run_softc *sc, struct mbuf *m, struct ieee80211_node *ni) * With multiple vaps or if_bridge, if_start() is called * with a non-sleepable lock, tcpinp. So, need to defer. */ -uint32_t i = RUN_CMDQ_GET(&sc->cmdq_store); +RUN_CMDQ_LOCK(sc); +uint8_t i = sc->cmdq_store; DPRINTFN(6, "cmdq_store=%d\n", i); -sc->cmdq[i].func = run_drain_fifo; -sc->cmdq[i].arg0 = sc; -ieee80211_runtask(ic, &sc->cmdq_task); +if (sc->cmdq[i].func != NULL) { +DPRINTF("cmdq is full\n"); +/* + * Keep going. + * no need to stop Tx + * just because reading Tx stats failed + */ +} else { +sc->cmdq[i].func = run_drain_fifo; +sc->cmdq[i].arg0 = sc; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; +ieee80211_runtask(ic, &sc->cmdq_task); +} +RUN_CMDQ_UNLOCK(sc); } } @@ -3188,7 +3266,7 @@ run_sendprot(struct run_softc *sc, ackrate = ieee80211_ack_rate(ic->ic_rt, rate); isshort = (ic->ic_flags & IEEE80211_F_SHPREAMBLE) != 0; -dur = ieee80211_compute_duration(ic->ic_rt, pktlen, rate, isshort); +dur = ieee80211_compute_duration(ic->ic_rt, pktlen, rate, isshort) + ieee80211_ack_duration(ic->ic_rt, rate, isshort); wflags = RT2860_TX_FRAG; @@ -3906,14 +3984,27 @@ run_update_beacon(struct ieee80211vap *vap, int item) { struct ieee80211com *ic = vap->iv_ic; struct run_softc *sc = ic->ic_ifp->if_softc; -uint32_t i; +uint8_t i; + +RUN_CMDQ_LOCK(sc); -i = RUN_CMDQ_GET(&sc->cmdq_store); +i = sc->cmdq_store; DPRINTF("cmdq_store=%d\n", i); + +if (sc->cmdq[i].func != NULL) { +DPRINTF("cmdq is full\n"); +RUN_CMDQ_UNLOCK(sc); +return; +} + sc->cmdq[i].func = run_update_beacon_cb; sc->cmdq[i].arg0 = vap; +sc->cmdq_store++; +sc->cmdq_store &= RUN_CMDQ_MASQ; ieee80211_runtask(ic, &sc->cmdq_task); +RUN_CMDQ_UNLOCK(sc); + return; } @@ -4693,14 +4784,6 @@ run_init_locked(struct run_softc *sc) /* clear WCID attribute table */ run_set_region_4(sc, RT2860_WCID_ATTR(0), 0, 8 * 32); -/* hostapd sets a key before init. So, don't clear it. */ -if(sc->cmdq_key_set != RUN_CMDQ_GO){ -/* clear shared key table */ -run_set_region_4(sc, RT2860_SKEY(0, 0), 0, 8 * 32); -/* clear shared key mode */ -run_set_region_4(sc, RT2860_SKEY_MODE_0_7, 0, 4); -} - run_read(sc, RT2860_US_CYC_CNT, &tmp); tmp = (tmp & ~0xff) | 0x1e; run_write(sc, RT2860_US_CYC_CNT, tmp); @@ -4807,7 +4890,7 @@ run_stop(void *arg) ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); sc->ratectl_run = RUN_RATECTL_OFF; -sc->cmdq_run = sc->cmdq_key_set; +sc->cmdq_run = RUN_CMDQ_ABORT; RUN_UNLOCK(sc); diff --git a/dev/usb/wlan/if_runvar.h b/dev/usb/wlan/if_runvar.h index 39addbf..a6fddaa 100644 --- a/dev/usb/wlan/if_runvar.h +++ b/dev/usb/wlan/if_runvar.h @@ -202,6 +202,7 @@ struct run_softc { uint8_tsc_bssid[6]; struct mtxsc_mtx; +struct mtxsc_cmdq_mtx; struct run_endpoint_queuesc_epq[RUN_EP_QUEUES]; @@ -210,12 +211,12 @@ struct run_softc { uint8_tratectl_run; #define RUN_RATECTL_OFF0 -/* need to be power of 2, otherwise RUN_CMDQ_GET fails */ +/* need to be power of 2, otherwise run_cmdq_cb() fails */ #define RUN_CMDQ_MAX16 #define RUN_CMDQ_MASQ(RUN_CMDQ_MAX - 1) struct run_cmdqcmdq[RUN_CMDQ_MAX]; struct taskcmdq_task; -uint32_tcmdq_store; +uint8_tcmdq_store; uint8_tcmdq_exec; uint8_tcmdq_run; uint8_tcmdq_key_set; @@ -255,4 +256,7 @@ struct run_softc { #define RUN_UNLOCK(sc)mtx_unlock(&(sc)->sc_mtx) #define RUN_LOCK_ASSERT(sc, t)mtx_assert(&(sc)->sc_mtx, t) +#define RUN_CMDQ_LOCK(sc)mtx_lock(&(sc)->sc_cmdq_mtx) +#define RUN_CMDQ_UNLOCK(sc)mtx_unlock(&(sc)->sc_cmdq_mtx) + #endif/* _IF_RUNVAR_H_ */ -- end patch --