From owner-svn-src-head@freebsd.org Sun Oct 20 22:39:41 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 14C6016CF47; Sun, 20 Oct 2019 22:39:41 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46xF7w6pxmz4SFC; Sun, 20 Oct 2019 22:39:40 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id CD0A71AB29; Sun, 20 Oct 2019 22:39:40 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id x9KMdeof075556; Sun, 20 Oct 2019 22:39:40 GMT (envelope-from kevans@FreeBSD.org) Received: (from kevans@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id x9KMdeGT075555; Sun, 20 Oct 2019 22:39:40 GMT (envelope-from kevans@FreeBSD.org) Message-Id: <201910202239.x9KMdeGT075555@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kevans set sender to kevans@FreeBSD.org using -f From: Kyle Evans Date: Sun, 20 Oct 2019 22:39:40 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r353785 - head/sys/net X-SVN-Group: head X-SVN-Commit-Author: kevans X-SVN-Commit-Paths: head/sys/net X-SVN-Commit-Revision: 353785 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 20 Oct 2019 22:39:41 -0000 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();