[PATCH net 0/4] nexthop: Various fixes

[PATCH net 0/4] nexthop: Various fixes

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This series contains various fixes for the nexthop code. The bugs were
uncovered during the development of resilient nexthop groups.

Patches #1-#2 fix the error path of nexthop_create_group(). I was not
able to trigger these bugs with current code, but it is possible with
the upcoming resilient nexthop groups code which adds a user
controllable memory allocation further in the function.

Patch #3 fixes wrong validation of netlink attributes.

Patch #4 fixes wrong invocation of mausezahn in a selftest.

Ido Schimmel (3):
  nexthop: Fix off-by-one error in error path
  nexthop: Unlink nexthop group entry in error path
  selftests: fib_nexthops: Fix wrong mausezahn invocation

Petr Machata (1):
  nexthop: Bounce NHA_GATEWAY in FDB nexthop groups

 net/ipv4/nexthop.c                          | 6 ++++--
 tools/testing/selftests/net/fib_nexthops.sh | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.29.2

[PATCH net 1/4] nexthop: Fix off-by-one error in error path

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

A reference was not taken for the current nexthop entry, so do not try
to put it in the error path.

Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@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 5e1b22d4f939..f8035cfa9c20 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1459,7 +1459,7 @@ static struct nexthop *nexthop_create_group(struct net *net,
 	return nh;
 
 out_no_nh:
-	for (; i >= 0; --i)
+	for (i--; i >= 0; --i)
 		nexthop_put(nhg->nh_entries[i].nh);
 
 	kfree(nhg->spare);
-- 
2.29.2

[PATCH net 2/4] nexthop: Unlink nexthop group entry in error path

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

In case of error, remove the nexthop group entry from the list to which
it was previously added.

Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 net/ipv4/nexthop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index f8035cfa9c20..712cdc061cde 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -1459,8 +1459,10 @@ static struct nexthop *nexthop_create_group(struct net *net,
 	return nh;
 
 out_no_nh:
-	for (i--; i >= 0; --i)
+	for (i--; i >= 0; --i) {
+		list_del(&nhg->nh_entries[i].nh_list);
 		nexthop_put(nhg->nh_entries[i].nh);
+	}
 
 	kfree(nhg->spare);
 	kfree(nhg);
-- 
2.29.2

[PATCH net 3/4] nexthop: Bounce NHA_GATEWAY in FDB nexthop groups

[Ido Schimmel]

From: Petr Machata <petrm@nvidia.com>

The function nh_check_attr_group() is called to validate nexthop groups.
The intention of that code seems to have been to bounce all attributes
above NHA_GROUP_TYPE except for NHA_FDB. However instead it bounces all
these attributes except when NHA_FDB attribute is present--then it accepts
them.

NHA_FDB validation that takes place before, in rtm_to_nh_config(), already
bounces NHA_OIF, NHA_BLACKHOLE, NHA_ENCAP and NHA_ENCAP_TYPE. Yet further
back, NHA_GROUPS and NHA_MASTER are bounced unconditionally.

But that still leaves NHA_GATEWAY as an attribute that would be accepted in
FDB nexthop groups (with no meaning), so long as it keeps the address
family as unspecified:

 # ip nexthop add id 1 fdb via 127.0.0.1
 # ip nexthop add id 10 fdb via default group 1

The nexthop code is still relatively new and likely not used very broadly,
and the FDB bits are newer still. Even though there is a reproducer out
there, it relies on an improbable gateway arguments "via default", "via
all" or "via any". Given all this, I believe it is OK to reformulate the
condition to do the right thing and bounce NHA_GATEWAY.

Fixes: 38428d68719c ("nexthop: support for fdb ecmp nexthops")
Signed-off-by: Petr Machata <petrm@nvidia.com>
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 712cdc061cde..e53e43aef785 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -627,7 +627,7 @@ static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
 	for (i = NHA_GROUP_TYPE + 1; i < __NHA_MAX; ++i) {
 		if (!tb[i])
 			continue;
-		if (tb[NHA_FDB])
+		if (i == NHA_FDB)
 			continue;
 		NL_SET_ERR_MSG(extack,
 			       "No other attributes can be set in nexthop groups");
-- 
2.29.2

[PATCH net 4/4] selftests: fib_nexthops: Fix wrong mausezahn invocation

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

For IPv6 traffic, mausezahn needs to be invoked with '-6'. Otherwise an
error is returned:

 # ip netns exec me mausezahn veth1 -B 2001:db8:101::2 -A 2001:db8:91::1 -c 0 -t tcp "dp=1-1023, flags=syn"
 Failed to set source IPv4 address. Please check if source is set to a valid IPv4 address.
  Invalid command line parameters!

Fixes: 7c741868ceab ("selftests: Add torture tests to nexthop tests")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 tools/testing/selftests/net/fib_nexthops.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
index eb693a3b7b4a..4c7d33618437 100755
--- a/tools/testing/selftests/net/fib_nexthops.sh
+++ b/tools/testing/selftests/net/fib_nexthops.sh
@@ -869,7 +869,7 @@ ipv6_torture()
 	pid3=$!
 	ip netns exec me ping -f 2001:db8:101::2 >/dev/null 2>&1 &
 	pid4=$!
-	ip netns exec me mausezahn veth1 -B 2001:db8:101::2 -A 2001:db8:91::1 -c 0 -t tcp "dp=1-1023, flags=syn" >/dev/null 2>&1 &
+	ip netns exec me mausezahn -6 veth1 -B 2001:db8:101::2 -A 2001:db8:91::1 -c 0 -t tcp "dp=1-1023, flags=syn" >/dev/null 2>&1 &
 	pid5=$!
 
 	sleep 300
-- 
2.29.2

Re: [PATCH net 1/4] nexthop: Fix off-by-one error in error path

[David Ahern]

On 1/7/21 7:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> A reference was not taken for the current nexthop entry, so do not try
> to put it in the error path.
> 
> Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 2/4] nexthop: Unlink nexthop group entry in error path

[David Ahern]

On 1/7/21 7:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> In case of error, remove the nexthop group entry from the list to which
> it was previously added.
> 
> Fixes: 430a049190de ("nexthop: Add support for nexthop groups")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 3/4] nexthop: Bounce NHA_GATEWAY in FDB nexthop groups

[David Ahern]

On 1/7/21 7:48 AM, Ido Schimmel wrote:
> From: Petr Machata <petrm@nvidia.com>
> 
> The function nh_check_attr_group() is called to validate nexthop groups.
> The intention of that code seems to have been to bounce all attributes
> above NHA_GROUP_TYPE except for NHA_FDB. However instead it bounces all
> these attributes except when NHA_FDB attribute is present--then it accepts
> them.
> 
> NHA_FDB validation that takes place before, in rtm_to_nh_config(), already
> bounces NHA_OIF, NHA_BLACKHOLE, NHA_ENCAP and NHA_ENCAP_TYPE. Yet further
> back, NHA_GROUPS and NHA_MASTER are bounced unconditionally.
> 
> But that still leaves NHA_GATEWAY as an attribute that would be accepted in
> FDB nexthop groups (with no meaning), so long as it keeps the address
> family as unspecified:
> 
>  # ip nexthop add id 1 fdb via 127.0.0.1
>  # ip nexthop add id 10 fdb via default group 1
> 
> The nexthop code is still relatively new and likely not used very broadly,
> and the FDB bits are newer still. Even though there is a reproducer out
> there, it relies on an improbable gateway arguments "via default", "via
> all" or "via any". Given all this, I believe it is OK to reformulate the
> condition to do the right thing and bounce NHA_GATEWAY.
> 
> Fixes: 38428d68719c ("nexthop: support for fdb ecmp nexthops")
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/nexthop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 4/4] selftests: fib_nexthops: Fix wrong mausezahn invocation

[David Ahern]

On 1/7/21 7:48 AM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> For IPv6 traffic, mausezahn needs to be invoked with '-6'. Otherwise an
> error is returned:
> 
>  # ip netns exec me mausezahn veth1 -B 2001:db8:101::2 -A 2001:db8:91::1 -c 0 -t tcp "dp=1-1023, flags=syn"
>  Failed to set source IPv4 address. Please check if source is set to a valid IPv4 address.
>   Invalid command line parameters!
> 
> Fixes: 7c741868ceab ("selftests: Add torture tests to nexthop tests")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> ---
>  tools/testing/selftests/net/fib_nexthops.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 0/4] nexthop: Various fixes

[Jakub Kicinski]

On Thu,  7 Jan 2021 16:48:20 +0200 Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This series contains various fixes for the nexthop code. The bugs were
> uncovered during the development of resilient nexthop groups.
> 
> Patches #1-#2 fix the error path of nexthop_create_group(). I was not
> able to trigger these bugs with current code, but it is possible with
> the upcoming resilient nexthop groups code which adds a user
> controllable memory allocation further in the function.
> 
> Patch #3 fixes wrong validation of netlink attributes.
> 
> Patch #4 fixes wrong invocation of mausezahn in a selftest.

Applied, thanks!