[PATCH v3 net-next 0/5] net/ipv6: Address checks need to consider the L3 domain

[PATCH v3 net-next 0/5] net/ipv6: Address checks need to consider the L3 domain

[David Ahern]

IPv6 prohibits a local address from being used as a gateway for a route.
However, it is ok for the local address to be in a different L3 domain
(e.g., VRF); this allows, for example, veth pairs to connect VRFs.

ip6_route_info_create calls ipv6_chk_addr_and_flags for gateway addresses
to determine if the address is a local one, but ipv6_chk_addr_and_flags
does not currently consider L3 domains. As a result routes can not be
added in one VRF with a nexthop that points to a local address in a
second VRF.

Resolve by comparing the l3mdev for the passed in device and requiring an
l3mdev match with the device containing an address. The intent of checking
for an address on the specified device versus any device in the domain is
mantained by a new argument to skip the check between the passed in device
and the device with the address.

Patch 1 moves the gateway validation from ip6_route_info_create into a
helper; the function is long enough and refactoring drops the indent
level.

Patch 2 adds l3mdev checks to ipv6_chk_addr_and_flags and fixes up
a few ipv6_chk_addr callers that pass a NULL device.

Patches 3 and 4 do some refactoring to the fib_tests script and then
patch 5 adds nexthop validation tests.

v3
- set skip_dev_check in ipv6_chk_addr based on dev == NULL

v2
- handle 2 variations of route spec with sane error path
- add test cases

David Ahern (5):
  net/ipv6: Refactor gateway validation on route add
  net/ipv6: Address checks need to consider the L3 domain
  selftests: fib_tests: Use an alias for ip command
  selftests: fib_tests: Allow user to run a specific test
  selftests: fib_tests: Add IPv6 nexthop spec tests

 include/net/addrconf.h                   |   4 +-
 net/ipv6/addrconf.c                      |  26 ++-
 net/ipv6/anycast.c                       |   9 +-
 net/ipv6/datagram.c                      |   5 +-
 net/ipv6/ip6_tunnel.c                    |  12 +-
 net/ipv6/ndisc.c                         |   2 +-
 net/ipv6/route.c                         | 139 +++++++-----
 tools/testing/selftests/net/fib_tests.sh | 359 +++++++++++++++++++++++--------
 8 files changed, 397 insertions(+), 159 deletions(-)

-- 
2.11.0

[PATCH v3 net-next 1/5] net/ipv6: Refactor gateway validation on route add

[David Ahern]

Move gateway validation code from ip6_route_info_create into
ip6_validate_gw. Code move plus adjustments to handle the potential
reset of dev and idev and to make checkpatch happy.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 120 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 66 insertions(+), 54 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f0ae58424c45..3851c3ccfd7a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2550,7 +2550,7 @@ static struct rt6_info *ip6_nh_lookup_table(struct net *net,
 
 static int ip6_route_check_nh_onlink(struct net *net,
 				     struct fib6_config *cfg,
-				     struct net_device *dev,
+				     const struct net_device *dev,
 				     struct netlink_ext_ack *extack)
 {
 	u32 tbid = l3mdev_fib_table(dev) ? : RT_TABLE_MAIN;
@@ -2626,6 +2626,68 @@ static int ip6_route_check_nh(struct net *net,
 	return err;
 }
 
+static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
+			   struct net_device **_dev, struct inet6_dev **idev,
+			   struct netlink_ext_ack *extack)
+{
+	const struct in6_addr *gw_addr = &cfg->fc_gateway;
+	int gwa_type = ipv6_addr_type(gw_addr);
+	const struct net_device *dev = *_dev;
+	int err = -EINVAL;
+
+	/* if gw_addr is local we will fail to detect this in case
+	 * address is still TENTATIVE (DAD in progress). rt6_lookup()
+	 * will return already-added prefix route via interface that
+	 * prefix route was assigned to, which might be non-loopback.
+	 */
+	if (ipv6_chk_addr_and_flags(net, gw_addr,
+				    gwa_type & IPV6_ADDR_LINKLOCAL ?
+				    dev : NULL, 0, 0)) {
+		NL_SET_ERR_MSG(extack, "Invalid gateway address");
+		goto out;
+	}
+
+	if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
+		/* IPv6 strictly inhibits using not link-local
+		 * addresses as nexthop address.
+		 * Otherwise, router will not able to send redirects.
+		 * It is very good, but in some (rare!) circumstances
+		 * (SIT, PtP, NBMA NOARP links) it is handy to allow
+		 * some exceptions. --ANK
+		 * We allow IPv4-mapped nexthops to support RFC4798-type
+		 * addressing
+		 */
+		if (!(gwa_type & (IPV6_ADDR_UNICAST | IPV6_ADDR_MAPPED))) {
+			NL_SET_ERR_MSG(extack, "Invalid gateway address");
+			goto out;
+		}
+
+		if (cfg->fc_flags & RTNH_F_ONLINK)
+			err = ip6_route_check_nh_onlink(net, cfg, dev, extack);
+		else
+			err = ip6_route_check_nh(net, cfg, _dev, idev);
+
+		if (err)
+			goto out;
+	}
+
+	/* reload in case device was changed */
+	dev = *_dev;
+
+	err = -EINVAL;
+	if (!dev) {
+		NL_SET_ERR_MSG(extack, "Egress device not specified");
+		goto out;
+	} else if (dev->flags & IFF_LOOPBACK) {
+		NL_SET_ERR_MSG(extack,
+			       "Egress device can not be loopback device for this route");
+		goto out;
+	}
+	err = 0;
+out:
+	return err;
+}
+
 static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
 					      struct netlink_ext_ack *extack)
 {
@@ -2808,61 +2870,11 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,
 	}
 
 	if (cfg->fc_flags & RTF_GATEWAY) {
-		const struct in6_addr *gw_addr;
-		int gwa_type;
-
-		gw_addr = &cfg->fc_gateway;
-		gwa_type = ipv6_addr_type(gw_addr);
-
-		/* if gw_addr is local we will fail to detect this in case
-		 * address is still TENTATIVE (DAD in progress). rt6_lookup()
-		 * will return already-added prefix route via interface that
-		 * prefix route was assigned to, which might be non-loopback.
-		 */
-		err = -EINVAL;
-		if (ipv6_chk_addr_and_flags(net, gw_addr,
-					    gwa_type & IPV6_ADDR_LINKLOCAL ?
-					    dev : NULL, 0, 0)) {
-			NL_SET_ERR_MSG(extack, "Invalid gateway address");
+		err = ip6_validate_gw(net, cfg, &dev, &idev, extack);
+		if (err)
 			goto out;
-		}
-		rt->rt6i_gateway = *gw_addr;
-
-		if (gwa_type != (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_UNICAST)) {
-			/* IPv6 strictly inhibits using not link-local
-			   addresses as nexthop address.
-			   Otherwise, router will not able to send redirects.
-			   It is very good, but in some (rare!) circumstances
-			   (SIT, PtP, NBMA NOARP links) it is handy to allow
-			   some exceptions. --ANK
-			   We allow IPv4-mapped nexthops to support RFC4798-type
-			   addressing
-			 */
-			if (!(gwa_type & (IPV6_ADDR_UNICAST |
-					  IPV6_ADDR_MAPPED))) {
-				NL_SET_ERR_MSG(extack,
-					       "Invalid gateway address");
-				goto out;
-			}
 
-			if (cfg->fc_flags & RTNH_F_ONLINK) {
-				err = ip6_route_check_nh_onlink(net, cfg, dev,
-								extack);
-			} else {
-				err = ip6_route_check_nh(net, cfg, &dev, &idev);
-			}
-			if (err)
-				goto out;
-		}
-		err = -EINVAL;
-		if (!dev) {
-			NL_SET_ERR_MSG(extack, "Egress device not specified");
-			goto out;
-		} else if (dev->flags & IFF_LOOPBACK) {
-			NL_SET_ERR_MSG(extack,
-				       "Egress device can not be loopback device for this route");
-			goto out;
-		}
+		rt->rt6i_gateway = cfg->fc_gateway;
 	}
 
 	err = -ENODEV;
-- 
2.11.0

[PATCH v3 net-next 2/5] net/ipv6: Address checks need to consider the L3 domain

[David Ahern]

ipv6_chk_addr_and_flags determines if an address is a local address. It
is called by ip6_route_info_create to validate a gateway address is not a
local address. It currently does not consider L3 domains and as a result
does not allow a route to be added in one VRF if the nexthop points to
an address in a second VRF. e.g.,

    $ ip route add 2001:db8:1::/64 vrf r2 via 2001:db8:102::23
    Error: Invalid gateway address.

where 2001:db8:102::23 is an address on an interface in vrf r1.

Resolve by comparing the l3mdev for the passed in device and requiring an
l3mdev match with the device containing an address. The intent of checking
for an address on the specified device versus any device in the domain is
mantained by a new argument to skip the check between the passed in device
and the device with the address.

Update the handful of users of ipv6_chk_addr with a NULL dev argument:
- anycast to call ipv6_chk_addr_and_flags. If the device is given by the
  user, look for the given address across the L3 domain. If the index is
  not given, the default table is presumed so only addresses on devices
  not enslaved are considered.

- ip6_tnl_rcv_ctl - local address must exist on device, remote address
  can not exist in L3 domain; only remote check needs to be updated but
  do both for consistency.

ip6_validate_gw needs to handle 2 cases - one where the device is given
as part of the nexthop spec and the other where the device is resolved.
There is at least 1 VRF case where deferring the check to only after
the route lookup has resolved the device fails with an unintuitive error
"RTNETLINK answers: No route to host" as opposed to the preferred
"Error: Gateway can not be a local address." The 'no route to host'
error is because of the fallback to a full lookup.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/net/addrconf.h |  4 ++--
 net/ipv6/addrconf.c    | 26 ++++++++++++++++++++++----
 net/ipv6/anycast.c     |  9 ++++++---
 net/ipv6/datagram.c    |  5 +++--
 net/ipv6/ip6_tunnel.c  | 12 ++++++++----
 net/ipv6/ndisc.c       |  2 +-
 net/ipv6/route.c       | 37 ++++++++++++++++++++++++++++---------
 7 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index c4185a7b0e90..132e5b95167a 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
 int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		  const struct net_device *dev, int strict);
 int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
