svn commit: r346670 - head/sys/net

[Kyle Evans]

Author: kevans
Date: Thu Apr 25 12:44:08 2019
New Revision: 346670
URL: https://svnweb.freebsd.org/changeset/base/346670

Log:
  tun/tap: close race between destroy/ioctl handler
  
  It seems that there should be a better way to handle this, but this seems to
  be the more common approach and it should likely get replaced in all of the
  places it happens... Basically, thread 1 is in the process of destroying the
  tun/tap while thread 2 is executing one of the ioctls that requires the
  tun/tap mutex and the mutex is destroyed before the ioctl handler can
  acquire it.
  
  This is only one of the races described/found in PR 233955.
  
  PR:		233955
  Reviewed by:	ae
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D20027

Modified:
  head/sys/net/if_tap.c
  head/sys/net/if_tun.c

Modified: head/sys/net/if_tap.c
==============================================================================
--- head/sys/net/if_tap.c	Thu Apr 25 12:02:17 2019	(r346669)
+++ head/sys/net/if_tap.c	Thu Apr 25 12:44:08 2019	(r346670)
@@ -41,6 +41,7 @@
 
 #include <sys/param.h>
 #include <sys/conf.h>
+#include <sys/lock.h>
 #include <sys/fcntl.h>
 #include <sys/filio.h>
 #include <sys/jail.h>
@@ -55,6 +56,7 @@
 #include <sys/signalvar.h>
 #include <sys/socket.h>
 #include <sys/sockio.h>
+#include <sys/sx.h>
 #include <sys/sysctl.h>
 #include <sys/systm.h>
 #include <sys/ttycom.h>
@@ -163,6 +165,9 @@ MALLOC_DECLARE(M_TAP);
 MALLOC_DEFINE(M_TAP, CDEV_NAME, "Ethernet tunnel interface");
 SYSCTL_INT(_debug, OID_AUTO, if_tap_debug, CTLFLAG_RW, &tapdebug, 0, "");
 
+static struct sx tap_ioctl_sx;
+SX_SYSINIT(tap_ioctl_sx, &tap_ioctl_sx, "tap_ioctl");
+
 SYSCTL_DECL(_net_link);
 static SYSCTL_NODE(_net_link, OID_AUTO, tap, CTLFLAG_RW, 0,
     "Ethernet tunnel software network interface");
@@ -217,6 +222,10 @@ tap_destroy(struct tap_softc *tp)
 	struct ifnet *ifp = tp->tap_ifp;
 
 	CURVNET_SET(ifp->if_vnet);
+	sx_xlock(&tap_ioctl_sx);
+	ifp->if_softc = NULL;
+	sx_xunlock(&tap_ioctl_sx);
+
 	destroy_dev(tp->tap_dev);
 	seldrain(&tp->tap_rsel);
 	knlist_clear(&tp->tap_rsel.si_note, 0);
@@ -600,12 +609,18 @@ tapifinit(void *xtp)
 static int
 tapifioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {
-	struct tap_softc	*tp = ifp->if_softc;
+	struct tap_softc	*tp;
 	struct ifreq		*ifr = (struct ifreq *)data;
 	struct ifstat		*ifs = NULL;
 	struct ifmediareq	*ifmr = NULL;
 	int			 dummy, error = 0;
 
+	sx_xlock(&tap_ioctl_sx);
+	tp = ifp->if_softc;
+	if (tp == NULL) {
+		error = ENXIO;
+		goto bad;
+	}
 	switch (cmd) {
 		case SIOCSIFFLAGS: /* XXX -- just like vmnet does */
 		case SIOCADDMULTI:
@@ -648,6 +663,8 @@ tapifioctl(struct ifnet *ifp, u_long cmd, caddr_t data
 			break;
 	}
 
+bad:
+	sx_xunlock(&tap_ioctl_sx);
 	return (error);
 } /* tapifioctl */
 

Modified: head/sys/net/if_tun.c
==============================================================================
--- head/sys/net/if_tun.c	Thu Apr 25 12:02:17 2019	(r346669)
+++ head/sys/net/if_tun.c	Thu Apr 25 12:44:08 2019	(r346670)
@@ -20,6 +20,7 @@
 #include "opt_inet6.h"
 
 #include <sys/param.h>
+#include <sys/lock.h>
 #include <sys/priv.h>
 #include <sys/proc.h>
 #include <sys/systm.h>
@@ -30,6 +31,7 @@
 #include <sys/fcntl.h>
 #include <sys/filio.h>
 #include <sys/sockio.h>
+#include <sys/sx.h>
 #include <sys/ttycom.h>
 #include <sys/poll.h>
 #include <sys/selinfo.h>
@@ -115,6 +117,9 @@ static struct clonedevs *tunclones;
 static TAILQ_HEAD(,tun_softc)	tunhead = TAILQ_HEAD_INITIALIZER(tunhead);
 SYSCTL_INT(_debug, OID_AUTO, if_tun_debug, CTLFLAG_RW, &tundebug, 0, "");
 
+static struct sx tun_ioctl_sx;
+SX_SYSINIT(tun_ioctl_sx, &tun_ioctl_sx, "tun_ioctl");
+
 SYSCTL_DECL(_net_link);
 static SYSCTL_NODE(_net_link, OID_AUTO, tun, CTLFLAG_RW, 0,
     "IP tunnel software network interface.");
@@ -278,6 +283,10 @@ tun_destroy(struct tun_softc *tp)
 		mtx_unlock(&tp->tun_mtx);
 
 	CURVNET_SET(TUN2IFP(tp)->if_vnet);
+	sx_xlock(&tun_ioctl_sx);
+	TUN2IFP(tp)->if_softc = NULL;
+	sx_xunlock(&tun_ioctl_sx);
+
 	dev = tp->tun_dev;
 	bpfdetach(TUN2IFP(tp));
 	if_detach(TUN2IFP(tp));
@@ -588,10 +597,16 @@ static int
 tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {
 	struct ifreq *ifr = (struct ifreq *)data;
-	struct tun_softc *tp = ifp->if_softc;
+	struct tun_softc *tp;
 	struct ifstat *ifs;
 	int		error = 0;
 
+	sx_xlock(&tun_ioctl_sx);
+	tp = ifp->if_softc;
+	if (tp == NULL) {
+		error = ENXIO;
+		goto bad;
+	}
 	switch(cmd) {
 	case SIOCGIFSTATUS:
 		ifs = (struct ifstat *)data;
@@ -618,6 +633,8 @@ tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t data
 	default:
 		error = EINVAL;
 	}
+bad:
+	sx_xunlock(&tun_ioctl_sx);
 	return (error);
 }