IPv6 ECMP route add/replace fixes
Subsystems (3)
cover IPv6 ECMP route add/replace fixes
(1) When adding a nexthop of a multipath route fails (e.g. because of a conflict with an existing route), we are supposed to delete nexthops already added. However, currently we try to also delete all nexthops we haven't even tried to add yet so that a "ip route add" command can actually remove pre-existing routes if it fails.
(2) Attempt to replace a multipath route results in a broken siblings linked list. Following commands (like "ip route del") can then either follow a link into freed memory or end in an infinite loop (if the slab object has been reused).
Michal Kubecek (2):
ipv6: do not delete previously existing ECMP routes if add fails
ipv6: fix ECMP route replacement
net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 9 ++++++--- 2 files changed, 20 insertions(+), 6 deletions(-)
Signature · 1 line
2.3.7
PATCH 1/2 ipv6: do not delete previously existing ECMP routes if add fails
If adding a nexthop of an IPv6 multipath route fails, comment in ip6_route_multipath() says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, running
ip route add 1234:5678::/64 \
nexthop via fe80::aa dev dummy1 \
nexthop via fe80::bb dev dummy1 \
nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
--- net/ipv6/route.c | 1 + 1 file changed, 1 insertion(+)
net/ipv6/route.c +1 -0
@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning; } }
Signature · 1 line
2.3.7
Re: [PATCH net 1/2] ipv6: do not delete previously existing ECMP routes if add fails
Quoted 26 lines · Michal Kubecek
If adding a nexthop of an IPv6 multipath route fails, comment in
ip6_route_multipath()says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, runningip route add 1234:5678::/64 \ nexthop via fe80::aa dev dummy1 \ nexthop via fe80::bb dev dummy1 \ nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
Fixes 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)")Signed-off-by Michal Kubecek <[email protected]>--- net/ipv6/route.c | 1 + 1 file changed, 1 insertion(+)net/ipv6/route.c +0
-0
net/ipv6/route.c beginning: · line 2540
Quoted 5 lines
net/ipv6/route.c +1
-0@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning;
Not sure to understand your fix. At the label beginning, the code is:
beginning:
rtnh = (struct rtnexthop *)cfg->fc_mp;
remaining = cfg->fc_mp_len;Hence, 'remaining' will be overridden. How does your patch work?
Re: [PATCH net 1/2] ipv6: do not delete previously existing ECMP routes if add fails
Quoted 34 lines · Nicolas Dichtel
Quoted 26 lines · Michal Kubecek
If adding a nexthop of an IPv6 multipath route fails, comment in
ip6_route_multipath()says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, runningip route add 1234:5678::/64 \ nexthop via fe80::aa dev dummy1 \ nexthop via fe80::bb dev dummy1 \ nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
Fixes 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)")Signed-off-by Michal Kubecek <[email protected]>--- net/ipv6/route.c | 1 + 1 file changed, 1 insertion(+)net/ipv6/route.c +0
-0net/ipv6/route.c beginning: · line 2540
Quoted 5 lines
net/ipv6/route.c +1
-0@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning;Not sure to understand your fix. At the label beginning, the code is:
Quoted 5 lines
beginning:
rtnh = (struct rtnexthop *)cfg->fc_mp; remaining = cfg->fc_mp_len;Hence, 'remaining' will be overridden. How does your patch work?
It does not, sorry. I'm still trying to find out what did I wrong while testing. I'll need to move the initialization of remaining above the label.
Michal Kubecek
Re: [PATCH net 1/2] ipv6: do not delete previously existing ECMP routes if add fails
Quoted 35 lines · Michal Kubecek
Quoted 34 lines · Nicolas Dichtel
Quoted 26 lines · Michal Kubecek
If adding a nexthop of an IPv6 multipath route fails, comment in
ip6_route_multipath()says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, runningip route add 1234:5678::/64 \ nexthop via fe80::aa dev dummy1 \ nexthop via fe80::bb dev dummy1 \ nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
Fixes 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)")Signed-off-by Michal Kubecek <[email protected]>--- net/ipv6/route.c | 1 + 1 file changed, 1 insertion(+)net/ipv6/route.c +0
-0net/ipv6/route.c beginning: · line 2540
Quoted 5 lines
net/ipv6/route.c +1
-0@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning;Not sure to understand your fix. At the label beginning, the code is:
Quoted 9 lines
Quoted 5 lines
beginning:
rtnh = (struct rtnexthop *)cfg->fc_mp; remaining = cfg->fc_mp_len;Hence, 'remaining' will be overridden. How does your patch work?
It does not, sorry. I'm still trying to find out what did I wrong while testing. I'll need to move the initialization of remaining above the label.
This looks like a similar bug i was trying to fix some time back:
http://patchwork.ozlabs.org/patch/362296/
(I am not sure if my full patch is still valid. I was thinking of re-spining it sometime soon. If you are interested in trying it out, please do)
Re: [PATCH net 1/2] ipv6: do not delete previously existing ECMP routes if add fails
Quoted 7 lines · roopa
Quoted 1 line
This looks like a similar bug i was trying to fix some time back:
http://patchwork.ozlabs.org/patch/362296/(I am not sure if my full patch is still valid. I was thinking of re-spining it sometime soon. If you are interested in trying it out, please do)
It's essentially the same idea but I prefer to use variable remaining which is already there and is needed anyway rather than introduce three extra variables for the cleanup (which is quite similar to the suggestion from Hannes' reply).
I've sent v2 few minutes ago.
Michal Kubecek
v2 cover IPv6 ECMP route add/replace fixes
In reply to Michal Kubecek(1) When adding a nexthop of a multipath route fails (e.g. because of a conflict with an existing route), we are supposed to delete nexthops already added. However, currently we try to also delete all nexthops we haven't even tried to add yet so that a "ip route add" command can actually remove pre-existing routes if it fails.
(2) Attempt to replace a multipath route results in a broken siblings linked list. Following commands (like "ip route del") can then either follow a link into freed memory or end in an infinite loop (if the slab object has been reused).
Michal Kubecek (2):
ipv6: do not delete previously existing ECMP routes if add fails
ipv6: fix ECMP route replacement
net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 11 +++++++---- 2 files changed, 21 insertions(+), 7 deletions(-)
Signature · 1 line
2.3.7
PATCH v2 1/2 ipv6: do not delete previously existing ECMP routes if add fails
If adding a nexthop of an IPv6 multipath route fails, comment in ip6_route_multipath() says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, running
ip route add 1234:5678::/64 \
nexthop via fe80::aa dev dummy1 \
nexthop via fe80::bb dev dummy1 \
nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
--- net/ipv6/route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv6/route.c +2 -1
@@ -2504,9 +2504,9 @@ static int ip6_route_multipath(struct fib6_config *cfg, int add)int attrlen;int err =0 , last_err =0 ; + remaining = cfg->fc_mp_len; beginning: rtnh = (struct rtnexthop *)cfg->fc_mp; - remaining = cfg->fc_mp_len;/* Parse a Multipath Entry */ while (rtnh_ok (rtnh, remaining)) {
@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning; } }
Signature · 1 line
2.3.7
Re: [PATCH net v2 1/2] ipv6: do not delete previously existing ECMP routes if add fails
Quoted 10 lines · Michal Kubecek
If adding a nexthop of an IPv6 multipath route fails, comment in
ip6_route_multipath()says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, runningip route add 1234:5678::/64 \ nexthop via fe80::aa dev dummy1 \ nexthop via fe80::bb dev dummy1 \ nexthop via fe80::cc dev dummy1
Quoted 7 lines
twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
Fixes 51ebd3181572 ("ipv6: add support of equal cost multipath (ECMP)")Signed-off-by Michal Kubecek <[email protected]>
PATCH v2 2/2 ipv6: fix ECMP route replacement
In reply to Michal KubecekWhen 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.
--- net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 8 +++++--- 2 files changed, 19 insertions(+), 6 deletions(-)
net/ipv6/ip6_fib.c +14 -3
@@ -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; + 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; + } }return 0 ;
net/ipv6/route.c +5 -3
@@ -2541,11 +2541,13 @@ beginning: } }/* Because each route is added like a single route we remove -* this flag after the first nexthop (if there is a collision, +* these flags after the first nexthop: if there is a collision, * we have already fail to add the first nexthop: -* fib6_add_rt2node() has reject it). +* fib6_add_rt2node() has reject it; when replacing, old +* nexthops are removed by first new, the rest is added to it. */ - cfg->fc_nlinfo.nlh->nlmsg_flags &= ~NLM_F_EXCL ; + cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL | +NLM_F_REPLACE ); rtnh =rtnh_next (rtnh, &remaining); }
Signature · 1 line
2.3.7
Re: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
Quoted 32 lines · Michal Kubecek
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_REPLACEflag 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 <[email protected]>--- net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 8 +++++--- 2 files changed, 19 insertions(+), 6 deletions(-)net/ipv6/ip6_fib.c +2
-0@@ -815,6 +815,8 @@ add: } }else { +struct rt6_info *next; +if (!found) {if (add) goto add;
net/ipv6/ip6_fib.c add: · line 837
Quoted 9 lines
net/ipv6/ip6_fib.c +5
-1@@ -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.
Quoted 5 lines
net/ipv6/ip6_fib.c +2
-0+ rt->dst.rt6_next = next; +atomic_inc (&rt->rt6i_ref);inet6_rt_notify (RTM_NEWROUTE , rt, info);if (!(fn->fn_flags &RTN_RTINFO )) {
net/ipv6/ip6_fib.c add: · line 850
Quoted 10 lines
net/ipv6/ip6_fib.c +5
-2info->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.
Re: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
Quoted 34 lines · Nicolas Dichtel
Quoted 33 lines · Michal Kubecek
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_REPLACEflag 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 <[email protected]>--- net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 8 +++++--- 2 files changed, 19 insertions(+), 6 deletions(-)net/ipv6/ip6_fib.c +2
-0@@ -815,6 +815,8 @@ add: } }else { +struct rt6_info *next; +if (!found) {if (add) goto add;@@ -828,15 +830,24 @@ add:
Quoted 11 lines
net/ipv6/ip6_fib.c add: · line 837
Quoted 8 lines
net/ipv6/ip6_fib.c +5
-1*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.
Michal Kubecek
Quoted 17 lines
Quoted 6 lines
net/ipv6/ip6_fib.c +2
-0+ rt->dst.rt6_next = next; +atomic_inc (&rt->rt6i_ref);inet6_rt_notify (RTM_NEWROUTE , rt, info);if (!(fn->fn_flags &RTN_RTINFO )) {net/ipv6/ip6_fib.c add: · line 850
Quoted 10 lines
net/ipv6/ip6_fib.c +5
-2info->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.
Re: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
Quoted 47 lines · Michal Kubecek
Quoted 34 lines · Nicolas Dichtel
Quoted 33 lines · Michal Kubecek
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_REPLACEflag 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 <[email protected]>--- net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 8 +++++--- 2 files changed, 19 insertions(+), 6 deletions(-)net/ipv6/ip6_fib.c +2
-0@@ -815,6 +815,8 @@ add: } }else { +struct rt6_info *next; +if (!found) {if (add) goto add;@@ -828,15 +830,24 @@ add:Quoted 11 lines
net/ipv6/ip6_fib.c add: · line 837
Quoted 8 lines
net/ipv6/ip6_fib.c +5
-1*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.
Quoted 6 lines
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
Quoted 16 lines
Quoted 16 lines
Quoted 5 lines
net/ipv6/ip6_fib.c +2
-0+ rt->dst.rt6_next = next; +atomic_inc (&rt->rt6i_ref);inet6_rt_notify (RTM_NEWROUTE , rt, info);if (!(fn->fn_flags &RTN_RTINFO )) {net/ipv6/ip6_fib.c add: · line 850
Quoted 10 lines
net/ipv6/ip6_fib.c +5
-2info->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.
David Miller
maintainerRe: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
From: Michal Kubecek <[email protected]>
Date: Fri, 15 May 2015 10:51:52 +0200
Quoted 2 lines
But I still rather feel like replacing all existing matching routes would better reflect what I expect "replace" to do.
What does IPV4 do?
Re: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
Quoted 7 lines · David Miller
From: Michal Kubecek <[email protected]>
Date: Fri, 15 May 2015 10:51:52 +0200Quoted 2 lines
But I still rather feel like replacing all existing matching routes would better reflect what I expect "replace" to do.
What does IPV4 do?
Apparently there is some problem too as on 4.1-rc3, "ip route replace" seems to always remove first matching IPv4 route (multipath or not) but doesn't add the new one unless there was no matching route before calling the command (then it behaves like "add").
I also tried 3.0 and it replaces first matching route, whether it was multipath or not and whether the new one is multipath or not. This behaviour cannot be directly transfered to IPv6 as there are no real IPv6 multipath routes; instead, the set of all routes under a node with the same metric satisfying rt6_qualify_for_ecmp() is handled as a multipath route (so that there can be only one). The semantics I suggested in my previous mail might be a reasonable approximation.
Michal Kubecek
David Miller
maintainerRe: [PATCH net v2 2/2] ipv6: fix ECMP route replacement
From: Michal Kubecek <[email protected]>
Date: Fri, 15 May 2015 19:41:56 +0200
Quoted 2 lines
The semantics I suggested in my previous mail might be a reasonable approximation.
Ok, then let's try for that.
v3 cover IPv6 ECMP route add/replace fixes
(1) When adding a nexthop of a multipath route fails (e.g. because of a conflict with an existing route), we are supposed to delete nexthops already added. However, currently we try to also delete all nexthops we haven't even tried to add yet so that a "ip route add" command can actually remove pre-existing routes if it fails.
(2) Attempt to replace a multipath route results in a broken siblings linked list. Following commands (like "ip route del") can then either follow a link into freed memory or end in an infinite loop (if the slab object has been reused).
v2: fix an omission in first patch
v3: change the semantics of replace operation to better match IPv4
Michal Kubecek (2):
ipv6: do not delete previously existing ECMP routes if add fails
ipv6: fix ECMP route replacement
net/ipv6/ip6_fib.c | 39 +++++++++++++++++++++++++++++++++++++-- net/ipv6/route.c | 14 +++++++++----- 2 files changed, 46 insertions(+), 7 deletions(-)
Signature · 1 line
2.4.1
PATCH v3 1/2 ipv6: do not delete previously existing ECMP routes if add fails
If adding a nexthop of an IPv6 multipath route fails, comment in ip6_route_multipath() says we are going to delete all nexthops already added. However, current implementation deletes even the routes it hasn't even tried to add yet. For example, running
ip route add 1234:5678::/64 \
nexthop via fe80::aa dev dummy1 \
nexthop via fe80::bb dev dummy1 \
nexthop via fe80::cc dev dummy1twice results in removing all routes first command added.
Limit the second (delete) run to nexthops that succeeded in the first (add) run.
--- net/ipv6/route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv6/route.c +2 -1
@@ -2504,9 +2504,9 @@ static int ip6_route_multipath(struct fib6_config *cfg, int add)int attrlen;int err =0 , last_err =0 ; + remaining = cfg->fc_mp_len; beginning: rtnh = (struct rtnexthop *)cfg->fc_mp; - remaining = cfg->fc_mp_len;/* Parse a Multipath Entry */ while (rtnh_ok (rtnh, remaining)) {
@@ -2536,6 +2536,7 @@ beginning:* next hops that have been already added. */ add =0 ; + remaining = cfg->fc_mp_len - remaining; goto beginning; } }
Signature · 1 line
2.4.1
PATCH v3 2/2 ipv6: fix ECMP route replacement
In reply to Michal KubecekWhen 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.
IPv6 ECMP implementation is a bit different from IPv4 so that route replacement cannot work in exactly the same way. This should be a reasonable approximation:
1. If the new route is ECMP-able and there is a matching ECMP-able one already, replace it and all its siblings (if any).
2. If the new route is ECMP-able and no matching ECMP-able route exists, replace first matching non-ECMP-able (if any) or just add the new one.
3. If the new route is not ECMP-able, replace first matching non-ECMP-able route (if any) or add the new route.
We also need to remove the NLM_F_REPLACE flag after replacing old route(s) by first nexthop of an ECMP route so that each subsequent nexthop does not replace previous one.
--- net/ipv6/ip6_fib.c | 39 +++++++++++++++++++++++++++++++++++++-- net/ipv6/route.c | 11 +++++++---- 2 files changed, 44 insertions(+), 6 deletions(-)
net/ipv6/ip6_fib.c +37 -2
@@ -693,6 +693,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt, {struct rt6_info *iter =NULL ;struct rt6_info **ins; +struct rt6_info **fallback_ins =NULL ;int replace = (info->nlh && (info->nlh->nlmsg_flags &NLM_F_REPLACE )); int add = (!info->nlh ||
@@ -716,8 +717,13 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt, (info->nlh->nlmsg_flags & NLM_F_EXCL))return -EEXIST ;if (replace) { - found++; -break ; +if (rt_can_ecmp ==rt6_qualify_for_ecmp (iter)) { + found++; +break ; + } +if (rt_can_ecmp) + fallback_ins = fallback_ins ?: ins; + goto next_iter; } if (iter->dst.dev == rt->dst.dev &&
@@ -753,9 +759,17 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,if (iter->rt6i_metric > rt->rt6i_metric)break ; +next_iter: ins = &iter->dst.rt6_next; } +if (fallback_ins && !found) { +/* No ECMP-able route found, replace first non-ECMP one */ + ins = fallback_ins; + iter = *ins; + found++; + } +/* Reset round-robin state, if necessary */ if (ins == &fn->leaf) fn->rr_ptr =NULL ;
@@ -815,6 +829,8 @@ add: } }else { +int nsiblings; +if (!found) {if (add) goto add;
@@ -835,8 +851,27 @@ add: info->nl_net->ipv6.rt6_stats->fib_route_nodes++; fn->fn_flags |=RTN_RTINFO ; } + nsiblings = iter->rt6i_nsiblings;fib6_purge_rt (iter, fn, info->nl_net);rt6_release (iter); + +if (nsiblings) { +/* Replacing an ECMP route, remove all siblings */ + ins = &rt->dst.rt6_next; + iter = *ins; +while (iter) { +if (rt6_qualify_for_ecmp (iter)) { + *ins = iter->dst.rt6_next; +fib6_purge_rt (iter, fn, info->nl_net); +rt6_release (iter); + nsiblings--; + }else { + ins = &iter->dst.rt6_next; + } + iter = *ins; + } +WARN_ON (nsiblings !=0 ); + } }return 0 ;
net/ipv6/route.c +7 -4
@@ -2541,11 +2541,14 @@ beginning: } }/* Because each route is added like a single route we remove -* this flag after the first nexthop (if there is a collision, -* we have already fail to add the first nexthop: -* fib6_add_rt2node() has reject it). +* these flags after the first nexthop: if there is a collision, +* we have already failed to add the first nexthop: +* fib6_add_rt2node() has rejected it; when replacing, old +* nexthops have been replaced by first new, the rest should +* be added to it. */ - cfg->fc_nlinfo.nlh->nlmsg_flags &= ~NLM_F_EXCL ; + cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL | +NLM_F_REPLACE ); rtnh =rtnh_next (rtnh, &remaining); }
Signature · 1 line
2.4.1
David Miller
maintainerRe: [PATCH net v3 2/2] ipv6: fix ECMP route replacement
From: Michal Kubecek <[email protected]>
Date: Mon, 18 May 2015 20:54:00 +0200 (CEST)
Quoted 19 lines
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.IPv6 ECMP implementation is a bit different from IPv4 so that route replacement cannot work in exactly the same way. This should be a reasonable approximation:
1. If the new route is ECMP-able and there is a matching ECMP-able one already, replace it and all its siblings (if any).
2. If the new route is ECMP-able and no matching ECMP-able route exists, replace first matching non-ECMP-able (if any) or just add the new one.
3. If the new route is not ECMP-able, replace first matching non-ECMP-able route (if any) or add the new route.
Quoted 6 lines
We also need to remove the
NLM_F_REPLACEflag after replacing old route(s) by first nexthop of an ECMP route 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 <[email protected]>
Can I get some reviews please from interested parties?
As far as I can tell, this faithfully implements the policy we decided upon at the end of the previous thread for this patch. But I could be wrong :-)
Thanks.
Re: [PATCH net v3 2/2] ipv6: fix ECMP route replacement
In reply to Michal KubecekQuoted 19 lines · Michal Kubecek
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.IPv6 ECMP implementation is a bit different from IPv4 so that route replacement cannot work in exactly the same way. This should be a reasonable approximation:
1. If the new route is ECMP-able and there is a matching ECMP-able one already, replace it and all its siblings (if any).
2. If the new route is ECMP-able and no matching ECMP-able route exists, replace first matching non-ECMP-able (if any) or just add the new one.
3. If the new route is not ECMP-able, replace first matching non-ECMP-able route (if any) or add the new route.
Quoted 6 lines
We also need to remove the
NLM_F_REPLACEflag after replacing old route(s) by first nexthop of an ECMP route 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 <[email protected]>
LGTM.
David Miller
maintainerFrom: Michal Kubecek <[email protected]>
Date: Mon, 18 May 2015 20:53:50 +0200 (CEST)
Quoted 6 lines
(1) When adding a nexthop of a multipath route fails (e.g. because of a conflict with an existing route), we are supposed to delete nexthops already added. However, currently we try to also delete all nexthops we haven't even tried to add yet so that a "ip route add" command can actually remove pre-existing routes if it fails.
Quoted 8 lines
(2) Attempt to replace a multipath route results in a broken siblings linked list. Following commands (like "ip route del") can then either follow a link into freed memory or end in an infinite loop (if the slab object has been reused).
v2: fix an omission in first patch
v3: change the semantics of replace operation to better match IPv4
Series applied, thanks.
PATCH 2/2 ipv6: fix ECMP route replacement
In reply to Michal KubecekWhen 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.
--- net/ipv6/ip6_fib.c | 17 ++++++++++++++--- net/ipv6/route.c | 8 +++++--- 2 files changed, 19 insertions(+), 6 deletions(-)
net/ipv6/ip6_fib.c +14 -3
@@ -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; + 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; + } }return 0 ;
net/ipv6/route.c +5 -3
@@ -2541,11 +2541,13 @@ beginning: } }/* Because each route is added like a single route we remove -* this flag after the first nexthop (if there is a collision, +* these flags after the first nexthop: if there is a collision, * we have already fail to add the first nexthop: -* fib6_add_rt2node() has reject it). +* fib6_add_rt2node() has reject it; when replacing, old +* nexthops are removed by first new, the rest is added to it. */ - cfg->fc_nlinfo.nlh->nlmsg_flags &= ~NLM_F_EXCL ; + cfg->fc_nlinfo.nlh->nlmsg_flags &= ~(NLM_F_EXCL | +NLM_F_REPLACE ); rtnh =rtnh_next (rtnh, &remaining); }
Signature · 1 line
2.3.7