[PATCH net-next 0/7] ipv4: nexthop: Various improvements

[PATCH net-next 0/7] ipv4: nexthop: Various improvements

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patch set contains various improvements that I made to the nexthop
object code while studying it towards my upcoming changes.

While patches #4 and #6 fix bugs, they are not regressions (never
worked). They also do not occur to me as critical issues, which is why I
am targeting them at net-next.

Tested with fib_nexthops.sh:

Tests passed: 134
Tests failed:   0

Ido Schimmel (7):
  ipv4: nexthop: Reduce allocation size of 'struct nh_group'
  ipv4: nexthop: Use nla_put_be32() for NHA_GATEWAY
  ipv4: nexthop: Remove unnecessary rtnl_dereference()
  ipv4: nexthop: Correctly update nexthop group when removing a nexthop
  selftests: fib_nexthops: Test IPv6 route with group after removing
    IPv4 nexthops
  ipv4: nexthop: Correctly update nexthop group when replacing a nexthop
  selftests: fib_nexthops: Test IPv6 route with group after replacing
    IPv4 nexthops

 net/ipv4/nexthop.c                          | 49 ++++++++++++++++++---
 tools/testing/selftests/net/fib_nexthops.sh | 30 +++++++++++++
 2 files changed, 72 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH net-next 1/7] ipv4: nexthop: Reduce allocation size of 'struct nh_group'

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The struct looks as follows:

struct nh_group {
	struct nh_group		*spare; /* spare group for removals */
	u16			num_nh;
	bool			mpath;
	bool			fdb_nh;
	bool			has_v4;
	struct nh_grp_entry	nh_entries[];
};

But its offset within 'struct nexthop' is also taken into account to
determine the allocation size.

