diff mbox

[RFC] ipv6: fix route selection if kernel is not compiled with CONFIG_IPV6_ROUTER_PREF

Message ID 20130711102441.GC5207@order.stressinduktion.org
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Hannes Frederic Sowa July 11, 2013, 10:24 a.m. UTC 
On Thu, Jul 11, 2013 at 10:04:47AM +0200, Nicolas Dichtel wrote:
> Le 10/07/2013 23:21, Hannes Frederic Sowa a écrit :
> >On Wed, Jul 10, 2013 at 04:10:58PM +0200, Nicolas Dichtel wrote:
> >>I wonder why expires is 0. Even if this route is cached, the flag
> >>RTF_EXPIRES should be set. Am I wrong?
> >
> >rt6_set_from deliberately clears the RTF_EXPIRES when creating a cached 
> >copy
> >of the route if the route is an autoconfigured default route.
> >
> >Maybe the criterion for exclusion of which routes can get into an ecmp 
> >route
> >set should be revisited? This could result in strange effects for users
> >working with two interfaces, both receiving a RA with default routes.
> Agreed. Here is a proposal, what do you think?
> 
> [PATCH] ipv6: don't use autoconfigured route for ecmp
> 
> The intention was already there by checking the flag RTF_EXPIRES, but this 
> flag
> is removed from the default route by rt6_set_from() when this route is 
> cached.
> 
> Let's add a check against RTF_ADDRCONF.
> 
> Spotted-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

I do think the patch is ok. I just wanted to show you a solution I did
on my laptop this night and did not have the time to send yesterday. It
is only compile-tested.

I did strengthen the RTF_EXPIRES check a bit. Also I am not sure what
did stop the search for the first route with the same metric to find a
RTF_EXPIRES route, so I also added the guard there, too.

I fear, I'll need to do a bit more research.

Thanks!

[PATCH RFC] ipv6: routes only qualify for ecmp if their original routes do not expire

Cloned routes get their RTF_EXPIRES flag reset if they are autoconfigured
default routes. Because these routes should not end up in the ecmp route
set, we check if the original route has the RTF_EXPIRES flag set.

Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/ip6_fib.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Hannes Frederic Sowa July 11, 2013, 2:46 p.m. UTC | #1 
On Thu, Jul 11, 2013 at 12:24:41PM +0200, Hannes Frederic Sowa wrote:
> I fear, I'll need to do a bit more research.

My proposal is to take my patch and check for RTF_ADDRCONF plus RTF_DYNAMIC,
too. The RTF_DYNAMIC check would prevent routes created from icmpv6 redirects
entering an ecmp route set.

Do you agree?

Thanks,

  Hannes


--
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
Nicolas Dichtel July 11, 2013, 2:57 p.m. UTC | #2 
Le 11/07/2013 16:46, Hannes Frederic Sowa a écrit :
> On Thu, Jul 11, 2013 at 12:24:41PM +0200, Hannes Frederic Sowa wrote:
>> I fear, I'll need to do a bit more research.
>
> My proposal is to take my patch and check for RTF_ADDRCONF plus RTF_DYNAMIC,
> too. The RTF_DYNAMIC check would prevent routes created from icmpv6 redirects
> entering an ecmp route set.
>
> Do you agree?
Yes.


Regards,
Nicolas
--
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
Hannes Frederic Sowa July 12, 2013, 8:51 a.m. UTC | #3 
Hello Nicolas,

On Thu, Jul 11, 2013 at 04:57:42PM +0200, Nicolas Dichtel wrote:
> Le 11/07/2013 16:46, Hannes Frederic Sowa a écrit :
> >On Thu, Jul 11, 2013 at 12:24:41PM +0200, Hannes Frederic Sowa wrote:
> >>I fear, I'll need to do a bit more research.
> >
> >My proposal is to take my patch and check for RTF_ADDRCONF plus 
> >RTF_DYNAMIC,
> >too. The RTF_DYNAMIC check would prevent routes created from icmpv6 
> >redirects
> >entering an ecmp route set.
> >
> >Do you agree?
> Yes.

There is still some window where things go wrong now, I fear. If we have ecmp
routes active and we update the pmtu of that rt6_info, we might end up with a
route in the ecmp set, which might not get recountet if another ecmp route
joins the set. I will have to think how to deal with this. Do you have an
idea?

Thanks,

  Hannes


