From owner-freebsd-hackers@FreeBSD.ORG Fri Jun 3 12:04:25 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 751DD1065690; Fri, 3 Jun 2011 12:04:25 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 349F28FC16; Fri, 3 Jun 2011 12:04:25 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id C849546B2C; Fri, 3 Jun 2011 08:04:24 -0400 (EDT) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 657208A02A; Fri, 3 Jun 2011 08:04:24 -0400 (EDT) From: John Baldwin To: freebsd-hackers@freebsd.org Date: Fri, 3 Jun 2011 08:04:22 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110325; KDE/4.5.5; amd64; ; ) References: <4DE7B935.9040004@aon.at> In-Reply-To: <4DE7B935.9040004@aon.at> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201106030804.23084.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Fri, 03 Jun 2011 08:04:24 -0400 (EDT) Cc: glebius@freebsd.org, Martin Birgmeier Subject: Re: some strange constructs (bugs?) in if_tun.c X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jun 2011 12:04:25 -0000 On Thursday, June 02, 2011 12:24:21 pm Martin Birgmeier wrote: > I am looking at net/if_tun.c, function tunwrite() (this is 7.4, but 8.2 > is nearly the same): > > There is a local variable "error" which is initialized to zero and then > seemingly never changed, until it is used as a return value if > m_uiotombuf() fails: > > ... > int error = 0; > ... > if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) { > ifp->if_ierrors++; > return (error); > } > ... > a little further down, we see > ... > if (m->m_len < sizeof(family) && > (m = m_pullup(m, sizeof(family))) == NULL) > return (ENOBUFS); > ... > > As far as I can see, the first return amounts to "drop the packet, but > don't tell anything about it", whereas the second amounts to "drop the > packet and say it's due to ENOBUFS". > > However, the first case is much more like ENOBUFS, so shouldn't we > simply say "return (ENOBUFS)" there and remove the "error" variable > altogether? Yes, this error seems to have been introduced in 137101 when if_tun was switched to use m_uiotombuf() rather than a home-rolled version. tap(4) had the same bug, but it was fixed in 163986. I think this patch should be ok for tun(4): Index: if_tun.c =================================================================== --- if_tun.c (revision 222565) +++ if_tun.c (working copy) @@ -126,7 +126,7 @@ static void tunclone(void *arg, struct ucred *cred int namelen, 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 void tuninit(struct ifnet *); static int tunmodevent(module_t, int, void *); static int tunoutput(struct ifnet *, struct mbuf *, struct sockaddr *, struct route *ro); @@ -494,14 +494,13 @@ tunclose(struct cdev *dev, int foo, int bar, struc return (0); } -static int +static void tuninit(struct ifnet *ifp) { struct tun_softc *tp = ifp->if_softc; #ifdef INET struct ifaddr *ifa; #endif - int error = 0; TUNDEBUG(ifp, "tuninit\n"); @@ -528,7 +527,6 @@ tuninit(struct ifnet *ifp) if_addr_runlock(ifp); #endif mtx_unlock(&tp->tun_mtx); - return (error); } /* @@ -552,12 +550,12 @@ tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t mtx_unlock(&tp->tun_mtx); break; case SIOCSIFADDR: - error = tuninit(ifp); - TUNDEBUG(ifp, "address set, error=%d\n", error); + tuninit(ifp); + TUNDEBUG(ifp, "address set\n"); break; case SIOCSIFDSTADDR: - error = tuninit(ifp); - TUNDEBUG(ifp, "destination address set, error=%d\n", error); + tuninit(ifp); + TUNDEBUG(ifp, "destination address set\n"); break; case SIOCSIFMTU: ifp->if_mtu = ifr->ifr_mtu; @@ -857,7 +855,6 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl struct tun_softc *tp = dev->si_drv1; struct ifnet *ifp = TUN2IFP(tp); struct mbuf *m; - int error = 0; uint32_t family; int isr; @@ -877,7 +874,7 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) { ifp->if_ierrors++; - return (error); + return (ENOBUFS); } m->m_pkthdr.rcvif = ifp; -- John Baldwin