Re: [PATCH net 1/2] ipv6: do not delete previously existing ECMP routes if add fails

On 5/13/15, 5:49 AM, Michal Kubecek wrote:

On Wed, May 13, 2015 at 02:28:57PM +0200, Nicolas Dichtel wrote:

quoted

Le 13/05/2015 11:50, Michal Kubecek a écrit :

quoted

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 <redacted>
---
  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)