[PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[Hannes Frederic Sowa]

Static routes in this case are non-expiring routes which did not get
configured by autoconf or by icmpv6 redirects.

The check for expiring routes is enhanced to also include the original
route. In case of autoconfiguration, this case should already be caught
by the RTF_ADDRCONF check. But this assumption may not hold in future
(and already did not in the past). So install this check as a safety net.

To make sure we actually get an ecmp route while searching for the first
one in this fib6_node's leafs, also make sure it matches the ecmp route
assumptions.

(the checkpatch error is intentional)

v2:
a) Also check RTF_ADDRCONF and RTF_DYNAMIC flags.
b) Only test rt for ecmp qualification once.

Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/ipv6/ip6_fib.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 192dd1a..2a23741 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -632,6 +632,19 @@ insert_above:
 	return ln;
 }
 
+static bool rt6_qualify_for_ecmp(struct rt6_info *rt0)
+{
+	struct rt6_info *rt;
+
+	if ((rt0->rt6i_flags & (RTF_GATEWAY|RTF_ADDRCONF|RTF_DYNAMIC)) !=
+	    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.
  */
@@ -646,6 +659,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 	int add = (!info->nlh ||
 		   (info->nlh->nlmsg_flags & NLM_F_CREATE));
 	int found = 0;
+	bool rt_can_ecmp = rt6_qualify_for_ecmp(rt);
 
 	ins = &fn->leaf;
 
@@ -691,9 +705,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 (rt_can_ecmp &&
+			    rt6_qualify_for_ecmp(iter))
 				rt->rt6i_nsiblings++;
 		}
 
@@ -715,7 +728,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;
-- 
1.8.1.4

Re: [PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 11 Jul 2013 19:02:31 +0200

> +	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES);
> +	     rt = (struct rt6_info *)rt->dst.from);
> +	return !(rt && (rt->rt6i_flags & RTF_EXPIRES));

Please make this logic clearer, something like:


	for (rt = rt0; rt; rt = (struct rt6_info *)rt->dst.from) {
		if (rt->rt6i_flags & RTF_EXPIRES)
			return false;
	}
	return true;

Thanks.

Re: [PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[Hannes Frederic Sowa]

On Thu, Jul 11, 2013 at 11:44:19AM -0700, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Thu, 11 Jul 2013 19:02:31 +0200
> 
> > +	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES);
> > +	     rt = (struct rt6_info *)rt->dst.from);
> > +	return !(rt && (rt->rt6i_flags & RTF_EXPIRES));
> 
> Please make this logic clearer, something like:
> 
> 
> 	for (rt = rt0; rt; rt = (struct rt6_info *)rt->dst.from) {
> 		if (rt->rt6i_flags & RTF_EXPIRES)
> 			return false;
> 	}
> 	return true;

