svn commit: r272391 - in head/sys: netinet netinet6

[Alexander V. Chernikov]

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) {
>
>