-			    const struct net_device *dev, int strict,
-			    u32 banned_flags);
+			    const struct net_device *dev, bool skip_dev_check,
+			    int strict, u32 banned_flags);
 
 #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
 int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index b5fd116c046a..17d5d3f42d21 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1851,22 +1851,40 @@ static int ipv6_count_addresses(const struct inet6_dev *idev)
 int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
 		  const struct net_device *dev, int strict)
 {
-	return ipv6_chk_addr_and_flags(net, addr, dev, strict, IFA_F_TENTATIVE);
+	return ipv6_chk_addr_and_flags(net, addr, dev, !dev,
+				       strict, IFA_F_TENTATIVE);
 }
 EXPORT_SYMBOL(ipv6_chk_addr);
 
+/* device argument is used to find the L3 domain of interest. If
+ * skip_dev_check is set, then the ifp device is not checked against
+ * the passed in dev argument. So the 2 cases for addresses checks are:
+ *   1. does the address exist in the L3 domain that dev is part of
+ *      (skip_dev_check = true), or
+ *
+ *   2. does the address exist on the specific device
+ *      (skip_dev_check = false)
+ */
 int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
-			    const struct net_device *dev, int strict,
-			    u32 banned_flags)
+			    const struct net_device *dev, bool skip_dev_check,
+			    int strict, u32 banned_flags)
 {
 	unsigned int hash = inet6_addr_hash(net, addr);
+	const struct net_device *l3mdev;
 	struct inet6_ifaddr *ifp;
 	u32 ifp_flags;
 
 	rcu_read_lock();
+
+	l3mdev = l3mdev_master_dev_rcu(dev);
+
 	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
 		if (!net_eq(dev_net(ifp->idev->dev), net))
 			continue;
+
+		if (l3mdev_master_dev_rcu(ifp->idev->dev) != l3mdev)
+			continue;
+
 		/* Decouple optimistic from tentative for evaluation here.
 		 * Ban optimistic addresses explicitly, when required.
 		 */
@@ -1875,7 +1893,7 @@ int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
 			    : ifp->flags;
 		if (ipv6_addr_equal(&ifp->addr, addr) &&
 		    !(ifp_flags&banned_flags) &&
-		    (!dev || ifp->idev->dev == dev ||
+		    (skip_dev_check || ifp->idev->dev == dev ||
 		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
 			rcu_read_unlock();
 			return 1;
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index c61718dba2e6..d580d4d456a5 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -66,7 +66,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 		return -EPERM;
 	if (ipv6_addr_is_multicast(addr))
 		return -EINVAL;
-	if (ipv6_chk_addr(net, addr, NULL, 0))
+
+	if (ifindex)
+		dev = __dev_get_by_index(net, ifindex);
+
+	if (ipv6_chk_addr_and_flags(net, addr, dev, true, 0, IFA_F_TENTATIVE))
 		return -EINVAL;
 
 	pac = sock_kmalloc(sk, sizeof(struct ipv6_ac_socklist), GFP_KERNEL);
@@ -90,8 +94,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			dev = __dev_get_by_flags(net, IFF_UP,
 						 IFF_UP | IFF_LOOPBACK);
 		}
-	} else
-		dev = __dev_get_by_index(net, ifindex);
+	}
 
 	if (!dev) {
 		err = -ENODEV;
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index fbf08ce3f5ab..b27333d7b099 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -801,8 +801,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 			if (addr_type != IPV6_ADDR_ANY) {
 				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
 				if (!(inet_sk(sk)->freebind || inet_sk(sk)->transparent) &&
-				    !ipv6_chk_addr(net, &src_info->ipi6_addr,
-						   strict ? dev : NULL, 0) &&
+				    !ipv6_chk_addr_and_flags(net, &src_info->ipi6_addr,
+							     dev, !strict, 0,
+							     IFA_F_TENTATIVE) &&
 				    !ipv6_chk_acast_addr_src(net, dev,
 							     &src_info->ipi6_addr))
 					err = -EINVAL;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 56c4967f1868..1ce8244e8aee 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -758,9 +758,11 @@ int ip6_tnl_rcv_ctl(struct ip6_tnl *t,
 			ldev = dev_get_by_index_rcu(net, p->link);
 
 		if ((ipv6_addr_is_multicast(laddr) ||
-		     likely(ipv6_chk_addr(net, laddr, ldev, 0))) &&
+		     likely(ipv6_chk_addr_and_flags(net, laddr, ldev, false,
+						    0, IFA_F_TENTATIVE))) &&
 		    ((p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) ||
-		     likely(!ipv6_chk_addr(net, raddr, NULL, 0))))
+		     likely(!ipv6_chk_addr_and_flags(net, raddr, ldev, true,
+						     0, IFA_F_TENTATIVE))))
 			ret = 1;
 	}
 	return ret;
@@ -990,12 +992,14 @@ int ip6_tnl_xmit_ctl(struct ip6_tnl *t,
 		if (p->link)
 			ldev = dev_get_by_index_rcu(net, p->link);
 
-		if (unlikely(!ipv6_chk_addr(net, laddr, ldev, 0)))
+		if (unlikely(!ipv6_chk_addr_and_flags(net, laddr, ldev, false,
+						      0, IFA_F_TENTATIVE)))
 			pr_warn("%s xmit: Local address not yet configured!\n",
 				p->name);
 		else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
 			 !ipv6_addr_is_multicast(raddr) &&
-			 unlikely(ipv6_chk_addr(net, raddr, NULL, 0)))
+			 unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
+							  true, 0, IFA_F_TENTATIVE)))
 			pr_warn("%s xmit: Routing loop! Remote address found on this node!\n",
 				p->name);
 		else
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 0a19ce3a6f7f..13bf775c7f1a 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -707,7 +707,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	int probes = atomic_read(&neigh->probes);
 
 	if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
-					   dev, 1,
+					   dev, false, 1,
 					   IFA_F_TENTATIVE|IFA_F_OPTIMISTIC))
 		saddr = &ipv6_hdr(skb)->saddr;
 	probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 3851c3ccfd7a..bbc62799eb3b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2633,18 +2633,25 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
 	const struct in6_addr *gw_addr = &cfg->fc_gateway;
 	int gwa_type = ipv6_addr_type(gw_addr);
 	const struct net_device *dev = *_dev;
+	bool need_local_addr_check = !dev;
 	int err = -EINVAL;
 
-	/* if gw_addr is local we will fail to detect this in case
-	 * address is still TENTATIVE (DAD in progress). rt6_lookup()
-	 * will return already-added prefix route via interface that
-	 * prefix route was assigned to, which might be non-loopback.
+	/* if route spec contains the device, check if gateway address
+	 * is a local address in the same L3 domain
 	 */
-	if (ipv6_chk_addr_and_flags(net, gw_addr,
-				    gwa_type & IPV6_ADDR_LINKLOCAL ?
-				    dev : NULL, 0, 0)) {
-		NL_SET_ERR_MSG(extack, "Invalid gateway address");
-		goto out;
+	if (dev) {
+		/* if gw_addr is local we will fail to detect this in case
+		 * address is still TENTATIVE (DAD in progress). rt6_lookup()
+		 * will return already-added prefix route via interface that
+		 * prefix route was assigned to, which might be non-loopback.
+		 */
+		if (ipv6_chk_addr_and_flags(net, gw_addr, dev,
+					    gwa_type & IPV6_ADDR_LINKLOCAL ?
+					    false : true, 0, 0)) {
+			NL_SET_ERR_MSG(extack,
+				       "Gateway can not be a local address");
+			goto out;
+		}
 	}
 
 	if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
@@ -2683,6 +2690,18 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
 			       "Egress device can not be loopback device for this route");
 		goto out;
 	}
+
+	/* if we did not check gw_addr above, do so now that the
+	 * egress device has been resolved.
+	 */
+	if (need_local_addr_check &&
+	    ipv6_chk_addr_and_flags(net, gw_addr, dev,
+				    gwa_type & IPV6_ADDR_LINKLOCAL ?
+				    false : true, 0, 0)) {
+		NL_SET_ERR_MSG(extack, "Gateway can not be a local address");
+		goto out;
+	}
+
 	err = 0;
 out:
 	return err;
-- 
2.11.0

Re: [PATCH v3 net-next 2/5] net/ipv6: Address checks need to consider the L3 domain

[Kirill Tkhai]