Instead, use struct_size() to allocate only the required number of
bytes.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/nexthop.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 134e92382275..d13730ff9aeb 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -133,12 +133,9 @@ static struct nexthop *nexthop_alloc(void)
 
 static struct nh_group *nexthop_grp_alloc(u16 num_nh)
 {
-	size_t sz = offsetof(struct nexthop, nh_grp)
-		    + sizeof(struct nh_group)
-		    + sizeof(struct nh_grp_entry) * num_nh;
 	struct nh_group *nhg;
 
-	nhg = kzalloc(sz, GFP_KERNEL);
+	nhg = kzalloc(struct_size(nhg, nh_entries, num_nh), GFP_KERNEL);
 	if (nhg)
 		nhg->num_nh = num_nh;
 
-- 
2.26.2

[PATCH net-next 2/7] ipv4: nexthop: Use nla_put_be32() for NHA_GATEWAY

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The code correctly uses nla_get_be32() to get the payload of the
attribute, but incorrectly uses nla_put_u32() to add the attribute to
the payload. This results in the following warning:

net/ipv4/nexthop.c:279:59: warning: incorrect type in argument 3 (different base types)
net/ipv4/nexthop.c:279:59:    expected unsigned int [usertype] value
net/ipv4/nexthop.c:279:59:    got restricted __be32 [usertype] ipv4

Suppress the warning by using nla_put_be32().

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

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index d13730ff9aeb..0823643a7dec 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -276,7 +276,7 @@ static int nh_fill_node(struct sk_buff *skb, struct nexthop *nh,
 	case AF_INET:
 		fib_nh = &nhi->fib_nh;
 		if (fib_nh->fib_nh_gw_family &&
-		    nla_put_u32(skb, NHA_GATEWAY, fib_nh->fib_nh_gw4))
+		    nla_put_be32(skb, NHA_GATEWAY, fib_nh->fib_nh_gw4))
 			goto nla_put_failure;
 		break;
 
-- 
2.26.2

[PATCH net-next 3/7] ipv4: nexthop: Remove unnecessary rtnl_dereference()

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The pointer is not RCU protected, so remove the unnecessary
rtnl_dereference(). This suppresses the following warning:

net/ipv4/nexthop.c:1101:24: error: incompatible types in comparison expression (different address spaces):
net/ipv4/nexthop.c:1101:24:    struct rb_node [noderef] __rcu *
net/ipv4/nexthop.c:1101:24:    struct rb_node *

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

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 0823643a7dec..1b736e3e1baa 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1098,7 +1098,7 @@ static int insert_nexthop(struct net *net, struct nexthop *new_nh,
 	while (1) {
 		struct nexthop *nh;
 
-		next = rtnl_dereference(*pp);
+		next = *pp;
 		if (!next)
 			break;
 
-- 
2.26.2

[PATCH net-next 4/7] ipv4: nexthop: Correctly update nexthop group when removing a nexthop

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Each nexthop group contains an indication if it has IPv4 nexthops
('has_v4'). Its purpose is to prevent IPv6 routes from using groups with
IPv4 nexthops.

However, the indication is not updated when a nexthop is removed. This
results in the kernel wrongly rejecting IPv6 routes from pointing to
groups that only contain IPv6 nexthops. Example:

# ip nexthop replace id 1 via 192.0.2.2 dev dummy10
# ip nexthop replace id 2 via 2001:db8:1::2 dev dummy10
# ip nexthop replace id 10 group 1/2
# ip nexthop del id 1
# ip route replace 2001:db8:10::/64 nhid 10
Error: IPv6 routes can not use an IPv4 nexthop.

Solve this by updating the indication according to the new set of
member nexthops.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/nexthop.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 1b736e3e1baa..5199a2815df6 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -797,7 +797,7 @@ static void remove_nh_grp_entry(struct net *net, struct nh_grp_entry *nhge,
 		return;
 	}
 
-	newg->has_v4 = nhg->has_v4;
+	newg->has_v4 = false;
 	newg->mpath = nhg->mpath;
 	newg->fdb_nh = nhg->fdb_nh;
 	newg->num_nh = nhg->num_nh;
@@ -806,12 +806,18 @@ static void remove_nh_grp_entry(struct net *net, struct nh_grp_entry *nhge,
 	nhges = nhg->nh_entries;
 	new_nhges = newg->nh_entries;
 	for (i = 0, j = 0; i < nhg->num_nh; ++i) {
+		struct nh_info *nhi;
+
 		/* current nexthop getting removed */
 		if (nhg->nh_entries[i].nh == nh) {
 			newg->num_nh--;
 			continue;
 		}
 
+		nhi = rtnl_dereference(nhges[i].nh->nh_info);
+		if (nhi->family == AF_INET)
+			newg->has_v4 = true;
+
 		list_del(&nhges[i].nh_list);
 		new_nhges[j].nh_parent = nhges[i].nh_parent;
 		new_nhges[j].nh = nhges[i].nh;
-- 
2.26.2

[PATCH net-next 5/7] selftests: fib_nexthops: Test IPv6 route with group after removing IPv4 nexthops

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Test that an IPv6 route can not use a nexthop group with mixed IPv4 and
IPv6 nexthops, but can use it after deleting the IPv4 nexthops.

Output without previous patch:

# ./fib_nexthops.sh -t ipv6_fcnal_runtime

IPv6 functional runtime
-----------------------
TEST: Route add                                                     [ OK ]
TEST: Route delete                                                  [ OK ]
TEST: Ping with nexthop                                             [ OK ]
TEST: Ping - multipath                                              [ OK ]
TEST: Ping - blackhole                                              [ OK ]
TEST: Ping - blackhole replaced with gateway                        [ OK ]
TEST: Ping - gateway replaced by blackhole                          [ OK ]
TEST: Ping - group with blackhole                                   [ OK ]
TEST: Ping - group blackhole replaced with gateways                 [ OK ]
TEST: IPv6 route with device only nexthop                           [ OK ]
TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
TEST: IPv6 route can not have a v4 gateway                          [ OK ]
TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after deleting v4 gateways           [FAIL]
TEST: Nexthop with default route and rpfilter                       [ OK ]
TEST: Nexthop with multipath default route and rpfilter             [ OK ]

Tests passed:  18
Tests failed:   1

Output with previous patch:

bash-5.0# ./fib_nexthops.sh -t ipv6_fcnal_runtime

IPv6 functional runtime
-----------------------
TEST: Route add                                                     [ OK ]
TEST: Route delete                                                  [ OK ]
TEST: Ping with nexthop                                             [ OK ]
TEST: Ping - multipath                                              [ OK ]
TEST: Ping - blackhole                                              [ OK ]
TEST: Ping - blackhole replaced with gateway                        [ OK ]
TEST: Ping - gateway replaced by blackhole                          [ OK ]
TEST: Ping - group with blackhole                                   [ OK ]
TEST: Ping - group blackhole replaced with gateways                 [ OK ]
TEST: IPv6 route with device only nexthop                           [ OK ]
TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
TEST: IPv6 route can not have a v4 gateway                          [ OK ]
TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after deleting v4 gateways           [ OK ]
TEST: Nexthop with default route and rpfilter                       [ OK ]
TEST: Nexthop with multipath default route and rpfilter             [ OK ]

Tests passed:  19
Tests failed:   0

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

diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
index 22dc2f3d428b..06e4f12e838d 100755
--- a/tools/testing/selftests/net/fib_nexthops.sh
+++ b/tools/testing/selftests/net/fib_nexthops.sh
@@ -739,6 +739,21 @@ ipv6_fcnal_runtime()
 	run_cmd "$IP nexthop replace id 81 via 172.16.1.1 dev veth1"
 	log_test $? 2 "Nexthop replace of group entry - v6 route, v4 nexthop"
 
+	run_cmd "$IP nexthop add id 86 via 2001:db8:92::2 dev veth3"
+	run_cmd "$IP nexthop add id 87 via 172.16.1.1 dev veth1"
+	run_cmd "$IP nexthop add id 88 via 172.16.1.1 dev veth1"
+	run_cmd "$IP nexthop add id 124 group 86/87/88"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
+
+	run_cmd "$IP nexthop del id 88"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
+
+	run_cmd "$IP nexthop del id 87"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 0 "IPv6 route using a group after removing v4 gateways"
+
 	$IP nexthop flush >/dev/null 2>&1
 
 	#
-- 
2.26.2

[PATCH net-next 6/7] ipv4: nexthop: Correctly update nexthop group when replacing a nexthop

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Each nexthop group contains an indication if it has IPv4 nexthops
('has_v4'). Its purpose is to prevent IPv6 routes from using groups with
IPv4 nexthops.

However, the indication is not updated when a nexthop is replaced. This
results in the kernel wrongly rejecting IPv6 routes from pointing to
groups that only contain IPv6 nexthops. Example:

# ip nexthop replace id 1 via 192.0.2.2 dev dummy10
# ip nexthop replace id 10 group 1
# ip nexthop replace id 1 via 2001:db8:1::2 dev dummy10
# ip route replace 2001:db8:10::/64 nhid 10
Error: IPv6 routes can not use an IPv4 nexthop.

Solve this by iterating over all the nexthop groups that the replaced
nexthop is a member of and potentially update their IPv4 indication
according to the new set of member nexthops.

Avoid wasting cycles by only performing the update in case an IPv4
nexthop is replaced by an IPv6 nexthop.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/nexthop.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 5199a2815df6..bf9d4cd2d6e5 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -964,6 +964,23 @@ static int replace_nexthop_grp(struct net *net, struct nexthop *old,
 	return 0;
 }
 
+static void nh_group_v4_update(struct nh_group *nhg)
+{
+	struct nh_grp_entry *nhges;
+	bool has_v4 = false;
+	int i;
+
+	nhges = nhg->nh_entries;
+	for (i = 0; i < nhg->num_nh; i++) {
+		struct nh_info *nhi;
+
+		nhi = rtnl_dereference(nhges[i].nh->nh_info);
+		if (nhi->family == AF_INET)
+			has_v4 = true;
+	}
+	nhg->has_v4 = has_v4;
+}
+
 static int replace_nexthop_single(struct net *net, struct nexthop *old,
 				  struct nexthop *new,
 				  struct netlink_ext_ack *extack)
@@ -987,6 +1004,21 @@ static int replace_nexthop_single(struct net *net, struct nexthop *old,
 	rcu_assign_pointer(old->nh_info, newi);
 	rcu_assign_pointer(new->nh_info, oldi);
 
+	/* When replacing an IPv4 nexthop with an IPv6 nexthop, potentially
+	 * update IPv4 indication in all the groups using the nexthop.
+	 */
+	if (oldi->family == AF_INET && newi->family == AF_INET6) {
+		struct nh_grp_entry *nhge;
+
+		list_for_each_entry(nhge, &old->grp_list, nh_list) {
+			struct nexthop *nhp = nhge->nh_parent;
+			struct nh_group *nhg;
+
+			nhg = rtnl_dereference(nhp->nh_grp);
+			nh_group_v4_update(nhg);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.26.2

[PATCH net-next 7/7] selftests: fib_nexthops: Test IPv6 route with group after replacing IPv4 nexthops

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Test that an IPv6 route can not use a nexthop group with mixed IPv4 and
IPv6 nexthops, but can use it after replacing the IPv4 nexthops with
IPv6 nexthops.

Output without previous patch:

# ./fib_nexthops.sh -t ipv6_fcnal_runtime

IPv6 functional runtime
-----------------------
TEST: Route add                                                     [ OK ]
TEST: Route delete                                                  [ OK ]
TEST: Ping with nexthop                                             [ OK ]
TEST: Ping - multipath                                              [ OK ]
TEST: Ping - blackhole                                              [ OK ]
TEST: Ping - blackhole replaced with gateway                        [ OK ]
TEST: Ping - gateway replaced by blackhole                          [ OK ]
TEST: Ping - group with blackhole                                   [ OK ]
TEST: Ping - group blackhole replaced with gateways                 [ OK ]
TEST: IPv6 route with device only nexthop                           [ OK ]
TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
TEST: IPv6 route can not have a v4 gateway                          [ OK ]
TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after removing v4 gateways           [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after replacing v4 gateways          [FAIL]
TEST: Nexthop with default route and rpfilter                       [ OK ]
TEST: Nexthop with multipath default route and rpfilter             [ OK ]

Tests passed:  21
Tests failed:   1

Output with previous patch:

# ./fib_nexthops.sh -t ipv6_fcnal_runtime

IPv6 functional runtime
-----------------------
TEST: Route add                                                     [ OK ]
TEST: Route delete                                                  [ OK ]
TEST: Ping with nexthop                                             [ OK ]
TEST: Ping - multipath                                              [ OK ]
TEST: Ping - blackhole                                              [ OK ]
TEST: Ping - blackhole replaced with gateway                        [ OK ]
TEST: Ping - gateway replaced by blackhole                          [ OK ]
TEST: Ping - group with blackhole                                   [ OK ]
TEST: Ping - group blackhole replaced with gateways                 [ OK ]
TEST: IPv6 route with device only nexthop                           [ OK ]
TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
TEST: IPv6 route can not have a v4 gateway                          [ OK ]
TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after removing v4 gateways           [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
TEST: IPv6 route using a group after replacing v4 gateways          [ OK ]
TEST: Nexthop with default route and rpfilter                       [ OK ]
TEST: Nexthop with multipath default route and rpfilter             [ OK ]

Tests passed:  22
Tests failed:   0

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

diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
index 06e4f12e838d..b74884d52913 100755
--- a/tools/testing/selftests/net/fib_nexthops.sh
+++ b/tools/testing/selftests/net/fib_nexthops.sh
@@ -754,6 +754,21 @@ ipv6_fcnal_runtime()
 	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
 	log_test $? 0 "IPv6 route using a group after removing v4 gateways"
 
+	run_cmd "$IP ro delete 2001:db8:101::1/128"
+	run_cmd "$IP nexthop add id 87 via 172.16.1.1 dev veth1"
+	run_cmd "$IP nexthop add id 88 via 172.16.1.1 dev veth1"
+	run_cmd "$IP nexthop replace id 124 group 86/87/88"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
+
+	run_cmd "$IP nexthop replace id 88 via 2001:db8:92::2 dev veth3"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
+
+	run_cmd "$IP nexthop replace id 87 via 2001:db8:92::2 dev veth3"
+	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
+	log_test $? 0 "IPv6 route using a group after replacing v4 gateways"
+
 	$IP nexthop flush >/dev/null 2>&1
 
 	#
-- 
2.26.2

Re: [PATCH net-next 1/7] ipv4: nexthop: Reduce allocation size of 'struct nh_group'

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> The struct looks as follows:
> 
> struct nh_group {
> 	struct nh_group		*spare; /* spare group for removals */
> 	u16			num_nh;
> 	bool			mpath;
> 	bool			fdb_nh;
> 	bool			has_v4;
> 	struct nh_grp_entry	nh_entries[];
> };
> 
> But its offset within 'struct nexthop' is also taken into account to
> determine the allocation size.

must be a leftover from initial versions. Thanks for catching this.

> 
> Instead, use struct_size() to allocate only the required number of
> bytes.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 134e92382275..d13730ff9aeb 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -133,12 +133,9 @@ static struct nexthop *nexthop_alloc(void)
>  
>  static struct nh_group *nexthop_grp_alloc(u16 num_nh)
>  {
> -	size_t sz = offsetof(struct nexthop, nh_grp)
> -		    + sizeof(struct nh_group)
> -		    + sizeof(struct nh_grp_entry) * num_nh;
>  	struct nh_group *nhg;
>  
> -	nhg = kzalloc(sz, GFP_KERNEL);
> +	nhg = kzalloc(struct_size(nhg, nh_entries, num_nh), GFP_KERNEL);
>  	if (nhg)
>  		nhg->num_nh = num_nh;
>  
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 2/7] ipv4: nexthop: Use nla_put_be32() for NHA_GATEWAY

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> The code correctly uses nla_get_be32() to get the payload of the
> attribute, but incorrectly uses nla_put_u32() to add the attribute to
> the payload. This results in the following warning:
> 
> net/ipv4/nexthop.c:279:59: warning: incorrect type in argument 3 (different base types)
> net/ipv4/nexthop.c:279:59:    expected unsigned int [usertype] value
> net/ipv4/nexthop.c:279:59:    got restricted __be32 [usertype] ipv4
> 
> Suppress the warning by using nla_put_be32().
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index d13730ff9aeb..0823643a7dec 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -276,7 +276,7 @@ static int nh_fill_node(struct sk_buff *skb, struct nexthop *nh,
>  	case AF_INET:
>  		fib_nh = &nhi->fib_nh;
>  		if (fib_nh->fib_nh_gw_family &&
> -		    nla_put_u32(skb, NHA_GATEWAY, fib_nh->fib_nh_gw4))
> +		    nla_put_be32(skb, NHA_GATEWAY, fib_nh->fib_nh_gw4))
>  			goto nla_put_failure;
>  		break;
>  
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 3/7] ipv4: nexthop: Remove unnecessary rtnl_dereference()

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> The pointer is not RCU protected, so remove the unnecessary
> rtnl_dereference(). This suppresses the following warning:
> 
> net/ipv4/nexthop.c:1101:24: error: incompatible types in comparison expression (different address spaces):
> net/ipv4/nexthop.c:1101:24:    struct rb_node [noderef] __rcu *
> net/ipv4/nexthop.c:1101:24:    struct rb_node *
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 0823643a7dec..1b736e3e1baa 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -1098,7 +1098,7 @@ static int insert_nexthop(struct net *net, struct nexthop *new_nh,
>  	while (1) {
>  		struct nexthop *nh;
>  
> -		next = rtnl_dereference(*pp);
> +		next = *pp;
>  		if (!next)
>  			break;
>  
> 

A left over from initial attempts to convert connected routes to nexthops.

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 4/7] ipv4: nexthop: Correctly update nexthop group when removing a nexthop

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Each nexthop group contains an indication if it has IPv4 nexthops
> ('has_v4'). Its purpose is to prevent IPv6 routes from using groups with
> IPv4 nexthops.
> 
> However, the indication is not updated when a nexthop is removed. This
> results in the kernel wrongly rejecting IPv6 routes from pointing to
> groups that only contain IPv6 nexthops. Example:
> 
> # ip nexthop replace id 1 via 192.0.2.2 dev dummy10
> # ip nexthop replace id 2 via 2001:db8:1::2 dev dummy10
> # ip nexthop replace id 10 group 1/2
> # ip nexthop del id 1
> # ip route replace 2001:db8:10::/64 nhid 10
> Error: IPv6 routes can not use an IPv4 nexthop.
> 
> Solve this by updating the indication according to the new set of
> member nexthops.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 1b736e3e1baa..5199a2815df6 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -797,7 +797,7 @@ static void remove_nh_grp_entry(struct net *net, struct nh_grp_entry *nhge,
>  		return;
>  	}
>  
> -	newg->has_v4 = nhg->has_v4;
> +	newg->has_v4 = false;
>  	newg->mpath = nhg->mpath;
>  	newg->fdb_nh = nhg->fdb_nh;
>  	newg->num_nh = nhg->num_nh;
> @@ -806,12 +806,18 @@ static void remove_nh_grp_entry(struct net *net, struct nh_grp_entry *nhge,
>  	nhges = nhg->nh_entries;
>  	new_nhges = newg->nh_entries;
>  	for (i = 0, j = 0; i < nhg->num_nh; ++i) {
> +		struct nh_info *nhi;
> +
>  		/* current nexthop getting removed */
>  		if (nhg->nh_entries[i].nh == nh) {
>  			newg->num_nh--;
>  			continue;
>  		}
>  
> +		nhi = rtnl_dereference(nhges[i].nh->nh_info);
> +		if (nhi->family == AF_INET)
> +			newg->has_v4 = true;
> +
>  		list_del(&nhges[i].nh_list);
>  		new_nhges[j].nh_parent = nhges[i].nh_parent;
>  		new_nhges[j].nh = nhges[i].nh;
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 5/7] selftests: fib_nexthops: Test IPv6 route with group after removing IPv4 nexthops

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Test that an IPv6 route can not use a nexthop group with mixed IPv4 and
> IPv6 nexthops, but can use it after deleting the IPv4 nexthops.
> 
> Output without previous patch:
> 
> # ./fib_nexthops.sh -t ipv6_fcnal_runtime
> 
> IPv6 functional runtime
> -----------------------
> TEST: Route add                                                     [ OK ]
> TEST: Route delete                                                  [ OK ]
> TEST: Ping with nexthop                                             [ OK ]
> TEST: Ping - multipath                                              [ OK ]
> TEST: Ping - blackhole                                              [ OK ]
> TEST: Ping - blackhole replaced with gateway                        [ OK ]
> TEST: Ping - gateway replaced by blackhole                          [ OK ]
> TEST: Ping - group with blackhole                                   [ OK ]
> TEST: Ping - group blackhole replaced with gateways                 [ OK ]
> TEST: IPv6 route with device only nexthop                           [ OK ]
> TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
> TEST: IPv6 route can not have a v4 gateway                          [ OK ]
> TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
> TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after deleting v4 gateways           [FAIL]
> TEST: Nexthop with default route and rpfilter                       [ OK ]
> TEST: Nexthop with multipath default route and rpfilter             [ OK ]
> 
> Tests passed:  18
> Tests failed:   1
> 
> Output with previous patch:
> 
> bash-5.0# ./fib_nexthops.sh -t ipv6_fcnal_runtime
> 
> IPv6 functional runtime
> -----------------------
> TEST: Route add                                                     [ OK ]
> TEST: Route delete                                                  [ OK ]
> TEST: Ping with nexthop                                             [ OK ]
> TEST: Ping - multipath                                              [ OK ]
> TEST: Ping - blackhole                                              [ OK ]
> TEST: Ping - blackhole replaced with gateway                        [ OK ]
> TEST: Ping - gateway replaced by blackhole                          [ OK ]
> TEST: Ping - group with blackhole                                   [ OK ]
> TEST: Ping - group blackhole replaced with gateways                 [ OK ]
> TEST: IPv6 route with device only nexthop                           [ OK ]
> TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
> TEST: IPv6 route can not have a v4 gateway                          [ OK ]
> TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
> TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after deleting v4 gateways           [ OK ]
> TEST: Nexthop with default route and rpfilter                       [ OK ]
> TEST: Nexthop with multipath default route and rpfilter             [ OK ]
> 
> Tests passed:  19
> Tests failed:   0
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  tools/testing/selftests/net/fib_nexthops.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index 22dc2f3d428b..06e4f12e838d 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -739,6 +739,21 @@ ipv6_fcnal_runtime()
>  	run_cmd "$IP nexthop replace id 81 via 172.16.1.1 dev veth1"
>  	log_test $? 2 "Nexthop replace of group entry - v6 route, v4 nexthop"
>  
> +	run_cmd "$IP nexthop add id 86 via 2001:db8:92::2 dev veth3"
> +	run_cmd "$IP nexthop add id 87 via 172.16.1.1 dev veth1"
> +	run_cmd "$IP nexthop add id 88 via 172.16.1.1 dev veth1"
> +	run_cmd "$IP nexthop add id 124 group 86/87/88"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
> +
> +	run_cmd "$IP nexthop del id 88"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
> +
> +	run_cmd "$IP nexthop del id 87"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 0 "IPv6 route using a group after removing v4 gateways"
> +
>  	$IP nexthop flush >/dev/null 2>&1
>  
>  	#
> 

Thanks for adding the tests!

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 6/7] ipv4: nexthop: Correctly update nexthop group when replacing a nexthop

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Each nexthop group contains an indication if it has IPv4 nexthops
> ('has_v4'). Its purpose is to prevent IPv6 routes from using groups with
> IPv4 nexthops.
> 
> However, the indication is not updated when a nexthop is replaced. This
> results in the kernel wrongly rejecting IPv6 routes from pointing to
> groups that only contain IPv6 nexthops. Example:
> 
> # ip nexthop replace id 1 via 192.0.2.2 dev dummy10
> # ip nexthop replace id 10 group 1
> # ip nexthop replace id 1 via 2001:db8:1::2 dev dummy10
> # ip route replace 2001:db8:10::/64 nhid 10
> Error: IPv6 routes can not use an IPv4 nexthop.
> 
> Solve this by iterating over all the nexthop groups that the replaced
> nexthop is a member of and potentially update their IPv4 indication
> according to the new set of member nexthops.
> 
> Avoid wasting cycles by only performing the update in case an IPv4
> nexthop is replaced by an IPv6 nexthop.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> index 5199a2815df6..bf9d4cd2d6e5 100644
> --- a/net/ipv4/nexthop.c
> +++ b/net/ipv4/nexthop.c
> @@ -964,6 +964,23 @@ static int replace_nexthop_grp(struct net *net, struct nexthop *old,
>  	return 0;
>  }
>  
> +static void nh_group_v4_update(struct nh_group *nhg)
> +{
> +	struct nh_grp_entry *nhges;
> +	bool has_v4 = false;
> +	int i;
> +
> +	nhges = nhg->nh_entries;
> +	for (i = 0; i < nhg->num_nh; i++) {
> +		struct nh_info *nhi;
> +
> +		nhi = rtnl_dereference(nhges[i].nh->nh_info);
> +		if (nhi->family == AF_INET)
> +			has_v4 = true;
> +	}
> +	nhg->has_v4 = has_v4;
> +}
> +
>  static int replace_nexthop_single(struct net *net, struct nexthop *old,
>  				  struct nexthop *new,
>  				  struct netlink_ext_ack *extack)
> @@ -987,6 +1004,21 @@ static int replace_nexthop_single(struct net *net, struct nexthop *old,
>  	rcu_assign_pointer(old->nh_info, newi);
>  	rcu_assign_pointer(new->nh_info, oldi);
>  
> +	/* When replacing an IPv4 nexthop with an IPv6 nexthop, potentially
> +	 * update IPv4 indication in all the groups using the nexthop.
> +	 */
> +	if (oldi->family == AF_INET && newi->family == AF_INET6) {
> +		struct nh_grp_entry *nhge;
> +
> +		list_for_each_entry(nhge, &old->grp_list, nh_list) {
> +			struct nexthop *nhp = nhge->nh_parent;
> +			struct nh_group *nhg;
> +
> +			nhg = rtnl_dereference(nhp->nh_grp);
> +			nh_group_v4_update(nhg);
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> 

Hopefully userspace apps create a new nexthop versus overwriting
existing ones with different address family. Thanks for handling this
case and adding a test case.

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 7/7] selftests: fib_nexthops: Test IPv6 route with group after replacing IPv4 nexthops

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Test that an IPv6 route can not use a nexthop group with mixed IPv4 and
> IPv6 nexthops, but can use it after replacing the IPv4 nexthops with
> IPv6 nexthops.
> 
> Output without previous patch:
> 
> # ./fib_nexthops.sh -t ipv6_fcnal_runtime
> 
> IPv6 functional runtime
> -----------------------
> TEST: Route add                                                     [ OK ]
> TEST: Route delete                                                  [ OK ]
> TEST: Ping with nexthop                                             [ OK ]
> TEST: Ping - multipath                                              [ OK ]
> TEST: Ping - blackhole                                              [ OK ]
> TEST: Ping - blackhole replaced with gateway                        [ OK ]
> TEST: Ping - gateway replaced by blackhole                          [ OK ]
> TEST: Ping - group with blackhole                                   [ OK ]
> TEST: Ping - group blackhole replaced with gateways                 [ OK ]
> TEST: IPv6 route with device only nexthop                           [ OK ]
> TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
> TEST: IPv6 route can not have a v4 gateway                          [ OK ]
> TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
> TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after removing v4 gateways           [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after replacing v4 gateways          [FAIL]
> TEST: Nexthop with default route and rpfilter                       [ OK ]
> TEST: Nexthop with multipath default route and rpfilter             [ OK ]
> 
> Tests passed:  21
> Tests failed:   1
> 
> Output with previous patch:
> 
> # ./fib_nexthops.sh -t ipv6_fcnal_runtime
> 
> IPv6 functional runtime
> -----------------------
> TEST: Route add                                                     [ OK ]
> TEST: Route delete                                                  [ OK ]
> TEST: Ping with nexthop                                             [ OK ]
> TEST: Ping - multipath                                              [ OK ]
> TEST: Ping - blackhole                                              [ OK ]
> TEST: Ping - blackhole replaced with gateway                        [ OK ]
> TEST: Ping - gateway replaced by blackhole                          [ OK ]
> TEST: Ping - group with blackhole                                   [ OK ]
> TEST: Ping - group blackhole replaced with gateways                 [ OK ]
> TEST: IPv6 route with device only nexthop                           [ OK ]
> TEST: IPv6 multipath route with nexthop mix - dev only + gw         [ OK ]
> TEST: IPv6 route can not have a v4 gateway                          [ OK ]
> TEST: Nexthop replace - v6 route, v4 nexthop                        [ OK ]
> TEST: Nexthop replace of group entry - v6 route, v4 nexthop         [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after removing v4 gateways           [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route can not have a group with v4 and v6 gateways       [ OK ]
> TEST: IPv6 route using a group after replacing v4 gateways          [ OK ]
> TEST: Nexthop with default route and rpfilter                       [ OK ]
> TEST: Nexthop with multipath default route and rpfilter             [ OK ]
> 
> Tests passed:  22
> Tests failed:   0
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  tools/testing/selftests/net/fib_nexthops.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index 06e4f12e838d..b74884d52913 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -754,6 +754,21 @@ ipv6_fcnal_runtime()
>  	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
>  	log_test $? 0 "IPv6 route using a group after removing v4 gateways"
>  
> +	run_cmd "$IP ro delete 2001:db8:101::1/128"
> +	run_cmd "$IP nexthop add id 87 via 172.16.1.1 dev veth1"
> +	run_cmd "$IP nexthop add id 88 via 172.16.1.1 dev veth1"
> +	run_cmd "$IP nexthop replace id 124 group 86/87/88"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
> +
> +	run_cmd "$IP nexthop replace id 88 via 2001:db8:92::2 dev veth3"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 2 "IPv6 route can not have a group with v4 and v6 gateways"
> +
> +	run_cmd "$IP nexthop replace id 87 via 2001:db8:92::2 dev veth3"
> +	run_cmd "$IP ro replace 2001:db8:101::1/128 nhid 124"
> +	log_test $? 0 "IPv6 route using a group after replacing v4 gateways"
> +
>  	$IP nexthop flush >/dev/null 2>&1
>  
>  	#
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 0/7] ipv4: nexthop: Various improvements

[David Ahern]

On 8/26/20 10:48 AM, Ido Schimmel wrote:
> While patches #4 and #6 fix bugs, they are not regressions (never
> worked). They also do not occur to me as critical issues, which is why I
> am targeting them at net-next.

I agree. Thanks for the work on this.

Re: [PATCH net-next 0/7] ipv4: nexthop: Various improvements

[David Miller]

From: Ido Schimmel <idosch@idosch.org>
Date: Wed, 26 Aug 2020 19:48:50 +0300

> This patch set contains various improvements that I made to the nexthop
> object code while studying it towards my upcoming changes.
> 
> While patches #4 and #6 fix bugs, they are not regressions (never
> worked). They also do not occur to me as critical issues, which is why I
> am targeting them at net-next.
> 
> Tested with fib_nexthops.sh:
> 
> Tests passed: 134
> Tests failed:   0

Series applied, thanks Ido.