From owner-svn-src-all@FreeBSD.ORG Thu Oct 2 08:17:21 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A389EFBC; Thu, 2 Oct 2014 08:17:21 +0000 (UTC) Received: from mail.ipfw.ru (mail.ipfw.ru [IPv6:2a01:4f8:120:6141::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3B400ACB; Thu, 2 Oct 2014 08:17:21 +0000 (UTC) Received: from [2a02:6b8:0:401:222:4dff:fe50:cd2f] (helo=ptichko.yndx.net) by mail.ipfw.ru with esmtpsa (TLSv1:DHE-RSA-AES128-SHA:128) (Exim 4.82 (FreeBSD)) (envelope-from ) id 1XZXaO-000BMn-5J; Thu, 02 Oct 2014 08:01:48 +0400 Message-ID: <542D09CF.5000803@FreeBSD.org> Date: Thu, 02 Oct 2014 12:16:15 +0400 From: "Alexander V. Chernikov" User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-Version: 1.0 To: Hiroki Sato , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r272391 - in head/sys: netinet netinet6 References: <201410020025.s920PvEW008958@svn.freebsd.org> In-Reply-To: <201410020025.s920PvEW008958@svn.freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Oct 2014 08:17:21 -0000 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) { > >