On 07.03.2018 06:58, David Ahern wrote:
> ipv6_chk_addr_and_flags determines if an address is a local address. It
> is called by ip6_route_info_create to validate a gateway address is not a
> local address. It currently does not consider L3 domains and as a result
> does not allow a route to be added in one VRF if the nexthop points to
> an address in a second VRF. e.g.,
> 
>     $ ip route add 2001:db8:1::/64 vrf r2 via 2001:db8:102::23
>     Error: Invalid gateway address.
> 
> where 2001:db8:102::23 is an address on an interface in vrf r1.
> 
> Resolve by comparing the l3mdev for the passed in device and requiring an
> l3mdev match with the device containing an address. The intent of checking
> for an address on the specified device versus any device in the domain is
> mantained by a new argument to skip the check between the passed in device
> and the device with the address.
> 
> Update the handful of users of ipv6_chk_addr with a NULL dev argument:
> - anycast to call ipv6_chk_addr_and_flags. If the device is given by the
>   user, look for the given address across the L3 domain. If the index is
>   not given, the default table is presumed so only addresses on devices
>   not enslaved are considered.
> 
> - ip6_tnl_rcv_ctl - local address must exist on device, remote address
>   can not exist in L3 domain; only remote check needs to be updated but
>   do both for consistency.
> 
> ip6_validate_gw needs to handle 2 cases - one where the device is given
> as part of the nexthop spec and the other where the device is resolved.
> There is at least 1 VRF case where deferring the check to only after
> the route lookup has resolved the device fails with an unintuitive error
> "RTNETLINK answers: No route to host" as opposed to the preferred
> "Error: Gateway can not be a local address." The 'no route to host'
> error is because of the fallback to a full lookup.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  include/net/addrconf.h |  4 ++--
>  net/ipv6/addrconf.c    | 26 ++++++++++++++++++++++----
>  net/ipv6/anycast.c     |  9 ++++++---
>  net/ipv6/datagram.c    |  5 +++--
>  net/ipv6/ip6_tunnel.c  | 12 ++++++++----
>  net/ipv6/ndisc.c       |  2 +-
>  net/ipv6/route.c       | 37 ++++++++++++++++++++++++++++---------
>  7 files changed, 70 insertions(+), 25 deletions(-)
> 
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index c4185a7b0e90..132e5b95167a 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>  		  const struct net_device *dev, int strict);
>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
> -			    const struct net_device *dev, int strict,
> -			    u32 banned_flags);
> +			    const struct net_device *dev, bool skip_dev_check,
> +			    int strict, u32 banned_flags);

This function already has 5 arguments, while this patch adds one more.
Can't we use new flags argument for both of them?

Also, the name of function and input parameters are already so big, that they
don't fit a single line already, while your patch adds more parameters.
Can't we make it more slim? Something like ipv6_chk_addr_fl() instead of current
name.

>  #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
>  int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index b5fd116c046a..17d5d3f42d21 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1851,22 +1851,40 @@ static int ipv6_count_addresses(const struct inet6_dev *idev)
>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>  		  const struct net_device *dev, int strict)
>  {
> -	return ipv6_chk_addr_and_flags(net, addr, dev, strict, IFA_F_TENTATIVE);
> +	return ipv6_chk_addr_and_flags(net, addr, dev, !dev,
> +				       strict, IFA_F_TENTATIVE);
>  }
>  EXPORT_SYMBOL(ipv6_chk_addr);

This function was not introduced by this commit, but since the commit modifies it,
and the function is pretty simple, we could declare it as static inline in header
in separate patch.

>  
> +/* device argument is used to find the L3 domain of interest. If
> + * skip_dev_check is set, then the ifp device is not checked against
> + * the passed in dev argument. So the 2 cases for addresses checks are:
> + *   1. does the address exist in the L3 domain that dev is part of
> + *      (skip_dev_check = true), or
> + *
> + *   2. does the address exist on the specific device
> + *      (skip_dev_check = false)
> + */
>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
> -			    const struct net_device *dev, int strict,
> -			    u32 banned_flags)
> +			    const struct net_device *dev, bool skip_dev_check,
> +			    int strict, u32 banned_flags)
>  {
>  	unsigned int hash = inet6_addr_hash(net, addr);
> +	const struct net_device *l3mdev;
>  	struct inet6_ifaddr *ifp;
>  	u32 ifp_flags;
>  
>  	rcu_read_lock();
> +
> +	l3mdev = l3mdev_master_dev_rcu(dev);
> +
>  	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
>  		if (!net_eq(dev_net(ifp->idev->dev), net))
>  			continue;
> +
> +		if (l3mdev_master_dev_rcu(ifp->idev->dev) != l3mdev)
> +			continue;
> +
>  		/* Decouple optimistic from tentative for evaluation here.
>  		 * Ban optimistic addresses explicitly, when required.
>  		 */
> @@ -1875,7 +1893,7 @@ int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>  			    : ifp->flags;
>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
>  		    !(ifp_flags&banned_flags) &&
> -		    (!dev || ifp->idev->dev == dev ||
> +		    (skip_dev_check || ifp->idev->dev == dev ||
>  		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>  			rcu_read_unlock();
>  			return 1;

There are two logical pieces in changes of this function:

1)You become always pass not NULL dev and add skip_dev_check argument.
2)l3mdev_master_dev_rcu() check is introduced.

They should go in separate patches.

> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index c61718dba2e6..d580d4d456a5 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
> @@ -66,7 +66,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  		return -EPERM;
>  	if (ipv6_addr_is_multicast(addr))
>  		return -EINVAL;
> -	if (ipv6_chk_addr(net, addr, NULL, 0))
> +
> +	if (ifindex)
> +		dev = __dev_get_by_index(net, ifindex);
> +
> +	if (ipv6_chk_addr_and_flags(net, addr, dev, true, 0, IFA_F_TENTATIVE))
>  		return -EINVAL;
>  
>  	pac = sock_kmalloc(sk, sizeof(struct ipv6_ac_socklist), GFP_KERNEL);
> @@ -90,8 +94,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  			dev = __dev_get_by_flags(net, IFF_UP,
>  						 IFF_UP | IFF_LOOPBACK);
>  		}
> -	} else
> -		dev = __dev_get_by_index(net, ifindex);
> +	}

The hunk moving __dev_get_by_index() dereference may go as separate change, as it's a refactoring.
This will make the review easier.

>  
>  	if (!dev) {
>  		err = -ENODEV;
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index fbf08ce3f5ab..b27333d7b099 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -801,8 +801,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>  			if (addr_type != IPV6_ADDR_ANY) {
>  				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
>  				if (!(inet_sk(sk)->freebind || inet_sk(sk)->transparent) &&
> -				    !ipv6_chk_addr(net, &src_info->ipi6_addr,
> -						   strict ? dev : NULL, 0) &&
> +				    !ipv6_chk_addr_and_flags(net, &src_info->ipi6_addr,
> +							     dev, !strict, 0,
> +							     IFA_F_TENTATIVE) &&
>  				    !ipv6_chk_acast_addr_src(net, dev,
>  							     &src_info->ipi6_addr))
>  					err = -EINVAL;
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 56c4967f1868..1ce8244e8aee 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -758,9 +758,11 @@ int ip6_tnl_rcv_ctl(struct ip6_tnl *t,
>  			ldev = dev_get_by_index_rcu(net, p->link);
>  
>  		if ((ipv6_addr_is_multicast(laddr) ||
> -		     likely(ipv6_chk_addr(net, laddr, ldev, 0))) &&
> +		     likely(ipv6_chk_addr_and_flags(net, laddr, ldev, false,
> +						    0, IFA_F_TENTATIVE))) &&
>  		    ((p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) ||
> -		     likely(!ipv6_chk_addr(net, raddr, NULL, 0))))
> +		     likely(!ipv6_chk_addr_and_flags(net, raddr, ldev, true,
> +						     0, IFA_F_TENTATIVE))))
>  			ret = 1;
>  	}
>  	return ret;
> @@ -990,12 +992,14 @@ int ip6_tnl_xmit_ctl(struct ip6_tnl *t,
>  		if (p->link)
>  			ldev = dev_get_by_index_rcu(net, p->link);
>  
> -		if (unlikely(!ipv6_chk_addr(net, laddr, ldev, 0)))
> +		if (unlikely(!ipv6_chk_addr_and_flags(net, laddr, ldev, false,
> +						      0, IFA_F_TENTATIVE)))
>  			pr_warn("%s xmit: Local address not yet configured!\n",
>  				p->name);
>  		else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
>  			 !ipv6_addr_is_multicast(raddr) &&
> -			 unlikely(ipv6_chk_addr(net, raddr, NULL, 0)))
> +			 unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
> +							  true, 0, IFA_F_TENTATIVE)))
>  			pr_warn("%s xmit: Routing loop! Remote address found on this node!\n",
>  				p->name);
>  		else
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 0a19ce3a6f7f..13bf775c7f1a 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -707,7 +707,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	int probes = atomic_read(&neigh->probes);
>  
>  	if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
> -					   dev, 1,
> +					   dev, false, 1,
>  					   IFA_F_TENTATIVE|IFA_F_OPTIMISTIC))
>  		saddr = &ipv6_hdr(skb)->saddr;
>  	probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 3851c3ccfd7a..bbc62799eb3b 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2633,18 +2633,25 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>  	const struct in6_addr *gw_addr = &cfg->fc_gateway;
>  	int gwa_type = ipv6_addr_type(gw_addr);
>  	const struct net_device *dev = *_dev;
> +	bool need_local_addr_check = !dev;
>  	int err = -EINVAL;
>  
> -	/* if gw_addr is local we will fail to detect this in case
> -	 * address is still TENTATIVE (DAD in progress). rt6_lookup()
> -	 * will return already-added prefix route via interface that
> -	 * prefix route was assigned to, which might be non-loopback.
> +	/* if route spec contains the device, check if gateway address
> +	 * is a local address in the same L3 domain
>  	 */
> -	if (ipv6_chk_addr_and_flags(net, gw_addr,
> -				    gwa_type & IPV6_ADDR_LINKLOCAL ?
> -				    dev : NULL, 0, 0)) {
> -		NL_SET_ERR_MSG(extack, "Invalid gateway address");
> -		goto out;
> +	if (dev) {
> +		/* if gw_addr is local we will fail to detect this in case
> +		 * address is still TENTATIVE (DAD in progress). rt6_lookup()
> +		 * will return already-added prefix route via interface that
> +		 * prefix route was assigned to, which might be non-loopback.
> +		 */
> +		if (ipv6_chk_addr_and_flags(net, gw_addr, dev,
> +					    gwa_type & IPV6_ADDR_LINKLOCAL ?
> +					    false : true, 0, 0)) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Gateway can not be a local address");
> +			goto out;
> +		}

Why do these two "if" go as tree, not as single "if (a && b)"?

>  	}
>  
>  	if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
> @@ -2683,6 +2690,18 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>  			       "Egress device can not be loopback device for this route");
>  		goto out;
>  	}
> +
> +	/* if we did not check gw_addr above, do so now that the
> +	 * egress device has been resolved.
> +	 */
> +	if (need_local_addr_check &&

Do we really need a variable with so long name? Can't we use "local_check" or something like this?

> +	    ipv6_chk_addr_and_flags(net, gw_addr, dev,
> +				    gwa_type & IPV6_ADDR_LINKLOCAL ?
> +				    false : true, 0, 0)) {

Second time there is "gwa_type & IPV6_ADDR_LINKLOCAL ? false : true". gwa_type is constant,
it doesn't change. Repeating constant expressions have to be be cached into local variable
for improving readability.

> +		NL_SET_ERR_MSG(extack, "Gateway can not be a local address");
> +		goto out;
> +	}
> +
>  	err = 0;
>  out:
>  	return err;
> 

