From d9b62cfaed2604d11a26c42728f40ee1c750e006 Mon Sep 17 00:00:00 2001 From: dyoung Date: Sun, 6 May 2007 02:47:52 +0000 Subject: [PATCH] Switch from spl(9) to mutex(9) and condvar(9). Fix a defect in the locking of file descriptors as we delegate a UDP socket from userland to the kernel. Move sc_fp out of sc_soparm. Synchronize access to sc_fp by gre_ioctl() and the kernel thread using a condition variable. For simplicity's sake, make it the kernel helper thread's responsibility to close its UDP socket. --- sys/net/if_gre.c | 214 ++++++++++++++++++++++++++--------------------- sys/net/if_gre.h | 18 ++-- 2 files changed, 129 insertions(+), 103 deletions(-) diff --git a/sys/net/if_gre.c b/sys/net/if_gre.c index 4f6f31a091a2..240bde02a36d 100644 --- a/sys/net/if_gre.c +++ b/sys/net/if_gre.c @@ -1,4 +1,4 @@ -/* $NetBSD: if_gre.c,v 1.92 2007/05/02 20:40:23 dyoung Exp $ */ +/* $NetBSD: if_gre.c,v 1.93 2007/05/06 02:47:52 dyoung Exp $ */ /* * Copyright (c) 1998 The NetBSD Foundation, Inc. @@ -48,7 +48,7 @@ */ #include -__KERNEL_RCSID(0, "$NetBSD: if_gre.c,v 1.92 2007/05/02 20:40:23 dyoung Exp $"); +__KERNEL_RCSID(0, "$NetBSD: if_gre.c,v 1.93 2007/05/06 02:47:52 dyoung Exp $"); #include "opt_gre.h" #include "opt_inet.h" @@ -72,6 +72,9 @@ __KERNEL_RCSID(0, "$NetBSD: if_gre.c,v 1.92 2007/05/02 20:40:23 dyoung Exp $"); #include #endif +#include +#include +#include #include #include @@ -138,38 +141,35 @@ static int gre_ioctl(struct ifnet *, u_long, void *); static int gre_compute_route(struct gre_softc *sc); +static void gre_closef(struct file **, struct lwp *); static int gre_getsockname(struct socket *, struct mbuf *, struct lwp *); static int gre_getpeername(struct socket *, struct mbuf *, struct lwp *); static int gre_getnames(struct socket *, struct lwp *, struct sockaddr_in *, struct sockaddr_in *); +/* Calling thread must hold sc->sc_mtx. */ static void -gre_stop(volatile int *running) +gre_stop(struct gre_softc *sc) { - *running = 0; - wakeup(running); + sc->sc_running = 0; + cv_signal(&sc->sc_join_cv); } +/* Calling thread must hold sc->sc_mtx. */ static void -gre_join(volatile int *running) +gre_join(struct gre_softc *sc) { - int s; - - s = splnet(); - while (*running != 0) { - splx(s); - tsleep(running, PSOCK, "grejoin", 0); - s = splnet(); - } - splx(s); + while (sc->sc_running != 0) + cv_wait(&sc->sc_join_cv, &sc->sc_mtx); } +/* Calling thread must hold sc->sc_mtx. */ static void gre_wakeup(struct gre_softc *sc) { GRE_DPRINTF(sc, "%s: enter\n", __func__); - sc->sc_waitchan = 1; - wakeup(&sc->sc_waitchan); + sc->sc_haswork = 1; + cv_signal(&sc->sc_work_cv); } static int @@ -179,6 +179,10 @@ gre_clone_create(struct if_clone *ifc, int unit) sc = malloc(sizeof(struct gre_softc), M_DEVBUF, M_WAITOK); memset(sc, 0, sizeof(struct gre_softc)); + mutex_init(&sc->sc_mtx, MUTEX_DRIVER, IPL_NET); + cv_init(&sc->sc_work_cv, "gre work"); + cv_init(&sc->sc_join_cv, "gre join"); + cv_init(&sc->sc_soparm_cv, "gre soparm"); snprintf(sc->sc_if.if_xname, sizeof(sc->sc_if.if_xname), "%s%d", ifc->ifc_name, unit); @@ -208,26 +212,23 @@ gre_clone_create(struct if_clone *ifc, int unit) static int gre_clone_destroy(struct ifnet *ifp) { - int s; struct gre_softc *sc = ifp->if_softc; LIST_REMOVE(sc, sc_list); #if NBPFILTER > 0 bpfdetach(ifp); #endif - s = splnet(); - ifp->if_flags &= ~IFF_UP; - gre_wakeup(sc); - splx(s); - gre_join(&sc->sc_thread); - s = splnet(); if_detach(ifp); + mutex_enter(&sc->sc_mtx); + gre_wakeup(sc); + gre_join(sc); + mutex_exit(&sc->sc_mtx); rtcache_free(&sc->route); - splx(s); - if (sc->sc_fp != NULL) { - closef(sc->sc_fp, curlwp); - sc->sc_fp = NULL; - } + + cv_destroy(&sc->sc_soparm_cv); + cv_destroy(&sc->sc_join_cv); + cv_destroy(&sc->sc_work_cv); + mutex_destroy(&sc->sc_mtx); free(sc, M_DEVBUF); return 0; @@ -357,18 +358,19 @@ out: static void gre_thread1(struct gre_softc *sc, struct lwp *l) { - int flags, rc, s; + int flags, rc; const struct gre_h *gh; struct ifnet *ifp = &sc->sc_if; struct mbuf *m; struct socket *so = NULL; struct uio uio; struct gre_soparm sp; + struct file *fp = NULL; GRE_DPRINTF(sc, "%s: enter\n", __func__); - s = splnet(); + mutex_enter(&sc->sc_mtx); - sc->sc_waitchan = 1; + sc->sc_haswork = 1; memset(&sp, 0, sizeof(sp)); memset(&uio, 0, sizeof(uio)); @@ -376,13 +378,11 @@ gre_thread1(struct gre_softc *sc, struct lwp *l) ifp->if_flags |= IFF_RUNNING; for (;;) { - while (sc->sc_waitchan == 0) { - splx(s); + while (sc->sc_haswork == 0) { GRE_DPRINTF(sc, "%s: sleeping\n", __func__); - tsleep(&sc->sc_waitchan, PSOCK, "grewait", 0); - s = splnet(); + cv_wait(&sc->sc_work_cv, &sc->sc_mtx); } - sc->sc_waitchan = 0; + sc->sc_haswork = 0; GRE_DPRINTF(sc, "%s: awake\n", __func__); if ((ifp->if_flags & IFF_UP) != IFF_UP) { GRE_DPRINTF(sc, "%s: not up & running; exiting\n", @@ -394,24 +394,26 @@ gre_thread1(struct gre_softc *sc, struct lwp *l) break; } /* XXX optimize */ - if (so == NULL || memcmp(&sp, &sc->sc_soparm, sizeof(sp)) != 0){ + if (so == NULL || sc->sc_fp != NULL || + memcmp(&sp, &sc->sc_soparm, sizeof(sp)) != 0) { GRE_DPRINTF(sc, "%s: parameters changed\n", __func__); - if (sp.sp_fp != NULL) { - FILE_UNUSE(sp.sp_fp, NULL); - sp.sp_fp = NULL; + if (fp != NULL) { + gre_closef(&fp, curlwp); so = NULL; } else if (so != NULL) gre_sodestroy(&so); if (sc->sc_fp != NULL) { - so = (struct socket *)sc->sc_fp->f_data; + fp = sc->sc_fp; + sc->sc_fp = NULL; + so = (struct socket *)fp->f_data; gre_upcall_add(so, sc); sp = sc->sc_soparm; - FILE_USE(sp.sp_fp); } else if (gre_socreate1(sc, l, &sp, &so) != 0) goto out; } + cv_signal(&sc->sc_soparm_cv); for (;;) { flags = MSG_DONTWAIT; uio.uio_resid = 1000000; @@ -439,7 +441,7 @@ gre_thread1(struct gre_softc *sc, struct lwp *l) } gh = mtod(m, const struct gre_h *); - if (gre_input3(sc, m, 0, gh) == 0) { + if (gre_input3(sc, m, 0, gh, 1) == 0) { GRE_DPRINTF(sc, "%s: dropping unsupported\n", __func__); m_freem(m); @@ -462,32 +464,26 @@ gre_thread1(struct gre_softc *sc, struct lwp *l) GRE_DPRINTF(sc, "%s: so_send failed\n", __func__); } - /* Give the software interrupt queues a chance to - * run, or else when I send a ping from gre0 to gre1 on - * the same host, gre0 will not wake for the reply. - */ - splx(s); - s = splnet(); } - if (sp.sp_fp != NULL) { + if (fp != NULL) { GRE_DPRINTF(sc, "%s: removing upcall\n", __func__); gre_upcall_remove(so); - FILE_UNUSE(sp.sp_fp, NULL); - sp.sp_fp = NULL; } else if (so != NULL) gre_sodestroy(&so); out: GRE_DPRINTF(sc, "%s: stopping\n", __func__); + if (fp != NULL) + gre_closef(&fp, curlwp); if (sc->sc_proto == IPPROTO_UDP) ifp->if_flags &= ~IFF_RUNNING; while (!IF_IS_EMPTY(&sc->sc_snd)) { IF_DEQUEUE(&sc->sc_snd, m); m_freem(m); } - gre_stop(&sc->sc_thread); + gre_stop(sc); /* must not touch sc after this! */ GRE_DPRINTF(sc, "%s: restore ipl\n", __func__); - splx(s); + mutex_exit(&sc->sc_mtx); } static void @@ -500,15 +496,16 @@ gre_thread(void *arg) kthread_exit(0); } +/* Calling thread must hold sc->sc_mtx. */ int gre_input3(struct gre_softc *sc, struct mbuf *m, int hlen, - const struct gre_h *gh) + const struct gre_h *gh, int mtx_held) { u_int16_t flags; #if NBPFILTER > 0 u_int32_t af = AF_INET; /* af passed to BPF tap */ #endif - int s, isr; + int isr; struct ifqueue *ifq; sc->sc_if.if_ipackets++; @@ -577,7 +574,8 @@ gre_input3(struct gre_softc *sc, struct mbuf *m, int hlen, m->m_pkthdr.rcvif = &sc->sc_if; - s = splnet(); /* possible */ + if (!mtx_held) + mutex_enter(&sc->sc_mtx); if (IF_QFULL(ifq)) { IF_DROP(ifq); m_freem(m); @@ -586,7 +584,8 @@ gre_input3(struct gre_softc *sc, struct mbuf *m, int hlen, } /* we need schednetisr since the address family may change */ schednetisr(isr); - splx(s); + if (!mtx_held) + mutex_exit(&sc->sc_mtx); return 1; /* packet is done, no further processing needed */ } @@ -797,10 +796,7 @@ gre_output(struct ifnet *ifp, struct mbuf *m, const struct sockaddr *dst, return error; } -/* gre_kick must be synchronized with network interrupts in order - * to synchronize access to gre_softc members, so call it with - * interrupt priority level set to IPL_NET or greater. - */ +/* Calling thread must hold sc->sc_mtx. */ static int gre_kick(struct gre_softc *sc) { @@ -808,11 +804,13 @@ gre_kick(struct gre_softc *sc) struct ifnet *ifp = &sc->sc_if; if (sc->sc_proto == IPPROTO_UDP && (ifp->if_flags & IFF_UP) == IFF_UP && - !sc->sc_thread) { - sc->sc_thread = 1; + !sc->sc_running) { + sc->sc_running = 1; + mutex_exit(&sc->sc_mtx); rc = kthread_create1(gre_thread, sc, NULL, ifp->if_xname); + mutex_enter(&sc->sc_mtx); if (rc != 0) - gre_stop(&sc->sc_thread); + gre_stop(sc); return rc; } else { gre_wakeup(sc); @@ -820,29 +818,28 @@ gre_kick(struct gre_softc *sc) } } +/* Calling thread must hold sc->sc_mtx. */ static int gre_getname(struct socket *so, int req, struct mbuf *nam, struct lwp *l) { - int s, error; - - s = splsoftnet(); - error = (*so->so_proto->pr_usrreq)(so, req, NULL, nam, NULL, l); - splx(s); - return error; + return (*so->so_proto->pr_usrreq)(so, req, NULL, nam, NULL, l); } +/* Calling thread must hold sc->sc_mtx. */ static int gre_getsockname(struct socket *so, struct mbuf *nam, struct lwp *l) { return gre_getname(so, PRU_SOCKADDR, nam, l); } +/* Calling thread must hold sc->sc_mtx. */ static int gre_getpeername(struct socket *so, struct mbuf *nam, struct lwp *l) { return gre_getname(so, PRU_PEERADDR, nam, l); } +/* Calling thread must hold sc->sc_mtx. */ static int gre_getnames(struct socket *so, struct lwp *l, struct sockaddr_in *src, struct sockaddr_in *dst) @@ -877,11 +874,22 @@ out: return rc; } +static void +gre_closef(struct file **fpp, struct lwp *l) +{ + struct file *fp = *fpp; + + simple_lock(&fp->f_slock); + FILE_USE(fp); + closef(fp, l); + *fpp = NULL; +} + static int gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) { u_char oproto; - struct file *fp, *ofp; + struct file *fp; struct socket *so; struct sockaddr_in dst, src; struct proc *p = curproc; /* XXX */ @@ -889,7 +897,6 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) struct ifreq *ifr = (struct ifreq *)data; struct if_laddrreq *lifr = (struct if_laddrreq *)data; struct gre_softc *sc = ifp->if_softc; - int s; struct sockaddr_in si; struct sockaddr *sa = NULL; int error = 0; @@ -913,7 +920,7 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; } - s = splnet(); + mutex_enter(&sc->sc_mtx); switch (cmd) { case SIOCSIFADDR: ifp->if_flags |= IFF_UP; @@ -1020,10 +1027,6 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) if (sc->sc_proto == IPPROTO_UDP || (sc->g_src.s_addr != INADDR_ANY && sc->g_dst.s_addr != INADDR_ANY)) { - if (sc->sc_fp != NULL) { - closef(sc->sc_fp, l); - sc->sc_fp = NULL; - } rtcache_free(&sc->route); if (sc->sc_proto == IPPROTO_UDP) error = gre_kick(sc); @@ -1050,18 +1053,19 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) ifr->ifr_addr = *sa; break; case GREDSOCK: - if (sc->sc_proto != IPPROTO_UDP) - return EINVAL; - if (sc->sc_fp != NULL) { - closef(sc->sc_fp, l); - sc->sc_fp = NULL; - error = gre_kick(sc); + if (sc->sc_proto != IPPROTO_UDP) { + error = EINVAL; + break; } + ifp->if_flags &= ~IFF_UP; + gre_wakeup(sc); break; case GRESSOCK: - if (sc->sc_proto != IPPROTO_UDP) - return EINVAL; - /* getsock() will FILE_USE() the descriptor for us */ + if (sc->sc_proto != IPPROTO_UDP) { + error = EINVAL; + break; + } + /* getsock() will FILE_USE() and unlock the descriptor for us */ if ((error = getsock(p->p_fd, (int)ifr->ifr_value, &fp)) != 0) break; so = (struct socket *)fp->f_data; @@ -1076,21 +1080,37 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) break; } - fp->f_count++; + /* Increase reference count. Now that our reference + * to the file descriptor is counted, this thread + * can release our "use" of the descriptor, but it + * will not be destroyed by some other thread's + * action. This thread needs to release its use, + * too, because one and only one thread can have + * use of the descriptor at once. The kernel thread + * will pick up the use if it needs it. + */ - ofp = sc->sc_fp; - sc->sc_fp = fp; - if ((error = gre_kick(sc)) != 0) { - closef(fp, l); - sc->sc_fp = ofp; + fp->f_count++; + FILE_UNUSE(fp, NULL); + + while (sc->sc_fp != NULL && error == 0) { + error = cv_timedwait_sig(&sc->sc_soparm_cv, &sc->sc_mtx, + MAX(1, hz / 2)); + } + if (error == 0) { + sc->sc_fp = fp; + ifp->if_flags |= IFF_UP; + } + + if (error != 0 || (error = gre_kick(sc)) != 0) { + gre_closef(&fp, l); break; } + /* fp does not any longer belong to this thread. */ sc->g_src = src.sin_addr; sc->g_srcport = src.sin_port; sc->g_dst = dst.sin_addr; sc->g_dstport = dst.sin_port; - if (ofp != NULL) - closef(ofp, l); break; case SIOCSLIFPHYADDR: if (lifr->addr.ss_family != AF_INET || @@ -1136,7 +1156,7 @@ gre_ioctl(struct ifnet *ifp, u_long cmd, void *data) error = EINVAL; break; } - splx(s); + mutex_exit(&sc->sc_mtx); return error; } diff --git a/sys/net/if_gre.h b/sys/net/if_gre.h index 321405dfe2ac..ba81b596e0f7 100644 --- a/sys/net/if_gre.h +++ b/sys/net/if_gre.h @@ -1,4 +1,4 @@ -/* $NetBSD: if_gre.h,v 1.21 2007/03/21 01:56:05 dyoung Exp $ */ +/* $NetBSD: if_gre.h,v 1.22 2007/05/06 02:47:52 dyoung Exp $ */ /* * Copyright (c) 1998 The NetBSD Foundation, Inc. @@ -40,6 +40,8 @@ #define _NET_IF_GRE_H_ #include +#include +#include #ifdef _KERNEL struct gre_soparm { @@ -47,15 +49,19 @@ struct gre_soparm { struct in_addr sp_dst; /* destination address of gre packets */ in_port_t sp_srcport; /* source port of gre packets */ in_port_t sp_dstport; /* destination port of gre packets */ - struct file *sp_fp; }; struct gre_softc { struct ifnet sc_if; - volatile int sc_waitchan; - volatile int sc_thread; + kmutex_t sc_mtx; + kcondvar_t sc_soparm_cv; + kcondvar_t sc_join_cv; + kcondvar_t sc_work_cv; + int sc_haswork; + int sc_running; struct ifqueue sc_snd; struct gre_soparm sc_soparm; + struct file *sc_fp; LIST_ENTRY(gre_softc) sc_list; struct route route; /* routing entry that determines, where a encapsulated packet should go */ @@ -65,7 +71,6 @@ struct gre_softc { #define g_srcport sc_soparm.sp_srcport #define g_dst sc_soparm.sp_dst #define g_dstport sc_soparm.sp_dstport -#define sc_fp sc_soparm.sp_fp struct gre_h { u_int16_t flags; /* GRE flags */ @@ -163,7 +168,8 @@ LIST_HEAD(gre_softc_head, gre_softc); extern struct gre_softc_head gre_softc_list; u_int16_t gre_in_cksum(u_short *, u_int); -int gre_input3(struct gre_softc *, struct mbuf *, int, const struct gre_h *); +int gre_input3(struct gre_softc *, struct mbuf *, int, const struct gre_h *, + int); #endif /* _KERNEL */ #endif /* !_NET_IF_GRE_H_ */