On Wed, May 18, 2016 at 10:56:34PM +0200, Martin Pieuchot wrote:
> Diff below convert the last offender explicitly passing NULL to an
> if_output() function: pf_route().
There is another one in phyint_send6() at netinet6/ip6_mroute.c:
/*
* We just call if_output instead of nd6_output here, since
* we need no ND for a multicast forwarded packet...right?
*/
error = ifp->if_output(ifp, mb_copy, sin6tosa(&ro.ro_dst),
NULL);
I wonder wether pf_route6() should pass the route to nd6_output()
for consistency. But pf_refragment6() also uses NULL, and I guess
IPv6 has much more issues.
> I'm interested in tests but keep in mind that it is highly probable
> that some code paths still path a NULL pointer as last argument, if
> rtalloc(9) fails and its return is not checked. That's why I added
> a KASSERT(), to fix these cases.
I have tested your route-to code with /usr/src/regress/sys/net/pf_forward.
Some tests fail, it seems that we have issues with path mtu discovery
in current. But it is not related to this change. So OK bluhm@
for the pf_route() part.
>
> Index: net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 if_ethersubr.c
> --- net/if_ethersubr.c 18 May 2016 20:15:14 -0000 1.236
> +++ net/if_ethersubr.c 18 May 2016 20:45:47 -0000
> @@ -193,8 +193,12 @@ ether_output(struct ifnet *ifp, struct m
> struct mbuf *mcopy = NULL;
> struct ether_header *eh;
> struct arpcom *ac = (struct arpcom *)ifp;
> + sa_family_t af = dst->sa_family;
> int error = 0;
>
> + KASSERT(rt != NULL || ISSET(m->m_flags, M_MCAST|M_BCAST) ||
> + af == AF_UNSPEC || af == pseudo_AF_HDRCMPLT);
> +
> #ifdef DIAGNOSTIC
> if (ifp->if_rdomain != rtable_l2(m->m_pkthdr.ph_rtableid)) {
> printf("%s: trying to send packet on wrong domain. "
> @@ -208,7 +212,7 @@ ether_output(struct ifnet *ifp, struct m
> if ((ifp->if_flags & (IFF_UP|IFF_RUNNING)) != (IFF_UP|IFF_RUNNING))
> senderr(ENETDOWN);
>
> - switch (dst->sa_family) {
> + switch (af) {
> case AF_INET:
> error = arpresolve(ifp, rt, m, dst, edst);
> if (error)
> Index: net/pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.970
> diff -u -p -r1.970 pf.c
> --- net/pf.c 3 May 2016 12:13:38 -0000 1.970
> +++ net/pf.c 18 May 2016 20:45:49 -0000
> @@ -5574,6 +5574,12 @@ pf_route(struct mbuf **m, struct pf_rule
> s->rt_addr.v4.s_addr;
> ifp = s->rt_kif ? s->rt_kif->pfik_ifp : NULL;
> }
> +
> + rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
> + if (rt == NULL) {
> + ipstat.ips_noroute++;
> + goto bad;
> + }
> }
> if (ifp == NULL)
> goto bad;
> @@ -5602,7 +5608,7 @@ pf_route(struct mbuf **m, struct pf_rule
> ipstat.ips_outswcsum++;
> ip->ip_sum = in_cksum(m0, ip->ip_hl << 2);
> }
> - error = ifp->if_output(ifp, m0, sintosa(dst), NULL);
> + error = ifp->if_output(ifp, m0, sintosa(dst), rt);
> goto done;
> }
>
> @@ -5631,7 +5637,7 @@ pf_route(struct mbuf **m, struct pf_rule
> m1 = m0->m_nextpkt;
> m0->m_nextpkt = 0;
> if (error == 0)
> - error = ifp->if_output(ifp, m0, sintosa(dst), NULL);
> + error = ifp->if_output(ifp, m0, sintosa(dst), rt);
> else
> m_freem(m0);
> }
> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.207
> diff -u -p -r1.207 if_ether.c
> --- netinet/if_ether.c 18 May 2016 20:15:14 -0000 1.207
> +++ netinet/if_ether.c 18 May 2016 20:45:50 -0000
> @@ -294,40 +294,28 @@ arpresolve(struct ifnet *ifp, struct rte
> return (0);
> }
>
> - if (rt0 != NULL) {
> - error = rt_checkgate(rt0, &rt);
> - if (error) {
> - m_freem(m);
> - return (error);
> - }
> + error = rt_checkgate(rt0, &rt);
> + if (error) {
> + m_freem(m);
> + return (error);
> + }
>
> - if ((rt->rt_flags & RTF_LLINFO) == 0) {
> - log(LOG_DEBUG, "%s: %s: route contains no arp"
> - " information\n", __func__, inet_ntop(AF_INET,
> - &satosin(rt_key(rt))->sin_addr, addr,
> - sizeof(addr)));
> - m_freem(m);
> - return (EINVAL);
> - }
> + if ((rt->rt_flags & RTF_LLINFO) == 0) {
> + log(LOG_DEBUG, "%s: %s: route contains no arp"
> + " information\n", __func__, inet_ntop(AF_INET,
> + &satosin(rt_key(rt))->sin_addr, addr,
> + sizeof(addr)));
> + m_freem(m);
> + return (EINVAL);
> + }
>
> - la = (struct llinfo_arp *)rt->rt_llinfo;
> - if (la == NULL)
> - log(LOG_DEBUG, "%s: %s: route without link "
> - "local address\n", __func__, inet_ntop(AF_INET,
> - &satosin(dst)->sin_addr, addr, sizeof(addr)));
> - } else {
> - rt = arplookup(satosin(dst)->sin_addr.s_addr, 1, 0,
> - ifp->if_rdomain);
> - if (rt != NULL) {
> - created = 1;
> - la = ((struct llinfo_arp *)rt->rt_llinfo);
> - }
> - if (la == NULL)
> - log(LOG_DEBUG, "%s: %s: can't allocate llinfo\n",
> - __func__,
> - inet_ntop(AF_INET, &satosin(dst)->sin_addr,
> - addr, sizeof(addr)));
> + la = (struct llinfo_arp *)rt->rt_llinfo;
> + if (la == NULL) {
> + inet_ntop(AF_INET, &satosin(dst)->sin_addr, addr, sizeof(addr));
> + log(LOG_DEBUG, "%s: %s: route without link local address\n",
> + __func__, addr);
> }
> +
> if (la == NULL || rt == NULL)
> goto bad;
> sdl = satosdl(rt->rt_gateway);
> Index: netinet6/nd6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.179
> diff -u -p -r1.179 nd6.c
> --- netinet6/nd6.c 17 May 2016 08:29:14 -0000 1.179
> +++ netinet6/nd6.c 18 May 2016 20:45:50 -0000
> @@ -1667,12 +1667,6 @@ nd6_storelladdr(struct ifnet *ifp, struc
> }
> }
>
> - if (rt0 == NULL) {
> - /* this could happen, if we could not allocate memory */
> - m_freem(m);
> - return (ENOMEM);
> - }
> -
> error = rt_checkgate(rt0, &rt);
> if (error) {
> m_freem(m);