Ok, I'll revisit this. The reason for this loop (which was taken from
rt6_update_expires) is, that it was only valid to dereference dst.from if
RTF_EXPIRES was not set (before commit ecd9883 ("ipv6: fix race condition
regarding dst->expires and dst->from") dst.from and dst.expires were a
union). I tried to keep this (possible) invariant.

I'll check if this is still needed.

Greetings,

  Hannes

Re: [PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[Hannes Frederic Sowa]

On Thu, Jul 11, 2013 at 10:02:06PM +0200, Hannes Frederic Sowa wrote:
> On Thu, Jul 11, 2013 at 11:44:19AM -0700, David Miller wrote:
> > From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Date: Thu, 11 Jul 2013 19:02:31 +0200
> > 
> > > +	for (rt = rt0; rt && !(rt->rt6i_flags & RTF_EXPIRES);
> > > +	     rt = (struct rt6_info *)rt->dst.from);
> > > +	return !(rt && (rt->rt6i_flags & RTF_EXPIRES));
> > 
> > Please make this logic clearer, something like:
> > 
> > 
> > 	for (rt = rt0; rt; rt = (struct rt6_info *)rt->dst.from) {
> > 		if (rt->rt6i_flags & RTF_EXPIRES)
> > 			return false;
> > 	}
> > 	return true;
> 
> Ok, I'll revisit this. The reason for this loop (which was taken from
> rt6_update_expires) is, that it was only valid to dereference dst.from if
> RTF_EXPIRES was not set (before commit ecd9883 ("ipv6: fix race condition
> regarding dst->expires and dst->from") dst.from and dst.expires were a
> union). I tried to keep this (possible) invariant.
> 
> I'll check if this is still needed.

Thanks for pointing this out, David.

We cannot do it correctly. If we try to add a new rt6_info which
matches one already installed *BUT* this new route does not expire,
we clear the RTF_EXPIRES flag before returingin -EEXIST but leave the
from field alone (we must not change dst.from after construction). So
we could actually deny a route to become part of ecmp set which should
actually be allowed to.

I'll resend the patch with this loop removed.

Thanks,

  Hannes

[PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[Hannes Frederic Sowa]

Static routes in this case are non-expiring routes which did not get
configured by autoconf or by icmpv6 redirects.

To make sure we actually get an ecmp route while searching for the first
one in this fib6_node's leafs, also make sure it matches the ecmp route
assumptions.

v2:
a) Removed RTF_EXPIRE check in dst.from chain. The check of RTF_ADDRCONF
   already ensures that this route, even if added manually again without
   RTF_EXPIRES (or RA switches to infinite timeout), does not cause the
   rt6i_nsiblings logic to go wrong if a later RA updates the expiration
   time later.

Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/ipv6/ip6_fib.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 192dd1a..662b90c 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -632,6 +632,13 @@ insert_above:
 	return ln;
 }
 
+static inline bool rt6_qualify_for_ecmp(struct rt6_info *rt)
+{
+	return (rt->rt6i_flags &
+		(RTF_EXPIRES|RTF_GATEWAY|RTF_ADDRCONF|RTF_DYNAMIC)) ==
+	       RTF_GATEWAY;
+}
+
 /*
  *	Insert routing information in a node.
  */
@@ -646,6 +653,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 	int add = (!info->nlh ||
 		   (info->nlh->nlmsg_flags & NLM_F_CREATE));
 	int found = 0;
+	bool rt_can_ecmp = rt6_qualify_for_ecmp(rt);
 
 	ins = &fn->leaf;
 
@@ -691,9 +699,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 (rt_can_ecmp &&
+			    rt6_qualify_for_ecmp(iter))
 				rt->rt6i_nsiblings++;
 		}
 
@@ -715,7 +722,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;
-- 
1.8.1.4

Re: [PATCH net v2] ipv6: only static routes qualify for equal cost multipathing

[Hannes Frederic Sowa]

On Fri, Jul 12, 2013 at 04:37:31AM +0200, Hannes Frederic Sowa wrote:
> Static routes in this case are non-expiring routes which did not get
> configured by autoconf or by icmpv6 redirects.
> 
> To make sure we actually get an ecmp route while searching for the first
> one in this fib6_node's leafs, also make sure it matches the ecmp route
> assumptions.
> 
> v2:
> a) Removed RTF_EXPIRE check in dst.from chain. The check of RTF_ADDRCONF
>    already ensures that this route, even if added manually again without
>    RTF_EXPIRES (or RA switches to infinite timeout), does not cause the
>    rt6i_nsiblings logic to go wrong if a later RA updates the expiration
>    time later.
> 
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  net/ipv6/ip6_fib.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 192dd1a..662b90c 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -632,6 +632,13 @@ insert_above:
>  	return ln;
>  }
>  
> +static inline bool rt6_qualify_for_ecmp(struct rt6_info *rt)
> +{
> +	return (rt->rt6i_flags &
> +		(RTF_EXPIRES|RTF_GATEWAY|RTF_ADDRCONF|RTF_DYNAMIC)) ==
> +	       RTF_GATEWAY;
> +}

I am in progress to revisit this patch if RTF_EXPIRES is necessary. Please
defer this patch until then.

Thanks,

  Hannes