[PATCH] ipv6: fix ecmp lookup on forwarding path

[PATCH] ipv6: fix ecmp lookup on forwarding path

[Nicolas Dichtel]

From: dingzhi <zhi.ding@6wind.com>

There is no reason to skip ECMP lookup when oif is specified. For example, when
a packet is forwarded, the input interface is specified.

Signed-off-by: dingzhi <zhi.ding@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/route.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ad0aa6b..9a6d637 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -742,7 +742,7 @@ static struct rt6_info *ip6_pol_route_lookup(struct net *net,
 restart:
 	rt = fn->leaf;
 	rt = rt6_device_match(net, rt, &fl6->saddr, fl6->flowi6_oif, flags);
-	if (rt->rt6i_nsiblings && fl6->flowi6_oif == 0)
+	if (rt->rt6i_nsiblings)
 		rt = rt6_multipath_select(rt, fl6);
 	BACKTRACK(net, &fl6->saddr);
 out:
@@ -875,7 +875,7 @@ restart_2:
 
 restart:
 	rt = rt6_select(fn, oif, strict | reachable);
-	if (rt->rt6i_nsiblings && oif == 0)
+	if (rt->rt6i_nsiblings)
 		rt = rt6_multipath_select(rt, fl6);
 	BACKTRACK(net, &fl6->saddr);
 	if (rt == net->ipv6.ip6_null_entry ||
-- 
1.8.2.1

Re: [PATCH] ipv6: fix ecmp lookup on forwarding path

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 13 Jun 2013 16:04:55 +0200

> From: dingzhi <zhi.ding@6wind.com>
> 
> There is no reason to skip ECMP lookup when oif is specified. For example, when
> a packet is forwarded, the input interface is specified.
> 
> Signed-off-by: dingzhi <zhi.ding@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

There is no way this is valid.

The multipath code makes no attempt to choose a route that goes via
the interface that matches the one the user asked for in the flow key.

We must absolutely respect the OIF in the flow key during the route
lookup, this means no multipath until that code is changed to strictly
respect the OIF in the lookup key.

Otherwise what's the point of the OIF if you're going to pick a route
that doesn't match it?

I'm not apply this patch, sorry.

[PATCH v2] ipv6: fix ecmp lookup when oif is specified

[Nicolas Dichtel]

There is no reason to skip ECMP lookup when oif is specified, but this implies
to check oif given by user when selecting another route.
When the new route does not match oif requirement, we simply keep the initial
one.

Spotted-by: dingzhi <zhi.ding@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: check oif requirement in rt6_multipath_select()

 net/ipv6/route.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ad0aa6b..4a5d259 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -83,6 +83,7 @@ static void		ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 					   struct sk_buff *skb, u32 mtu);
 static void		rt6_do_redirect(struct dst_entry *dst, struct sock *sk,
 					struct sk_buff *skb);
+static int rt6_score_route(struct rt6_info *rt, int oif, int strict);
 
 #ifdef CONFIG_IPV6_ROUTE_INFO
 static struct rt6_info *rt6_add_route_info(struct net *net,
@@ -394,7 +395,8 @@ static int rt6_info_hash_nhsfn(unsigned int candidate_count,
 }
 
 static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
-					     struct flowi6 *fl6)
+					     struct flowi6 *fl6, int oif,
+					     int strict)
 {
 	struct rt6_info *sibling, *next_sibling;
 	int route_choosen;
@@ -408,6 +410,8 @@ static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
 				&match->rt6i_siblings, rt6i_siblings) {
 			route_choosen--;
 			if (route_choosen == 0) {
+				if (rt6_score_route(sibling, oif, strict) < 0)
+					break;
 				match = sibling;
 				break;
 			}
@@ -742,8 +746,8 @@ static struct rt6_info *ip6_pol_route_lookup(struct net *net,
 restart:
 	rt = fn->leaf;
 	rt = rt6_device_match(net, rt, &fl6->saddr, fl6->flowi6_oif, flags);
-	if (rt->rt6i_nsiblings && fl6->flowi6_oif == 0)
-		rt = rt6_multipath_select(rt, fl6);
+	if (rt->rt6i_nsiblings)
+		rt = rt6_multipath_select(rt, fl6, fl6->flowi6_oif, flags);
 	BACKTRACK(net, &fl6->saddr);
 out:
 	dst_use(&rt->dst, jiffies);
@@ -875,8 +879,8 @@ restart_2:
 
 restart:
 	rt = rt6_select(fn, oif, strict | reachable);
-	if (rt->rt6i_nsiblings && oif == 0)
-		rt = rt6_multipath_select(rt, fl6);
+	if (rt->rt6i_nsiblings)
+		rt = rt6_multipath_select(rt, fl6, oif, strict | reachable);
 	BACKTRACK(net, &fl6->saddr);
 	if (rt == net->ipv6.ip6_null_entry ||
 	    rt->rt6i_flags & RTF_CACHE)
-- 
1.8.2.1

Re: [PATCH v2] ipv6: fix ecmp lookup when oif is specified

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 14 Jun 2013 10:32:58 +0200

> There is no reason to skip ECMP lookup when oif is specified, but this implies
> to check oif given by user when selecting another route.
> When the new route does not match oif requirement, we simply keep the initial
> one.
> 
> Spotted-by: dingzhi <zhi.ding@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

IPV4 routing has elided the multipath lookup when the interface is specified
for nearly two decades.

So two things:

1) A decision that old must have a good reason, and you must do some
   research to figure out exactly what that reason is.

2) If ipv4 is found to be wrong too, we must bring both ipv4 and ipv6
   into the same behavior at the same time.

