some strange constructs (bugs?) in if_tun.c

[John Baldwin]

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