Date: Sun, 12 Jul 2009 09:05:19 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Kip Macy <kmacy@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r195628 - user/kmacy/head_ppacket/sys/net Message-ID: <alpine.BSF.2.00.0907120901400.10745@fledge.watson.org> In-Reply-To: <200907112257.n6BMv3Vm065726@svn.freebsd.org> References: <200907112257.n6BMv3Vm065726@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 11 Jul 2009, Kip Macy wrote: > Author: kmacy > Date: Sat Jul 11 22:57:02 2009 > New Revision: 195628 > URL: http://svn.freebsd.org/changeset/base/195628 > > Log: > - remove tun_pid - TUN_OPEN is used to avoid multiple users FWIW, I think my comment about tun_pid is not 100% correct. For the purposes of the kernel implementation, it's true that it is used only for the purpose I describe. However, I think it's also reported to userspace when status is queried so that ifconfig can printf the pid of the process that opened (although not necessarily the one still using) the tunnel device. > - make teardown of the ifaddr list SMP / PREEMPTION safe As I recall, these paths are hard to lock correctly due to calling out to other code that not only modifies the same lists, but also acquires address list locks, etc. Per our chat last night, I'm aware of the issue and will try to take a look at it soon. It similarly affects other code paths that do the same sorts of things -- walk per-ifnet address lists and then rtinit each address, etc. Robert N M Watson Computer Laboratory University of Cambridge > - add tun_rwait_cv to avoid TUN_RWAIT setting sleep / wakeup dance > - serialize access to softc structures every place they're touched > - remove spl in all places where the tun lock now protects state > > Modified: > user/kmacy/head_ppacket/sys/net/if_tun.c > > Modified: user/kmacy/head_ppacket/sys/net/if_tun.c > ============================================================================== > --- user/kmacy/head_ppacket/sys/net/if_tun.c Sat Jul 11 22:43:20 2009 (r195627) > +++ user/kmacy/head_ppacket/sys/net/if_tun.c Sat Jul 11 22:57:02 2009 (r195628) > @@ -76,25 +76,18 @@ struct tun_softc { > #define TUN_IASET 0x0008 > #define TUN_DSTADDR 0x0010 > #define TUN_LMODE 0x0020 > -#define TUN_RWAIT 0x0040 > + > #define TUN_ASYNC 0x0080 > #define TUN_IFHEAD 0x0100 > > #define TUN_READY (TUN_OPEN | TUN_INITED) > > - /* > - * XXXRW: tun_pid is used to exclusively lock /dev/tun. Is this > - * actually needed? Can we just return EBUSY if already open? > - * Problem is that this involved inherent races when a tun device > - * is handed off from one process to another, as opposed to just > - * being slightly stale informationally. > - */ > - pid_t tun_pid; /* owning pid */ > struct ifnet *tun_ifp; /* the interface */ > struct sigio *tun_sigio; /* information for async I/O */ > struct selinfo tun_rsel; /* read select */ > struct mtx tun_mtx; /* protect mutable softc fields */ > struct cv tun_cv; /* protect against ref'd dev destroy */ > + struct cv tun_rwait_cv; /* rwait wakeup */ > }; > #define TUN2IFP(sc) ((sc)->tun_ifp) > > @@ -347,10 +340,7 @@ tunstart(struct ifnet *ifp) > } > > mtx_lock(&tp->tun_mtx); > - if (tp->tun_flags & TUN_RWAIT) { > - tp->tun_flags &= ~TUN_RWAIT; > - wakeup(tp); > - } > + cv_broadcast(&tp->tun_rwait_cv); > if (tp->tun_flags & TUN_ASYNC && tp->tun_sigio) { > mtx_unlock(&tp->tun_mtx); > pgsigio(&tp->tun_sigio, SIGIO, 0); > @@ -371,7 +361,8 @@ tuncreate(const char *name, struct cdev > > sc = malloc(sizeof(*sc), M_TUN, M_WAITOK | M_ZERO); > mtx_init(&sc->tun_mtx, "tun_mtx", NULL, MTX_DEF); > - cv_init(&sc->tun_cv, "tun_condvar"); > + cv_init(&sc->tun_cv, "tun_ref_cv"); > + cv_init(&sc->tun_rwait_cv, "tun_rwait_cv"); > sc->tun_flags = TUN_INITED; > sc->tun_dev = dev; > mtx_lock(&tunmtx); > @@ -417,18 +408,11 @@ tunopen(struct cdev *dev, int flag, int > tp = dev->si_drv1; > } > > - /* > - * XXXRW: This use of tun_pid is subject to error due to the > - * fact that a reference to the tunnel can live beyond the > - * death of the process that created it. Can we replace this > - * with a simple busy flag? > - */ > mtx_lock(&tp->tun_mtx); > - if (tp->tun_pid != 0 && tp->tun_pid != td->td_proc->p_pid) { > + if (tp->tun_flags & TUN_OPEN) { > mtx_unlock(&tp->tun_mtx); > return (EBUSY); > } > - tp->tun_pid = td->td_proc->p_pid; > > tp->tun_flags |= TUN_OPEN; > mtx_unlock(&tp->tun_mtx); > @@ -448,36 +432,37 @@ tunclose(struct cdev *dev, int foo, int > { > struct tun_softc *tp; > struct ifnet *ifp; > - int s; > - > + struct ifaddrhead head; > + > tp = dev->si_drv1; > ifp = TUN2IFP(tp); > - > + > + TAILQ_INIT(&head); > mtx_lock(&tp->tun_mtx); > tp->tun_flags &= ~TUN_OPEN; > - tp->tun_pid = 0; > mtx_unlock(&tp->tun_mtx); > > /* > * junk all pending output > */ > CURVNET_SET(ifp->if_vnet); > - s = splimp(); > IFQ_PURGE(&ifp->if_snd); > - splx(s); > > if (ifp->if_flags & IFF_UP) { > - s = splimp(); > + mtx_lock(&tp->tun_mtx); > if_down(ifp); > - splx(s); > + mtx_unlock(&tp->tun_mtx); > } > > /* Delete all addresses and routes which reference this interface. */ > if (ifp->if_drv_flags & IFF_DRV_RUNNING) { > struct ifaddr *ifa; > > - s = splimp(); > - TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) { > + while (!TAILQ_EMPTY(&ifp->if_addrhead)) { > + IF_ADDR_LOCK(ifp); > + ifa = TAILQ_FIRST(&ifp->if_addrhead); > + TAILQ_REMOVE(&ifp->if_addrhead, ifa, ifa_link); > + IF_ADDR_UNLOCK(ifp); > /* deal w/IPv4 PtP destination; unlocked read */ > if (ifa->ifa_addr->sa_family == AF_INET) { > rtinit(ifa, (int)RTM_DELETE, > @@ -486,9 +471,13 @@ tunclose(struct cdev *dev, int foo, int > rtinit(ifa, (int)RTM_DELETE, 0); > } > } > + IF_ADDR_LOCK(ifp); > + TAILQ_CONCAT(&ifp->if_addrhead, &head, ifa_link); > if_purgeaddrs(ifp); > + IF_ADDR_UNLOCK(ifp); > + mtx_lock(&tp->tun_mtx); > ifp->if_drv_flags &= ~IFF_DRV_RUNNING; > - splx(s); > + mtx_unlock(&tp->tun_mtx); > } > if_link_state_change(ifp, LINK_STATE_DOWN); > CURVNET_RESTORE(); > @@ -507,12 +496,13 @@ tunclose(struct cdev *dev, int foo, int > static int > tuninit(struct ifnet *ifp) > { > + int error = 0; > #ifdef INET > struct tun_softc *tp = ifp->if_softc; > struct ifaddr *ifa; > + > + mtx_assert(&tp->tun_mtx, MA_OWNED); > #endif > - int error = 0; > - > TUNDEBUG(ifp, "tuninit\n"); > > ifp->if_flags |= IFF_UP; > @@ -526,14 +516,12 @@ tuninit(struct ifnet *ifp) > struct sockaddr_in *si; > > si = (struct sockaddr_in *)ifa->ifa_addr; > - mtx_lock(&tp->tun_mtx); > if (si->sin_addr.s_addr) > tp->tun_flags |= TUN_IASET; > > si = (struct sockaddr_in *)ifa->ifa_dstaddr; > if (si && si->sin_addr.s_addr) > tp->tun_flags |= TUN_DSTADDR; > - mtx_unlock(&tp->tun_mtx); > } > } > if_addr_runlock(ifp); > @@ -550,17 +538,12 @@ tunifioctl(struct ifnet *ifp, u_long cmd > struct ifreq *ifr = (struct ifreq *)data; > struct tun_softc *tp = ifp->if_softc; > struct ifstat *ifs; > - int error = 0, s; > + int error = 0; > > - s = splimp(); > + mtx_lock(&tp->tun_mtx); > switch(cmd) { > case SIOCGIFSTATUS: > ifs = (struct ifstat *)data; > - mtx_lock(&tp->tun_mtx); > - if (tp->tun_pid) > - sprintf(ifs->ascii + strlen(ifs->ascii), > - "\tOpened by PID %d\n", tp->tun_pid); > - mtx_unlock(&tp->tun_mtx); > break; > case SIOCSIFADDR: > error = tuninit(ifp); > @@ -581,7 +564,7 @@ tunifioctl(struct ifnet *ifp, u_long cmd > default: > error = EINVAL; > } > - splx(s); > + mtx_unlock(&tp->tun_mtx); > return (error); > } > > @@ -687,7 +670,6 @@ tunoutput( > static int > tunioctl(struct cdev *dev, u_long cmd, caddr_t data, int flag, struct thread *td) > { > - int s; > int error; > struct tun_softc *tp = dev->si_drv1; > struct tuninfo *tunp; > @@ -758,11 +740,6 @@ tunioctl(struct cdev *dev, u_long cmd, c > return(EINVAL); > } > break; > - case TUNSIFPID: > - mtx_lock(&tp->tun_mtx); > - tp->tun_pid = curthread->td_proc->p_pid; > - mtx_unlock(&tp->tun_mtx); > - break; > case FIONBIO: > break; > case FIOASYNC: > @@ -774,7 +751,6 @@ tunioctl(struct cdev *dev, u_long cmd, c > mtx_unlock(&tp->tun_mtx); > break; > case FIONREAD: > - s = splimp(); > if (!IFQ_IS_EMPTY(&TUN2IFP(tp)->if_snd)) { > struct mbuf *mb; > IFQ_LOCK(&TUN2IFP(tp)->if_snd); > @@ -784,7 +760,6 @@ tunioctl(struct cdev *dev, u_long cmd, c > IFQ_UNLOCK(&TUN2IFP(tp)->if_snd); > } else > *(int *)data = 0; > - splx(s); > break; > case FIOSETOWN: > return (fsetown(*(int *)data, &tp->tun_sigio)); > @@ -818,7 +793,7 @@ tunread(struct cdev *dev, struct uio *ui > struct tun_softc *tp = dev->si_drv1; > struct ifnet *ifp = TUN2IFP(tp); > struct mbuf *m; > - int error=0, len, s; > + int error=0, len; > > TUNDEBUG (ifp, "read\n"); > mtx_lock(&tp->tun_mtx); > @@ -827,29 +802,21 @@ tunread(struct cdev *dev, struct uio *ui > TUNDEBUG (ifp, "not ready 0%o\n", tp->tun_flags); > return (EHOSTDOWN); > } > - > - tp->tun_flags &= ~TUN_RWAIT; > mtx_unlock(&tp->tun_mtx); > > - s = splimp(); > do { > IFQ_DEQUEUE(&ifp->if_snd, m); > if (m == NULL) { > if (flag & O_NONBLOCK) { > - splx(s); > return (EWOULDBLOCK); > } > mtx_lock(&tp->tun_mtx); > - tp->tun_flags |= TUN_RWAIT; > + error = cv_wait_sig(&tp->tun_rwait_cv, &tp->tun_mtx); > mtx_unlock(&tp->tun_mtx); > - if ((error = tsleep(tp, PCATCH | (PZERO + 1), > - "tunread", 0)) != 0) { > - splx(s); > + if (error) > return (error); > - } > } > } while (m == NULL); > - splx(s); > > while (m && uio->uio_resid > 0 && error == 0) { > len = min(uio->uio_resid, m->m_len); > @@ -962,13 +929,11 @@ tunwrite(struct cdev *dev, struct uio *u > static int > tunpoll(struct cdev *dev, int events, struct thread *td) > { > - int s; > struct tun_softc *tp = dev->si_drv1; > struct ifnet *ifp = TUN2IFP(tp); > int revents = 0; > struct mbuf *m; > > - s = splimp(); > TUNDEBUG(ifp, "tunpoll\n"); > > if (events & (POLLIN | POLLRDNORM)) { > @@ -986,7 +951,6 @@ tunpoll(struct cdev *dev, int events, st > if (events & (POLLOUT | POLLWRNORM)) > revents |= events & (POLLOUT | POLLWRNORM); > > - splx(s); > return (revents); > } > > @@ -1034,12 +998,11 @@ tunkqfilter(struct cdev *dev, struct kno > static int > tunkqread(struct knote *kn, long hint) > { > - int ret, s; > + int ret; > struct cdev *dev = (struct cdev *)(kn->kn_hook); > struct tun_softc *tp = dev->si_drv1; > struct ifnet *ifp = TUN2IFP(tp); > > - s = splimp(); > if ((kn->kn_data = ifp->if_snd.ifq_len) > 0) { > TUNDEBUG(ifp, > "%s have data in the queue. Len = %d, minor = %#x\n", > @@ -1051,7 +1014,6 @@ tunkqread(struct knote *kn, long hint) > dev2unit(dev)); > ret = 0; > } > - splx(s); > > return (ret); > } > @@ -1062,13 +1024,12 @@ tunkqread(struct knote *kn, long hint) > static int > tunkqwrite(struct knote *kn, long hint) > { > - int s; > struct tun_softc *tp = ((struct cdev *)kn->kn_hook)->si_drv1; > struct ifnet *ifp = TUN2IFP(tp); > > - s = splimp(); > + mtx_lock(&tp->tun_mtx); > kn->kn_data = ifp->if_mtu; > - splx(s); > + mtx_unlock(&tp->tun_mtx); > > return (1); > } >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.0907120901400.10745>