Thanks,
Kirill

Re: [PATCH v3 net-next 2/5] net/ipv6: Address checks need to consider the L3 domain

[Kirill Tkhai]

On 07.03.2018 14:53, Kirill Tkhai wrote:
> On 07.03.2018 06:58, David Ahern wrote:
>> ipv6_chk_addr_and_flags determines if an address is a local address. It
>> is called by ip6_route_info_create to validate a gateway address is not a
>> local address. It currently does not consider L3 domains and as a result
>> does not allow a route to be added in one VRF if the nexthop points to
>> an address in a second VRF. e.g.,
>>
>>     $ ip route add 2001:db8:1::/64 vrf r2 via 2001:db8:102::23
>>     Error: Invalid gateway address.
>>
>> where 2001:db8:102::23 is an address on an interface in vrf r1.
>>
>> Resolve by comparing the l3mdev for the passed in device and requiring an
>> l3mdev match with the device containing an address. The intent of checking
>> for an address on the specified device versus any device in the domain is
>> mantained by a new argument to skip the check between the passed in device
>> and the device with the address.
>>
>> Update the handful of users of ipv6_chk_addr with a NULL dev argument:
>> - anycast to call ipv6_chk_addr_and_flags. If the device is given by the
>>   user, look for the given address across the L3 domain. If the index is
>>   not given, the default table is presumed so only addresses on devices
>>   not enslaved are considered.
>>
>> - ip6_tnl_rcv_ctl - local address must exist on device, remote address
>>   can not exist in L3 domain; only remote check needs to be updated but
>>   do both for consistency.
>>
>> ip6_validate_gw needs to handle 2 cases - one where the device is given
>> as part of the nexthop spec and the other where the device is resolved.
>> There is at least 1 VRF case where deferring the check to only after
>> the route lookup has resolved the device fails with an unintuitive error
>> "RTNETLINK answers: No route to host" as opposed to the preferred
>> "Error: Gateway can not be a local address." The 'no route to host'
>> error is because of the fallback to a full lookup.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> ---
>>  include/net/addrconf.h |  4 ++--
>>  net/ipv6/addrconf.c    | 26 ++++++++++++++++++++++----
>>  net/ipv6/anycast.c     |  9 ++++++---
>>  net/ipv6/datagram.c    |  5 +++--
>>  net/ipv6/ip6_tunnel.c  | 12 ++++++++----
>>  net/ipv6/ndisc.c       |  2 +-
>>  net/ipv6/route.c       | 37 ++++++++++++++++++++++++++++---------
>>  7 files changed, 70 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>> index c4185a7b0e90..132e5b95167a 100644
>> --- a/include/net/addrconf.h
>> +++ b/include/net/addrconf.h
>> @@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
>>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>  		  const struct net_device *dev, int strict);
>>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> -			    const struct net_device *dev, int strict,
>> -			    u32 banned_flags);
>> +			    const struct net_device *dev, bool skip_dev_check,
>> +			    int strict, u32 banned_flags);
> 
> This function already has 5 arguments, while this patch adds one more.
> Can't we use new flags argument for both of them?

Them are skip_dev_check and strict.
 
> Also, the name of function and input parameters are already so big, that they
> don't fit a single line already, while your patch adds more parameters.
> Can't we make it more slim? Something like ipv6_chk_addr_fl() instead of current
> name.
> 
>>  #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
>>  int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index b5fd116c046a..17d5d3f42d21 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1851,22 +1851,40 @@ static int ipv6_count_addresses(const struct inet6_dev *idev)
>>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>  		  const struct net_device *dev, int strict)
>>  {
>> -	return ipv6_chk_addr_and_flags(net, addr, dev, strict, IFA_F_TENTATIVE);
>> +	return ipv6_chk_addr_and_flags(net, addr, dev, !dev,
>> +				       strict, IFA_F_TENTATIVE);
>>  }
>>  EXPORT_SYMBOL(ipv6_chk_addr);
> 
> This function was not introduced by this commit, but since the commit modifies it,
> and the function is pretty simple, we could declare it as static inline in header
> in separate patch.
> 
>>  
>> +/* device argument is used to find the L3 domain of interest. If
>> + * skip_dev_check is set, then the ifp device is not checked against
>> + * the passed in dev argument. So the 2 cases for addresses checks are:
>> + *   1. does the address exist in the L3 domain that dev is part of
>> + *      (skip_dev_check = true), or
>> + *
>> + *   2. does the address exist on the specific device
>> + *      (skip_dev_check = false)
>> + */
>>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> -			    const struct net_device *dev, int strict,
>> -			    u32 banned_flags)
>> +			    const struct net_device *dev, bool skip_dev_check,
>> +			    int strict, u32 banned_flags)
>>  {
>>  	unsigned int hash = inet6_addr_hash(net, addr);
>> +	const struct net_device *l3mdev;
>>  	struct inet6_ifaddr *ifp;
>>  	u32 ifp_flags;
>>  
>>  	rcu_read_lock();
>> +
>> +	l3mdev = l3mdev_master_dev_rcu(dev);
>> +
>>  	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
>>  		if (!net_eq(dev_net(ifp->idev->dev), net))
>>  			continue;
>> +
>> +		if (l3mdev_master_dev_rcu(ifp->idev->dev) != l3mdev)
>> +			continue;
>> +
>>  		/* Decouple optimistic from tentative for evaluation here.
>>  		 * Ban optimistic addresses explicitly, when required.
>>  		 */
>> @@ -1875,7 +1893,7 @@ int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>>  			    : ifp->flags;
>>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
>>  		    !(ifp_flags&banned_flags) &&
>> -		    (!dev || ifp->idev->dev == dev ||
>> +		    (skip_dev_check || ifp->idev->dev == dev ||
>>  		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>>  			rcu_read_unlock();
>>  			return 1;
> 
> There are two logical pieces in changes of this function:
> 
> 1)You become always pass not NULL dev and add skip_dev_check argument.
> 2)l3mdev_master_dev_rcu() check is introduced.
> 
> They should go in separate patches.
> 
>> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
>> index c61718dba2e6..d580d4d456a5 100644
>> --- a/net/ipv6/anycast.c
>> +++ b/net/ipv6/anycast.c
>> @@ -66,7 +66,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>>  		return -EPERM;
>>  	if (ipv6_addr_is_multicast(addr))
>>  		return -EINVAL;
>> -	if (ipv6_chk_addr(net, addr, NULL, 0))
>> +
>> +	if (ifindex)
>> +		dev = __dev_get_by_index(net, ifindex);
>> +
>> +	if (ipv6_chk_addr_and_flags(net, addr, dev, true, 0, IFA_F_TENTATIVE))
>>  		return -EINVAL;
>>  
>>  	pac = sock_kmalloc(sk, sizeof(struct ipv6_ac_socklist), GFP_KERNEL);
>> @@ -90,8 +94,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>>  			dev = __dev_get_by_flags(net, IFF_UP,
>>  						 IFF_UP | IFF_LOOPBACK);
>>  		}
>> -	} else
>> -		dev = __dev_get_by_index(net, ifindex);
>> +	}
> 
> The hunk moving __dev_get_by_index() dereference may go as separate change, as it's a refactoring.
> This will make the review easier.
> 
>>  
>>  	if (!dev) {
>>  		err = -ENODEV;
>> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
>> index fbf08ce3f5ab..b27333d7b099 100644
>> --- a/net/ipv6/datagram.c
>> +++ b/net/ipv6/datagram.c
>> @@ -801,8 +801,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>>  			if (addr_type != IPV6_ADDR_ANY) {
>>  				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
>>  				if (!(inet_sk(sk)->freebind || inet_sk(sk)->transparent) &&
>> -				    !ipv6_chk_addr(net, &src_info->ipi6_addr,
>> -						   strict ? dev : NULL, 0) &&
>> +				    !ipv6_chk_addr_and_flags(net, &src_info->ipi6_addr,
>> +							     dev, !strict, 0,
>> +							     IFA_F_TENTATIVE) &&
>>  				    !ipv6_chk_acast_addr_src(net, dev,
>>  							     &src_info->ipi6_addr))
>>  					err = -EINVAL;
>> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
>> index 56c4967f1868..1ce8244e8aee 100644
>> --- a/net/ipv6/ip6_tunnel.c
>> +++ b/net/ipv6/ip6_tunnel.c
>> @@ -758,9 +758,11 @@ int ip6_tnl_rcv_ctl(struct ip6_tnl *t,
>>  			ldev = dev_get_by_index_rcu(net, p->link);
>>  
>>  		if ((ipv6_addr_is_multicast(laddr) ||
>> -		     likely(ipv6_chk_addr(net, laddr, ldev, 0))) &&
>> +		     likely(ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> +						    0, IFA_F_TENTATIVE))) &&
>>  		    ((p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) ||
>> -		     likely(!ipv6_chk_addr(net, raddr, NULL, 0))))
>> +		     likely(!ipv6_chk_addr_and_flags(net, raddr, ldev, true,
>> +						     0, IFA_F_TENTATIVE))))
>>  			ret = 1;
>>  	}
>>  	return ret;
>> @@ -990,12 +992,14 @@ int ip6_tnl_xmit_ctl(struct ip6_tnl *t,
>>  		if (p->link)
>>  			ldev = dev_get_by_index_rcu(net, p->link);
>>  
>> -		if (unlikely(!ipv6_chk_addr(net, laddr, ldev, 0)))
>> +		if (unlikely(!ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> +						      0, IFA_F_TENTATIVE)))
>>  			pr_warn("%s xmit: Local address not yet configured!\n",
>>  				p->name);
>>  		else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
>>  			 !ipv6_addr_is_multicast(raddr) &&
>> -			 unlikely(ipv6_chk_addr(net, raddr, NULL, 0)))
>> +			 unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
>> +							  true, 0, IFA_F_TENTATIVE)))
>>  			pr_warn("%s xmit: Routing loop! Remote address found on this node!\n",
>>  				p->name);
>>  		else
>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>> index 0a19ce3a6f7f..13bf775c7f1a 100644
>> --- a/net/ipv6/ndisc.c
>> +++ b/net/ipv6/ndisc.c
>> @@ -707,7 +707,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>>  	int probes = atomic_read(&neigh->probes);
>>  
>>  	if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
>> -					   dev, 1,
>> +					   dev, false, 1,
>>  					   IFA_F_TENTATIVE|IFA_F_OPTIMISTIC))
>>  		saddr = &ipv6_hdr(skb)->saddr;
>>  	probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 3851c3ccfd7a..bbc62799eb3b 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2633,18 +2633,25 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>>  	const struct in6_addr *gw_addr = &cfg->fc_gateway;
>>  	int gwa_type = ipv6_addr_type(gw_addr);
>>  	const struct net_device *dev = *_dev;
>> +	bool need_local_addr_check = !dev;
>>  	int err = -EINVAL;
>>  
>> -	/* if gw_addr is local we will fail to detect this in case
>> -	 * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> -	 * will return already-added prefix route via interface that
>> -	 * prefix route was assigned to, which might be non-loopback.
>> +	/* if route spec contains the device, check if gateway address
>> +	 * is a local address in the same L3 domain
>>  	 */
>> -	if (ipv6_chk_addr_and_flags(net, gw_addr,
>> -				    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> -				    dev : NULL, 0, 0)) {
>> -		NL_SET_ERR_MSG(extack, "Invalid gateway address");
>> -		goto out;
>> +	if (dev) {
>> +		/* if gw_addr is local we will fail to detect this in case
>> +		 * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> +		 * will return already-added prefix route via interface that
>> +		 * prefix route was assigned to, which might be non-loopback.
>> +		 */
>> +		if (ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> +					    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> +					    false : true, 0, 0)) {
>> +			NL_SET_ERR_MSG(extack,
>> +				       "Gateway can not be a local address");
>> +			goto out;
>> +		}
> 
> Why do these two "if" go as tree, not as single "if (a && b)"?
> 
>>  	}
>>  
>>  	if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
>> @@ -2683,6 +2690,18 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>>  			       "Egress device can not be loopback device for this route");
>>  		goto out;
>>  	}
>> +
>> +	/* if we did not check gw_addr above, do so now that the
>> +	 * egress device has been resolved.
>> +	 */
>> +	if (need_local_addr_check &&
> 
> Do we really need a variable with so long name? Can't we use "local_check" or something like this?
> 
>> +	    ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> +				    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> +				    false : true, 0, 0)) {
> 
> Second time there is "gwa_type & IPV6_ADDR_LINKLOCAL ? false : true". gwa_type is constant,
> it doesn't change. Repeating constant expressions have to be be cached into local variable
> for improving readability.
> 
>> +		NL_SET_ERR_MSG(extack, "Gateway can not be a local address");
>> +		goto out;
>> +	}
>> +
>>  	err = 0;
>>  out:
>>  	return err;
>>
> 
> Thanks,
> Kirill
> 

