On 23/05/16(Mon) 13:52, Alexander Bluhm wrote:
> 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);
This is ok because the packet has M_MCAST set.
> 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.
This is my next diff.
> > 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.
Thanks.
>
> >
> > 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);
>
