From owner-p4-projects@FreeBSD.ORG Fri Mar 7 19:51:50 2008 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 1A1BD106567B; Fri, 7 Mar 2008 19:51:50 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CE6021065676 for ; Fri, 7 Mar 2008 19:51:49 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from repoman.freebsd.org (repoman.freebsd.org [IPv6:2001:4f8:fff6::29]) by mx1.freebsd.org (Postfix) with ESMTP id BD0208FC28 for ; Fri, 7 Mar 2008 19:51:49 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.14.1/8.14.1) with ESMTP id m27Jpn09081230 for ; Fri, 7 Mar 2008 19:51:49 GMT (envelope-from sam@freebsd.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.14.1/8.14.1/Submit) id m27JpnMV081228 for perforce@freebsd.org; Fri, 7 Mar 2008 19:51:49 GMT (envelope-from sam@freebsd.org) Date: Fri, 7 Mar 2008 19:51:49 GMT Message-Id: <200803071951.m27JpnMV081228@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to sam@freebsd.org using -f From: Sam Leffler To: Perforce Change Reviews Cc: Subject: PERFORCE change 137106 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Mar 2008 19:51:50 -0000 http://perforce.freebsd.org/chv.cgi?CH=137106 Change 137106 by sam@sam_ebb on 2008/03/07 19:51:41 Bandaid vap teardown races. On vap detach we block/reject use from above by marking the ifnet OACTIVE (to block xmit) and (hold breath for evil back) clearing if_softc so calls through if_init and if_ioctl can be ignored/rejected. This fixes destroy of a vap being controlled by wpa_supplicant--previously we would clock the vap to the INIT state and wpa_supplicant would wakeup and immediately try to scan which raced against the teardown. The better way to deal with this would seem to be destroying the ifnet before the net80211 state so nothing is visible to user space but that is hard because much code assumes this does not happen and changing that assumption may be costly (e.g. additional checks in fast paths). Affected files ... .. //depot/projects/vap/sys/net80211/ieee80211.c#28 edit .. //depot/projects/vap/sys/net80211/ieee80211_ioctl.c#38 edit .. //depot/projects/vap/sys/net80211/ieee80211_proto.c#23 edit .. //depot/projects/vap/sys/net80211/ieee80211_proto.h#17 edit Differences ... ==== //depot/projects/vap/sys/net80211/ieee80211.c#28 (text+ko) ==== @@ -462,7 +462,10 @@ } /* - * Tear down vap state prior to reclaiming the ifnet. + * Tear down vap state and reclaim the ifnet. + * The driver is assumed to have prepared for + * this; e.g. by turning off interrupts for the + * underlying device. */ void ieee80211_vap_detach(struct ieee80211vap *vap) @@ -474,17 +477,24 @@ __func__, ieee80211_opmode_name[vap->iv_opmode], ic->ic_ifp->if_xname); + IEEE80211_LOCK(ic); + /* block traffic from above */ + ifp->if_drv_flags |= IFF_DRV_OACTIVE; + /* + * Evil hack. Clear the backpointer from the ifnet to the + * vap so any requests from above will return an error or + * be ignored. In particular this short-circuits requests + * by the bridge to turn off promiscuous mode as a result + * of calling ether_ifdetach. + */ + ifp->if_softc = NULL; /* - * Mark interface down so we ignore calls by the bridge - * to turn off promiscuous mode as a result of calling - * ether_ifdetach. + * Stop the vap before detaching the ifnet. Ideally we'd + * do this in the other order so the ifnet is inaccessible + * while we cleanup internal state but that is hard. */ - ifp->if_flags &= ~IFF_UP; - ieee80211_stop(vap); - bpfdetach(ifp); - ether_ifdetach(ifp); + ieee80211_stop_locked(vap); - IEEE80211_LOCK(ic); /* XXX accumulate iv_stats in ic_stats? */ TAILQ_REMOVE(&ic->ic_vaps, vap, iv_next); ieee80211_syncflag_locked(ic, IEEE80211_F_WME); @@ -497,6 +507,10 @@ ieee80211_syncifflag_locked(ic, IFF_ALLMULTI); IEEE80211_UNLOCK(ic); + /* XXX can't hold com lock */ + /* NB: bpfattach is called by ether_ifdetach and claims all taps */ + ether_ifdetach(ifp); + ifmedia_removeall(&vap->iv_media); ieee80211_regdomain_vdetach(vap); ==== //depot/projects/vap/sys/net80211/ieee80211_ioctl.c#38 (text+ko) ==== @@ -3127,19 +3127,33 @@ int ieee80211_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data) { - struct ieee80211vap *vap = ifp->if_softc; - struct ieee80211com *ic = vap->iv_ic; - struct ifnet *parent = ic->ic_ifp; + struct ieee80211vap *vap; + struct ieee80211com *ic; + struct ifnet *parent; int error = 0; struct ifreq *ifr; struct ifaddr *ifa; /* XXX */ + vap = ifp->if_softc; + if (vap == NULL) { + /* + * During detach we clear the backpointer in the softc + * so any ioctl request through the ifnet that arrives + * before teardown is ignored/rejected. In particular + * this hack handles destroying a vap used by an app + * like wpa_supplicant that will respond to the vap + * being forced into INIT state by immediately trying + * to force it back up. We can yank this hack if/when + * we can destroy the ifnet before cleaning up vap state. + */ + return ENXIO; + } switch (cmd) { case SIOCSIFFLAGS: + ic = vap->iv_ic; IEEE80211_LOCK(ic); ieee80211_syncifflag_locked(ic, IFF_PROMISC); ieee80211_syncifflag_locked(ic, IFF_ALLMULTI); - IEEE80211_UNLOCK(ic); if (ifp->if_flags & IFF_UP) { /* * Bring ourself up unless we're already operational. @@ -3148,18 +3162,20 @@ * side-effect of bringing ourself up. */ if (vap->iv_state == IEEE80211_S_INIT) - ieee80211_init(vap); + ieee80211_start_locked(vap); } else if (ifp->if_drv_flags & IFF_DRV_RUNNING) { /* * Stop ourself. If we are the last vap to be * marked down the parent will also be taken down. */ - ieee80211_stop(vap); + ieee80211_stop_locked(vap); } + IEEE80211_UNLOCK(ic); break; case SIOCADDMULTI: case SIOCDELMULTI: /* XXX merge vap lists into parent */ + parent = vap->iv_ic->ic_ifp; if (parent->if_drv_flags & IFF_DRV_RUNNING) { /* XXX propagate multicast address to device */ error = parent->if_ioctl(parent, cmd, data); ==== //depot/projects/vap/sys/net80211/ieee80211_proto.c#23 (text+ko) ==== @@ -1075,7 +1075,7 @@ * set running on the underlying device then we * automatically bring the device up. */ -static void +void ieee80211_start_locked(struct ieee80211vap *vap) { struct ifnet *ifp = vap->iv_ifp; @@ -1161,14 +1161,23 @@ ieee80211_init(void *arg) { struct ieee80211vap *vap = arg; - struct ieee80211com *ic = vap->iv_ic; - IEEE80211_DPRINTF(vap, - IEEE80211_MSG_STATE | IEEE80211_MSG_DEBUG, "%s\n", __func__); + /* + * This routine is publicly accessible through the vap's + * if_init method so guard against calls during detach. + * ieee80211_vap_detach null's the backpointer before + * tearing down state to signal any callback should be + * rejected/ignored. + */ + if (vap != NULL) { + IEEE80211_DPRINTF(vap, + IEEE80211_MSG_STATE | IEEE80211_MSG_DEBUG, + "%s\n", __func__); - IEEE80211_LOCK(ic); - ieee80211_start_locked(vap); - IEEE80211_UNLOCK(ic); + IEEE80211_LOCK(vap->iv_ic); + ieee80211_start_locked(vap); + IEEE80211_UNLOCK(vap->iv_ic); + } } /* @@ -1196,16 +1205,17 @@ * next vap is brought up. */ void -ieee80211_stop(struct ieee80211vap *vap) +ieee80211_stop_locked(struct ieee80211vap *vap) { struct ieee80211com *ic = vap->iv_ic; struct ifnet *ifp = vap->iv_ifp; struct ifnet *parent = ic->ic_ifp; + IEEE80211_LOCK_ASSERT(ic); + IEEE80211_DPRINTF(vap, IEEE80211_MSG_STATE | IEEE80211_MSG_DEBUG, "stop running, %d vaps running\n", ic->ic_nrunning); - IEEE80211_LOCK(ic); ieee80211_new_state_locked(vap, IEEE80211_S_INIT, -1); if (ifp->if_drv_flags & IFF_DRV_RUNNING) { ifp->if_drv_flags &= ~IFF_DRV_RUNNING; /* mark us stopped */ @@ -1215,10 +1225,21 @@ IEEE80211_MSG_STATE | IEEE80211_MSG_DEBUG, "down parent %s\n", parent->if_xname); parent->if_flags &= ~IFF_UP; - /* XXX holding lock */ + /* XXX must drop lock */ + IEEE80211_UNLOCK(ic); parent->if_ioctl(parent, SIOCSIFFLAGS, NULL); + IEEE80211_LOCK(ic); } } +} + +void +ieee80211_stop(struct ieee80211vap *vap) +{ + struct ieee80211com *ic = vap->iv_ic; + + IEEE80211_LOCK(ic); + ieee80211_stop_locked(vap); IEEE80211_UNLOCK(ic); } ==== //depot/projects/vap/sys/net80211/ieee80211_proto.h#17 (text+ko) ==== @@ -253,8 +253,10 @@ return tid; } +void ieee80211_start_locked(struct ieee80211vap *); void ieee80211_init(void *); void ieee80211_start_all(struct ieee80211com *); +void ieee80211_stop_locked(struct ieee80211vap *); void ieee80211_stop(struct ieee80211vap *); void ieee80211_stop_all(struct ieee80211com *); void ieee80211_dturbo_switch(struct ieee80211vap *, int newflags);