Re: [PATCH v3 net-next 2/5] net/ipv6: Address checks need to consider the L3 domain

[David Ahern]

On 3/7/18 4:53 AM, Kirill Tkhai wrote:
>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>> index c4185a7b0e90..132e5b95167a 100644
>> --- a/include/net/addrconf.h
>> +++ b/include/net/addrconf.h
>> @@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
>>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>  		  const struct net_device *dev, int strict);
>>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> -			    const struct net_device *dev, int strict,
>> -			    u32 banned_flags);
>> +			    const struct net_device *dev, bool skip_dev_check,
>> +			    int strict, u32 banned_flags);
> 
> This function already has 5 arguments, while this patch adds one more.
> Can't we use new flags argument for both of them?
> 
> Also, the name of function and input parameters are already so big, that they
> don't fit a single line already, while your patch adds more parameters.
> Can't we make it more slim? Something like ipv6_chk_addr_fl() instead of current
> name.

I think I can combine strict and the new skip_dev_check. I am going to
leave the function name as is.

> 
>>  #if defined(CONFIG_IPV6_MIP6) || defined(CONFIG_IPV6_MIP6_MODULE)
>>  int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr);
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index b5fd116c046a..17d5d3f42d21 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1851,22 +1851,40 @@ static int ipv6_count_addresses(const struct inet6_dev *idev)
>>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>  		  const struct net_device *dev, int strict)
>>  {
>> -	return ipv6_chk_addr_and_flags(net, addr, dev, strict, IFA_F_TENTATIVE);
>> +	return ipv6_chk_addr_and_flags(net, addr, dev, !dev,
>> +				       strict, IFA_F_TENTATIVE);
>>  }
>>  EXPORT_SYMBOL(ipv6_chk_addr);
> 
> This function was not introduced by this commit, but since the commit modifies it,
> and the function is pretty simple, we could declare it as static inline in header
> in separate patch.

That function is needed by netfilter code. Any consolidation is outside
the scope of this patch set.

> 
>>  
>> +/* device argument is used to find the L3 domain of interest. If
>> + * skip_dev_check is set, then the ifp device is not checked against
>> + * the passed in dev argument. So the 2 cases for addresses checks are:
>> + *   1. does the address exist in the L3 domain that dev is part of
>> + *      (skip_dev_check = true), or
>> + *
>> + *   2. does the address exist on the specific device
>> + *      (skip_dev_check = false)
>> + */
>>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>> -			    const struct net_device *dev, int strict,
>> -			    u32 banned_flags)
>> +			    const struct net_device *dev, bool skip_dev_check,
>> +			    int strict, u32 banned_flags)
>>  {
>>  	unsigned int hash = inet6_addr_hash(net, addr);
>> +	const struct net_device *l3mdev;
>>  	struct inet6_ifaddr *ifp;
>>  	u32 ifp_flags;
>>  
>>  	rcu_read_lock();
>> +
>> +	l3mdev = l3mdev_master_dev_rcu(dev);
>> +
>>  	hlist_for_each_entry_rcu(ifp, &inet6_addr_lst[hash], addr_lst) {
>>  		if (!net_eq(dev_net(ifp->idev->dev), net))
>>  			continue;
>> +
>> +		if (l3mdev_master_dev_rcu(ifp->idev->dev) != l3mdev)
>> +			continue;
>> +
>>  		/* Decouple optimistic from tentative for evaluation here.
>>  		 * Ban optimistic addresses explicitly, when required.
>>  		 */
>> @@ -1875,7 +1893,7 @@ int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>>  			    : ifp->flags;
>>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
>>  		    !(ifp_flags&banned_flags) &&
>> -		    (!dev || ifp->idev->dev == dev ||
>> +		    (skip_dev_check || ifp->idev->dev == dev ||
>>  		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>>  			rcu_read_unlock();
>>  			return 1;
> 
> There are two logical pieces in changes of this function:
> 
> 1)You become always pass not NULL dev and add skip_dev_check argument.
> 2)l3mdev_master_dev_rcu() check is introduced.

dev can still be null; l3mdev lookup handles a null argument.

> 
> They should go in separate patches.

The change needs to go in together since I am altering how this function
works.

> 
>> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
>> index c61718dba2e6..d580d4d456a5 100644
>> --- a/net/ipv6/anycast.c
>> +++ b/net/ipv6/anycast.c
>> @@ -66,7 +66,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>>  		return -EPERM;
>>  	if (ipv6_addr_is_multicast(addr))
>>  		return -EINVAL;
>> -	if (ipv6_chk_addr(net, addr, NULL, 0))
>> +
>> +	if (ifindex)
>> +		dev = __dev_get_by_index(net, ifindex);
>> +
>> +	if (ipv6_chk_addr_and_flags(net, addr, dev, true, 0, IFA_F_TENTATIVE))
>>  		return -EINVAL;
>>  
>>  	pac = sock_kmalloc(sk, sizeof(struct ipv6_ac_socklist), GFP_KERNEL);
>> @@ -90,8 +94,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>>  			dev = __dev_get_by_flags(net, IFF_UP,
>>  						 IFF_UP | IFF_LOOPBACK);
>>  		}
>> -	} else
>> -		dev = __dev_get_by_index(net, ifindex);
>> +	}
> 
> The hunk moving __dev_get_by_index() dereference may go as separate change, as it's a refactoring.
> This will make the review easier.

Moving 1 line of code up a few lines should not need a standalone patch;
I think the above is understandable.

