From: mpi <mpi@openbsd.org>
Date: Thu, 8 Jan 2015 14:29:18 +0000 (+0000)
Subject: Factorize various duplicated chunks of (old and horrible) code, checking
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=8c023157f568c27d8c9df36edc287db0b4676746;p=openbsd

Factorize various duplicated chunks of (old and horrible) code, checking
for the validity of a given outgoing route entry into a single function.

This change is inspired from FreeBSD r111767.  The function introduced
here, rt_checkgate(), should hopefully die in a near future.  Why should
it die?  Well, it is way too late to do such validity checks: if your
kernel can ends up in ether_output() with an invalid route, please do
not let it try to find a new one that might do the job.

Go read this function if you're wondering why you're getting messages
like:

"arpresolve: X.X.X.X: route without link local address"

Since this horrible logic has survived 20 years of copy & past and small
modifications for workarounds without a single clear commit message, let's
assume it is full of dragons and try to play safe.  This factorization is
not intended to change any behavior.

With much inputs from bluhm@, tested by weerd@ and florian@ on setups
with p2p IPv6 interfaces.

ok bluhm@, benno@, florian@
---

diff --git a/sys/net/if_ethersubr.c b/sys/net/if_ethersubr.c
index 27193378ab8..d9d5da16ffe 100644
--- a/sys/net/if_ethersubr.c
+++ b/sys/net/if_ethersubr.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_ethersubr.c,v 1.184 2014/12/19 17:14:39 tedu Exp $	*/
+/*	$OpenBSD: if_ethersubr.c,v 1.185 2015/01/08 14:29:18 mpi Exp $	*/
 /*	$NetBSD: if_ethersubr.c,v 1.19 1996/05/07 02:40:30 thorpej Exp $	*/
 
 /*
@@ -238,14 +238,13 @@ ether_addheader(struct mbuf **m, struct ifnet *ifp, u_int16_t etype,
  */
 int
 ether_output(struct ifnet *ifp0, struct mbuf *m0, struct sockaddr *dst,
-    struct rtentry *rt0)
+    struct rtentry *rt)
 {
 	u_int16_t etype;
 	int s, len, error = 0;
 	u_char edst[ETHER_ADDR_LEN];
 	u_char *esrc;
 	struct mbuf *m = m0;
-	struct rtentry *rt;
 	struct mbuf *mcopy = NULL;
 	struct ether_header *eh;
 	struct arpcom *ac = (struct arpcom *)ifp0;
@@ -283,38 +282,12 @@ ether_output(struct ifnet *ifp0, struct mbuf *m0, struct sockaddr *dst,
 
 	if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING))
 		senderr(ENETDOWN);
