From owner-freebsd-bugs@FreeBSD.ORG Mon Nov 6 22:00:32 2006 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 002F816A407 for ; Mon, 6 Nov 2006 22:00:31 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 4A32D43D58 for ; Mon, 6 Nov 2006 22:00:31 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id kA6M0Ve5084212 for ; Mon, 6 Nov 2006 22:00:31 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id kA6M0V9J084211; Mon, 6 Nov 2006 22:00:31 GMT (envelope-from gnats) Resent-Date: Mon, 6 Nov 2006 22:00:31 GMT Resent-Message-Id: <200611062200.kA6M0V9J084211@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Landon Fuller Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 83B4416A40F for ; Mon, 6 Nov 2006 21:51:21 +0000 (UTC) (envelope-from landonf@smtp.earth.threerings.net) Received: from smtp.earth.threerings.net (mail.threerings.net [64.127.109.101]) by mx1.FreeBSD.org (Postfix) with ESMTP id 0A1E143D8B for ; Mon, 6 Nov 2006 21:51:17 +0000 (GMT) (envelope-from landonf@smtp.earth.threerings.net) Received: by smtp.earth.threerings.net (Postfix, from userid 1001) id D152566FB; Mon, 6 Nov 2006 13:51:17 -0800 (PST) Message-Id: <20061106215117.D152566FB@smtp.earth.threerings.net> Date: Mon, 6 Nov 2006 13:51:17 -0800 (PST) From: Landon Fuller To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Subject: kern/105228: [PATCH] if_clone support for tun(4) and tap(4) X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Nov 2006 22:00:32 -0000 >Number: 105228 >Category: kern >Synopsis: [PATCH] if_clone support for tun(4) and tap(4) >Confidential: no >Severity: non-critical >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Mon Nov 06 22:00:30 GMT 2006 >Closed-Date: >Last-Modified: >Originator: Landon Fuller >Release: FreeBSD 7.0-CURRENT i386 >Organization: Three Rings Design, Inc. >Environment: System: FreeBSD freebsd-current.localdomain 7.0-CURRENT FreeBSD 7.0-CURRENT #3: Thu Oct 26 15:45:10 PDT 2006 root@freebsd-current.localdomain:/usr/obj/usr/src/sys/GENERIC i386 >Description: The attached patch (patch-tuntap_ifclone) adds interface cloning support to the tun(4) and tap(4) drivers. I maintained backwards-compatible support for devfs cloning, which is now disabled by default and must be enabled via a sysctl. Tun/tap interfaces that are created via devfs cloning may still be removed via ifconfig destroy. Additionally, I stumbled across a bug in the clone_create() function triggered by my changes -- due to an (unanticipated) unsigned integer comparison, devices are added out of order. GCC promotes the comparison to unsigned due to ORing one of the operands with an unsigned value: if (u < (unit | extra)) The fix is to simply cast the result as signed. A patch (patch-kern_conf.c) is also attached containing this fix. >How-To-Repeat: To test: ifconfig tun0 create; ifconfig tun0 destroy >Fix: See attached patches. --- patch-tuntap_ifclone begins here --- --- sys/net/if_tap.c Wed Sep 27 12:57:01 2006 +++ sys/net/if_tap.c Fri Nov 3 15:33:32 2006 @@ -61,6 +61,7 @@ #include #include #include +#include #include #include #include @@ -76,8 +77,8 @@ #define TAP "tap" #define VMNET "vmnet" -#define TAPMAXUNIT 0x7fff #define VMNET_DEV_MASK CLONE_FLAG0 +#define TAPMAXUNIT 0x7fff /* module */ static int tapmodevent(module_t, int, void *); @@ -85,13 +86,21 @@ /* device */ static void tapclone(void *, struct ucred *, char *, int, struct cdev **); -static void tapcreate(struct cdev *); +static void tapcreate(struct cdev *, int); /* network interface */ static void tapifstart(struct ifnet *); static int tapifioctl(struct ifnet *, u_long, caddr_t); static void tapifinit(void *); +static int tap_clone_create(struct if_clone *, int, caddr_t); +static void tap_clone_destroy(struct ifnet *); +static int vmnet_clone_create(struct if_clone *, int, caddr_t); +static void vmnet_clone_destroy(struct ifnet *); + +IFC_SIMPLE_DECLARE(tap, 0); +IFC_SIMPLE_DECLARE(vmnet, 0); + /* character device */ static d_open_t tapopen; static d_close_t tapclose; @@ -141,6 +150,7 @@ static struct mtx tapmtx; static int tapdebug = 0; /* debug flag */ static int tapuopen = 0; /* allow user open() */ +static int tapdclone = 0; /* enable devfs cloning support */ static SLIST_HEAD(, tap_softc) taphead; /* first device */ static struct clonedevs *tapclones; @@ -153,10 +163,93 @@ "Ethernet tunnel software network interface"); SYSCTL_INT(_net_link_tap, OID_AUTO, user_open, CTLFLAG_RW, &tapuopen, 0, "Allow user to open /dev/tap (based on node permissions)"); +SYSCTL_INT(_net_link_tap, OID_AUTO, devfs_cloning, CTLFLAG_RW, &tapdclone, 0, + "Enably legacy devfs interface creation"); SYSCTL_INT(_net_link_tap, OID_AUTO, debug, CTLFLAG_RW, &tapdebug, 0, ""); DEV_MODULE(if_tap, tapmodevent, NULL); +static int +tap_clone_create(struct if_clone *ifc, int unit, caddr_t params) +{ + struct cdev *dev; + int i; + u_int extra; + + if (strcmp(ifc->ifc_name, VMNET) == 0) + extra = VMNET_DEV_MASK; + else + extra = 0; + + /* find any existing device, or allocate new unit number */ + i = clone_create(&tapclones, &tap_cdevsw, &unit, &dev, extra); + if (i) { + dev = make_dev(&tap_cdevsw, unit2minor(unit | extra), + UID_ROOT, GID_WHEEL, 0600, "%s%d", ifc->ifc_name, unit); + if (dev != NULL) { + dev_ref(dev); + dev->si_flags |= SI_CHEAPCLONE; + } + } + + if (extra == VMNET_DEV_MASK) { + /* vmnet device */ + tapcreate(dev, 1); + } else { + /* tap device */ + tapcreate(dev, 0); + } + + return (0); +} + +/* vmnet devices are tap devices in disguise */ +static int +vmnet_clone_create(struct if_clone *ifc, int unit, caddr_t params) +{ + return tap_clone_create(ifc, unit, params); +} + +static void +tap_destroy(struct tap_softc *tp) +{ + struct ifnet *ifp = tp->tap_ifp; + int s; + + /* Unlocked read. */ + KASSERT(!(tp->tap_flags & TAP_OPEN), + ("%s flags is out of sync", ifp->if_xname)); + + knlist_destroy(&tp->tap_rsel.si_note); + destroy_dev(tp->tap_dev); + s = splimp(); + ether_ifdetach(ifp); + if_free_type(ifp, IFT_ETHER); + splx(s); + + mtx_destroy(&tp->tap_mtx); + free(tp, M_TAP); +} + +static void +tap_clone_destroy(struct ifnet *ifp) +{ + struct tap_softc *tp = ifp->if_softc; + + mtx_lock(&tapmtx); + SLIST_REMOVE(&taphead, tp, tap_softc, tap_next); + mtx_unlock(&tapmtx); + tap_destroy(tp); +} + +/* vmnet devices are tap devices in disguise */ +static void +vmnet_clone_destroy(struct ifnet *ifp) +{ + tap_clone_destroy(ifp); +} + + /* * tapmodevent * @@ -168,7 +261,6 @@ static eventhandler_tag eh_tag = NULL; struct tap_softc *tp = NULL; struct ifnet *ifp = NULL; - int s; switch (type) { case MOD_LOAD: @@ -185,6 +277,8 @@ mtx_destroy(&tapmtx); return (ENOMEM); } + if_clone_attach(&tap_cloner); + if_clone_attach(&vmnet_cloner); return (0); case MOD_UNLOAD: @@ -193,6 +287,8 @@ * guarantee that this is race-free since we have to * release the tap mtx to deregister the clone handler. */ + if_clone_detach(&tap_cloner); + if_clone_detach(&vmnet_cloner); mtx_lock(&tapmtx); SLIST_FOREACH(tp, &taphead, tap_next) { mtx_lock(&tp->tap_mtx); @@ -211,24 +307,10 @@ while ((tp = SLIST_FIRST(&taphead)) != NULL) { SLIST_REMOVE_HEAD(&taphead, tap_next); mtx_unlock(&tapmtx); - ifp = tp->tap_ifp; TAPDEBUG("detaching %s\n", ifp->if_xname); - - /* Unlocked read. */ - KASSERT(!(tp->tap_flags & TAP_OPEN), - ("%s flags is out of sync", ifp->if_xname)); - - knlist_destroy(&tp->tap_rsel.si_note); - destroy_dev(tp->tap_dev); - s = splimp(); - ether_ifdetach(ifp); - if_free_type(ifp, IFT_ETHER); - splx(s); - - mtx_destroy(&tp->tap_mtx); - free(tp, M_TAP); + tap_destroy(tp); mtx_lock(&tapmtx); } mtx_unlock(&tapmtx); @@ -254,38 +336,63 @@ static void tapclone(void *arg, struct ucred *cred, char *name, int namelen, struct cdev **dev) { + char devname[SPECNAMELEN + 1]; + int i, unit, append_unit; u_int extra; - int i, unit; - char *device_name = name; if (*dev != NULL) return; - device_name = TAP; + /* + * If tap cloning is enabled, only the superuser can create + * an interface. + */ + if (!tapdclone || suser_cred(cred, 0) != 0) + return; + + unit = 0; + append_unit = 0; extra = 0; + + /* We're interested in only tap/vmnet devices. */ if (strcmp(name, TAP) == 0) { unit = -1; } else if (strcmp(name, VMNET) == 0) { - device_name = VMNET; - extra = VMNET_DEV_MASK; unit = -1; - } else if (dev_stdclone(name, NULL, device_name, &unit) != 1) { - device_name = VMNET; extra = VMNET_DEV_MASK; - if (dev_stdclone(name, NULL, device_name, &unit) != 1) + } else if (dev_stdclone(name, NULL, TAP, &unit) != 1) { + if (dev_stdclone(name, NULL, VMNET, &unit) != 1) { return; + } else { + extra = VMNET_DEV_MASK; + } } + if (unit == -1) + append_unit = 1; + /* find any existing device, or allocate new unit number */ i = clone_create(&tapclones, &tap_cdevsw, &unit, dev, extra); if (i) { + if (append_unit) { + /* + * We were passed 'tun' or 'tap', with no unit specified + * so we'll need to append it now. + */ + namelen = snprintf(devname, sizeof(devname), "%s%d", name, + unit); + name = devname; + } + *dev = make_dev(&tap_cdevsw, unit2minor(unit | extra), - UID_ROOT, GID_WHEEL, 0600, "%s%d", device_name, unit); + UID_ROOT, GID_WHEEL, 0600, "%s", name); if (*dev != NULL) { dev_ref(*dev); (*dev)->si_flags |= SI_CHEAPCLONE; } } + + if_clone_create(name, namelen, NULL); } /* tapclone */ @@ -295,7 +402,7 @@ * to create interface */ static void -tapcreate(struct cdev *dev) +tapcreate(struct cdev *dev, int vmnet) { struct ifnet *ifp = NULL; struct tap_softc *tp = NULL; @@ -316,7 +423,7 @@ unit = dev2unit(dev); /* select device: tap or vmnet */ - if (unit & VMNET_DEV_MASK) { + if (vmnet) { name = VMNET; tp->tap_flags |= TAP_VMNET; } else @@ -381,16 +488,7 @@ if ((dev2unit(dev) & CLONE_UNITMASK) > TAPMAXUNIT) return (ENXIO); - /* - * XXXRW: Non-atomic test-and-set of si_drv1. Currently protected - * by Giant, but the race actually exists under memory pressure as - * well even when running with Giant, as malloc() may sleep. - */ tp = dev->si_drv1; - if (tp == NULL) { - tapcreate(dev); - tp = dev->si_drv1; - } mtx_lock(&tp->tap_mtx); if (tp->tap_flags & TAP_OPEN) { --- sys/net/if_tun.c Thu Oct 26 13:03:10 2006 +++ sys/net/if_tun.c Fri Nov 3 17:00:52 2006 @@ -13,7 +13,7 @@ * UCL. This driver is based much more on read/write/poll mode of * operation though. * - * $FreeBSD: src/sys/net/if_tun.c,v 1.159 2006/10/22 11:52:15 rwatson Exp $ + * $FreeBSD: src/sys/net/if_tun.c,v 1.158 2006/08/08 19:22:25 rwatson Exp $ */ #include "opt_atalk.h" @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,7 @@ #include #include +#include #include #include #include @@ -55,8 +57,6 @@ #include -#include - /* * tun_list is protected by global tunmtx. Other mutable fields are * protected by tun->tun_mtx, or by their owning subsystem. tun_dev is @@ -104,13 +104,20 @@ static struct mtx tunmtx; static MALLOC_DEFINE(M_TUN, TUNNAME, "Tunnel Interface"); static int tundebug = 0; +static int tundclone = 0; 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, ""); +SYSCTL_DECL(_net_link); +SYSCTL_NODE(_net_link, OID_AUTO, tun, CTLFLAG_RW, 0, + "IP tunnel software network interface."); +SYSCTL_INT(_net_link_tun, OID_AUTO, devfs_cloning, CTLFLAG_RW, &tundclone, 0, + "Enable legacy devfs interface creation."); + static void tunclone(void *arg, struct ucred *cred, char *name, int namelen, struct cdev **dev); -static void tuncreate(struct cdev *dev); +static void tuncreate(const char *name, struct cdev *dev); static int tunifioctl(struct ifnet *, u_long, caddr_t); static int tuninit(struct ifnet *); static int tunmodevent(module_t, int, void *); @@ -118,6 +125,11 @@ struct rtentry *rt); static void tunstart(struct ifnet *); +static int tun_clone_create(struct if_clone *, int, caddr_t); +static void tun_clone_destroy(struct ifnet *); + +IFC_SIMPLE_DECLARE(tun, 0); + static d_open_t tunopen; static d_close_t tunclose; static d_read_t tunread; @@ -157,15 +169,45 @@ .d_name = TUNNAME, }; +static int +tun_clone_create(struct if_clone *ifc, int unit, caddr_t params) +{ + struct cdev *dev; + int i; + + /* find any existing device, or allocate new unit number */ + i = clone_create(&tunclones, &tun_cdevsw, &unit, &dev, 0); + if (i) { + /* No preexisting struct cdev *, create one */ + dev = make_dev(&tun_cdevsw, unit2minor(unit), + UID_UUCP, GID_DIALER, 0600, "%s%d", ifc->ifc_name, unit); + if (dev != NULL) { + dev_ref(dev); + dev->si_flags |= SI_CHEAPCLONE; + } + } + tuncreate(ifc->ifc_name, dev); + + return (0); +} + static void tunclone(void *arg, struct ucred *cred, char *name, int namelen, struct cdev **dev) { - int u, i; + char devname[SPECNAMELEN + 1]; + int u, i, append_unit; if (*dev != NULL) return; + /* + * If tun cloning is enabled, only the superuser can create an + * interface. + */ + if (!tundclone || suser_cred(cred, 0) != 0) + return; + if (strcmp(name, TUNNAME) == 0) { u = -1; } else if (dev_stdclone(name, NULL, TUNNAME, &u) != 1) @@ -173,17 +215,29 @@ if (u != -1 && u > IF_MAXUNIT) return; /* Unit number too high */ + if (u == -1) + append_unit = 1; + else + append_unit = 0; + /* find any existing device, or allocate new unit number */ i = clone_create(&tunclones, &tun_cdevsw, &u, dev, 0); if (i) { + if (append_unit) { + namelen = snprintf(devname, sizeof(devname), "%s%d", name, + u); + name = devname; + } /* No preexisting struct cdev *, create one */ *dev = make_dev(&tun_cdevsw, unit2minor(u), - UID_UUCP, GID_DIALER, 0600, "tun%d", u); + UID_UUCP, GID_DIALER, 0600, "%s", name); if (*dev != NULL) { dev_ref(*dev); (*dev)->si_flags |= SI_CHEAPCLONE; } } + + if_clone_create(name, namelen, NULL); } static void @@ -205,6 +259,17 @@ free(tp, M_TUN); } +static void +tun_clone_destroy(struct ifnet *ifp) +{ + struct tun_softc *tp = ifp->if_softc; + + mtx_lock(&tunmtx); + TAILQ_REMOVE(&tunhead, tp, tun_list); + mtx_unlock(&tunmtx); + tun_destroy(tp); +} + static int tunmodevent(module_t mod, int type, void *data) { @@ -218,8 +283,10 @@ tag = EVENTHANDLER_REGISTER(dev_clone, tunclone, 0, 1000); if (tag == NULL) return (ENOMEM); + if_clone_attach(&tun_cloner); break; case MOD_UNLOAD: + if_clone_detach(&tun_cloner); EVENTHANDLER_DEREGISTER(dev_clone, tag); mtx_lock(&tunmtx); @@ -280,7 +347,7 @@ /* XXX: should return an error code so it can fail. */ static void -tuncreate(struct cdev *dev) +tuncreate(const char *name, struct cdev *dev) { struct tun_softc *sc; struct ifnet *ifp; @@ -299,7 +366,7 @@ if (ifp == NULL) panic("%s%d: failed to if_alloc() interface.\n", TUNNAME, dev2unit(dev)); - if_initname(ifp, TUNNAME, dev2unit(dev)); + if_initname(ifp, name, dev2unit(dev)); ifp->if_mtu = TUNMTU; ifp->if_ioctl = tunifioctl; ifp->if_output = tunoutput; @@ -330,7 +397,7 @@ */ tp = dev->si_drv1; if (!tp) { - tuncreate(dev); + tuncreate(TUNNAME, dev); tp = dev->si_drv1; } --- patch-tuntap_ifclone ends here --- --- patch-kern_conf.c begins here --- --- sys/kern/kern_conf.c Fri Oct 27 08:34:47 2006 +++ sys/kern/kern_conf.c Fri Nov 3 03:27:06 2006 @@ -847,10 +847,10 @@ low++; de = dev; continue; - } else if (u < (unit | extra)) { + } else if (u < (int) (unit | extra)) { de = dev; continue; - } else if (u > (unit | extra)) { + } else if (u > (int) (unit | extra)) { dl = dev; break; } --- patch-kern_conf.c ends here --- >Release-Note: >Audit-Trail: >Unformatted: