Skip site navigation (1)Skip section navigation (2) 
Date:      Sun, 20 Oct 2019 22:39:40 +0000 (UTC)
From:      Kyle Evans <kevans@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r353785 - head/sys/net
Message-ID:  <201910202239.x9KMdeGT075555@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kevans
Date: Sun Oct 20 22:39:40 2019
New Revision: 353785
URL: https://svnweb.freebsd.org/changeset/base/353785

Log:
  tuntap(4): Use make_dev_s to avoid si_drv1 race
  
  This allows us to avoid some dance in tunopen for dealing with the
  possibility of dev->si_drv1 being NULL as it's set prior to the devfs node
  being created in all cases.
  
  There's still the possibility that the tun device hasn't been fully
  initialized, since that's done after the devfs node was created. Alleviate
  this by returning ENXIO if we're not to that point of tuncreate yet.
  
  This work is what sparked r353128, full initialization of cloned devices
  w/ specified make_dev_args.

Modified:
  head/sys/net/if_tuntap.c

Modified: head/sys/net/if_tuntap.c
==============================================================================
--- head/sys/net/if_tuntap.c	Sun Oct 20 22:05:57 2019	(r353784)
+++ head/sys/net/if_tuntap.c	Sun Oct 20 22:39:40 2019	(r353785)
@@ -209,6 +209,8 @@ SYSCTL_INT(_net_link_tap, OID_AUTO, devfs_cloning, CTL
     "Enable legacy devfs interface creation");
 SYSCTL_INT(_net_link_tap, OID_AUTO, debug, CTLFLAG_RW, &tundebug, 0, "");
 
+static int	tun_create_device(struct tuntap_driver *drv, int unit,
+    struct ucred *cr, struct cdev **dev, const char *name);
 static int	tun_busy_locked(struct tuntap_softc *tp);
 static void	tun_unbusy_locked(struct tuntap_softc *tp);
 static int	tun_busy(struct tuntap_softc *tp);
@@ -217,7 +219,7 @@ static void	tun_unbusy(struct tuntap_softc *tp);
 static int	tuntap_name2info(const char *name, int *unit, int *flags);
 static void	tunclone(void *arg, struct ucred *cred, char *name,
 		    int namelen, struct cdev **dev);
-static void	tuncreate(struct cdev *dev, struct tuntap_driver *);
+static void	tuncreate(struct cdev *dev);
 static void	tunrename(void *arg, struct ifnet *ifp);
 static int	tunifioctl(struct ifnet *, u_long, caddr_t);
 static void	tuninit(struct ifnet *);
@@ -544,16 +546,15 @@ tun_clone_create(struct if_clone *ifc, char *name, siz
 	snprintf(name, IFNAMSIZ, "%s%d", drv->cdevsw.d_name, unit);
 
 	/* find any existing device, or allocate new unit number */
+	dev = NULL;
 	i = clone_create(&drv->clones, &drv->cdevsw, &unit, &dev, 0);
-	if (i) {
-		/* No preexisting struct cdev *, create one */
-		dev = make_dev(&drv->cdevsw, unit, UID_UUCP, GID_DIALER, 0600,
-		    "%s%d", drv->cdevsw.d_name, unit);
-	}
+	/* No preexisting struct cdev *, create one */
+	if (i != 0)
+		i = tun_create_device(drv, unit, NULL, &dev, name);
+	if (i == 0)
+		tuncreate(dev);
 
-	tuncreate(dev, drv);
-
-	return (0);
+	return (i);
 }
 
 static void
@@ -608,12 +609,11 @@ tunclone(void *arg, struct ucred *cred, char *name, in
 			    name, u);
 			name = devname;
 		}
-		/* No preexisting struct cdev *, create one */
-		*dev = make_dev_credf(MAKEDEV_REF, &drv->cdevsw, u, cred,
-		    UID_UUCP, GID_DIALER, 0600, "%s", name);
-	}
 
-	if_clone_create(name, namelen, NULL);
+		i = tun_create_device(drv, u, cred, dev, name);
+	}
+	if (i == 0)
+		if_clone_create(name, namelen, NULL);
 out:
 	CURVNET_RESTORE();
 }
@@ -791,6 +791,46 @@ MODULE_VERSION(if_tuntap, 1);
 MODULE_VERSION(if_tun, 1);
 MODULE_VERSION(if_tap, 1);
 
+static int
+tun_create_device(struct tuntap_driver *drv, int unit, struct ucred *cr,
+    struct cdev **dev, const char *name)
+{
+	struct make_dev_args args;
+	struct tuntap_softc *tp;
+	int error;
+
+	tp = malloc(sizeof(*tp), M_TUN, M_WAITOK | M_ZERO);
+	mtx_init(&tp->tun_mtx, "tun_mtx", NULL, MTX_DEF);
+	cv_init(&tp->tun_cv, "tun_condvar");
+	tp->tun_flags = drv->ident_flags;
+	tp->tun_drv = drv;
+
+	make_dev_args_init(&args);
+	if (cr != NULL)
+		args.mda_flags = MAKEDEV_REF;
+	args.mda_devsw = &drv->cdevsw;
+	args.mda_cr = cr;
+	args.mda_uid = UID_UUCP;
+	args.mda_gid = GID_DIALER;
+	args.mda_mode = 0600;
+	args.mda_unit = unit;
+	args.mda_si_drv1 = tp;
+	error = make_dev_s(&args, dev, "%s", name);
+	if (error != 0) {
+		free(tp, M_TUN);
+		return (error);
+	}
+
+	KASSERT((*dev)->si_drv1 != NULL,
+	    ("Failed to set si_drv1 at %s creation", name));
+	tp->tun_dev = *dev;
+	knlist_init_mtx(&tp->tun_rsel.si_note, &tp->tun_mtx);
+	mtx_lock(&tunmtx);
+	TAILQ_INSERT_TAIL(&tunhead, tp, tun_list);
+	mtx_unlock(&tunmtx);
+	return (0);
+}
+
 static void
 tunstart(struct ifnet *ifp)
 {
@@ -883,49 +923,43 @@ tunstart_l2(struct ifnet *ifp)
 	TUN_UNLOCK(tp);
 } /* tunstart_l2 */
 
