From 8c023157f568c27d8c9df36edc287db0b4676746 Mon Sep 17 00:00:00 2001 From: mpi Date: Thu, 8 Jan 2015 14:29:18 +0000 Subject: [PATCH] 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@ --- sys/net/if_ethersubr.c | 47 +++++---------------- sys/net/route.c | 41 +++++++++++++++++- sys/net/route.h | 7 +++- sys/netinet/if_ether.c | 36 ++++++++++------ sys/netinet6/nd6.c | 95 ++++++++++++++++-------------------------- 5 files changed, 116 insertions(+), 110 deletions(-) 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); } /* -- 2.20.1