[PATCH] mpls: Do not decrement alive counter for unregister events

[PATCH] mpls: Do not decrement alive counter for unregister events

[David Ahern]

Multipath routes can be rendered usesless when a device in one of the
paths is deleted. For example:

$ ip -f mpls ro ls
100
	nexthop as to 200 via inet 172.16.2.2  dev virt12
	nexthop as to 300 via inet 172.16.3.2  dev br0
101
	nexthop as to 201 via inet6 2000:2::2  dev virt12
	nexthop as to 301 via inet6 2000:3::2  dev br0

$ ip li del br0

When br0 is deleted the other hop is not considered in
mpls_select_multipath because of the alive check -- rt_nhn_alive
is 0.

rt_nhn_alive is decremented once in mpls_ifdown when the device is taken
down (NETDEV_DOWN) and again when it is deleted (NETDEV_UNREGISTER). For
a 2 hop route, deleting one device drops the alive count to 0. Since
devices are taken down before unregistering, the decrement on
NETDEV_UNREGISTER is redundant.

Fixes: c89359a42e2a4 ("mpls: support for dead routes")
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/mpls/af_mpls.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index ccdac9c44fdc..22a9971aa484 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1288,7 +1288,8 @@ static void mpls_ifdown(struct net_device *dev, int event)
 				/* fall through */
 			case NETDEV_CHANGE:
 				nh->nh_flags |= RTNH_F_LINKDOWN;
-				ACCESS_ONCE(rt->rt_nhn_alive) = rt->rt_nhn_alive - 1;
+				if (event != NETDEV_UNREGISTER)
+					ACCESS_ONCE(rt->rt_nhn_alive) = rt->rt_nhn_alive - 1;
 				break;
 			}
 			if (event == NETDEV_UNREGISTER)
-- 
2.1.4

Re: [PATCH] mpls: Do not decrement alive counter for unregister events

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Fri, 10 Mar 2017 14:11:39 -0800

> Multipath routes can be rendered usesless when a device in one of the
> paths is deleted. For example:
> 
> $ ip -f mpls ro ls
> 100
> 	nexthop as to 200 via inet 172.16.2.2  dev virt12
> 	nexthop as to 300 via inet 172.16.3.2  dev br0
> 101
> 	nexthop as to 201 via inet6 2000:2::2  dev virt12
> 	nexthop as to 301 via inet6 2000:3::2  dev br0
> 
> $ ip li del br0
> 
> When br0 is deleted the other hop is not considered in
> mpls_select_multipath because of the alive check -- rt_nhn_alive
> is 0.
> 
> rt_nhn_alive is decremented once in mpls_ifdown when the device is taken
> down (NETDEV_DOWN) and again when it is deleted (NETDEV_UNREGISTER). For
> a 2 hop route, deleting one device drops the alive count to 0. Since
> devices are taken down before unregistering, the decrement on
> NETDEV_UNREGISTER is redundant.
> 
> Fixes: c89359a42e2a4 ("mpls: support for dead routes")
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

Applied and queued up for -stable, thanks David.

Re: [PATCH] mpls: Do not decrement alive counter for unregister events

[Robert Shearman]

On 10/03/17 22:11, David Ahern wrote:
> Multipath routes can be rendered usesless when a device in one of the
> paths is deleted. For example:
>
> $ ip -f mpls ro ls
> 100
> 	nexthop as to 200 via inet 172.16.2.2  dev virt12
> 	nexthop as to 300 via inet 172.16.3.2  dev br0
> 101
> 	nexthop as to 201 via inet6 2000:2::2  dev virt12
> 	nexthop as to 301 via inet6 2000:3::2  dev br0
>
> $ ip li del br0
>
> When br0 is deleted the other hop is not considered in
> mpls_select_multipath because of the alive check -- rt_nhn_alive
> is 0.
>
> rt_nhn_alive is decremented once in mpls_ifdown when the device is taken
> down (NETDEV_DOWN) and again when it is deleted (NETDEV_UNREGISTER). For
> a 2 hop route, deleting one device drops the alive count to 0. Since
> devices are taken down before unregistering, the decrement on
> NETDEV_UNREGISTER is redundant.
>
> Fixes: c89359a42e2a4 ("mpls: support for dead routes")
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  net/mpls/af_mpls.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index ccdac9c44fdc..22a9971aa484 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -1288,7 +1288,8 @@ static void mpls_ifdown(struct net_device *dev, int event)
>  				/* fall through */
>  			case NETDEV_CHANGE:
>  				nh->nh_flags |= RTNH_F_LINKDOWN;
> -				ACCESS_ONCE(rt->rt_nhn_alive) = rt->rt_nhn_alive - 1;
> +				if (event != NETDEV_UNREGISTER)
> +					ACCESS_ONCE(rt->rt_nhn_alive) = rt->rt_nhn_alive - 1;
>  				break;
>  			}
>  			if (event == NETDEV_UNREGISTER)
>

Doesn't this leave the problem that if the device's link goes down and 
then the device gets deleted the alive count will be decremented twice 
for the same path?

Perhaps it would be better to change the condition for decrementing the 
alive count to be "!(nh->nh_flags & (RTNH_F_LINKDOWN | RTNH_F_DEAD))"?

Thanks,
Rob

Re: [PATCH] mpls: Do not decrement alive counter for unregister events

[David Ahern]

On 3/13/17 5:10 AM, Robert Shearman wrote:
> Doesn't this leave the problem that if the device's link goes down and
> then the device gets deleted the alive count will be decremented twice
> for the same path?

yes. and it exposes another bug in multipath selection.

> 
> Perhaps it would be better to change the condition for decrementing the
> alive count to be "!(nh->nh_flags & (RTNH_F_LINKDOWN | RTNH_F_DEAD))"?

or maybe the logic in mpls_ifup is the way to go -- reset the alive
counter based on the sum of each nexhop's status.

I'll send more patches soon.

Re: [PATCH] mpls: Do not decrement alive counter for unregister events

[David Ahern]

On 3/13/17 3:11 PM, David Ahern wrote:
> On 3/13/17 5:10 AM, Robert Shearman wrote:
>> Doesn't this leave the problem that if the device's link goes down and
>> then the device gets deleted the alive count will be decremented twice
>> for the same path?
> yes. and it exposes another bug in multipath selection.
> 

nevermind. I did not set the sysctl to keep ipv6 addresses; link down on
the veth device took out the address and route