[RFC PATCH net-next 00/19] ipv6: Align nexthop behaviour with IPv4

[RFC PATCH net-next 00/19] ipv6: Align nexthop behaviour with IPv4

[Ido Schimmel]

This set tries to eliminate some differences between IPv4's and IPv6's
treatment of nexthops. These differences are most likely a side effect
of IPv6's data structures (specifically 'rt6_info') that incorporate
both the route and the nexthop and the late addition of ECMP support in
commit 51ebd3181572 ("ipv6: add support of equal cost multipath
(ECMP)").

When a netdev is unregistered, IPv4 flushes all the routes using the
netdev as their nexthop device. This includes multipath routes. In IPv6,
sibling routes in a multipath route are unaffected.

In addition, a netdev that is put administratively down causes IPv4 to
mark all the nexthops using it as 'dead'. A route is only flushed when
all of its nexthops are dead. IPv6 on the other hand simply flushes all
the routes using the netdev as their nexthop device. This includes
sibling routes in a multipath route.

These differences stem from the fact that the IPv6 code evaluates routes
without taking into consideration the state of their siblings in a
multipath route. This makes it difficult to introduce features such as
non-equal-cost multipath that are built on top of this set [1].

The first 12 patches introduce non-functional changes that store the
RTNH_F_DEAD and RTNH_F_LINKDOWN flags in IPv6 routes based on netdev
events, in a similar fashion to IPv4. This allows us to remove the
carrier check performed during route lookup and dump.

The next four patches finally eliminate the above mentioned differences.
First, by flushing all the sibling routes when a nexthop device in a
sibling route is unregistered. Then, by only flushing a multipath route
when all of its nexthops are dead.

Last three patches add test cases for IPv4/IPv6 FIB. These verify that
both address families react similarly to netdev events.

1. https://github.com/idosch/linux/tree/ipv6-nexthops

Ido Schimmel (19):
  ipv6: Remove redundant route flushing during namespace dismantle
  ipv6: Mark dead nexthops with appropriate flags
  ipv6: Clear nexthop flags upon netdev up
  ipv6: Prepare to handle multiple netdev events
  ipv6: Set nexthop flags upon carrier change
  ipv6: Set nexthop flags during route creation
  ipv6: Check nexthop flags during route lookup instead of carrier
  ipv6: Check nexthop flags in route dump instead of carrier
  ipv6: Ignore dead routes during lookup
  ipv6: Report dead flag during route dump
  ipv6: Add explicit flush indication to routes
  ipv6: Teach tree walker to skip multipath routes
  ipv6: Flush all sibling routes upon NETDEV_UNREGISTER
  ipv6: Export sernum update function
  ipv6: Take table lock outside of sernum update function
  ipv6: Flush multipath routes when all siblings are dead
  selftests: fib_tests: Add test cases for IPv4/IPv6 FIB
  selftests: fib_tests: Add test cases for netdev down
  selftests: fib_tests: Add test cases for netdev carrier change

 include/net/ip6_fib.h                    |   4 +-
 include/net/ip6_route.h                  |   3 +
 net/ipv6/addrconf.c                      |   9 +-
 net/ipv6/ip6_fib.c                       |  28 +-
 net/ipv6/route.c                         | 189 ++++++++++++--
 tools/testing/selftests/net/Makefile     |   1 +
 tools/testing/selftests/net/fib_tests.sh | 428 +++++++++++++++++++++++++++++++
 7 files changed, 622 insertions(+), 40 deletions(-)
 create mode 100755 tools/testing/selftests/net/fib_tests.sh

-- 
2.14.3

[RFC PATCH net-next 01/19] ipv6: Remove redundant route flushing during namespace dismantle

[Ido Schimmel]

By the time fib6_net_exit() is executed all the netdevs in the namespace
have been either unregistered or pushed back to the default namespace.
That is because pernet subsys operations are always ordered before
pernet device operations and therefore invoked after them during
namespace dismantle.

Thus, all the routing tables in the namespace are empty by the time
fib6_net_exit() is invoked and the call to rt6_ifdown() can be removed.

This allows us to simplify the condition in fib6_ifdown() as it's only
ever called with an actual netdev.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/ip6_fib.c | 1 -
 net/ipv6/route.c   | 8 +++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index a64d559fa513..3bbb89d8187d 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -2103,7 +2103,6 @@ static void fib6_net_exit(struct net *net)
 {
 	unsigned int i;
 
-	rt6_ifdown(net, NULL);
 	del_timer_sync(&net->ipv6.ip6_fib_timer);
 
 	for (i = 0; i < FIB6_TABLE_HASHSZ; i++) {
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 2490280b3394..c557362daa23 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3470,10 +3470,9 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
 	const struct arg_dev_net *adn = arg;
 	const struct net_device *dev = adn->dev;
 
-	if ((rt->dst.dev == dev || !dev) &&
+	if (rt->dst.dev == dev &&
 	    rt != adn->net->ipv6.ip6_null_entry &&
-	    (rt->rt6i_nsiblings == 0 ||
-	     (dev && netdev_unregistering(dev)) ||
+	    (rt->rt6i_nsiblings == 0 || netdev_unregistering(dev) ||
 	     !rt->rt6i_idev->cnf.ignore_routes_with_linkdown))
 		return -1;
 
@@ -3488,8 +3487,7 @@ void rt6_ifdown(struct net *net, struct net_device *dev)
 	};
 
 	fib6_clean_all(net, fib6_ifdown, &adn);
-	if (dev)
-		rt6_uncached_list_flush_dev(net, dev);
+	rt6_uncached_list_flush_dev(net, dev);
 }
 
 struct rt6_mtu_change_arg {
-- 
2.14.3

[RFC PATCH net-next 02/19] ipv6: Mark dead nexthops with appropriate flags

[Ido Schimmel]

When a netdev is put administratively down or unregistered all the
nexthops using it as their nexthop device should be marked with the
'dead' and 'linkdown' flags.

Currently, when a route is dumped its nexthop device is tested and the
flags are set accordingly. A similar check is performed during route
lookup.

Instead, we can simply mark the nexthops based on netdev events and
avoid checking the netdev's state during route dump and lookup.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c557362daa23..f5eda0aeab55 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3473,8 +3473,10 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
 	if (rt->dst.dev == dev &&
 	    rt != adn->net->ipv6.ip6_null_entry &&
 	    (rt->rt6i_nsiblings == 0 || netdev_unregistering(dev) ||
-	     !rt->rt6i_idev->cnf.ignore_routes_with_linkdown))
+	     !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)) {
+		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
 		return -1;
+	}
 
 	return 0;
 }
-- 
2.14.3

[RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[Ido Schimmel]

Previous patch marked nexthops with the 'dead' and 'linkdown' flags.
Clear these flags when the netdev comes back up.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/ip6_route.h |  1 +
 net/ipv6/addrconf.c     |  3 +++
 net/ipv6/route.c        | 29 +++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 18e442ea93d8..caad39198c2a 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -169,6 +169,7 @@ void rt6_ifdown(struct net *net, struct net_device *dev);
 void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
 void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
 void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
+void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
 
 static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
 {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ed06b1190f05..b6405568ed7b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			if (run_pending)
 				addrconf_dad_run(idev);
 
+			/* Device has an address by now */
+			rt6_sync_up(dev, RTNH_F_DEAD);
+
 			/*
 			 * If the MTU changed during the interface down,
 			 * when the interface up, the changed MTU must be
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f5eda0aeab55..4796d87e0b93 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3459,6 +3459,35 @@ void rt6_clean_tohost(struct net *net, struct in6_addr *gateway)
 	fib6_clean_all(net, fib6_clean_tohost, gateway);
 }
 
+struct arg_netdev_event {
+	const struct net_device *dev;
+	unsigned int nh_flags;
+};
+
+static int fib6_ifup(struct rt6_info *rt, void *p_arg)
+{
+	const struct arg_netdev_event *arg = p_arg;
+	const struct net *net = dev_net(arg->dev);
+
+	if (rt != net->ipv6.ip6_null_entry && rt->dst.dev == arg->dev)
+		rt->rt6i_nh_flags &= ~arg->nh_flags;
+
+	return 0;
+}
+
+void rt6_sync_up(struct net_device *dev, unsigned int nh_flags)
+{
+	struct arg_netdev_event arg = {
+		.dev = dev,
+		.nh_flags = nh_flags,
+	};
+
+	if (nh_flags & RTNH_F_DEAD && netif_carrier_ok(dev))
+		arg.nh_flags |= RTNH_F_LINKDOWN;
+
+	fib6_clean_all(dev_net(dev), fib6_ifup, &arg);
+}
+
 struct arg_dev_net {
 	struct net_device *dev;
 	struct net *net;
-- 
2.14.3

[RFC PATCH net-next 04/19] ipv6: Prepare to handle multiple netdev events

[Ido Schimmel]

To make IPv6 more in line with IPv4 we need to be able to respond
differently to different netdev events. For example, when a netdev is
unregistered all the routes using it as their nexthop device should be
flushed, whereas when the netdev's carrier changes only the 'linkdown'
flag should be toggled.

Currently, this is not possible, as the function that traverses the
routing tables is not aware of the triggering event.

Propagate the triggering event down, so that it could be used in later
patches.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/ip6_route.h |  1 +
 net/ipv6/addrconf.c     |  4 ++--
 net/ipv6/route.c        | 35 ++++++++++++++++++++---------------
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index caad39198c2a..8205402ff3dc 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -170,6 +170,7 @@ void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
 void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
 void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
 void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
+void rt6_disable_ip(struct net_device *dev, unsigned long event);
 
 static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
 {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index b6405568ed7b..a13e1ffe87ec 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3580,6 +3580,7 @@ static bool addr_is_local(const struct in6_addr *addr)
 
 static int addrconf_ifdown(struct net_device *dev, int how)
 {
+	unsigned long event = how ? NETDEV_UNREGISTER : NETDEV_DOWN;
 	struct net *net = dev_net(dev);
 	struct inet6_dev *idev;
 	struct inet6_ifaddr *ifa, *tmp;
@@ -3589,8 +3590,7 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 
 	ASSERT_RTNL();
 
-	rt6_ifdown(net, dev);
-	neigh_ifdown(&nd_tbl, dev);
+	rt6_disable_ip(dev, event);
 
 	idev = __in6_dev_get(dev);
 	if (!idev)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4796d87e0b93..9d1c34f2c7ed 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3461,7 +3461,10 @@ void rt6_clean_tohost(struct net *net, struct in6_addr *gateway)
 
 struct arg_netdev_event {
 	const struct net_device *dev;
-	unsigned int nh_flags;
+	union {
+		unsigned int nh_flags;
+		unsigned long event;
+	};
 };
 
 static int fib6_ifup(struct rt6_info *rt, void *p_arg)
@@ -3488,19 +3491,15 @@ void rt6_sync_up(struct net_device *dev, unsigned int nh_flags)
 	fib6_clean_all(dev_net(dev), fib6_ifup, &arg);
 }
 
-struct arg_dev_net {
-	struct net_device *dev;
-	struct net *net;
-};
-
 /* called with write lock held for table with rt */
-static int fib6_ifdown(struct rt6_info *rt, void *arg)
+static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 {
-	const struct arg_dev_net *adn = arg;
-	const struct net_device *dev = adn->dev;
+	const struct arg_netdev_event *arg = p_arg;
+	const struct net_device *dev = arg->dev;
+	const struct net *net = dev_net(dev);
 
 	if (rt->dst.dev == dev &&
-	    rt != adn->net->ipv6.ip6_null_entry &&
+	    rt != net->ipv6.ip6_null_entry &&
 	    (rt->rt6i_nsiblings == 0 || netdev_unregistering(dev) ||
 	     !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)) {
 		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
@@ -3510,15 +3509,21 @@ static int fib6_ifdown(struct rt6_info *rt, void *arg)
 	return 0;
 }
 
-void rt6_ifdown(struct net *net, struct net_device *dev)
+static void rt6_sync_down_dev(struct net_device *dev, unsigned long event)
 {
-	struct arg_dev_net adn = {
+	struct arg_netdev_event arg = {
 		.dev = dev,
-		.net = net,
+		.event = event,
 	};
 
-	fib6_clean_all(net, fib6_ifdown, &adn);
-	rt6_uncached_list_flush_dev(net, dev);
+	fib6_clean_all(dev_net(dev), fib6_ifdown, &arg);
+}
+
+void rt6_disable_ip(struct net_device *dev, unsigned long event)
+{
+	rt6_sync_down_dev(dev, event);
+	rt6_uncached_list_flush_dev(dev_net(dev), dev);
+	neigh_ifdown(&nd_tbl, dev);
 }
 
 struct rt6_mtu_change_arg {
-- 
2.14.3

[RFC PATCH net-next 05/19] ipv6: Set nexthop flags upon carrier change

[Ido Schimmel]

Similar to IPv4, when the carrier of a netdev changes we should toggle
the 'linkdown' flag on all the nexthops using it as their nexthop
device.

This will later allow us to test for the presence of this flag during
route lookup and dump.

Up until commit 4832c30d5458 ("net: ipv6: put host and anycast routes on
device with address") host and anycast routes used the loopback netdev
as their nexthop device and thus were not marked with the 'linkdown'
flag. The patch preserves this behavior and allows one to ping the local
address even when the nexthop device does not have a carrier and the
'ignore_routes_with_linkdown' sysctl is set.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/ip6_route.h |  1 +
 net/ipv6/addrconf.c     |  2 ++
 net/ipv6/route.c        | 23 +++++++++++++++++------
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 8205402ff3dc..60902dde5ff2 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -171,6 +171,7 @@ void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
 void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
 void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
 void rt6_disable_ip(struct net_device *dev, unsigned long event);
+void rt6_sync_down_dev(struct net_device *dev, unsigned long event);
 
 static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
 {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index a13e1ffe87ec..2435f7ab070b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3438,6 +3438,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 		} else if (event == NETDEV_CHANGE) {
 			if (!addrconf_link_ready(dev)) {
 				/* device is still not ready. */
+				rt6_sync_down_dev(dev, event);
 				break;
 			}
 
@@ -3449,6 +3450,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 					 * multicast snooping switches
 					 */
 					ipv6_mc_up(idev);
+					rt6_sync_up(dev, RTNH_F_LINKDOWN);
 					break;
 				}
 				idev->if_flags |= IF_READY;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 9d1c34f2c7ed..1f331f332491 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3498,18 +3498,29 @@ static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 	const struct net_device *dev = arg->dev;
 	const struct net *net = dev_net(dev);
 
-	if (rt->dst.dev == dev &&
-	    rt != net->ipv6.ip6_null_entry &&
-	    (rt->rt6i_nsiblings == 0 || netdev_unregistering(dev) ||
-	     !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)) {
-		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
+	if (rt->dst.dev != dev || rt == net->ipv6.ip6_null_entry)
+		return 0;
+
+	switch (arg->event) {
+	case NETDEV_UNREGISTER:
 		return -1;
+	case NETDEV_DOWN:
+		if (rt->rt6i_nsiblings == 0 ||
+		    !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
+			return -1;
+		rt->rt6i_nh_flags |= RTNH_F_DEAD;
+		/* fall through */
+	case NETDEV_CHANGE:
+		if (rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST))
+			break;
+		rt->rt6i_nh_flags |= RTNH_F_LINKDOWN;
+		break;
 	}
 
 	return 0;
 }
 
-static void rt6_sync_down_dev(struct net_device *dev, unsigned long event)
+void rt6_sync_down_dev(struct net_device *dev, unsigned long event)
 {
 	struct arg_netdev_event arg = {
 		.dev = dev,
-- 
2.14.3

[RFC PATCH net-next 06/19] ipv6: Set nexthop flags during route creation

[Ido Schimmel]

It is valid to install routes with a nexthop device that does not have a
carrier, so we need to make sure they're marked accordingly.

As explained in the previous patch, host and anycast routes are never
marked with the 'linkdown' flag.

Note that reject routes are unaffected, as these use the loopback device
which always has a carrier.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 1f331f332491..13238c4e76cc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2746,6 +2746,9 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
 	rt->rt6i_flags = cfg->fc_flags;
 
 install_route:
+	if (!(rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST)) &&
+	    !netif_carrier_ok(dev))
+		rt->rt6i_nh_flags |= RTNH_F_LINKDOWN;
 	rt->dst.dev = dev;
 	rt->rt6i_idev = idev;
 	rt->rt6i_table = table;
-- 
2.14.3

[RFC PATCH net-next 07/19] ipv6: Check nexthop flags during route lookup instead of carrier

[Ido Schimmel]

Now that the RTNH_F_LINKDOWN flag is set in nexthops, we can avoid the
need to dereference the nexthop device and check its carrier and instead
check for the presence of the flag.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 13238c4e76cc..3c4e679614fd 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -474,7 +474,7 @@ static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
 			if (route_choosen == 0) {
 				struct inet6_dev *idev = sibling->rt6i_idev;
 
-				if (!netif_carrier_ok(sibling->dst.dev) &&
+				if (sibling->rt6i_nh_flags & RTNH_F_LINKDOWN &&
 				    idev->cnf.ignore_routes_with_linkdown)
 					break;
 				if (rt6_score_route(sibling, oif, strict) < 0)
@@ -679,10 +679,9 @@ static struct rt6_info *find_match(struct rt6_info *rt, int oif, int strict,
 	int m;
 	bool match_do_rr = false;
 	struct inet6_dev *idev = rt->rt6i_idev;
-	struct net_device *dev = rt->dst.dev;
 
-	if (dev && !netif_carrier_ok(dev) &&
-	    idev->cnf.ignore_routes_with_linkdown &&
+	if (idev->cnf.ignore_routes_with_linkdown &&
+	    rt->rt6i_nh_flags & RTNH_F_LINKDOWN &&
 	    !(strict & RT6_LOOKUP_F_IGNORE_LINKSTATE))
 		goto out;
 
-- 
2.14.3

[RFC PATCH net-next 08/19] ipv6: Check nexthop flags in route dump instead of carrier

[Ido Schimmel]

Similar to previous patch, there is no need to check for the carrier of
the nexthop device when dumping the route and we can instead check for
the presence of the RTNH_F_LINKDOWN flag.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 3c4e679614fd..2bef8d1f279d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4039,7 +4039,7 @@ static size_t rt6_nlmsg_size(struct rt6_info *rt)
 static int rt6_nexthop_info(struct sk_buff *skb, struct rt6_info *rt,
 			    unsigned int *flags, bool skip_oif)
 {
-	if (!netif_running(rt->dst.dev) || !netif_carrier_ok(rt->dst.dev)) {
+	if (rt->rt6i_nh_flags & RTNH_F_LINKDOWN) {
 		*flags |= RTNH_F_LINKDOWN;
 		if (rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
 			*flags |= RTNH_F_DEAD;
-- 
2.14.3

[RFC PATCH net-next 09/19] ipv6: Ignore dead routes during lookup

[Ido Schimmel]

Currently, dead routes are only present in the routing tables in case
the 'ignore_routes_with_linkdown' sysctl is set. Otherwise, they are
flushed.

Subsequent patches are going to remove the reliance on this sysctl and
make IPv6 more consistent with IPv4.

Before this is done, we need to make sure dead routes are skipped during
route lookup, so as to not cause packet loss.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 2bef8d1f279d..0927b02b2819 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -474,6 +474,8 @@ static struct rt6_info *rt6_multipath_select(struct rt6_info *match,
 			if (route_choosen == 0) {
 				struct inet6_dev *idev = sibling->rt6i_idev;
 
+				if (sibling->rt6i_nh_flags & RTNH_F_DEAD)
+					break;
 				if (sibling->rt6i_nh_flags & RTNH_F_LINKDOWN &&
 				    idev->cnf.ignore_routes_with_linkdown)
 					break;
@@ -499,12 +501,15 @@ static inline struct rt6_info *rt6_device_match(struct net *net,
 	struct rt6_info *local = NULL;
 	struct rt6_info *sprt;
 
-	if (!oif && ipv6_addr_any(saddr))
-		goto out;
+	if (!oif && ipv6_addr_any(saddr) && !(rt->rt6i_nh_flags & RTNH_F_DEAD))
+		return rt;
 
 	for (sprt = rt; sprt; sprt = rcu_dereference(sprt->rt6_next)) {
 		struct net_device *dev = sprt->dst.dev;
 
+		if (sprt->rt6i_nh_flags & RTNH_F_DEAD)
+			continue;
+
 		if (oif) {
 			if (dev->ifindex == oif)
 				return sprt;
@@ -533,8 +538,8 @@ static inline struct rt6_info *rt6_device_match(struct net *net,
 		if (flags & RT6_LOOKUP_F_IFACE)
 			return net->ipv6.ip6_null_entry;
 	}
-out:
-	return rt;
+
+	return rt->rt6i_nh_flags & RTNH_F_DEAD ? net->ipv6.ip6_null_entry : rt;
 }
 
 #ifdef CONFIG_IPV6_ROUTER_PREF
@@ -680,6 +685,9 @@ static struct rt6_info *find_match(struct rt6_info *rt, int oif, int strict,
 	bool match_do_rr = false;
 	struct inet6_dev *idev = rt->rt6i_idev;
 
+	if (rt->rt6i_nh_flags & RTNH_F_DEAD)
+		goto out;
+
 	if (idev->cnf.ignore_routes_with_linkdown &&
 	    rt->rt6i_nh_flags & RTNH_F_LINKDOWN &&
 	    !(strict & RT6_LOOKUP_F_IGNORE_LINKSTATE))
@@ -2153,6 +2161,8 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
 	fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
 restart:
 	for_each_fib6_node_rt_rcu(fn) {
+		if (rt->rt6i_nh_flags & RTNH_F_DEAD)
+			continue;
 		if (rt6_check_expired(rt))
 			continue;
 		if (rt->dst.error)
-- 
2.14.3

[RFC PATCH net-next 10/19] ipv6: Report dead flag during route dump

[Ido Schimmel]

Up until now the RTNH_F_DEAD flag was only reported in route dump when
the 'ignore_routes_with_linkdown' sysctl was set. This is expected as
dead routes were flushed otherwise.

The reliance on this sysctl is going to be removed, so we need to report
the flag regardless of the sysctl's value.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0927b02b2819..4d0d32309bcc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4049,6 +4049,9 @@ static size_t rt6_nlmsg_size(struct rt6_info *rt)
 static int rt6_nexthop_info(struct sk_buff *skb, struct rt6_info *rt,
 			    unsigned int *flags, bool skip_oif)
 {
+	if (rt->rt6i_nh_flags & RTNH_F_DEAD)
+		*flags |= RTNH_F_DEAD;
+
 	if (rt->rt6i_nh_flags & RTNH_F_LINKDOWN) {
 		*flags |= RTNH_F_LINKDOWN;
 		if (rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
-- 
2.14.3

[RFC PATCH net-next 11/19] ipv6: Add explicit flush indication to routes

[Ido Schimmel]

When routes that are a part of a multipath route are evaluated by
fib6_ifdown() in response to NETDEV_DOWN and NETDEV_UNREGISTER events
the state of their sibling routes is not considered.

This will change in subsequent patches in order to align IPv6 with
IPv4's behavior. For example, when the last sibling in a multipath route
becomes dead, the entire multipath route needs to be removed.

To prevent the tree walker from re-evaluating all the sibling routes
each time, we can simply evaluate them once - when the first sibling is
traversed.

If we determine the entire multipath route needs to be removed, then the
'should_flush' bit is set in all the siblings, which will cause the
walker to flush them when it traverses them.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/ip6_fib.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 44d96a91e745..affea1aa6ae4 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -173,7 +173,8 @@ struct rt6_info {
 	unsigned short			rt6i_nfheader_len;
 	u8				rt6i_protocol;
 	u8				exception_bucket_flushed:1,
-					unused:7;
+					should_flush:1,
+					unused:6;
 };
 
 #define for_each_fib6_node_rt_rcu(fn)					\
-- 
2.14.3

[RFC PATCH net-next 12/19] ipv6: Teach tree walker to skip multipath routes

[Ido Schimmel]

As explained in previous patch, fib6_ifdown() needs to consider the
state of all the sibling routes when a multipath route is traversed.

This is done by evaluating all the siblings when the first sibling in a
multipath route is traversed. If the multipath route does not need to be
flushed (e.g., not all siblings are dead), then we should just skip the
multipath route as our work is done.

Have the tree walker jump to the last sibling when it is determined that
the multipath route needs to be skipped.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/ip6_fib.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 3bbb89d8187d..5e4b5eef0ddd 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1887,7 +1887,7 @@ static int fib6_clean_node(struct fib6_walker *w)
 
 	for_each_fib6_walker_rt(w) {
 		res = c->func(rt, c->arg);
-		if (res < 0) {
+		if (res == -1) {
 			w->leaf = rt;
 			res = fib6_del(rt, &info);
 			if (res) {
@@ -1900,6 +1900,12 @@ static int fib6_clean_node(struct fib6_walker *w)
 				continue;
 			}
 			return 0;
+		} else if (res == -2) {
+			if (WARN_ON(!rt->rt6i_nsiblings))
+				continue;
+			rt = list_last_entry(&rt->rt6i_siblings,
+					     struct rt6_info, rt6i_siblings);
+			continue;
 		}
 		WARN_ON(res != 0);
 	}
@@ -1911,7 +1917,8 @@ static int fib6_clean_node(struct fib6_walker *w)
  *	Convenient frontend to tree walker.
  *
  *	func is called on each route.
- *		It may return -1 -> delete this route.
+ *		It may return -2 -> skip multipath route.
+ *			      -1 -> delete this route.
  *		              0  -> continue walking
  */
 
-- 
2.14.3

[RFC PATCH net-next 13/19] ipv6: Flush all sibling routes upon NETDEV_UNREGISTER

[Ido Schimmel]

IPv4 and IPv6 react differently to a netdev being unregistered. In IPv4,
in case the netdev is used as a nexthop device in a multipath route, the
entire route is flushed.

However, IPv6 only removes the nexthops associated with the unregistered
netdev.

Align IPv4 and IPv6 and flush all the sibling routes when a nexthop
device is unregistered.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4d0d32309bcc..cf895989f2bb 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3503,6 +3503,29 @@ void rt6_sync_up(struct net_device *dev, unsigned int nh_flags)
 	fib6_clean_all(dev_net(dev), fib6_ifup, &arg);
 }
 
+static bool rt6_multipath_uses_dev(const struct rt6_info *rt,
+				   const struct net_device *dev)
+{
+	struct rt6_info *iter;
+
+	if (rt->dst.dev == dev)
+		return true;
+	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
+		if (iter->dst.dev == dev)
+			return true;
+
+	return false;
+}
+
+static void rt6_multipath_flush(struct rt6_info *rt)
+{
+	struct rt6_info *iter;
+
+	rt->should_flush = 1;
+	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
+		iter->should_flush = 1;
+}
+
 /* called with write lock held for table with rt */
 static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 {
@@ -3510,20 +3533,31 @@ static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 	const struct net_device *dev = arg->dev;
 	const struct net *net = dev_net(dev);
 
-	if (rt->dst.dev != dev || rt == net->ipv6.ip6_null_entry)
+	if (rt == net->ipv6.ip6_null_entry)
 		return 0;
 
 	switch (arg->event) {
 	case NETDEV_UNREGISTER:
-		return -1;
+		if (rt->should_flush)
+			return -1;
+		if (!rt->rt6i_nsiblings)
+			return rt->dst.dev == dev ? -1 : 0;
+		if (rt6_multipath_uses_dev(rt, dev)) {
+			rt6_multipath_flush(rt);
+			return -1;
+		}
+		return -2;
 	case NETDEV_DOWN:
+		if (rt->dst.dev != dev)
+			break;
 		if (rt->rt6i_nsiblings == 0 ||
 		    !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
 			return -1;
-		rt->rt6i_nh_flags |= RTNH_F_DEAD;
-		/* fall through */
+		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
+		break;
 	case NETDEV_CHANGE:
-		if (rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST))
+		if (rt->dst.dev != dev ||
+		    rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST))
 			break;
 		rt->rt6i_nh_flags |= RTNH_F_LINKDOWN;
 		break;
-- 
2.14.3

[RFC PATCH net-next 14/19] ipv6: Export sernum update function

[Ido Schimmel]

We are going to allow dead routes to stay in the FIB tree (e.g., when
they are part of a multipath route, directly connected route with no
carrier) and revive them when their nexthop device gains carrier or when
it is put administratively up.

This is equivalent to the addition of the route to the FIB tree and we
should therefore take care of updating the sernum of all the parent
nodes of the node where the route is stored. Otherwise, we risk sockets
caching and using sub-optimal dst entries.

Export the function that performs the above, so that it could be invoked
from fib6_ifup() later on.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/ip6_fib.h |  1 +
 net/ipv6/ip6_fib.c    | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index affea1aa6ae4..ddf53dd1e948 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -405,6 +405,7 @@ unsigned int fib6_tables_seq_read(struct net *net);
 int fib6_tables_dump(struct net *net, struct notifier_block *nb);
 
 void fib6_update_sernum(struct rt6_info *rt);
+void fib6_update_sernum_upto_root(struct net *net, struct rt6_info *rt);
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 int fib6_rules_init(void);
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 5e4b5eef0ddd..c1bbe7bf9fdd 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1102,8 +1102,8 @@ void fib6_force_start_gc(struct net *net)
 			  jiffies + net->ipv6.sysctl.ip6_rt_gc_interval);
 }
 
-static void fib6_update_sernum_upto_root(struct rt6_info *rt,
-					 int sernum)
+static void __fib6_update_sernum_upto_root(struct rt6_info *rt,
+					   int sernum)
 {
 	struct fib6_node *fn = rcu_dereference_protected(rt->rt6i_node,
 				lockdep_is_held(&rt->rt6i_table->tb6_lock));
@@ -1117,6 +1117,11 @@ static void fib6_update_sernum_upto_root(struct rt6_info *rt,
 	}
 }
 
+void fib6_update_sernum_upto_root(struct net *net, struct rt6_info *rt)
+{
+	__fib6_update_sernum_upto_root(rt, fib6_new_sernum(net));
+}
+
 /*
  *	Add routing information to the routing tree.
  *	<destination addr>/<source addr>
@@ -1230,7 +1235,7 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt,
 
 	err = fib6_add_rt2node(fn, rt, info, mxc, extack);
 	if (!err) {
-		fib6_update_sernum_upto_root(rt, sernum);
+		__fib6_update_sernum_upto_root(rt, sernum);
 		fib6_start_gc(info->nl_net, rt);
 	}
 
-- 
2.14.3

[RFC PATCH net-next 15/19] ipv6: Take table lock outside of sernum update function

[Ido Schimmel]

The next patch is going to allow dead routes to remain in the FIB tree
in certain situations.

When this happens we need to be sure to bump the sernum of the nodes
where these are stored so that potential copies cached in sockets are
invalidated.

The function that performs this update assumes the table lock is not
taken when it is invoked, but that will not be the case when it is
invoked by the tree walker.

Have the function assume the lock is taken and make the single caller
take the lock itself.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/ip6_fib.c | 5 +----
 net/ipv6/route.c   | 2 ++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index c1bbe7bf9fdd..edda5ad3b405 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -107,16 +107,13 @@ enum {
 
 void fib6_update_sernum(struct rt6_info *rt)
 {
-	struct fib6_table *table = rt->rt6i_table;
 	struct net *net = dev_net(rt->dst.dev);
 	struct fib6_node *fn;
 
-	spin_lock_bh(&table->tb6_lock);
 	fn = rcu_dereference_protected(rt->rt6i_node,
-			lockdep_is_held(&table->tb6_lock));
+			lockdep_is_held(&rt->rt6i_table->tb6_lock));
 	if (fn)
 		fn->fn_sernum = fib6_new_sernum(net);
-	spin_unlock_bh(&table->tb6_lock);
 }
 
 /*
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cf895989f2bb..ab95e07b5e74 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1353,7 +1353,9 @@ static int rt6_insert_exception(struct rt6_info *nrt,
 
 	/* Update fn->fn_sernum to invalidate all cached dst */
 	if (!err) {
+		spin_lock_bh(&ort->rt6i_table->tb6_lock);
 		fib6_update_sernum(ort);
+		spin_unlock_bh(&ort->rt6i_table->tb6_lock);
 		fib6_force_start_gc(net);
 	}
 
-- 
2.14.3

[RFC PATCH net-next 16/19] ipv6: Flush multipath routes when all siblings are dead

[Ido Schimmel]

By default, IPv6 deletes nexthops from a multipath route when the
nexthop device is put administratively down. This differs from IPv4
where the nexthops are kept, but marked with the RTNH_F_DEAD flag. A
multipath route is flushed when all of its nexthops become dead.

Align IPv6 with IPv4 and have it conform to the same guidelines.

In case the multipath route needs to be flushed, its siblings are
flushed one by one. Otherwise, the nexthops are marked with the
appropriate flags and the tree walker is instructed to skip all the
siblings.

As explained in previous patches, care is taken to update the sernum of
the affected tree nodes, so as to prevent the use of wrong dst entries.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/ipv6/route.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ab95e07b5e74..9b142eb83f3c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -3486,8 +3486,10 @@ static int fib6_ifup(struct rt6_info *rt, void *p_arg)
 	const struct arg_netdev_event *arg = p_arg;
 	const struct net *net = dev_net(arg->dev);
 
-	if (rt != net->ipv6.ip6_null_entry && rt->dst.dev == arg->dev)
+	if (rt != net->ipv6.ip6_null_entry && rt->dst.dev == arg->dev) {
 		rt->rt6i_nh_flags &= ~arg->nh_flags;
+		fib6_update_sernum_upto_root(dev_net(rt->dst.dev), rt);
+	}
 
 	return 0;
 }
@@ -3528,6 +3530,35 @@ static void rt6_multipath_flush(struct rt6_info *rt)
 		iter->should_flush = 1;
 }
 
+static unsigned int rt6_multipath_dead_count(const struct rt6_info *rt,
+					     const struct net_device *down_dev)
+{
+	struct rt6_info *iter;
+	unsigned int dead = 0;
+
+	if (rt->dst.dev == down_dev || rt->rt6i_nh_flags & RTNH_F_DEAD)
+		dead++;
+	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
+		if (iter->dst.dev == down_dev ||
+		    iter->rt6i_nh_flags & RTNH_F_DEAD)
+			dead++;
+
+	return dead;
+}
+
+static void rt6_multipath_nh_flags_set(struct rt6_info *rt,
+				       const struct net_device *dev,
+				       unsigned int nh_flags)
+{
+	struct rt6_info *iter;
+
+	if (rt->dst.dev == dev)
+		rt->rt6i_nh_flags |= nh_flags;
+	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
+		if (iter->dst.dev == dev)
+			iter->rt6i_nh_flags |= nh_flags;
+}
+
 /* called with write lock held for table with rt */
 static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 {
@@ -3550,13 +3581,21 @@ static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
 		}
 		return -2;
 	case NETDEV_DOWN:
-		if (rt->dst.dev != dev)
-			break;
-		if (rt->rt6i_nsiblings == 0 ||
-		    !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
+		if (rt->should_flush)
 			return -1;
-		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
-		break;
+		if (!rt->rt6i_nsiblings)
+			return rt->dst.dev == dev ? -1 : 0;
+		if (rt6_multipath_uses_dev(rt, dev)) {
+			if (rt->rt6i_nsiblings + 1 ==
+			    rt6_multipath_dead_count(rt, dev)) {
+				rt6_multipath_flush(rt);
+				return -1;
+			}
+			rt6_multipath_nh_flags_set(rt, dev, RTNH_F_DEAD |
+						   RTNH_F_LINKDOWN);
+			fib6_update_sernum(rt);
+		}
+		return -2;
 	case NETDEV_CHANGE:
 		if (rt->dst.dev != dev ||
 		    rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST))
-- 
2.14.3

[RFC PATCH net-next 17/19] selftests: fib_tests: Add test cases for IPv4/IPv6 FIB

[Ido Schimmel]

Add test cases to check that IPv4 and IPv6 react the same when a netdev
is being unregistered.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/net/Makefile     |   1 +
 tools/testing/selftests/net/fib_tests.sh | 145 +++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)
 create mode 100755 tools/testing/selftests/net/fib_tests.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 500c74db746c..d7c30d366935 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -5,6 +5,7 @@ CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
 CFLAGS += -I../../../../usr/include/
 
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
+TEST_PROGS += fib_tests.sh
 TEST_GEN_FILES =  socket
 TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy
 TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
new file mode 100755
index 000000000000..c517a599047b
--- /dev/null
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -0,0 +1,145 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This test is for checking IPv4 and IPv6 FIB behavior in response to
+# different events.
+
+ret=0
+
+check_err()
+{
+	if [ $ret -eq 0 ]; then
+		ret=$1
+	fi
+}
+
+check_fail()
+{
+	if [ $1 -eq 0 ]; then
+		ret=1
+	fi
+}
+
+netns_create()
+{
+	local testns=$1
+
+	ip netns add $testns
+	ip netns exec $testns ip link set dev lo up
+}
+
+fib_unreg_unicast_test()
+{
+	ret=0
+
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip link del dev dummy0
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_fail $?
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: unicast route test"
+		return 1
+	fi
+	echo "PASS: unicast route test"
+}
+
+fib_unreg_multipath_test()
+{
+	ret=0
+
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip link add dummy1 type dummy
+	ip netns exec testns ip link set dev dummy1 up
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip address add 192.0.2.1/24 dev dummy1
+	ip netns exec testns ip -6 address add 2001:db8:2::1/64 dev dummy1
+
+	ip netns exec testns ip route add 203.0.113.0/24 \
+		nexthop via 198.51.100.2 dev dummy0 \
+		nexthop via 192.0.2.2 dev dummy1
+	ip netns exec testns ip -6 route add 2001:db8:3::/64 \
+		nexthop via 2001:db8:1::2 dev dummy0 \
+		nexthop via 2001:db8:2::2 dev dummy1
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip link del dev dummy0
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 &> /dev/null
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	check_fail $?
+
+	ip netns exec testns ip link del dev dummy1
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: multipath route test"
+		return 1
+	fi
+	echo "PASS: multipath route test"
+}
+
+fib_unreg_test()
+{
+	echo "Running netdev unregister tests"
+
+	fib_unreg_unicast_test
+	fib_unreg_multipath_test
+}
+
+fib_test()
+{
+	fib_unreg_test
+}
+
+if [ "$(id -u)" -ne 0 ];then
+	echo "SKIP: Need root privileges"
+	exit 0
+fi
+
+if [ ! -x "$(command -v ip)" ]; then
+	echo "SKIP: Could not run test without ip tool"
+	exit 0
+fi
+
+ip route help 2>&1 | grep -q fibmatch
+if [ $? -ne 0 ]; then
+	echo "SKIP: iproute2 too old, missing fibmatch"
+	exit 0
+fi
+
+fib_test
+
+exit $ret
-- 
2.14.3

[RFC PATCH net-next 18/19] selftests: fib_tests: Add test cases for netdev down

[Ido Schimmel]

Check that IPv4 and IPv6 react the same when a netdev is being put
administratively down.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/net/fib_tests.sh | 141 +++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index c517a599047b..b245bac956d1 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -119,9 +119,150 @@ fib_unreg_test()
 	fib_unreg_multipath_test
 }
 
+fib_down_unicast_test()
+{
+	ret=0
+
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip link set dev dummy0 down
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_fail $?
+
+	ip netns exec testns ip link del dev dummy0
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: unicast route test"
+		return 1
+	fi
+	echo "PASS: unicast route test"
+}
+
+fib_down_multipath_test_do()
+{
+	local down_dev=$1
+	local up_dev=$2
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 \
+		oif $down_dev &> /dev/null
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 \
+		oif $down_dev &> /dev/null
+	check_fail $?
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 \
+		oif $up_dev &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 \
+		oif $up_dev &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 | \
+		grep $down_dev | grep -q "dead linkdown"
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 | \
+		grep $down_dev | grep -q "dead linkdown"
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 | \
+		grep $up_dev | grep -q "dead linkdown"
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 | \
+		grep $up_dev | grep -q "dead linkdown"
+	check_fail $?
+}
+
+fib_down_multipath_test()
+{
+	ret=0
+
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip link add dummy1 type dummy
+	ip netns exec testns ip link set dev dummy1 up
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip address add 192.0.2.1/24 dev dummy1
+	ip netns exec testns ip -6 address add 2001:db8:2::1/64 dev dummy1
+
+	ip netns exec testns ip route add 203.0.113.0/24 \
+		nexthop via 198.51.100.2 dev dummy0 \
+		nexthop via 192.0.2.2 dev dummy1
+	ip netns exec testns ip -6 route add 2001:db8:3::/64 \
+		nexthop via 2001:db8:1::2 dev dummy0 \
+		nexthop via 2001:db8:2::2 dev dummy1
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip link set dev dummy0 down
+	check_err $?
+
+	fib_down_multipath_test_do "dummy0" "dummy1"
+
+	ip netns exec testns ip link set dev dummy0 up
+	check_err $?
+	ip netns exec testns ip link set dev dummy1 down
+	check_err $?
+
+	fib_down_multipath_test_do "dummy1" "dummy0"
+
+	ip netns exec testns ip link set dev dummy0 down
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 203.0.113.1 &> /dev/null
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	check_fail $?
+
+	ip netns exec testns ip link del dev dummy1
+	ip netns exec testns ip link del dev dummy0
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: multipath route test"
+		return 1
+	fi
+	echo "PASS: multipath route test"
+}
+
+fib_down_test()
+{
+	echo "Running netdev down tests"
+
+	fib_down_unicast_test
+	fib_down_multipath_test
+}
+
 fib_test()
 {
 	fib_unreg_test
+	fib_down_test
 }
 
 if [ "$(id -u)" -ne 0 ];then
-- 
2.14.3

[RFC PATCH net-next 19/19] selftests: fib_tests: Add test cases for netdev carrier change

[Ido Schimmel]

Check that IPv4 and IPv6 react the same when the carrier of a netdev is
toggled. Local routes should not be affected by this, whereas unicast
routes should.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 tools/testing/selftests/net/fib_tests.sh | 142 +++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index b245bac956d1..9f2208e2d676 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -259,10 +259,152 @@ fib_down_test()
 	fib_down_multipath_test
 }
 
+fib_carrier_local_test()
+{
+	ret=0
+
+	# Local routes should not be affected when carrier changes.
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip link set dev dummy0 carrier on
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 198.51.100.1 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::1 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.1 | \
+		grep -q "linkdown"
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::1 | \
+		grep -q "linkdown"
+	check_fail $?
+
+	ip netns exec testns ip link set dev dummy0 carrier off
+
+	ip netns exec testns ip route get fibmatch 198.51.100.1 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::1 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.1 | \
+		grep -q "linkdown"
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::1 | \
+		grep -q "linkdown"
+	check_fail $?
+
+	ip netns exec testns ip address add 192.0.2.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:2::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 192.0.2.1 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:2::1 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 192.0.2.1 | \
+		grep -q "linkdown"
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:2::1 | \
+		grep -q "linkdown"
+	check_fail $?
+
+	ip netns exec testns ip link del dev dummy0
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: local route carrier test"
+		return 1
+	fi
+	echo "PASS: local route carrier test"
+}
+
+fib_carrier_unicast_test()
+{
+	ret=0
+
+	netns_create "testns"
+
+	ip netns exec testns ip link add dummy0 type dummy
+	ip netns exec testns ip link set dev dummy0 up
+
+	ip netns exec testns ip link set dev dummy0 carrier on
+
+	ip netns exec testns ip address add 198.51.100.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:1::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 | \
+		grep -q "linkdown"
+	check_fail $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 | \
+		grep -q "linkdown"
+	check_fail $?
+
+	ip netns exec testns ip link set dev dummy0 carrier off
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 198.51.100.2 | \
+		grep -q "linkdown"
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:1::2 | \
+		grep -q "linkdown"
+	check_err $?
+
+	ip netns exec testns ip address add 192.0.2.1/24 dev dummy0
+	ip netns exec testns ip -6 address add 2001:db8:2::1/64 dev dummy0
+
+	ip netns exec testns ip route get fibmatch 192.0.2.2 &> /dev/null
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:2::2 &> /dev/null
+	check_err $?
+
+	ip netns exec testns ip route get fibmatch 192.0.2.2 | \
+		grep -q "linkdown"
+	check_err $?
+	ip netns exec testns ip -6 route get fibmatch 2001:db8:2::2 | \
+		grep -q "linkdown"
+	check_err $?
+
+	ip netns exec testns ip link del dev dummy0
+
+	ip netns del testns
+
+	if [ $ret -ne 0 ]; then
+		echo "FAIL: unicast route carrier test"
+		return 1
+	fi
+	echo "PASS: unicast route carrier test"
+}
+
+fib_carrier_test()
+{
+	echo "Running netdev carrier change tests"
+
+	fib_carrier_local_test
+	fib_carrier_unicast_test
+}
+
 fib_test()
 {
 	fib_unreg_test
 	fib_down_test
+	fib_carrier_test
 }
 
 if [ "$(id -u)" -ne 0 ];then
-- 
2.14.3

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[David Ahern]

On 12/31/17 9:14 AM, Ido Schimmel wrote:
> Previous patch marked nexthops with the 'dead' and 'linkdown' flags.
> Clear these flags when the netdev comes back up.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  include/net/ip6_route.h |  1 +
>  net/ipv6/addrconf.c     |  3 +++
>  net/ipv6/route.c        | 29 +++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 18e442ea93d8..caad39198c2a 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -169,6 +169,7 @@ void rt6_ifdown(struct net *net, struct net_device *dev);
>  void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
>  void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
>  void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
> +void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
>  
>  static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
>  {
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ed06b1190f05..b6405568ed7b 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>  			if (run_pending)
>  				addrconf_dad_run(idev);
>  
> +			/* Device has an address by now */
> +			rt6_sync_up(dev, RTNH_F_DEAD);
> +

Seems like this should be in the NETDEV_UP section, say after
addrconf_permanent_addr.

Re: [RFC PATCH net-next 04/19] ipv6: Prepare to handle multiple netdev events

[David Ahern]

On 12/31/17 9:14 AM, Ido Schimmel wrote:
> To make IPv6 more in line with IPv4 we need to be able to respond
> differently to different netdev events. For example, when a netdev is
> unregistered all the routes using it as their nexthop device should be
> flushed, whereas when the netdev's carrier changes only the 'linkdown'
> flag should be toggled.
> 
> Currently, this is not possible, as the function that traverses the
> routing tables is not aware of the triggering event.
> 
> Propagate the triggering event down, so that it could be used in later
> patches.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  include/net/ip6_route.h |  1 +
>  net/ipv6/addrconf.c     |  4 ++--
>  net/ipv6/route.c        | 35 ++++++++++++++++++++---------------
>  3 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index caad39198c2a..8205402ff3dc 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h

rt6_ifdown is done with this change, so the declaration of rt6_ifdown
can be removed from this file as well.


> @@ -170,6 +170,7 @@ void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
>  void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
>  void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
>  void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
> +void rt6_disable_ip(struct net_device *dev, unsigned long event);
>  
>  static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
>  {

Re: [RFC PATCH net-next 16/19] ipv6: Flush multipath routes when all siblings are dead

[David Ahern]

On 12/31/17 9:15 AM, Ido Schimmel wrote:
> By default, IPv6 deletes nexthops from a multipath route when the
> nexthop device is put administratively down. This differs from IPv4
> where the nexthops are kept, but marked with the RTNH_F_DEAD flag. A
> multipath route is flushed when all of its nexthops become dead.
> 
> Align IPv6 with IPv4 and have it conform to the same guidelines.
> 
> In case the multipath route needs to be flushed, its siblings are
> flushed one by one. Otherwise, the nexthops are marked with the
> appropriate flags and the tree walker is instructed to skip all the
> siblings.
> 
> As explained in previous patches, care is taken to update the sernum of
> the affected tree nodes, so as to prevent the use of wrong dst entries.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  net/ipv6/route.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index ab95e07b5e74..9b142eb83f3c 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3486,8 +3486,10 @@ static int fib6_ifup(struct rt6_info *rt, void *p_arg)
>  	const struct arg_netdev_event *arg = p_arg;
>  	const struct net *net = dev_net(arg->dev);
>  
> -	if (rt != net->ipv6.ip6_null_entry && rt->dst.dev == arg->dev)
> +	if (rt != net->ipv6.ip6_null_entry && rt->dst.dev == arg->dev) {
>  		rt->rt6i_nh_flags &= ~arg->nh_flags;
> +		fib6_update_sernum_upto_root(dev_net(rt->dst.dev), rt);
> +	}
>  
>  	return 0;
>  }
> @@ -3528,6 +3530,35 @@ static void rt6_multipath_flush(struct rt6_info *rt)
>  		iter->should_flush = 1;
>  }
>  
> +static unsigned int rt6_multipath_dead_count(const struct rt6_info *rt,
> +					     const struct net_device *down_dev)
> +{
> +	struct rt6_info *iter;
> +	unsigned int dead = 0;
> +
> +	if (rt->dst.dev == down_dev || rt->rt6i_nh_flags & RTNH_F_DEAD)
> +		dead++;
> +	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
> +		if (iter->dst.dev == down_dev ||
> +		    iter->rt6i_nh_flags & RTNH_F_DEAD)
> +			dead++;
> +
> +	return dead;
> +}
> +
> +static void rt6_multipath_nh_flags_set(struct rt6_info *rt,
> +				       const struct net_device *dev,
> +				       unsigned int nh_flags)
> +{
> +	struct rt6_info *iter;
> +
> +	if (rt->dst.dev == dev)
> +		rt->rt6i_nh_flags |= nh_flags;
> +	list_for_each_entry(iter, &rt->rt6i_siblings, rt6i_siblings)
> +		if (iter->dst.dev == dev)
> +			iter->rt6i_nh_flags |= nh_flags;
> +}
> +
>  /* called with write lock held for table with rt */
>  static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
>  {
> @@ -3550,13 +3581,21 @@ static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
>  		}
>  		return -2;
>  	case NETDEV_DOWN:
> -		if (rt->dst.dev != dev)
> -			break;
> -		if (rt->rt6i_nsiblings == 0 ||
> -		    !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
> +		if (rt->should_flush)
>  			return -1;
> -		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
> -		break;
> +		if (!rt->rt6i_nsiblings)
> +			return rt->dst.dev == dev ? -1 : 0;
> +		if (rt6_multipath_uses_dev(rt, dev)) {
> +			if (rt->rt6i_nsiblings + 1 ==
> +			    rt6_multipath_dead_count(rt, dev)) {

I'd prefer a tmp variable to make that line more readable and still
within the 80 column guideline.
			unsigned int count;

			count = rt6_multipath_dead_count(rt, dev);
			if (rt->rt6i_nsiblings + 1 == count) {


> +				rt6_multipath_flush(rt);
> +				return -1;
> +			}
> +			rt6_multipath_nh_flags_set(rt, dev, RTNH_F_DEAD |
> +						   RTNH_F_LINKDOWN);
> +			fib6_update_sernum(rt);
> +		}
> +		return -2;
>  	case NETDEV_CHANGE:
>  		if (rt->dst.dev != dev ||
>  		    rt->rt6i_flags & (RTF_LOCAL | RTF_ANYCAST))
> 

Re: [RFC PATCH net-next 13/19] ipv6: Flush all sibling routes upon NETDEV_UNREGISTER

[David Ahern]

On 12/31/17 9:15 AM, Ido Schimmel wrote:
> IPv4 and IPv6 react differently to a netdev being unregistered. In IPv4,
> in case the netdev is used as a nexthop device in a multipath route, the
> entire route is flushed.
> 
> However, IPv6 only removes the nexthops associated with the unregistered
> netdev.
> 
> Align IPv4 and IPv6 and flush all the sibling routes when a nexthop
> device is unregistered.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  net/ipv6/route.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 5 deletions(-)

not so sure about this one.

When we get to nexthops as separate objects, we can bring in consistency
by allowing ipv4 routes to just drop a single nexthop in the route
versus the behavior today.

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[Ido Schimmel]

Hi David,

On Tue, Jan 02, 2018 at 09:20:47AM -0700, David Ahern wrote:
> On 12/31/17 9:14 AM, Ido Schimmel wrote:
> > Previous patch marked nexthops with the 'dead' and 'linkdown' flags.
> > Clear these flags when the netdev comes back up.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > ---
> >  include/net/ip6_route.h |  1 +
> >  net/ipv6/addrconf.c     |  3 +++
> >  net/ipv6/route.c        | 29 +++++++++++++++++++++++++++++
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> > index 18e442ea93d8..caad39198c2a 100644
> > --- a/include/net/ip6_route.h
> > +++ b/include/net/ip6_route.h
> > @@ -169,6 +169,7 @@ void rt6_ifdown(struct net *net, struct net_device *dev);
> >  void rt6_mtu_change(struct net_device *dev, unsigned int mtu);
> >  void rt6_remove_prefsrc(struct inet6_ifaddr *ifp);
> >  void rt6_clean_tohost(struct net *net, struct in6_addr *gateway);
> > +void rt6_sync_up(struct net_device *dev, unsigned int nh_flags);
> >  
> >  static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)
> >  {
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index ed06b1190f05..b6405568ed7b 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> >  			if (run_pending)
> >  				addrconf_dad_run(idev);
> >  
> > +			/* Device has an address by now */
> > +			rt6_sync_up(dev, RTNH_F_DEAD);
> > +
> 
> Seems like this should be in the NETDEV_UP section, say after
> addrconf_permanent_addr.

Unless the `keep_addr_on_down` sysctl is set, then at this stage the
netdev doesn't have an IP address and we shouldn't clear the dead flag
just yet.

This is consistent with IPv4 that clears the dead flag from nexthops in
a multipath route only if the nexthop device has an IP address. When the
last IPv4 address is removed from a netdev all the routes using it are
flushed and there's nothing to clear upon NETDEV_UP.

Assuming you're OK with that, I can reword the commit message to make it
clearer.

Re: [RFC PATCH net-next 04/19] ipv6: Prepare to handle multiple netdev events

[Ido Schimmel]

On Tue, Jan 02, 2018 at 09:29:42AM -0700, David Ahern wrote:
> On 12/31/17 9:14 AM, Ido Schimmel wrote:
> > To make IPv6 more in line with IPv4 we need to be able to respond
> > differently to different netdev events. For example, when a netdev is
> > unregistered all the routes using it as their nexthop device should be
> > flushed, whereas when the netdev's carrier changes only the 'linkdown'
> > flag should be toggled.
> > 
> > Currently, this is not possible, as the function that traverses the
> > routing tables is not aware of the triggering event.
> > 
> > Propagate the triggering event down, so that it could be used in later
> > patches.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > ---
> >  include/net/ip6_route.h |  1 +
> >  net/ipv6/addrconf.c     |  4 ++--
> >  net/ipv6/route.c        | 35 ++++++++++++++++++++---------------
> >  3 files changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> > index caad39198c2a..8205402ff3dc 100644
> > --- a/include/net/ip6_route.h
> > +++ b/include/net/ip6_route.h
> 
> rt6_ifdown is done with this change, so the declaration of rt6_ifdown
> can be removed from this file as well.

Yes, I missed that. grepping for `rt6_ifdown` I now see that I need to
reword a comment referencing it in icmp6_dst_alloc().

Thanks

Re: [RFC PATCH net-next 13/19] ipv6: Flush all sibling routes upon NETDEV_UNREGISTER

[Ido Schimmel]

On Tue, Jan 02, 2018 at 10:42:51AM -0700, David Ahern wrote:
> On 12/31/17 9:15 AM, Ido Schimmel wrote:
> > IPv4 and IPv6 react differently to a netdev being unregistered. In IPv4,
> > in case the netdev is used as a nexthop device in a multipath route, the
> > entire route is flushed.
> > 
> > However, IPv6 only removes the nexthops associated with the unregistered
> > netdev.
> > 
> > Align IPv4 and IPv6 and flush all the sibling routes when a nexthop
> > device is unregistered.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > ---
> >  net/ipv6/route.c | 44 +++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> not so sure about this one.

Yea, I wasn't sure about it either. I'll drop it and fix the tests
accordingly.

> When we get to nexthops as separate objects, we can bring in consistency
> by allowing ipv4 routes to just drop a single nexthop in the route
> versus the behavior today.

Agreed.

Re: [RFC PATCH net-next 16/19] ipv6: Flush multipath routes when all siblings are dead

[Ido Schimmel]

On Tue, Jan 02, 2018 at 10:38:19AM -0700, David Ahern wrote:
> On 12/31/17 9:15 AM, Ido Schimmel wrote:
> > @@ -3550,13 +3581,21 @@ static int fib6_ifdown(struct rt6_info *rt, void *p_arg)
> >  		}
> >  		return -2;
> >  	case NETDEV_DOWN:
> > -		if (rt->dst.dev != dev)
> > -			break;
> > -		if (rt->rt6i_nsiblings == 0 ||
> > -		    !rt->rt6i_idev->cnf.ignore_routes_with_linkdown)
> > +		if (rt->should_flush)
> >  			return -1;
> > -		rt->rt6i_nh_flags |= (RTNH_F_DEAD | RTNH_F_LINKDOWN);
> > -		break;
> > +		if (!rt->rt6i_nsiblings)
> > +			return rt->dst.dev == dev ? -1 : 0;
> > +		if (rt6_multipath_uses_dev(rt, dev)) {
> > +			if (rt->rt6i_nsiblings + 1 ==
> > +			    rt6_multipath_dead_count(rt, dev)) {
> 
> I'd prefer a tmp variable to make that line more readable and still
> within the 80 column guideline.
> 			unsigned int count;
> 
> 			count = rt6_multipath_dead_count(rt, dev);
> 			if (rt->rt6i_nsiblings + 1 == count) {

OK, will change.

Thanks!

> 
> 
> > +				rt6_multipath_flush(rt);
> > +				return -1;
> > +			}
> > +			rt6_multipath_nh_flags_set(rt, dev, RTNH_F_DEAD |
> > +						   RTNH_F_LINKDOWN);
> > +			fib6_update_sernum(rt);
> > +		}
> > +		return -2;

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[David Ahern]

On 1/3/18 12:44 AM, Ido Schimmel wrote:
>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>> index ed06b1190f05..b6405568ed7b 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>>>  			if (run_pending)
>>>  				addrconf_dad_run(idev);
>>>  
>>> +			/* Device has an address by now */
>>> +			rt6_sync_up(dev, RTNH_F_DEAD);
>>> +
>>
>> Seems like this should be in the NETDEV_UP section, say after
>> addrconf_permanent_addr.
> 
> Unless the `keep_addr_on_down` sysctl is set, then at this stage the
> netdev doesn't have an IP address and we shouldn't clear the dead flag
> just yet.
> 
> This is consistent with IPv4 that clears the dead flag from nexthops in
> a multipath route only if the nexthop device has an IP address. When the
> last IPv4 address is removed from a netdev all the routes using it are
> flushed and there's nothing to clear upon NETDEV_UP.

I have a bug about that IPv4 handling from the FRR team:

$ ip link add dummy1 type dummy
$ ip li set dummy1 up
$ ip route add 1.1.1.0/24 dev dummy1

$ ip addr add dev dummy1 2.2.2.1/24
$ ip ro ls | grep dummy1
1.1.1.0/24 dev dummy1 scope link
2.2.2.0/24 dev dummy1 proto kernel scope link src 2.2.2.1

$ ip addr del dev dummy1 2.2.2.1/24
$ ip ro ls | grep dummy1
<no outpu>

The 1.1.1.0/24 route was removed as well the 2.2.2.0 connected route.

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[Ido Schimmel]

On Wed, Jan 03, 2018 at 08:32:51AM -0700, David Ahern wrote:
> On 1/3/18 12:44 AM, Ido Schimmel wrote:
> >>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >>> index ed06b1190f05..b6405568ed7b 100644
> >>> --- a/net/ipv6/addrconf.c
> >>> +++ b/net/ipv6/addrconf.c
> >>> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> >>>  			if (run_pending)
> >>>  				addrconf_dad_run(idev);
> >>>  
> >>> +			/* Device has an address by now */
> >>> +			rt6_sync_up(dev, RTNH_F_DEAD);
> >>> +
> >>
> >> Seems like this should be in the NETDEV_UP section, say after
> >> addrconf_permanent_addr.
> > 
> > Unless the `keep_addr_on_down` sysctl is set, then at this stage the
> > netdev doesn't have an IP address and we shouldn't clear the dead flag
> > just yet.
> > 
> > This is consistent with IPv4 that clears the dead flag from nexthops in
> > a multipath route only if the nexthop device has an IP address. When the
> > last IPv4 address is removed from a netdev all the routes using it are
> > flushed and there's nothing to clear upon NETDEV_UP.
> 
> I have a bug about that IPv4 handling from the FRR team:
> 
> $ ip link add dummy1 type dummy
> $ ip li set dummy1 up
> $ ip route add 1.1.1.0/24 dev dummy1
> 
> $ ip addr add dev dummy1 2.2.2.1/24
> $ ip ro ls | grep dummy1
> 1.1.1.0/24 dev dummy1 scope link
> 2.2.2.0/24 dev dummy1 proto kernel scope link src 2.2.2.1
> 
> $ ip addr del dev dummy1 2.2.2.1/24
> $ ip ro ls | grep dummy1
> <no outpu>
> 
> The 1.1.1.0/24 route was removed as well the 2.2.2.0 connected route.

If you're going to skip the flushing in this case, at least mark the
nexthops as dead.

And this is my second reason to have rt6_sync_up() where I put it. I'm
preparing another set which sends FIB_EVENT_NH_ADD events from
rt6_sync_up() similar to what we've in fib_sync_up(). When mlxsw (others
in the future) processes the event it needs to add the nexthop back to
the forwarding plane. To do that, it needs to have a RIF for the
nexthop device. For the nexthop device to have a RIF, it needs at least
one IP address configured on the netdev.

Agree / disagree?

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[David Ahern]

On 1/3/18 9:43 AM, Ido Schimmel wrote:
> On Wed, Jan 03, 2018 at 08:32:51AM -0700, David Ahern wrote:
>> On 1/3/18 12:44 AM, Ido Schimmel wrote:
>>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>>>> index ed06b1190f05..b6405568ed7b 100644
>>>>> --- a/net/ipv6/addrconf.c
>>>>> +++ b/net/ipv6/addrconf.c
>>>>> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>>>>>  			if (run_pending)
>>>>>  				addrconf_dad_run(idev);
>>>>>  
>>>>> +			/* Device has an address by now */
>>>>> +			rt6_sync_up(dev, RTNH_F_DEAD);
>>>>> +
>>>>
>>>> Seems like this should be in the NETDEV_UP section, say after
>>>> addrconf_permanent_addr.
>>>
>>> Unless the `keep_addr_on_down` sysctl is set, then at this stage the
>>> netdev doesn't have an IP address and we shouldn't clear the dead flag
>>> just yet.
>>>
>>> This is consistent with IPv4 that clears the dead flag from nexthops in
>>> a multipath route only if the nexthop device has an IP address. When the
>>> last IPv4 address is removed from a netdev all the routes using it are
>>> flushed and there's nothing to clear upon NETDEV_UP.
>>
>> I have a bug about that IPv4 handling from the FRR team:
>>
>> $ ip link add dummy1 type dummy
>> $ ip li set dummy1 up
>> $ ip route add 1.1.1.0/24 dev dummy1
>>
>> $ ip addr add dev dummy1 2.2.2.1/24
>> $ ip ro ls | grep dummy1
>> 1.1.1.0/24 dev dummy1 scope link
>> 2.2.2.0/24 dev dummy1 proto kernel scope link src 2.2.2.1
>>
>> $ ip addr del dev dummy1 2.2.2.1/24
>> $ ip ro ls | grep dummy1
>> <no outpu>
>>
>> The 1.1.1.0/24 route was removed as well the 2.2.2.0 connected route.
> 
> If you're going to skip the flushing in this case, at least mark the
> nexthops as dead.

On a down event, yes. If the device is still up then a route such as:
$ ip route add 1.1.1.0/24 dev dummy1
should still be usable even without an address on it.

> 
> And this is my second reason to have rt6_sync_up() where I put it. I'm
> preparing another set which sends FIB_EVENT_NH_ADD events from
> rt6_sync_up() similar to what we've in fib_sync_up(). When mlxsw (others

On a tangent here, but I have been meaning to ask why you have
FIB_EVENT_NH_ADD events as opposed to handling netdev events. What does
a FIB_EVENT_NH_ADD provide that you can't do from a netdev event handler?


> in the future) processes the event it needs to add the nexthop back to
> the forwarding plane. To do that, it needs to have a RIF for the
> nexthop device. For the nexthop device to have a RIF, it needs at least
> one IP address configured on the netdev.

Why is that?
$ ip addr sh dev swp1s0.51
44: swp1s0.51@swp1s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
noqueue master vrf1101 state UP group default qlen 1000
    link/ether 7c:fe:90:e8:3a:7d brd ff:ff:ff:ff:ff:ff

$ ip ro add vrf vrf1101 1.1.1.0/24 dev swp1s0.51

$ ip ro ls vrf vrf1101
unreachable default metric 8192
1.1.1.0/24 dev swp1s0.51 scope link offload

In this case, I take it mlxsw allocates a rif because of the vlan. The
above does not work on just swp1s0 -- ie., that route is not offloaded:

$ # ip ro ls
...
1.1.1.0/24 dev swp1s0 scope link
...

Interesting.

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[Ido Schimmel]

On Wed, Jan 03, 2018 at 09:56:02AM -0700, David Ahern wrote:
> On 1/3/18 9:43 AM, Ido Schimmel wrote:
> > On Wed, Jan 03, 2018 at 08:32:51AM -0700, David Ahern wrote:
> >> On 1/3/18 12:44 AM, Ido Schimmel wrote:
> >>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> >>>>> index ed06b1190f05..b6405568ed7b 100644
> >>>>> --- a/net/ipv6/addrconf.c
> >>>>> +++ b/net/ipv6/addrconf.c
> >>>>> @@ -3484,6 +3484,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
> >>>>>  			if (run_pending)
> >>>>>  				addrconf_dad_run(idev);
> >>>>>  
> >>>>> +			/* Device has an address by now */
> >>>>> +			rt6_sync_up(dev, RTNH_F_DEAD);
> >>>>> +
> >>>>
> >>>> Seems like this should be in the NETDEV_UP section, say after
> >>>> addrconf_permanent_addr.
> >>>
> >>> Unless the `keep_addr_on_down` sysctl is set, then at this stage the
> >>> netdev doesn't have an IP address and we shouldn't clear the dead flag
> >>> just yet.
> >>>
> >>> This is consistent with IPv4 that clears the dead flag from nexthops in
> >>> a multipath route only if the nexthop device has an IP address. When the
> >>> last IPv4 address is removed from a netdev all the routes using it are
> >>> flushed and there's nothing to clear upon NETDEV_UP.
> >>
> >> I have a bug about that IPv4 handling from the FRR team:
> >>
> >> $ ip link add dummy1 type dummy
> >> $ ip li set dummy1 up
> >> $ ip route add 1.1.1.0/24 dev dummy1
> >>
> >> $ ip addr add dev dummy1 2.2.2.1/24
> >> $ ip ro ls | grep dummy1
> >> 1.1.1.0/24 dev dummy1 scope link
> >> 2.2.2.0/24 dev dummy1 proto kernel scope link src 2.2.2.1
> >>
> >> $ ip addr del dev dummy1 2.2.2.1/24
> >> $ ip ro ls | grep dummy1
> >> <no outpu>
> >>
> >> The 1.1.1.0/24 route was removed as well the 2.2.2.0 connected route.
> > 
> > If you're going to skip the flushing in this case, at least mark the
> > nexthops as dead.
> 
> On a down event, yes. If the device is still up then a route such as:
> $ ip route add 1.1.1.0/24 dev dummy1
> should still be usable even without an address on it.

mlxsw will trap all the packets hitting the route until you assign an IP
address to dummy1.

> > And this is my second reason to have rt6_sync_up() where I put it. I'm
> > preparing another set which sends FIB_EVENT_NH_ADD events from
> > rt6_sync_up() similar to what we've in fib_sync_up(). When mlxsw (others
> 
> On a tangent here, but I have been meaning to ask why you have
> FIB_EVENT_NH_ADD events as opposed to handling netdev events. What does
> a FIB_EVENT_NH_ADD provide that you can't do from a netdev event handler?

It'll make switch drivers more complex than they already are. Why every
driver needs to duplicate the logic in call_fib_nh_notifiers()?

> > in the future) processes the event it needs to add the nexthop back to
> > the forwarding plane. To do that, it needs to have a RIF for the
> > nexthop device. For the nexthop device to have a RIF, it needs at least
> > one IP address configured on the netdev.
> 
> Why is that?
> $ ip addr sh dev swp1s0.51
> 44: swp1s0.51@swp1s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
> noqueue master vrf1101 state UP group default qlen 1000
>     link/ether 7c:fe:90:e8:3a:7d brd ff:ff:ff:ff:ff:ff
> 
> $ ip ro add vrf vrf1101 1.1.1.0/24 dev swp1s0.51
> 
> $ ip ro ls vrf vrf1101
> unreachable default metric 8192
> 1.1.1.0/24 dev swp1s0.51 scope link offload
> 
> In this case, I take it mlxsw allocates a rif because of the vlan. The
> above does not work on just swp1s0 -- ie., that route is not offloaded:
> 
> $ # ip ro ls
> ...
> 1.1.1.0/24 dev swp1s0 scope link
> ...
> 
> Interesting.

It allocates the RIF because of the enslavement to a VRF, which is an
explicit indication the user wants to use the interface for L3
forwarding.

David, can we please get back to the issue at hand? What's the problem
with the location of the call to rt6_sync_up()?

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[David Ahern]

On 1/3/18 10:40 AM, Ido Schimmel wrote:
> David, can we please get back to the issue at hand? What's the problem
> with the location of the call to rt6_sync_up()?

My original comment was asking why do it on NETDEV_CHANGE when it should
only be needed on NETDEV_UP.

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[Ido Schimmel]

On Wed, Jan 03, 2018 at 11:47:16AM -0700, David Ahern wrote:
> On 1/3/18 10:40 AM, Ido Schimmel wrote:
> > David, can we please get back to the issue at hand? What's the problem
> > with the location of the call to rt6_sync_up()?
> 
> My original comment was asking why do it on NETDEV_CHANGE when it should
> only be needed on NETDEV_UP.

I can condition the call to rt6_sync_up() on the event being NETDEV_UP,
but the location needs to stay the same. Before that the interface still
doesn't have an IP address.

Reason for this requirement is that rt6_sync_up() is going to generate
FIB_EVENT_NH_ADD events that instruct switch drivers to populate their
adjacency tables with the notified nexthop. For this to happen, the
nexthop device needs to have L3 configuration (e.g., RIF in mlxsw) which
is dependent on the presence of an IP address. 

Re: [RFC PATCH net-next 03/19] ipv6: Clear nexthop flags upon netdev up

[David Ahern]

On 1/3/18 1:53 PM, Ido Schimmel wrote:
> On Wed, Jan 03, 2018 at 11:47:16AM -0700, David Ahern wrote:
>> On 1/3/18 10:40 AM, Ido Schimmel wrote:
>>> David, can we please get back to the issue at hand? What's the problem
>>> with the location of the call to rt6_sync_up()?
>>
>> My original comment was asking why do it on NETDEV_CHANGE when it should
>> only be needed on NETDEV_UP.
> 
> I can condition the call to rt6_sync_up() on the event being NETDEV_UP,
> but the location needs to stay the same. Before that the interface still
> doesn't have an IP address.

it's fine as is. The NETDEV_CHANGE section has a break after calling
rt6_sync_up(RTNH_F_LINKDOWN) which is added later. To your point,
addrconf_dev_config will add a linklocal address.