-	if ((rt = rt0) != NULL) {
-		if ((rt->rt_flags & RTF_UP) == 0) {
-			if ((rt0 = rt = rtalloc(dst, RT_REPORT|RT_RESOLVE,
-			    m->m_pkthdr.ph_rtableid)) != NULL)
-				rt->rt_refcnt--;
-			else
-				senderr(EHOSTUNREACH);
-		}
 
-		if (rt->rt_flags & RTF_GATEWAY) {
-			if (rt->rt_gwroute == NULL)
-				goto lookup;
-			if (((rt = rt->rt_gwroute)->rt_flags & RTF_UP) == 0) {
-				rtfree(rt);
-				rt = rt0;
-			lookup:
-				rt->rt_gwroute = rtalloc(rt->rt_gateway,
-				    RT_REPORT|RT_RESOLVE, ifp->if_rdomain);
-				if ((rt = rt->rt_gwroute) == NULL)
-					senderr(EHOSTUNREACH);
-			}
-		}
-		if (rt->rt_flags & RTF_REJECT)
-			if (rt->rt_rmx.rmx_expire == 0 ||
-			    time_second < rt->rt_rmx.rmx_expire)
-				senderr(rt == rt0 ? EHOSTDOWN : EHOSTUNREACH);
-	}
 	switch (dst->sa_family) {
-
 	case AF_INET:
-		if (!arpresolve(ac, rt, m, dst, edst))
-			return (0);	/* if not yet resolved */
+		error = arpresolve(ac, rt, m, dst, edst);
+		if (error)
+			return (error == EAGAIN ? 0 : error);
 		/* If broadcasting on a simplex interface, loopback a copy */
 		if ((m->m_flags & M_BCAST) && (ifp->if_flags & IFF_SIMPLEX) &&
 		    !m->m_pkthdr.pf.routed)
@@ -323,8 +296,9 @@ ether_output(struct ifnet *ifp0, struct mbuf *m0, struct sockaddr *dst,
 		break;
 #ifdef INET6
 	case AF_INET6:
-		if (!nd6_storelladdr(ifp, rt, m, dst, (u_char *)edst))
-			return (0); /* it must be impossible, but... */
+		error = nd6_storelladdr(ifp, rt, m, dst, (u_char *)edst);
+		if (error)
+			return (error);
 		etype = htons(ETHERTYPE_IPV6);
 		break;
 #endif
@@ -347,8 +321,9 @@ ether_output(struct ifnet *ifp0, struct mbuf *m0, struct sockaddr *dst,
 				    sizeof(edst));
 				break;
 			case AF_INET:
-				if (!arpresolve(ac, rt, m, dst, edst))
-					return (0); /* if not yet resolved */
+				error = arpresolve(ac, rt, m, dst, edst);
+				if (error)
+					return (error == EAGAIN ? 0 : error);
 				break;
 			default:
 				senderr(EHOSTUNREACH);
diff --git a/sys/net/route.c b/sys/net/route.c
index 05f56763c50..427656a13c7 100644
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: route.c,v 1.196 2014/12/29 11:53:58 mpi Exp $	*/
+/*	$OpenBSD: route.c,v 1.197 2015/01/08 14:29:18 mpi Exp $	*/
 /*	$NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $	*/
 
 /*
@@ -1032,6 +1032,45 @@ rt_setgate(struct rtentry *rt, struct sockaddr *dst, struct sockaddr *gate,
 	return (0);
 }
 
+int
+rt_checkgate(struct ifnet *ifp, struct rtentry *rt, struct sockaddr *dst,
+    unsigned int rtableid, struct rtentry **rtp)
+{
+	struct rtentry *rt0;
+
+	KASSERT(rt != NULL);
+
+	if ((rt->rt_flags & RTF_UP) == 0) {
+		rt = rtalloc(dst, RT_REPORT|RT_RESOLVE, rtableid);
+		rt->rt_refcnt--;
+		if (rt == NULL || rt->rt_ifp != ifp)
+			return (EHOSTUNREACH);
+	}
+
+	rt0 = rt;
+
+	if (rt->rt_flags & RTF_GATEWAY) {
+		if (rt->rt_gwroute && !(rt->rt_gwroute->rt_flags & RTF_UP)) {
+			rtfree(rt->rt_gwroute);
+			rt->rt_gwroute = NULL;
+		}
+		if (rt->rt_gwroute == NULL) {
+			rt->rt_gwroute = rtalloc(rt->rt_gateway,
+			    RT_REPORT|RT_RESOLVE, rtableid);
+			if (rt->rt_gwroute == NULL)
+				return (EHOSTUNREACH);
+		}
+		rt = rt->rt_gwroute;
+	}
+
+	if (rt->rt_flags & RTF_REJECT)
+		if (rt->rt_expire == 0 || time_second < rt->rt_expire)
+			return (rt == rt0 ? EHOSTDOWN : EHOSTUNREACH);
+
+	*rtp = rt;
+	return (0);
+}
+
 void
 rt_maskedcopy(struct sockaddr *src, struct sockaddr *dst,
     struct sockaddr *netmask)
diff --git a/sys/net/route.h b/sys/net/route.h
index c48905bf4f2..6fb9659b2cf 100644
--- a/sys/net/route.h
+++ b/sys/net/route.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: route.h,v 1.101 2014/11/24 12:43:54 mpi Exp $	*/
+/*	$OpenBSD: route.h,v 1.102 2015/01/08 14:29:18 mpi Exp $	*/
 /*	$NetBSD: route.h,v 1.9 1996/02/13 22:00:49 christos Exp $	*/
 
 /*
@@ -113,7 +113,8 @@ struct rtentry {
 	u_int16_t rt_labelid;		/* route label ID */
 	u_int8_t rt_priority;		/* routing priority to use */
 };
-#define	rt_use	rt_rmx.rmx_pksent
+#define	rt_use		rt_rmx.rmx_pksent
+#define	rt_expire	rt_rmx.rmx_expire
 
 #endif /* _KERNEL */
 
@@ -360,6 +361,8 @@ void	 rt_missmsg(int, struct rt_addrinfo *, int, struct ifnet *, int,
 void	 rt_newaddrmsg(int, struct ifaddr *, int, struct rtentry *);
 int	 rt_setgate(struct rtentry *, struct sockaddr *,
 	    struct sockaddr *, u_int);
+int	 rt_checkgate(struct ifnet *, struct rtentry *, struct sockaddr *,
+	    unsigned int, struct rtentry **);
 void	 rt_setmetrics(u_long, struct rt_metrics *, struct rt_kmetrics *);
 void	 rt_getmetrics(struct rt_kmetrics *, struct rt_metrics *);
 
diff --git a/sys/netinet/if_ether.c b/sys/netinet/if_ether.c
index d49a55cc537..4f70ce698d0 100644
--- a/sys/netinet/if_ether.c
+++ b/sys/netinet/if_ether.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_ether.c,v 1.139 2014/12/19 17:14:40 tedu Exp $	*/
+/*	$OpenBSD: if_ether.c,v 1.140 2015/01/08 14:29:18 mpi Exp $	*/
 /*	$NetBSD: if_ether.c,v 1.31 1996/05/11 12:59:58 mycroft Exp $	*/
 
 /*
@@ -81,7 +81,6 @@
 int	arpt_prune = (5*60*1);	/* walk list every 5 minutes */
 int	arpt_keep = (20*60);	/* once resolved, good for 20 more minutes */
 int	arpt_down = 20;		/* once declared down, don't send for 20 secs */
-#define	rt_expire rt_rmx.rmx_expire
 
 void arptfree(struct llinfo_arp *);
 void arptimer(void *);
@@ -337,29 +336,40 @@ arprequest(struct ifnet *ifp, u_int32_t *sip, u_int32_t *tip, u_int8_t *enaddr)
  * desten is filled in.  If there is no entry in arptab,
  * set one up and broadcast a request for the IP address.
  * Hold onto this mbuf and resend it once the address
- * is finally resolved.  A return value of 1 indicates
+ * is finally resolved.  A return value of 0 indicates
  * that desten has been filled in and the packet should be sent
- * normally; a 0 return indicates that the packet has been
- * taken over here, either now or for later transmission.
+ * normally; A return value of EAGAIN indicates that the packet
+ * has been taken over here, either now or for later transmission.
+ * Any other return value indicates an error.
  */
 int
-arpresolve(struct arpcom *ac, struct rtentry *rt, struct mbuf *m,
+arpresolve(struct arpcom *ac, struct rtentry *rt0, struct mbuf *m,
     struct sockaddr *dst, u_char *desten)
 {
 	struct llinfo_arp *la;
 	struct sockaddr_dl *sdl;
+	struct rtentry *rt;
 	struct mbuf *mh;
 	char addr[INET_ADDRSTRLEN];
+	int error;
 
 	if (m->m_flags & M_BCAST) {	/* broadcast */
 		memcpy(desten, etherbroadcastaddr, sizeof(etherbroadcastaddr));
-		return (1);
+		return (0);
 	}
 	if (m->m_flags & M_MCAST) {	/* multicast */
 		ETHER_MAP_IP_MULTICAST(&satosin(dst)->sin_addr, desten);
-		return (1);
+		return (0);
 	}
-	if (rt) {
+
+	if (rt0 != NULL) {
+		error = rt_checkgate(&ac->ac_if, rt0, dst,
+		    m->m_pkthdr.ph_rtableid, &rt);
+		if (error) {
+			m_freem(m);
+			return (error);
+		}
+
 		la = (struct llinfo_arp *)rt->rt_llinfo;
 		if (la == NULL)
 			log(LOG_DEBUG, "arpresolve: %s: route without link "
@@ -377,7 +387,7 @@ arpresolve(struct arpcom *ac, struct rtentry *rt, struct mbuf *m,
 	}
 	if (la == 0 || rt == 0) {
 		m_freem(m);
-		return (0);
+		return (EINVAL);
 	}
 	sdl = SDL(rt->rt_gateway);
 	/*
@@ -387,11 +397,11 @@ arpresolve(struct arpcom *ac, struct rtentry *rt, struct mbuf *m,
 	if ((rt->rt_expire == 0 || rt->rt_expire > time_second) &&
 	    sdl->sdl_family == AF_LINK && sdl->sdl_alen != 0) {
 		memcpy(desten, LLADDR(sdl), sdl->sdl_alen);
-		return 1;
+		return (0);
 	}
 	if (((struct ifnet *)ac)->if_flags & IFF_NOARP) {
 		m_freem(m);
-		return 0;
+		return (EINVAL);
 	}
 
 	/*
@@ -468,7 +478,7 @@ arpresolve(struct arpcom *ac, struct rtentry *rt, struct mbuf *m,
 			}
 		}
 	}
-	return (0);
+	return (EAGAIN);
 }
 
 /*
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index 7cb3cbd40fa..9bf0232d280 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: nd6.c,v 1.128 2014/12/22 11:05:53 mpi Exp $	*/
+/*	$OpenBSD: nd6.c,v 1.129 2015/01/08 14:29:18 mpi Exp $	*/
 /*	$KAME: nd6.c,v 1.280 2002/06/08 19:52:07 itojun Exp $	*/
 
 /*
@@ -1651,7 +1651,6 @@ nd6_output(struct ifnet *ifp, struct mbuf *m0, struct sockaddr_in6 *dst,
 {
 	struct mbuf *m = m0;
 	struct rtentry *rt = rt0;
-	struct sockaddr_in6 *gw6 = NULL;
 	struct llinfo_nd6 *ln = NULL;
 	int error = 0;
 #ifdef IPSEC
@@ -1665,57 +1664,28 @@ nd6_output(struct ifnet *ifp, struct mbuf *m0, struct sockaddr_in6 *dst,
 		goto sendpkt;
 
 	/*
-	 * next hop determination.  This routine is derived from ether_output.
+	 * next hop determination.
 	 */
-	if (rt) {
-		if ((rt->rt_flags & RTF_UP) == 0) {
-			if ((rt0 = rt = rtalloc(sin6tosa(dst),
-			    RT_REPORT|RT_RESOLVE,
-			    m->m_pkthdr.ph_rtableid)) != NULL)
-			{
-				rt->rt_refcnt--;
-				if (rt->rt_ifp != ifp)
-					senderr(EHOSTUNREACH);
-			} else
-				senderr(EHOSTUNREACH);
+	if (rt0 != NULL) {
+		error = rt_checkgate(ifp, rt0, sin6tosa(dst),
+		    m->m_pkthdr.ph_rtableid, &rt);
+		if (error) {
+			m_freem(m);
+			return (error);
 		}
 
-		if (rt->rt_flags & RTF_GATEWAY) {
-			gw6 = satosin6(rt->rt_gateway);
-
-			/*
-			 * We skip link-layer address resolution and NUD
-			 * if the gateway is not a neighbor from ND point
-			 * of view, regardless of the value of nd_ifinfo.flags.
-			 * The second condition is a bit tricky; we skip
-			 * if the gateway is our own address, which is
-			 * sometimes used to install a route to a p2p link.
-			 */
-			if (!nd6_is_addr_neighbor(gw6, ifp) ||
-			    in6ifa_ifpwithaddr(ifp, &gw6->sin6_addr)) {
-				/*
-				 * We allow this kind of tricky route only
-				 * when the outgoing interface is p2p.
-				 * XXX: we may need a more generic rule here.
-				 */
-				if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
-					senderr(EHOSTUNREACH);
-
-				goto sendpkt;
-			}
-
-			if (rt->rt_gwroute == 0)
-				goto lookup;
-			if (((rt = rt->rt_gwroute)->rt_flags & RTF_UP) == 0) {
-				rtfree(rt); rt = rt0;
-			lookup:
-				rt->rt_gwroute = rtalloc(rt->rt_gateway,
-				    RT_REPORT|RT_RESOLVE,
-				    m->m_pkthdr.ph_rtableid);
-				if ((rt = rt->rt_gwroute) == 0)
-					senderr(EHOSTUNREACH);
-			}
-		}
+		/*
+		 * We skip link-layer address resolution and NUD
+		 * if the gateway is not a neighbor from ND point
+		 * of view, regardless of the value of nd_ifinfo.flags.
+		 * The second condition is a bit tricky; we skip
+		 * if the gateway is our own address, which is
+		 * sometimes used to install a route to a p2p link.
+		 */
+		if ((ifp->if_flags & IFF_POINTOPOINT) &&
+		    ((nd6_is_addr_neighbor(satosin6(rt_key(rt)), ifp) == 0) ||
+		    in6ifa_ifpwithaddr(ifp, &satosin6(rt_key(rt))->sin6_addr)))
+			goto sendpkt;
 	}
 
 	/*
@@ -1860,33 +1830,42 @@ nd6_need_cache(struct ifnet *ifp)
 }
 
 int
-nd6_storelladdr(struct ifnet *ifp, struct rtentry *rt, struct mbuf *m, 
+nd6_storelladdr(struct ifnet *ifp, struct rtentry *rt0, struct mbuf *m,
     struct sockaddr *dst, u_char *desten)
 {
 	struct sockaddr_dl *sdl;
+	struct rtentry *rt;
+	int error;
 
 	if (m->m_flags & M_MCAST) {
 		switch (ifp->if_type) {
 		case IFT_ETHER:
 			ETHER_MAP_IPV6_MULTICAST(&satosin6(dst)->sin6_addr,
 						 desten);
-			return (1);
+			return (0);
 			break;
 		default:
 			m_freem(m);
-			return (0);
+			return (EINVAL);
 		}
 	}
 
-	if (rt == NULL) {
+	if (rt0 == NULL) {
 		/* this could happen, if we could not allocate memory */
 		m_freem(m);
-		return (0);
+		return (ENOMEM);
 	}
+
+	error = rt_checkgate(ifp, rt0, dst, m->m_pkthdr.ph_rtableid, &rt);
+	if (error) {
+		m_freem(m);
+		return (error);
+	}
+
 	if (rt->rt_gateway->sa_family != AF_LINK) {
 		printf("nd6_storelladdr: something odd happens\n");
 		m_freem(m);
-		return (0);
+		return (EINVAL);
 	}
 	sdl = SDL(rt->rt_gateway);
 	if (sdl->sdl_alen == 0) {
@@ -1897,11 +1876,11 @@ nd6_storelladdr(struct ifnet *ifp, struct rtentry *rt, struct mbuf *m,
 			addr, sizeof(addr)),
 		    ifp->if_xname);
 		m_freem(m);
-		return (0);
+		return (EINVAL);
 	}
 
 	bcopy(LLADDR(sdl), desten, sdl->sdl_alen);
-	return (1);
+	return (0);
 }
 
 /*