-
 /* XXX: should return an error code so it can fail. */
 static void
-tuncreate(struct cdev *dev, struct tuntap_driver *drv)
+tuncreate(struct cdev *dev)
 {
-	struct tuntap_softc *sc;
+	struct tuntap_driver *drv;
+	struct tuntap_softc *tp;
 	struct ifnet *ifp;
 	struct ether_addr eaddr;
 	int iflags;
 	u_char type;
 
-	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");
-	sc->tun_flags = drv->ident_flags;
-	sc->tun_dev = dev;
-	sc->tun_drv = drv;
-	mtx_lock(&tunmtx);
-	TAILQ_INSERT_TAIL(&tunhead, sc, tun_list);
-	mtx_unlock(&tunmtx);
+	tp = dev->si_drv1;
+	KASSERT(tp != NULL,
+	    ("si_drv1 should have been initialized at creation"));
 
+	drv = tp->tun_drv;
 	iflags = IFF_MULTICAST;
-	if ((sc->tun_flags & TUN_L2) != 0) {
+	if ((tp->tun_flags & TUN_L2) != 0) {
 		type = IFT_ETHER;
 		iflags |= IFF_BROADCAST | IFF_SIMPLEX;
 	} else {
 		type = IFT_PPP;
 		iflags |= IFF_POINTOPOINT;
 	}
-	ifp = sc->tun_ifp = if_alloc(type);
+	ifp = tp->tun_ifp = if_alloc(type);
 	if (ifp == NULL)
 		panic("%s%d: failed to if_alloc() interface.\n",
 		    drv->cdevsw.d_name, dev2unit(dev));
-	ifp->if_softc = sc;
+	ifp->if_softc = tp;
 	if_initname(ifp, drv->cdevsw.d_name, dev2unit(dev));
 	ifp->if_ioctl = tunifioctl;
 	ifp->if_flags = iflags;
 	IFQ_SET_MAXLEN(&ifp->if_snd, ifqmaxlen);
-	knlist_init_mtx(&sc->tun_rsel.si_note, &sc->tun_mtx);
 	ifp->if_capabilities |= IFCAP_LINKSTATE;
 	ifp->if_capenable |= IFCAP_LINKSTATE;
 
-	if ((sc->tun_flags & TUN_L2) != 0) {
+	if ((tp->tun_flags & TUN_L2) != 0) {
 		ifp->if_mtu = ETHERMTU;
 		ifp->if_init = tunifinit;
 		ifp->if_start = tunstart_l2;
@@ -943,11 +977,10 @@ tuncreate(struct cdev *dev, struct tuntap_driver *drv)
 		if_attach(ifp);
 		bpfattach(ifp, DLT_NULL, sizeof(u_int32_t));
 	}
-	dev->si_drv1 = sc;
 
-	TUN_LOCK(sc);
-	sc->tun_flags |= TUN_INITED;
-	TUN_UNLOCK(sc);
+	TUN_LOCK(tp);
+	tp->tun_flags |= TUN_INITED;
+	TUN_UNLOCK(tp);
 
 	TUNDEBUG(ifp, "interface %s is created, minor = %#x\n",
 	    ifp->if_xname, dev2unit(dev));
@@ -1008,7 +1041,6 @@ static int
 tunopen(struct cdev *dev, int flag, int mode, struct thread *td)
 {
 	struct ifnet	*ifp;
-	struct tuntap_driver *drv;
 	struct tuntap_softc *tp;
 	int error, tunflags;
 
@@ -1031,22 +1063,16 @@ tunopen(struct cdev *dev, int flag, int mode, struct t
 		}
 	}
 
-	/*
-	 * XXXRW: Non-atomic test and set of dev->si_drv1 requires
-	 * synchronization.
-	 */
 	tp = dev->si_drv1;
-	if (!tp) {
-		drv = tuntap_driver_from_flags(tunflags);
-		if (drv == NULL) {
-			CURVNET_RESTORE();
-			return (ENXIO);
-		}
-		tuncreate(dev, drv);
-		tp = dev->si_drv1;
-	}
+	KASSERT(tp != NULL,
+	    ("si_drv1 should have been initialized at creation"));
 
 	TUN_LOCK(tp);
+	if ((tp->tun_flags & TUN_INITED) == 0) {
+		TUN_UNLOCK(tp);
+		CURVNET_RESTORE();
+		return (ENXIO);
+	}
 	if ((tp->tun_flags & (TUN_OPEN | TUN_DYING)) != 0) {
 		TUN_UNLOCK(tp);
 		CURVNET_RESTORE();

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201910202239.x9KMdeGT075555>