some strange constructs (bugs?) in if_tun.c

[Gleb Smirnoff]

On Fri, Jun 03, 2011 at 08:04:22AM -0400, John Baldwin wrote:
J> On Thursday, June 02, 2011 12:24:21 pm Martin Birgmeier wrote:
J> > I am looking at net/if_tun.c, function tunwrite() (this is 7.4, but 8.2 
J> > is nearly the same):
J> > 
J> > There is a local variable "error" which is initialized to zero and then 
J> > seemingly never changed, until it is used as a return value if 
J> > m_uiotombuf() fails:
J> > 
J> > ...
J> >          int             error = 0;
J> > ...
J> >          if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) {
J> >                  ifp->if_ierrors++;
J> >                  return (error);
J> >          }
J> > ...
J> > a little further down, we see
J> > ...
J> >                  if (m->m_len < sizeof(family) &&
J> >                      (m = m_pullup(m, sizeof(family))) == NULL)
J> >                          return (ENOBUFS);
J> > ...
J> > 
J> > As far as I can see, the first return amounts to "drop the packet, but 
J> > don't tell anything about it", whereas the second amounts to "drop the 
J> > packet and say it's due to ENOBUFS".
J> > 
J> > However, the first case is much more like ENOBUFS, so shouldn't we 
J> > simply say "return (ENOBUFS)" there and remove the "error" variable 
J> > altogether?
J> 
J> Yes, this error seems to have been introduced in 137101 when if_tun was 
J> switched to use m_uiotombuf() rather than a home-rolled version.  tap(4) had 
J> the same bug, but it was fixed in 163986.  I think this patch should be ok for 
J> tun(4):
J> 
J> Index: if_tun.c
J> ===================================================================
J> --- if_tun.c	(revision 222565)
J> +++ if_tun.c	(working copy)
J> @@ -126,7 +126,7 @@ static void	tunclone(void *arg, struct ucred *cred
J>  		    int namelen, struct cdev **dev);
J>  static void	tuncreate(const char *name, struct cdev *dev);
J>  static int	tunifioctl(struct ifnet *, u_long, caddr_t);
J> -static int	tuninit(struct ifnet *);
J> +static void	tuninit(struct ifnet *);
J>  static int	tunmodevent(module_t, int, void *);
J>  static int	tunoutput(struct ifnet *, struct mbuf *, struct sockaddr *,
J>  		    struct route *ro);
J> @@ -494,14 +494,13 @@ tunclose(struct cdev *dev, int foo, int bar, struc
J>  	return (0);
J>  }
J>  
J> -static int
J> +static void
J>  tuninit(struct ifnet *ifp)
J>  {
J>  	struct tun_softc *tp = ifp->if_softc;
J>  #ifdef INET
J>  	struct ifaddr *ifa;
J>  #endif
J> -	int error = 0;
J>  
J>  	TUNDEBUG(ifp, "tuninit\n");
J>  
J> @@ -528,7 +527,6 @@ tuninit(struct ifnet *ifp)
J>  	if_addr_runlock(ifp);
J>  #endif
J>  	mtx_unlock(&tp->tun_mtx);
J> -	return (error);
J>  }
J>  
J>  /*
J> @@ -552,12 +550,12 @@ tunifioctl(struct ifnet *ifp, u_long cmd, caddr_t
J>  		mtx_unlock(&tp->tun_mtx);
J>  		break;
J>  	case SIOCSIFADDR:
J> -		error = tuninit(ifp);
J> -		TUNDEBUG(ifp, "address set, error=%d\n", error);
J> +		tuninit(ifp);
J> +		TUNDEBUG(ifp, "address set\n");
J>  		break;
J>  	case SIOCSIFDSTADDR:
J> -		error = tuninit(ifp);
J> -		TUNDEBUG(ifp, "destination address set, error=%d\n", error);
J> +		tuninit(ifp);
J> +		TUNDEBUG(ifp, "destination address set\n");
J>  		break;
J>  	case SIOCSIFMTU:
J>  		ifp->if_mtu = ifr->ifr_mtu;
J> @@ -857,7 +855,6 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl
J>  	struct tun_softc *tp = dev->si_drv1;
J>  	struct ifnet	*ifp = TUN2IFP(tp);
J>  	struct mbuf	*m;
J> -	int		error = 0;
J>  	uint32_t	family;
J>  	int 		isr;
J>  
J> @@ -877,7 +874,7 @@ tunwrite(struct cdev *dev, struct uio *uio, int fl
J>  
J>  	if ((m = m_uiotombuf(uio, M_DONTWAIT, 0, 0, M_PKTHDR)) == NULL) {
J>  		ifp->if_ierrors++;
J> -		return (error);
J> +		return (ENOBUFS);
J>  	}
J>  
J>  	m->m_pkthdr.rcvif = ifp;

Yes, that would be correct. Sorry, my bug :(

-- 
Totus tuus, Glebius.