Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934135AbbEOIwA (ORCPT ); Fri, 15 May 2015 04:52:00 -0400 Received: from cantor2.suse.de ([195.135.220.15]:36312 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932092AbbEOIv4 (ORCPT ); Fri, 15 May 2015 04:51:56 -0400 Date: Fri, 15 May 2015 10:51:52 +0200 From: Michal Kubecek To: Nicolas Dichtel Cc: "David S. Miller" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Kuznetsov , James Morris , Hideaki YOSHIFUJI , Patrick McHardy , roopa Subject: Re: [PATCH net v2 2/2] ipv6: fix ECMP route replacement Message-ID: <20150515085151.GA32190@unicorn.suse.cz> References: <5554F073.4080501@6wind.com> <20150514214907.GA20301@lion> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150514214907.GA20301@lion> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4108 Lines: 99 On Thu, May 14, 2015 at 11:49:07PM +0200, Michal Kubecek wrote: > On Thu, May 14, 2015 at 08:58:59PM +0200, Nicolas Dichtel wrote: > > Le 13/05/2015 21:59, Michal Kubecek a ?crit : > > >When replacing an IPv6 multipath route with "ip route replace", i.e. > > >NLM_F_CREATE | NLM_F_REPLACE, fib6_add_rt2node() replaces only first > > >matching route without fixing its siblings, resulting in corrupted > > >siblings linked list; removing one of the siblings can then end in an > > >infinite loop. > > > > > >Replacing the whole set of nexthops does IMHO make more sense than > > >replacing a random one. We also need to remove the NLM_F_REPLACE flag > > >after replacing old nexthops by first new so that each subsequent > > >nexthop does not replace previous one. > > > > > >Fixes: 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)") > > >Signed-off-by: Michal Kubecek > > >--- > > > net/ipv6/ip6_fib.c | 17 ++++++++++++++--- > > > net/ipv6/route.c | 8 +++++--- > > > 2 files changed, 19 insertions(+), 6 deletions(-) > > > > > >diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c > > >index 96dbffff5a24..abf4e4e5bdab 100644 > > >--- a/net/ipv6/ip6_fib.c > > >+++ b/net/ipv6/ip6_fib.c > > >@@ -815,6 +815,8 @@ add: > > > } > > > > > > } else { > > >+ struct rt6_info *next; > > >+ > > > if (!found) { > > > if (add) > > > goto add; > > >@@ -828,15 +830,24 @@ add: > > > > > > *ins = rt; > > > rt->rt6i_node = fn; > > >- rt->dst.rt6_next = iter->dst.rt6_next; > > >+ > > >+ /* skip potential siblings */ > > >+ next = iter->dst.rt6_next; > > >+ while (next && next->rt6i_metric == rt->rt6i_metric) > > >+ next = next->dst.rt6_next; > > I wonder if we should not loop over the siblings list here > > (rt->rt6i_siblings). Only routes that match 'rt6_qualify_for_ecmp()' > > are siblings. > > Problem with looping over the siblings list is that then we would have > to find each of them in the (unidirectional) list linked by dst.rt6_next > to be able to delete them from this list. Do we at least know that all > routes in this list with matching metric and rt6_qualify_for_ecmp() are > siblings? If so, we could still do the cleanup on one pass over the > dst.rt6_next list. Hm... it's still a bit more complicated. In the "replace" case, we break the loop once we find any route with matching metric, i.e. we can find a non-ECMP one even if there are some ECMP siblings farther in the chain. As far as I can see, replacing such route would cause an inconsistency as nsiblings would no longer match the number of ECMP-able routes in the chain. IMHO it's not completely clear what the "replace" semantics should be for multiple routes (and, worse, for a mix of non-ECMP and ECMP ones). One possible approach would be - when new route is ECMP-able, try to find an ECMP-able route and replace it with all its siblings - if there is none, fall back to replacing first matching non-ECMP route (or just add if creating is allowed) - when new route is not ECMP-able (can this really happen with NLM_F_REPLACE?), replace first matching non-ECMP or insert new one But I still rather feel like replacing all existing matching routes would better reflect what I expect "replace" to do. Michal Kubecek > > >+ rt->dst.rt6_next = next; > > >+ > > > atomic_inc(&rt->rt6i_ref); > > > inet6_rt_notify(RTM_NEWROUTE, rt, info); > > > if (!(fn->fn_flags & RTN_RTINFO)) { > > > info->nl_net->ipv6.rt6_stats->fib_route_nodes++; > > > fn->fn_flags |= RTN_RTINFO; > > > } > > >- fib6_purge_rt(iter, fn, info->nl_net); > > >- rt6_release(iter); > > >+ while (iter != next) { > > >+ fib6_purge_rt(iter, fn, info->nl_net); > > >+ rt6_release(iter); > > >+ iter = iter->dst.rt6_next; > > >+ } > > Same here. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/