Date: Fri, 3 Jun 2011 08:04:22 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-hackers@freebsd.org Cc: glebius@freebsd.org, Martin Birgmeier <la5lbtyi@aon.at> Subject: Re: some strange constructs (bugs?) in if_tun.c Message-ID: <201106030804.23084.jhb@freebsd.org> In-Reply-To: <4DE7B935.9040004@aon.at> References: <4DE7B935.9040004@aon.at>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201106030804.23084.jhb>