> 
>>  
>>  	if (!dev) {
>>  		err = -ENODEV;
>> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
>> index fbf08ce3f5ab..b27333d7b099 100644
>> --- a/net/ipv6/datagram.c
>> +++ b/net/ipv6/datagram.c
>> @@ -801,8 +801,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>>  			if (addr_type != IPV6_ADDR_ANY) {
>>  				int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
>>  				if (!(inet_sk(sk)->freebind || inet_sk(sk)->transparent) &&
>> -				    !ipv6_chk_addr(net, &src_info->ipi6_addr,
>> -						   strict ? dev : NULL, 0) &&
>> +				    !ipv6_chk_addr_and_flags(net, &src_info->ipi6_addr,
>> +							     dev, !strict, 0,
>> +							     IFA_F_TENTATIVE) &&
>>  				    !ipv6_chk_acast_addr_src(net, dev,
>>  							     &src_info->ipi6_addr))
>>  					err = -EINVAL;
>> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
>> index 56c4967f1868..1ce8244e8aee 100644
>> --- a/net/ipv6/ip6_tunnel.c
>> +++ b/net/ipv6/ip6_tunnel.c
>> @@ -758,9 +758,11 @@ int ip6_tnl_rcv_ctl(struct ip6_tnl *t,
>>  			ldev = dev_get_by_index_rcu(net, p->link);
>>  
>>  		if ((ipv6_addr_is_multicast(laddr) ||
>> -		     likely(ipv6_chk_addr(net, laddr, ldev, 0))) &&
>> +		     likely(ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> +						    0, IFA_F_TENTATIVE))) &&
>>  		    ((p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) ||
>> -		     likely(!ipv6_chk_addr(net, raddr, NULL, 0))))
>> +		     likely(!ipv6_chk_addr_and_flags(net, raddr, ldev, true,
>> +						     0, IFA_F_TENTATIVE))))
>>  			ret = 1;
>>  	}
>>  	return ret;
>> @@ -990,12 +992,14 @@ int ip6_tnl_xmit_ctl(struct ip6_tnl *t,
>>  		if (p->link)
>>  			ldev = dev_get_by_index_rcu(net, p->link);
>>  
>> -		if (unlikely(!ipv6_chk_addr(net, laddr, ldev, 0)))
>> +		if (unlikely(!ipv6_chk_addr_and_flags(net, laddr, ldev, false,
>> +						      0, IFA_F_TENTATIVE)))
>>  			pr_warn("%s xmit: Local address not yet configured!\n",
>>  				p->name);
>>  		else if (!(p->flags & IP6_TNL_F_ALLOW_LOCAL_REMOTE) &&
>>  			 !ipv6_addr_is_multicast(raddr) &&
>> -			 unlikely(ipv6_chk_addr(net, raddr, NULL, 0)))
>> +			 unlikely(ipv6_chk_addr_and_flags(net, raddr, ldev,
>> +							  true, 0, IFA_F_TENTATIVE)))
>>  			pr_warn("%s xmit: Routing loop! Remote address found on this node!\n",
>>  				p->name);
>>  		else
>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>> index 0a19ce3a6f7f..13bf775c7f1a 100644
>> --- a/net/ipv6/ndisc.c
>> +++ b/net/ipv6/ndisc.c
>> @@ -707,7 +707,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>>  	int probes = atomic_read(&neigh->probes);
>>  
>>  	if (skb && ipv6_chk_addr_and_flags(dev_net(dev), &ipv6_hdr(skb)->saddr,
>> -					   dev, 1,
>> +					   dev, false, 1,
>>  					   IFA_F_TENTATIVE|IFA_F_OPTIMISTIC))
>>  		saddr = &ipv6_hdr(skb)->saddr;
>>  	probes -= NEIGH_VAR(neigh->parms, UCAST_PROBES);
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 3851c3ccfd7a..bbc62799eb3b 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2633,18 +2633,25 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>>  	const struct in6_addr *gw_addr = &cfg->fc_gateway;
>>  	int gwa_type = ipv6_addr_type(gw_addr);
>>  	const struct net_device *dev = *_dev;
>> +	bool need_local_addr_check = !dev;
>>  	int err = -EINVAL;
>>  
>> -	/* if gw_addr is local we will fail to detect this in case
>> -	 * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> -	 * will return already-added prefix route via interface that
>> -	 * prefix route was assigned to, which might be non-loopback.
>> +	/* if route spec contains the device, check if gateway address
>> +	 * is a local address in the same L3 domain
>>  	 */
>> -	if (ipv6_chk_addr_and_flags(net, gw_addr,
>> -				    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> -				    dev : NULL, 0, 0)) {
>> -		NL_SET_ERR_MSG(extack, "Invalid gateway address");
>> -		goto out;
>> +	if (dev) {
>> +		/* if gw_addr is local we will fail to detect this in case
>> +		 * address is still TENTATIVE (DAD in progress). rt6_lookup()
>> +		 * will return already-added prefix route via interface that
>> +		 * prefix route was assigned to, which might be non-loopback.
>> +		 */
>> +		if (ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> +					    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> +					    false : true, 0, 0)) {
>> +			NL_SET_ERR_MSG(extack,
>> +				       "Gateway can not be a local address");
>> +			goto out;
>> +		}
> 
> Why do these two "if" go as tree, not as single "if (a && b)"?

yes, I should combine that.


> 
>>  	}
>>  
>>  	if (gwa_type != (IPV6_ADDR_LINKLOCAL | IPV6_ADDR_UNICAST)) {
>> @@ -2683,6 +2690,18 @@ static int ip6_validate_gw(struct net *net, struct fib6_config *cfg,
>>  			       "Egress device can not be loopback device for this route");
>>  		goto out;
>>  	}
>> +
>> +	/* if we did not check gw_addr above, do so now that the
>> +	 * egress device has been resolved.
>> +	 */
>> +	if (need_local_addr_check &&
> 
> Do we really need a variable with so long name? Can't we use "local_check" or something like this?

given the limited use of the flag, I believe the longer name is more
readable. Making it shorter does not allow the 2 checks to consolidate
into 1 line, so nothing is gained by an overly short variable name.

> 
>> +	    ipv6_chk_addr_and_flags(net, gw_addr, dev,
>> +				    gwa_type & IPV6_ADDR_LINKLOCAL ?
>> +				    false : true, 0, 0)) {
> 
> Second time there is "gwa_type & IPV6_ADDR_LINKLOCAL ? false : true". gwa_type is constant,
> it doesn't change. Repeating constant expressions have to be be cached into local variable
> for improving readability.

they don't have to be but I will make a bool for it.


> 
>> +		NL_SET_ERR_MSG(extack, "Gateway can not be a local address");
>> +		goto out;
>> +	}
>> +
>>  	err = 0;
>>  out:
>>  	return err;
>>
> 
> Thanks,
> Kirill
> 

Re: [PATCH v3 net-next 2/5] net/ipv6: Address checks need to consider the L3 domain

[David Ahern]

On 3/7/18 10:28 AM, David Ahern wrote:
> On 3/7/18 4:53 AM, Kirill Tkhai wrote:
>>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>>> index c4185a7b0e90..132e5b95167a 100644
>>> --- a/include/net/addrconf.h
>>> +++ b/include/net/addrconf.h
>>> @@ -69,8 +69,8 @@ int addrconf_set_dstaddr(struct net *net, void __user *arg);
>>>  int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
>>>  		  const struct net_device *dev, int strict);
>>>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>>> -			    const struct net_device *dev, int strict,
>>> -			    u32 banned_flags);
>>> +			    const struct net_device *dev, bool skip_dev_check,
>>> +			    int strict, u32 banned_flags);
>>
>> This function already has 5 arguments, while this patch adds one more.
>> Can't we use new flags argument for both of them?
>>
>> Also, the name of function and input parameters are already so big, that they
>> don't fit a single line already, while your patch adds more parameters.
>> Can't we make it more slim? Something like ipv6_chk_addr_fl() instead of current
>> name.
> 
> I think I can combine strict and the new skip_dev_check. I am going to
> leave the function name as is.
> 

Upon further review, I can not combine those flags; I missed a level of
() around the scope check which is what strict modifies. They need to be
separate arguments.

[PATCH v3 net-next 3/5] selftests: fib_tests: Use an alias for ip command

[David Ahern]

Replace 'ip -netns testns' with the alias IP. Shortens the line lengths
and makes running the commands manually a bit easier.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/testing/selftests/net/fib_tests.sh | 169 ++++++++++++++++---------------
 1 file changed, 85 insertions(+), 84 deletions(-)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index b617985ecdc1..953254439e39 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -7,6 +7,7 @@
 ret=0
 
 PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
