netdev - Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190410173616.fzj5mkm5yq4nio67@kafai-mbp.dhcp.thefacebook.com>
Date:   Wed, 10 Apr 2019 17:36:18 +0000
From:   Martin Lau <kafai@...com>
To:     David Ahern <dsahern@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "idosch@...lanox.com" <idosch@...lanox.com>,
        "David Ahern" <dsahern@...il.com>
Subject: Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

On Tue, Apr 09, 2019 at 02:41:19PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@...il.com>
> 
> Move the nexthop evaluation of a fib entry to a helper that can be
> leveraged for each fib6_nh in a multipath nexthop object.
> 
> In the move, 'continue' statements means the helper returns false
> (loop should continue) and 'break' means return true (found the entry
> of interest).
> 
> Signed-off-by: David Ahern <dsahern@...il.com>
> ---
>  net/ipv6/route.c | 56 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 0e8becb1e455..d555edaaff13 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2407,6 +2407,35 @@ void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
>  		      NULL);
>  }
>  
> +static bool ip6_redirect_nh_match(struct fib6_info *f6i,
> +				  struct fib6_nh *nh,
Why both "struct fib6_info" and "struct fib6_nh" are needed?
I assume nh cannot be obtained from f6i in the later patch
when nh is decoupled from f6i?

> +				  struct flowi6 *fl6,
> +				  const struct in6_addr *gw,
> +				  struct rt6_info **ret)
> +{
> +	if (nh->fib_nh_flags & RTNH_F_DEAD || !nh->fib_nh_gw_family ||
> +	    fl6->flowi6_oif != nh->fib_nh_dev->ifindex)
> +		return false;
> +
> +	/* rt_cache's gateway might be different from its 'parent'
> +	 * in the case of an ip redirect.
> +	 * So we keep searching in the exception table if the gateway
> +	 * is different.
> +	 */
> +	if (!ipv6_addr_equal(gw, &nh->fib_nh_gw6)) {
> +		struct rt6_info *rt_cache;
> +
> +		rt_cache = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
> +		if (rt_cache &&
> +		    ipv6_addr_equal(gw, &rt_cache->rt6i_gateway)) {
> +			*ret = rt_cache;
> +			return true;
> +		}
> +		return false;
> +	}
> +	return true;
> +}
> +
>  /* Handle redirects */
>  struct ip6rd_flowi {
>  	struct flowi6 fl6;
> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>  					     int flags)
>  {
>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
> -	struct rt6_info *ret = NULL, *rt_cache;
> +	struct rt6_info *ret = NULL;
>  	struct fib6_info *rt;
The "rt" naming is becoming hard to review.
It is fortunate that the type context is kept in this diff.

In this local case, the s/rt/f6i/ rename is quite doable?
It can be the first clean-up step.

>  	struct fib6_node *fn;
>  
> @@ -2438,34 +2467,15 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>  	fn = fib6_node_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>  restart:
>  	for_each_fib6_node_rt_rcu(fn) {
> -		if (rt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
> -			continue;
>  		if (fib6_check_expired(rt))
>  			continue;
>  		if (rt->fib6_flags & RTF_REJECT)
>  			break;
> -		if (!rt->fib6_nh.fib_nh_gw_family)
> -			continue;
>  		if (fl6->flowi6_oif != rt->fib6_nh.fib_nh_dev->ifindex)
This check is also copied to the new ip6_redirect_nh_match.
Should this one be removed from here?

>  			continue;
> -		/* rt_cache's gateway might be different from its 'parent'
> -		 * in the case of an ip redirect.
> -		 * So we keep searching in the exception table if the gateway
> -		 * is different.
> -		 */
> -		if (!ipv6_addr_equal(&rdfl->gateway, &rt->fib6_nh.fib_nh_gw6)) {
> -			rt_cache = rt6_find_cached_rt(rt,
> -						      &fl6->daddr,
> -						      &fl6->saddr);
> -			if (rt_cache &&
> -			    ipv6_addr_equal(&rdfl->gateway,
> -					    &rt_cache->rt6i_gateway)) {
> -				ret = rt_cache;
> -				break;
> -			}
> -			continue;
> -		}
> -		break;
> +		if (ip6_redirect_nh_match(rt, &rt->fib6_nh, fl6,
> +					  &rdfl->gateway, &ret))
> +			goto out;
>  	}
>  
>  	if (!rt)
> -- 
> 2.11.0
>