But I do not want to make this change for another reason, it potentially
breaks things for someone, somewhere.  And I see very miniscule, if any,
value to this new behavior.  The risk far outweighs the gains.

Sorry, I really have to reject this patch for now.

Re: [PATCH v2] ipv6: fix ecmp lookup when oif is specified

[Nicolas Dichtel]

Le 24/06/2013 03:40, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Fri, 14 Jun 2013 10:32:58 +0200
>
>> There is no reason to skip ECMP lookup when oif is specified, but this implies
>> to check oif given by user when selecting another route.
>> When the new route does not match oif requirement, we simply keep the initial
>> one.
>>
>> Spotted-by: dingzhi <zhi.ding@6wind.com>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> IPV4 routing has elided the multipath lookup when the interface is specified
> for nearly two decades.
>
> So two things:
>
> 1) A decision that old must have a good reason, and you must do some
>     research to figure out exactly what that reason is.
>
> 2) If ipv4 is found to be wrong too, we must bring both ipv4 and ipv6
>     into the same behavior at the same time.
In fact, routing engines are different in forwarding case:

IPv4 case:
fl4->flowi4_oif is 0 when a packet is forwarded and this value is checked to 
select or not multipath functions.

IPv6 case:
The check is done against the argument oif (not fl6->flowi6_oif) of the function 
ip6_pol_route(). And this argument is set to the input interface by 
ip6_pol_route_input().

Functions call:
   - ip6_rcv_finish() -> ip6_route_input() -> ip6_route_input_lookup() -> 
fib6_rule_lookup() -> ip6_pol_route_input() -> ip6_pol_route()

Will you accept the patch if I remove only the check of oif argument in 
ip6_pol_route() (and leave ip6_pol_route_lookup() untouched).

My goal was only to have ECMP working in the forwarding case.

Re: [PATCH v2] ipv6: fix ecmp lookup when oif is specified

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 24 Jun 2013 18:44:24 +0200

> Will you accept the patch if I remove only the check of oif argument
> in ip6_pol_route() (and leave ip6_pol_route_lookup() untouched).
> 
> My goal was only to have ECMP working in the forwarding case.

Yes, if you make it such that, like ipv4, oif is respected for output
route lookups.

[PATCH v3] ipv6: fix ecmp lookup when oif is specified

[Nicolas Dichtel]

There is no reason to skip ECMP lookup when oif is specified, but this implies
to check oif given by user when selecting another route.
When the new route does not match oif requirement, we simply keep the initial
one.

Spotted-by: dingzhi <zhi.ding@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v3: leave the oif check in ip6_pol_route_lookup()

v2: check oif requirement in rt6_multipath_select()

 net/ipv6/route.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ad0aa6b..fb4f91d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -83,6 +83,7 @@ static void		ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 					   struct sk_buff *skb, u32 mtu);
 static void		rt6_do_redirect(struct dst_entry *dst, struct sock *sk,
 					struct sk_buff *skb);
+static int rt6_score_route(struct rt6_info *rt, int oif, int strict);
 
 #ifdef CONFIG_IPV6_ROUTE_INFO
 static struct rt6_info *rt6_add_route_info(struct net *net,
@@ -394,7 +395,8 @@ static int rt6_info_hash_nhsfn(unsigned int candidate_count,
 }
 
 static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
-					     struct flowi6 *fl6)
+					     struct flowi6 *fl6, int oif,
+					     int strict)
 {
 	struct rt6_info *sibling, *next_sibling;
 	int route_choosen;
@@ -408,6 +410,8 @@ static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
 				&match->rt6i_siblings, rt6i_siblings) {
 			route_choosen--;
 			if (route_choosen == 0) {
+				if (rt6_score_route(sibling, oif, strict) < 0)
+					break;
 				match = sibling;
 				break;
 			}
@@ -743,7 +747,7 @@ restart:
 	rt = fn->leaf;
 	rt = rt6_device_match(net, rt, &fl6->saddr, fl6->flowi6_oif, flags);
 	if (rt->rt6i_nsiblings && fl6->flowi6_oif == 0)
-		rt = rt6_multipath_select(rt, fl6);
+		rt = rt6_multipath_select(rt, fl6, fl6->flowi6_oif, flags);
 	BACKTRACK(net, &fl6->saddr);
 out:
 	dst_use(&rt->dst, jiffies);
@@ -875,8 +879,8 @@ restart_2:
 
 restart:
 	rt = rt6_select(fn, oif, strict | reachable);
-	if (rt->rt6i_nsiblings && oif == 0)
-		rt = rt6_multipath_select(rt, fl6);
+	if (rt->rt6i_nsiblings)
+		rt = rt6_multipath_select(rt, fl6, oif, strict | reachable);
 	BACKTRACK(net, &fl6->saddr);
 	if (rt == net->ipv6.ip6_null_entry ||
 	    rt->rt6i_flags & RTF_CACHE)
-- 
1.8.2.1

Re: [PATCH v3] ipv6: fix ecmp lookup when oif is specified

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 28 Jun 2013 17:35:48 +0200

> There is no reason to skip ECMP lookup when oif is specified, but this implies
> to check oif given by user when selecting another route.
> When the new route does not match oif requirement, we simply keep the initial
> one.
> 
> Spotted-by: dingzhi <zhi.ding@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied, thanks Nicolas.