+IP="ip -netns testns"
 
 log_test()
 {
@@ -32,19 +33,19 @@ setup()
 {
 	set -e
 	ip netns add testns
-	ip -netns testns link set dev lo up
+	$IP link set dev lo up
 
-	ip -netns testns link add dummy0 type dummy
-	ip -netns testns link set dev dummy0 up
-	ip -netns testns address add 198.51.100.1/24 dev dummy0
-	ip -netns testns -6 address add 2001:db8:1::1/64 dev dummy0
+	$IP link add dummy0 type dummy
+	$IP link set dev dummy0 up
+	$IP address add 198.51.100.1/24 dev dummy0
+	$IP -6 address add 2001:db8:1::1/64 dev dummy0
 	set +e
 
 }
 
 cleanup()
 {
-	ip -netns testns link del dev dummy0 &> /dev/null
+	$IP link del dev dummy0 &> /dev/null
 	ip netns del testns
 }
 
@@ -56,19 +57,19 @@ fib_unreg_unicast_test()
 	setup
 
 	echo "    Start point"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
 	set -e
-	ip -netns testns link del dev dummy0
+	$IP link del dev dummy0
 	set +e
 
 	echo "    Nexthop device deleted"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 2 "IPv4 fibmatch - no route"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 2 "IPv6 fibmatch - no route"
 
 	cleanup
@@ -83,43 +84,43 @@ fib_unreg_multipath_test()
 	setup
 
 	set -e
-	ip -netns testns link add dummy1 type dummy
-	ip -netns testns link set dev dummy1 up
-	ip -netns testns address add 192.0.2.1/24 dev dummy1
-	ip -netns testns -6 address add 2001:db8:2::1/64 dev dummy1
+	$IP link add dummy1 type dummy
+	$IP link set dev dummy1 up
+	$IP address add 192.0.2.1/24 dev dummy1
+	$IP -6 address add 2001:db8:2::1/64 dev dummy1
 
-	ip -netns testns route add 203.0.113.0/24 \
+	$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 testns -6 route add 2001:db8:3::/64 \
+	$IP -6 route add 2001:db8:3::/64 \
 		nexthop via 2001:db8:1::2 dev dummy0 \
 		nexthop via 2001:db8:2::2 dev dummy1
 	set +e
 
 	echo "    Start point"
-	ip -netns testns route get fibmatch 203.0.113.1 &> /dev/null
+	$IP route get fibmatch 203.0.113.1 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:3::1 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
 	set -e
-	ip -netns testns link del dev dummy0
+	$IP link del dev dummy0
 	set +e
 
 	echo "    One nexthop device deleted"
-	ip -netns testns route get fibmatch 203.0.113.1 &> /dev/null
+	$IP route get fibmatch 203.0.113.1 &> /dev/null
 	log_test $? 2 "IPv4 - multipath route removed on delete"
 
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:3::1 &> /dev/null
 	# In IPv6 we do not flush the entire multipath route.
 	log_test $? 0 "IPv6 - multipath down to single path"
 
 	set -e
-	ip -netns testns link del dev dummy1
+	$IP link del dev dummy1
 	set +e
 
 	echo "    Second nexthop device deleted"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:3::1 &> /dev/null
 	log_test $? 2 "IPv6 - no route"
 
 	cleanup
@@ -139,19 +140,19 @@ fib_down_unicast_test()
 	setup
 
 	echo "    Start point"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
 	set -e
-	ip -netns testns link set dev dummy0 down
+	$IP link set dev dummy0 down
 	set +e
 
 	echo "    Route deleted on down"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 2 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 2 "IPv6 fibmatch"
 
 	cleanup
@@ -162,31 +163,31 @@ fib_down_multipath_test_do()
 	local down_dev=$1
 	local up_dev=$2
 
-	ip -netns testns route get fibmatch 203.0.113.1 \
+	$IP route get fibmatch 203.0.113.1 \
 		oif $down_dev &> /dev/null
 	log_test $? 2 "IPv4 fibmatch on down device"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 \
+	$IP -6 route get fibmatch 2001:db8:3::1 \
 		oif $down_dev &> /dev/null
 	log_test $? 2 "IPv6 fibmatch on down device"
 
-	ip -netns testns route get fibmatch 203.0.113.1 \
+	$IP route get fibmatch 203.0.113.1 \
 		oif $up_dev &> /dev/null
 	log_test $? 0 "IPv4 fibmatch on up device"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 \
+	$IP -6 route get fibmatch 2001:db8:3::1 \
 		oif $up_dev &> /dev/null
 	log_test $? 0 "IPv6 fibmatch on up device"
 
-	ip -netns testns route get fibmatch 203.0.113.1 | \
+	$IP route get fibmatch 203.0.113.1 | \
 		grep $down_dev | grep -q "dead linkdown"
 	log_test $? 0 "IPv4 flags on down device"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 | \
+	$IP -6 route get fibmatch 2001:db8:3::1 | \
 		grep $down_dev | grep -q "dead linkdown"
 	log_test $? 0 "IPv6 flags on down device"
 
-	ip -netns testns route get fibmatch 203.0.113.1 | \
+	$IP route get fibmatch 203.0.113.1 | \
 		grep $up_dev | grep -q "dead linkdown"
 	log_test $? 1 "IPv4 flags on up device"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 | \
+	$IP -6 route get fibmatch 2001:db8:3::1 | \
 		grep $up_dev | grep -q "dead linkdown"
 	log_test $? 1 "IPv6 flags on up device"
 }
@@ -199,53 +200,53 @@ fib_down_multipath_test()
 	setup
 
 	set -e
-	ip -netns testns link add dummy1 type dummy
-	ip -netns testns link set dev dummy1 up
+	$IP link add dummy1 type dummy
+	$IP link set dev dummy1 up
 
-	ip -netns testns address add 192.0.2.1/24 dev dummy1
-	ip -netns testns -6 address add 2001:db8:2::1/64 dev dummy1
+	$IP address add 192.0.2.1/24 dev dummy1
+	$IP -6 address add 2001:db8:2::1/64 dev dummy1
 
-	ip -netns testns route add 203.0.113.0/24 \
+	$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 testns -6 route add 2001:db8:3::/64 \
+	$IP -6 route add 2001:db8:3::/64 \
 		nexthop via 2001:db8:1::2 dev dummy0 \
 		nexthop via 2001:db8:2::2 dev dummy1
 	set +e
 
 	echo "    Verify start point"
-	ip -netns testns route get fibmatch 203.0.113.1 &> /dev/null
+	$IP route get fibmatch 203.0.113.1 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
 
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:3::1 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
 	set -e
-	ip -netns testns link set dev dummy0 down
+	$IP link set dev dummy0 down
 	set +e
 
 	echo "    One device down, one up"
 	fib_down_multipath_test_do "dummy0" "dummy1"
 
 	set -e
-	ip -netns testns link set dev dummy0 up
-	ip -netns testns link set dev dummy1 down
+	$IP link set dev dummy0 up
+	$IP link set dev dummy1 down
 	set +e
 
 	echo "    Other device down and up"
 	fib_down_multipath_test_do "dummy1" "dummy0"
 
 	set -e
-	ip -netns testns link set dev dummy0 down
+	$IP link set dev dummy0 down
 	set +e
 
 	echo "    Both devices down"
-	ip -netns testns route get fibmatch 203.0.113.1 &> /dev/null
+	$IP route get fibmatch 203.0.113.1 &> /dev/null
 	log_test $? 2 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:3::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:3::1 &> /dev/null
 	log_test $? 2 "IPv6 fibmatch"
 
-	ip -netns testns link del dev dummy1
+	$IP link del dev dummy1
 	cleanup
 }
 
@@ -264,55 +265,55 @@ fib_carrier_local_test()
 	setup
 
 	set -e
-	ip -netns testns link set dev dummy0 carrier on
+	$IP link set dev dummy0 carrier on
 	set +e
 
 	echo "    Start point"
-	ip -netns testns route get fibmatch 198.51.100.1 &> /dev/null
+	$IP route get fibmatch 198.51.100.1 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::1 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 198.51.100.1 | \
+	$IP route get fibmatch 198.51.100.1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv4 - no linkdown flag"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::1 | \
+	$IP -6 route get fibmatch 2001:db8:1::1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv6 - no linkdown flag"
 
 	set -e
-	ip -netns testns link set dev dummy0 carrier off
+	$IP link set dev dummy0 carrier off
 	sleep 1
 	set +e
 
 	echo "    Carrier off on nexthop"
-	ip -netns testns route get fibmatch 198.51.100.1 &> /dev/null
+	$IP route get fibmatch 198.51.100.1 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::1 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 198.51.100.1 | \
+	$IP route get fibmatch 198.51.100.1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv4 - linkdown flag set"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::1 | \
+	$IP -6 route get fibmatch 2001:db8:1::1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv6 - linkdown flag set"
 
 	set -e
-	ip -netns testns address add 192.0.2.1/24 dev dummy0
-	ip -netns testns -6 address add 2001:db8:2::1/64 dev dummy0
+	$IP address add 192.0.2.1/24 dev dummy0
+	$IP -6 address add 2001:db8:2::1/64 dev dummy0
 	set +e
 
 	echo "    Route to local address with carrier down"
-	ip -netns testns route get fibmatch 192.0.2.1 &> /dev/null
+	$IP route get fibmatch 192.0.2.1 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:2::1 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:2::1 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 192.0.2.1 | \
+	$IP route get fibmatch 192.0.2.1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv4 linkdown flag set"
-	ip -netns testns -6 route get fibmatch 2001:db8:2::1 | \
+	$IP -6 route get fibmatch 2001:db8:2::1 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv6 linkdown flag set"
 
@@ -329,54 +330,54 @@ fib_carrier_unicast_test()
 	setup
 
 	set -e
-	ip -netns testns link set dev dummy0 carrier on
+	$IP link set dev dummy0 carrier on
 	set +e
 
 	echo "    Start point"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 198.51.100.2 | \
+	$IP route get fibmatch 198.51.100.2 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv4 no linkdown flag"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 | \
+	$IP -6 route get fibmatch 2001:db8:1::2 | \
 		grep -q "linkdown"
 	log_test $? 1 "IPv6 no linkdown flag"
 
 	set -e
-	ip -netns testns link set dev dummy0 carrier off
+	$IP link set dev dummy0 carrier off
 	set +e
 
 	echo "    Carrier down"
-	ip -netns testns route get fibmatch 198.51.100.2 &> /dev/null
+	$IP route get fibmatch 198.51.100.2 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:1::2 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 198.51.100.2 | \
+	$IP route get fibmatch 198.51.100.2 | \
 		grep -q "linkdown"
 	log_test $? 0 "IPv4 linkdown flag set"
-	ip -netns testns -6 route get fibmatch 2001:db8:1::2 | \
+	$IP -6 route get fibmatch 2001:db8:1::2 | \
 		grep -q "linkdown"
 	log_test $? 0 "IPv6 linkdown flag set"
 
 	set -e
-	ip -netns testns address add 192.0.2.1/24 dev dummy0
-	ip -netns testns -6 address add 2001:db8:2::1/64 dev dummy0
+	$IP address add 192.0.2.1/24 dev dummy0
+	$IP -6 address add 2001:db8:2::1/64 dev dummy0
 	set +e
 
 	echo "    Second address added with carrier down"
-	ip -netns testns route get fibmatch 192.0.2.2 &> /dev/null
+	$IP route get fibmatch 192.0.2.2 &> /dev/null
 	log_test $? 0 "IPv4 fibmatch"
