| Message ID | 4e3d075f2be93d9655bc1372a107584368a29eab.1431500953.git.mkubecek@suse.cz |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 13/05/2015 11:50, Michal Kubecek a écrit : > 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 dummy1 > > 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 <mkubecek@suse.cz> > --- > net/ipv6/route.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index d3588885f097..18b92c05b541 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 13, 2015 at 02:28:57PM +0200, Nicolas Dichtel wrote: > Le 13/05/2015 11:50, Michal Kubecek a écrit : > >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 dummy1 > > > >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 <mkubecek@suse.cz> > >--- > > net/ipv6/route.c | 1 + > > 1 file changed, 1 insertion(+) > > > >diff --git a/net/ipv6/route.c b/net/ipv6/route.c > >index d3588885f097..18b92c05b541 100644 > >--- a/net/ipv6/route.c > >+++ b/net/ipv6/route.c > >@@ -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? 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/13/15, 5:49 AM, Michal Kubecek wrote: > On Wed, May 13, 2015 at 02:28:57PM +0200, Nicolas Dichtel wrote: >> Le 13/05/2015 11:50, Michal Kubecek a écrit : >>> 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 dummy1 >>> >>> 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 <mkubecek@suse.cz> >>> --- >>> net/ipv6/route.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index d3588885f097..18b92c05b541 100644 >>> --- a/net/ipv6/route.c >>> +++ b/net/ipv6/route.c >>> @@ -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? > 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) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(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(-)
On Wed, May 13, 2015 at 06:30:20AM -0700, roopa wrote: > > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index d3588885f097..18b92c05b541 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -2536,6 +2536,7 @@ beginning: * next hops that have been already added. */ add = 0; + remaining = cfg->fc_mp_len - remaining; goto beginning; } }
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 dummy1 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 <mkubecek@suse.cz> --- net/ipv6/route.c | 1 + 1 file changed, 1 insertion(+)