Date: Thu, 02 Oct 2014 12:16:15 +0400 From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: Hiroki Sato <hrs@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r272391 - in head/sys: netinet netinet6 Message-ID: <542D09CF.5000803@FreeBSD.org> In-Reply-To: <201410020025.s920PvEW008958@svn.freebsd.org> References: <201410020025.s920PvEW008958@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 02.10.2014 04:25, Hiroki Sato wrote: > Author: hrs > Date: Thu Oct 2 00:25:57 2014 > New Revision: 272391 > URL: https://svnweb.freebsd.org/changeset/base/272391 > > Log: > Add an additional routing table lookup when m->m_pkthdr.fibnum is changed > at a PFIL hook in ip{,6}_output(). IPFW setfib rule did not perform > a routing table lookup when the destination address was not changed. > > CR: D805 > > Modified: > head/sys/netinet/ip_output.c > head/sys/netinet6/ip6_output.c I'm not very happy with this. We already have large conditional for checking if dst has changed, how you have added another conditional for fib.. This idea is quite weird: why should we try to check all this stuff for every packet instead of asking pfil consumers to provide information they already know? We'd better discuss something like M_DSTCHANGED flag instead of doing these hacks. Additionally, you haven't changed "ip_fastfwd" part so the behavior is now inconsistent. > > Modified: head/sys/netinet/ip_output.c > ============================================================================== > --- head/sys/netinet/ip_output.c Thu Oct 2 00:19:24 2014 (r272390) > +++ head/sys/netinet/ip_output.c Thu Oct 2 00:25:57 2014 (r272391) > @@ -136,7 +136,9 @@ ip_output(struct mbuf *m, struct mbuf *o > struct rtentry *rte; /* cache for ro->ro_rt */ > struct in_addr odst; > struct m_tag *fwd_tag = NULL; > + uint32_t fibnum; > int have_ia_ref; > + int needfiblookup; > #ifdef IPSEC > int no_route_but_check_spd = 0; > #endif > @@ -202,6 +204,7 @@ ip_output(struct mbuf *m, struct mbuf *o > * therefore we need restore gw if we're redoing lookup. > */ > gw = dst = (struct sockaddr_in *)&ro->ro_dst; > + fibnum = (inp != NULL) ? inp->inp_inc.inc_fibnum : M_GETFIB(m); > again: > ia = NULL; > have_ia_ref = 0; > @@ -283,10 +286,9 @@ again: > #ifdef RADIX_MPATH > rtalloc_mpath_fib(ro, > ntohl(ip->ip_src.s_addr ^ ip->ip_dst.s_addr), > - inp ? inp->inp_inc.inc_fibnum : M_GETFIB(m)); > + fibnum); > #else > - in_rtalloc_ign(ro, 0, > - inp ? inp->inp_inc.inc_fibnum : M_GETFIB(m)); > + in_rtalloc_ign(ro, 0, fibnum); > #endif > rte = ro->ro_rt; > } > @@ -504,6 +506,7 @@ sendit: > goto done; > > ip = mtod(m, struct ip *); > + needfiblookup = 0; > > /* See if destination IP address was changed by packet filter. */ > if (odst.s_addr != ip->ip_dst.s_addr) { > @@ -529,9 +532,18 @@ sendit: > } else { > if (have_ia_ref) > ifa_free(&ia->ia_ifa); > - goto again; /* Redo the routing table lookup. */ > + needfiblookup = 1; /* Redo the routing table lookup. */ > } > } > + /* See if fib was changed by packet filter. */ > + if (fibnum != M_GETFIB(m)) { > + m->m_flags |= M_SKIP_FIREWALL; > + fibnum = M_GETFIB(m); > + RO_RTFREE(ro); > + needfiblookup = 1; > + } > + if (needfiblookup) > + goto again; > > /* See if local, if yes, send it to netisr with IP_FASTFWD_OURS. */ > if (m->m_flags & M_FASTFWD_OURS) { > > Modified: head/sys/netinet6/ip6_output.c > ============================================================================== > --- head/sys/netinet6/ip6_output.c Thu Oct 2 00:19:24 2014 (r272390) > +++ head/sys/netinet6/ip6_output.c Thu Oct 2 00:25:57 2014 (r272391) > @@ -255,6 +255,8 @@ ip6_output(struct mbuf *m0, struct ip6_p > struct route_in6 *ro_pmtu = NULL; > int hdrsplit = 0; > int sw_csum, tso; > + int needfiblookup; > + uint32_t fibnum; > struct m_tag *fwd_tag = NULL; > > ip6 = mtod(m, struct ip6_hdr *); > @@ -448,6 +450,7 @@ ip6_output(struct mbuf *m0, struct ip6_p > if (ro->ro_rt == NULL) > (void )flowtable_lookup(AF_INET6, m, (struct route *)ro); > #endif > + fibnum = (inp != NULL) ? inp->inp_inc.inc_fibnum : M_GETFIB(m); > again: > /* > * if specified, try to fill in the traffic class field. > @@ -489,7 +492,7 @@ again: > dst_sa.sin6_addr = ip6->ip6_dst; > } > error = in6_selectroute_fib(&dst_sa, opt, im6o, ro, &ifp, > - &rt, inp ? inp->inp_inc.inc_fibnum : M_GETFIB(m)); > + &rt, fibnum); > if (error != 0) { > if (ifp != NULL) > in6_ifstat_inc(ifp, ifs6_out_discard); > @@ -649,7 +652,7 @@ again: > > /* Determine path MTU. */ > if ((error = ip6_getpmtu(ro_pmtu, ro, ifp, &finaldst, &mtu, > - &alwaysfrag, inp ? inp->inp_inc.inc_fibnum : M_GETFIB(m))) != 0) > + &alwaysfrag, fibnum)) != 0) > goto bad; > > /* > @@ -727,6 +730,7 @@ again: > goto done; > ip6 = mtod(m, struct ip6_hdr *); > > + needfiblookup = 0; > /* See if destination IP address was changed by packet filter. */ > if (!IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst)) { > m->m_flags |= M_SKIP_FIREWALL; > @@ -747,8 +751,17 @@ again: > error = netisr_queue(NETISR_IPV6, m); > goto done; > } else > - goto again; /* Redo the routing table lookup. */ > + needfiblookup = 1; /* Redo the routing table lookup. */ > } > + /* See if fib was changed by packet filter. */ > + if (fibnum != M_GETFIB(m)) { > + m->m_flags |= M_SKIP_FIREWALL; > + fibnum = M_GETFIB(m); > + RO_RTFREE(ro); > + needfiblookup = 1; > + } > + if (needfiblookup) > + goto again; > > /* See if local, if yes, send it to netisr. */ > if (m->m_flags & M_FASTFWD_OURS) { > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?542D09CF.5000803>