-	ip -netns testns -6 route get fibmatch 2001:db8:2::2 &> /dev/null
+	$IP -6 route get fibmatch 2001:db8:2::2 &> /dev/null
 	log_test $? 0 "IPv6 fibmatch"
 
-	ip -netns testns route get fibmatch 192.0.2.2 | \
+	$IP route get fibmatch 192.0.2.2 | \
 		grep -q "linkdown"
 	log_test $? 0 "IPv4 linkdown flag set"
-	ip -netns testns -6 route get fibmatch 2001:db8:2::2 | \
+	$IP -6 route get fibmatch 2001:db8:2::2 | \
 		grep -q "linkdown"
 	log_test $? 0 "IPv6 linkdown flag set"
 
-- 
2.11.0

[PATCH v3 net-next 4/5] selftests: fib_tests: Allow user to run a specific test

[David Ahern]

Allow a user to run just a specific fib test by setting the TEST
environment variable.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/testing/selftests/net/fib_tests.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 953254439e39..cfdeb35bfed5 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -392,9 +392,13 @@ fib_carrier_test()
 
 fib_test()
 {
-	fib_unreg_test
-	fib_down_test
-	fib_carrier_test
+	if [ -n "$TEST" ]; then
+		eval $TEST
+	else
+		fib_unreg_test
+		fib_down_test
+		fib_carrier_test
+	fi
 }
 
 if [ "$(id -u)" -ne 0 ];then
-- 
2.11.0

[PATCH v3 net-next 5/5] selftests: fib_tests: Add IPv6 nexthop spec tests

[David Ahern]

Add series of tests for valid and invalid nexthop specs for IPv6.

$ TEST=fib_nexthop_test ./fib_tests.sh
...
IPv6 nexthop tests
    TEST: Directly connected nexthop, unicast address              [ OK ]
    TEST: Directly connected nexthop, unicast address with device  [ OK ]
    TEST: Gateway is linklocal address                             [ OK ]
    TEST: Gateway is linklocal address, no device                  [ OK ]
    TEST: Gateway can not be local unicast address                 [ OK ]
    TEST: Gateway can not be local unicast address, with device    [ OK ]
    TEST: Gateway can not be a local linklocal address             [ OK ]
    TEST: Gateway can be local address in a VRF                    [ OK ]
    TEST: Gateway can be local address in a VRF, with device       [ OK ]
    TEST: Gateway can be local linklocal address in a VRF          [ OK ]
    TEST: Redirect to VRF lookup                                   [ OK ]
    TEST: VRF route, gateway can be local address in default VRF   [ OK ]
    TEST: VRF route, gateway can not be a local address            [ OK ]
    TEST: VRF route, gateway can not be a local addr with device   [ OK ]

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/testing/selftests/net/fib_tests.sh | 180 ++++++++++++++++++++++++++++++-
 1 file changed, 178 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index cfdeb35bfed5..9164e60d4b66 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -6,6 +6,7 @@
 
 ret=0
 
+VERBOSE=${VERBOSE:=0}
 PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
 IP="ip -netns testns"
 
@@ -16,10 +17,10 @@ log_test()
 	local msg="$3"
 
 	if [ ${rc} -eq ${expected} ]; then
-		printf "        %-60s  [ OK ]\n" "${msg}"
+		printf "    TEST: %-60s  [ OK ]\n" "${msg}"
 	else
 		ret=1
-		printf "        %-60s  [FAIL]\n" "${msg}"
+		printf "    TEST: %-60s  [FAIL]\n" "${msg}"
 		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
 		echo
 			echo "hit enter to continue, 'q' to quit"
@@ -49,6 +50,28 @@ cleanup()
 	ip netns del testns
 }
 
+get_linklocal()
+{
+	local dev=$1
+	local addr
+
+	addr=$($IP -6 -br addr show dev ${dev} | \
+	awk '{
+		for (i = 3; i <= NF; ++i) {
+			if ($i ~ /^fe80/)
+				print $i
+		}
+	}'
+	)
+	addr=${addr/\/*}
+
+	[ -z "$addr" ] && return 1
+
+	echo $addr
+
+	return 0
+}
+
 fib_unreg_unicast_test()
 {
 	echo
@@ -390,6 +413,158 @@ fib_carrier_test()
 	fib_carrier_unicast_test
 }
 
+################################################################################
+# Tests on nexthop spec
+
+# run 'ip route add' with given spec
+add_rt()
+{
+	local desc="$1"
+	local erc=$2
+	local vrf=$3
+	local pfx=$4
+	local gw=$5
+	local dev=$6
+	local cmd out rc
+
+	[ "$vrf" = "-" ] && vrf="default"
+	[ -n "$gw" ] && gw="via $gw"
+	[ -n "$dev" ] && dev="dev $dev"
+
+	cmd="$IP route add vrf $vrf $pfx $gw $dev"
+	if [ "$VERBOSE" = "1" ]; then
+		printf "\n    COMMAND: $cmd\n"
+	fi
+
+	out=$(eval $cmd 2>&1)
+	rc=$?
+	if [ "$VERBOSE" = "1" -a -n "$out" ]; then
+		echo "    $out"
+	fi
+	log_test $rc $erc "$desc"
+}
+
+fib4_nexthop()
+{
+	echo
+	echo "IPv4 nexthop tests"
+
+	echo "<<< write me >>>"
+}
+
+fib6_nexthop()
+{
+	local lldummy=$(get_linklocal dummy0)
+	local llv1=$(get_linklocal dummy0)
+
+	if [ -z "$lldummy" ]; then
+		echo "Failed to get linklocal address for dummy0"
+		return 1
+	fi
+	if [ -z "$llv1" ]; then
+		echo "Failed to get linklocal address for veth1"
+		return 1
+	fi
+
+	echo
+	echo "IPv6 nexthop tests"
+
+	add_rt "Directly connected nexthop, unicast address" 0 \
+		- 2001:db8:101::/64 2001:db8:1::2
+	add_rt "Directly connected nexthop, unicast address with device" 0 \
+		- 2001:db8:102::/64 2001:db8:1::2 "dummy0"
+	add_rt "Gateway is linklocal address" 0 \
+		- 2001:db8:103::1/64 $llv1 "veth0"
+
+	# fails because LL address requires a device
+	add_rt "Gateway is linklocal address, no device" 2 \
+		- 2001:db8:104::1/64 $llv1
+
+	# local address can not be a gateway
+	add_rt "Gateway can not be local unicast address" 2 \
+		- 2001:db8:105::/64 2001:db8:1::1
+	add_rt "Gateway can not be local unicast address, with device" 2 \
+		- 2001:db8:106::/64 2001:db8:1::1 "dummy0"
+	add_rt "Gateway can not be a local linklocal address" 2 \
+		- 2001:db8:107::1/64 $lldummy "dummy0"
+
+	# VRF tests
+	add_rt "Gateway can be local address in a VRF" 0 \
+		- 2001:db8:108::/64 2001:db8:51::2
+	add_rt "Gateway can be local address in a VRF, with device" 0 \
+		- 2001:db8:109::/64 2001:db8:51::2 "veth0"
+	add_rt "Gateway can be local linklocal address in a VRF" 0 \
+		- 2001:db8:110::1/64 $llv1 "veth0"
+
+	add_rt "Redirect to VRF lookup" 0 \
+		- 2001:db8:111::/64 "" "red"
+
+	add_rt "VRF route, gateway can be local address in default VRF" 0 \
+		red 2001:db8:112::/64 2001:db8:51::1
+
+	# local address in same VRF fails
+	add_rt "VRF route, gateway can not be a local address" 2 \
+		red 2001:db8:113::1/64 2001:db8:2::1
+	add_rt "VRF route, gateway can not be a local addr with device" 2 \
+		red 2001:db8:114::1/64 2001:db8:2::1 "dummy1"
+}
+
+# Default VRF:
+#   dummy0 - 198.51.100.1/24 2001:db8:1::1/64
+#   veth0  - 192.0.2.1/24    2001:db8:51::1/64
+#
+# VRF red:
+#   dummy1 - 192.168.2.1/24 2001:db8:2::1/64
+#   veth1  - 192.0.2.2/24   2001:db8:51::2/64
+#
+#  [ dummy0   veth0 ]--[ veth1   dummy1 ]
+
+fib_nexthop_test()
+{
+	setup
+
+	set -e
+
+	$IP -4 rule add pref 32765 table local
+	$IP -4 rule del pref 0
+	$IP -6 rule add pref 32765 table local
+	$IP -6 rule del pref 0
+
+	$IP link add red type vrf table 1
+	$IP link set red up
+	$IP -4 route add vrf red unreachable default metric 4278198272
+	$IP -6 route add vrf red unreachable default metric 4278198272
+
+	$IP link add veth0 type veth peer name veth1
+	$IP link set dev veth0 up
+	$IP address add 192.0.2.1/24 dev veth0
+	$IP -6 address add 2001:db8:51::1/64 dev veth0
+
+	$IP link set dev veth1 vrf red up
+	$IP address add 192.0.2.2/24 dev veth1
+	$IP -6 address add 2001:db8:51::2/64 dev veth1
+
+	$IP link add dummy1 type dummy
+	$IP link set dev dummy1 vrf red up
+	$IP address add 192.168.2.1/24 dev dummy1
+	$IP -6 address add 2001:db8:2::1/64 dev dummy1
+	set +e
+
+	sleep 1
+	fib4_nexthop
+	fib6_nexthop
+
+	(
+	$IP link del dev dummy1
+	$IP link del veth0
+	$IP link del red
+	) 2>/dev/null
+	cleanup
+}
+
+################################################################################
+#
+
 fib_test()
 {
 	if [ -n "$TEST" ]; then
@@ -398,6 +573,7 @@ fib_test()
 		fib_unreg_test
 		fib_down_test
 		fib_carrier_test
+		fib_nexthop_test
 	fi
 }
 
-- 
2.11.0