--
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
Nicolas Dichtel July 12, 2013, 12:04 p.m. UTC | #4 
Le 12/07/2013 10:51, Hannes Frederic Sowa a écrit :
> Hello Nicolas,
>
> On Thu, Jul 11, 2013 at 04:57:42PM +0200, Nicolas Dichtel wrote:
>> Le 11/07/2013 16:46, Hannes Frederic Sowa a écrit :
>>> On Thu, Jul 11, 2013 at 12:24:41PM +0200, Hannes Frederic Sowa wrote:
>>>> I fear, I'll need to do a bit more research.
>>>
>>> My proposal is to take my patch and check for RTF_ADDRCONF plus
>>> RTF_DYNAMIC,
>>> too. The RTF_DYNAMIC check would prevent routes created from icmpv6
>>> redirects
>>> entering an ecmp route set.
>>>
>>> Do you agree?
>> Yes.
>
> There is still some window where things go wrong now, I fear. If we have ecmp
> routes active and we update the pmtu of that rt6_info, we might end up with a
> route in the ecmp set, which might not get recountet if another ecmp route
> joins the set. I will have to think how to deal with this. Do you have an
> idea?
It's possible to add a glue to check this counter when we play with these flags, 
but it's ugly.

Maybe the check against RTF_EXPIRES is fundamentally wrong. Checking 
RTF_ADDRCONF|RTF_DYNAMIC should be enough, what do you think?

In another hand, we can discuss about the initial assumption, that was "only 
static routes are part of ECMP routes". I'm thinking of what are the consequence 
if we accept to accept all routes, without checking any flags.


Regards,
Nicolas
--
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
Hannes Frederic Sowa July 12, 2013, 4:19 p.m. UTC | #5 
On Fri, Jul 12, 2013 at 02:04:45PM +0200, Nicolas Dichtel wrote:
> It's possible to add a glue to check this counter when we play with these 
> flags, but it's ugly.
> 
> Maybe the check against RTF_EXPIRES is fundamentally wrong. Checking 
> RTF_ADDRCONF|RTF_DYNAMIC should be enough, what do you think?

Yes, this seems to be the best option now. I will audit the source if
RTF_ADDRCONF and RTF_DYNAMIC are immutable after dst construction and
if other errors could arise for that and would go with this solution then.

What do you think about making ecmp routes explicit by adding RTF_ECMP
flag?

> In another hand, we can discuss about the initial assumption, that was 
> "only static routes are part of ECMP routes". I'm thinking of what are the 
> consequence if we accept to accept all routes, without checking any flags.

I don't have a good feeling about that. But I may be wrong.

Greetings,

  Hannes

--
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
Nicolas Dichtel July 12, 2013, 7:01 p.m. UTC | #6 
Le 12/07/2013 18:19, Hannes Frederic Sowa a écrit :
> On Fri, Jul 12, 2013 at 02:04:45PM +0200, Nicolas Dichtel wrote:
>> It's possible to add a glue to check this counter when we play with these
>> flags, but it's ugly.
>>
>> Maybe the check against RTF_EXPIRES is fundamentally wrong. Checking
>> RTF_ADDRCONF|RTF_DYNAMIC should be enough, what do you think?
>
> Yes, this seems to be the best option now. I will audit the source if
> RTF_ADDRCONF and RTF_DYNAMIC are immutable after dst construction and
> if other errors could arise for that and would go with this solution then.
>
> What do you think about making ecmp routes explicit by adding RTF_ECMP
> flag?
Why not, but you will have to be careful on insertion and deletion. Next hop can 
be added one by one and deleted one by one.

>
>> In another hand, we can discuss about the initial assumption, that was
>> "only static routes are part of ECMP routes". I'm thinking of what are the
>> consequence if we accept to accept all routes, without checking any flags.
>
> I don't have a good feeling about that. But I may be wrong.
Same for me, but for now, I don't have any argument ;-) The above solution is 
probably the better way for now.


Nicolas
--
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 mbox

Patch

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 192dd1a..3bd5fcd 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -632,6 +632,18 @@  insert_above:
 	return ln;
 }
 
+static bool rt6_qualify_for_ecmp(struct rt6_info *rt0)
+{
+	struct rt6_info *rt;
+
+	if (!(rt0->rt6i_flags & RTF_GATEWAY))
+		return false;
+
+	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES);
+	     rt = (struct rt6_info *)rt->dst.from);
+	return !(rt && (rt->rt6i_flags & RTF_EXPIRES));
+}
+
 /*
  *	Insert routing information in a node.
  */
@@ -691,9 +703,8 @@  static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 			 * To avoid long list, we only had siblings if the
 			 * route have a gateway.
 			 */
-			if (rt->rt6i_flags & RTF_GATEWAY &&
-			    !(rt->rt6i_flags & RTF_EXPIRES) &&
-			    !(iter->rt6i_flags & RTF_EXPIRES))
+			if (rt6_qualify_for_ecmp(rt) &&
+			    rt6_qualify_for_ecmp(iter))
 				rt->rt6i_nsiblings++;
 		}
 
@@ -715,7 +726,8 @@  static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 		/* Find the first route that have the same metric */
 		sibling = fn->leaf;
 		while (sibling) {
-			if (sibling->rt6i_metric == rt->rt6i_metric) {
+			if (sibling->rt6i_metric == rt->rt6i_metric &&
+			    rt6_qualify_for_ecmp(sibling)) {
 				list_add_tail(&rt->rt6i_siblings,
 					      &sibling->rt6i_siblings);
 				break;