netdev - Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only 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]

Date:   Sat, 15 Feb 2020 11:00:08 -0700
From:   David Ahern <dsahern@...il.com>
To:     Benjamin Poirier <bpoirier@...ulusnetworks.com>,
        netdev@...r.kernel.org
Cc:     Michal Kubeček <mkubecek@...e.cz>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        Ido Schimmel <idosch@...sch.org>
Subject: Re: [PATCH net 1/2] ipv6: Fix route replacement with dev-only route

On 2/11/20 6:41 PM, Benjamin Poirier wrote:
> After commit 27596472473a ("ipv6: fix ECMP route replacement") it is no
> longer possible to replace an ECMP-able route by a non ECMP-able route.
> For example,
> 	ip route add 2001:db8::1/128 via fe80::1 dev dummy0
> 	ip route replace 2001:db8::1/128 dev dummy0
> does not work as expected.
> 
> Tweak the replacement logic so that point 3 in the log of the above commit
> becomes:
> 3. If the new route is not ECMP-able, and no matching non-ECMP-able route
> exists, replace matching ECMP-able route (if any) or add the new route.
> 
> We can now summarize the entire replace semantics to:
> When doing a replace, prefer replacing a matching route of the same
> "ECMP-able-ness" as the replace argument. If there is no such candidate,
> fallback to the first route found.
> 
> Fixes: 27596472473a ("ipv6: fix ECMP route replacement")
> Signed-off-by: Benjamin Poirier <bpoirier@...ulusnetworks.com>
> ---
>  net/ipv6/ip6_fib.c                       | 7 ++++---
>  tools/testing/selftests/net/fib_tests.sh | 6 ++++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 58fbde244381..72abf892302f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1102,8 +1102,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>  					found++;
>  					break;
>  				}
> -				if (rt_can_ecmp)
> -					fallback_ins = fallback_ins ?: ins;
> +				fallback_ins = fallback_ins ?: ins;
>  				goto next_iter;
>  			}
>  
> @@ -1146,7 +1145,9 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>  	}
>  
>  	if (fallback_ins && !found) {
> -		/* No ECMP-able route found, replace first non-ECMP one */
> +		/* No matching route with same ecmp-able-ness found, replace
> +		 * first matching route
> +		 */
>  		ins = fallback_ins;
>  		iter = rcu_dereference_protected(*ins,
>  				    lockdep_is_held(&rt->fib6_table->tb6_lock));
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 6dd403103800..60273f1bc7d9 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -910,6 +910,12 @@ ipv6_rt_replace_mpath()
>  	check_route6 "2001:db8:104::/64 via 2001:db8:101::3 dev veth1 metric 1024"
>  	log_test $? 0 "Multipath with single path via multipath attribute"
>  
> +	# multipath with dev-only
> +	add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
> +	run_cmd "$IP -6 ro replace 2001:db8:104::/64 dev veth1"
> +	check_route6 "2001:db8:104::/64 dev veth1 metric 1024"
> +	log_test $? 0 "Multipath with dev-only"
> +
>  	# route replace fails - invalid nexthop 1
>  	add_initial_route6 "nexthop via 2001:db8:101::2 nexthop via 2001:db8:103::2"
>  	run_cmd "$IP -6 ro replace 2001:db8:104::/64 nexthop via 2001:db8:111::3 nexthop via 2001:db8:103::3"
> 

Thanks for adding a test case. I take this to mean that all existing
tests pass with this change. We have found this code to be extremely
sensitive to seemingly obvious changes.

Added Ido.