[PATCH net 0/2] ipv4: Two bug fixes

[PATCH net 0/2] ipv4: Two bug fixes

[Ido Schimmel]

Two small fixes for bugs in IPv4 routing code.

A variation of the second bug was reported by an FRR 5.0 (released
06/18) user as this version was setting a table ID of 0 for the default
VRF, unlike iproute2 and newer FRR versions.

The first bug was discovered while fixing the second.

Both bugs are not regressions (never worked) and are not critical in my
opinion, so the fixes can be applied to net-next, if desired.

No regressions in other tests:

 # ./fib_tests.sh
 ...
 Tests passed: 191
 Tests failed:   0

Ido Schimmel (2):
  ipv4: Fix incorrect route flushing when source address is deleted
  ipv4: Fix incorrect route flushing when table ID 0 is used

 net/ipv4/fib_frontend.c                  |  3 ++
 net/ipv4/fib_semantics.c                 |  1 +
 tools/testing/selftests/net/fib_tests.sh | 37 ++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

-- 
2.37.3

[PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted

[Ido Schimmel]

Cited commit added the table ID to the FIB info structure, but did not
prevent structures with different table IDs from being consolidated.
This can lead to routes being flushed from a VRF when an address is
deleted from a different VRF.

Fix by taking the table ID into account when looking for a matching FIB
info. This is already done for FIB info structures backed by a nexthop
object in fib_find_info_nh().

Add test cases that fail before the fix:

 # ./fib_tests.sh -t ipv4_del_addr

 IPv4 delete address route tests
     Regular FIB info
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Identical FIB info with different table ID
     TEST: Route removed from VRF when source address deleted            [FAIL]
     TEST: Route in default VRF not removed                              [ OK ]
 RTNETLINK answers: File exists
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [FAIL]

 Tests passed:   6
 Tests failed:   2

And pass after:

 # ./fib_tests.sh -t ipv4_del_addr

 IPv4 delete address route tests
     Regular FIB info
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Identical FIB info with different table ID
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]

 Tests passed:   8
 Tests failed:   0

Fixes: 5a56a0b3a45d ("net: Don't delete routes in different VRFs")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/fib_semantics.c                 |  1 +
 tools/testing/selftests/net/fib_tests.sh | 27 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 19a662003eef..ce9ff3c62e84 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -423,6 +423,7 @@ static struct fib_info *fib_find_info(struct fib_info *nfi)
 		    nfi->fib_prefsrc == fi->fib_prefsrc &&
 		    nfi->fib_priority == fi->fib_priority &&
 		    nfi->fib_type == fi->fib_type &&
+		    nfi->fib_tb_id == fi->fib_tb_id &&
 		    memcmp(nfi->fib_metrics, fi->fib_metrics,
 			   sizeof(u32) * RTAX_MAX) == 0 &&
 		    !((nfi->fib_flags ^ fi->fib_flags) & ~RTNH_COMPARE_MASK) &&
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 2271a8727f62..11c89148b19f 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -1711,13 +1711,19 @@ ipv4_del_addr_test()
 
 	$IP addr add dev dummy1 172.16.104.1/24
 	$IP addr add dev dummy1 172.16.104.11/24
+	$IP addr add dev dummy1 172.16.104.12/24
 	$IP addr add dev dummy2 172.16.104.1/24
 	$IP addr add dev dummy2 172.16.104.11/24
+	$IP addr add dev dummy2 172.16.104.12/24
 	$IP route add 172.16.105.0/24 via 172.16.104.2 src 172.16.104.11
+	$IP route add 172.16.106.0/24 dev lo src 172.16.104.12
 	$IP route add vrf red 172.16.105.0/24 via 172.16.104.2 src 172.16.104.11
+	$IP route add vrf red 172.16.106.0/24 dev lo src 172.16.104.12
 	set +e
 
 	# removing address from device in vrf should only remove route from vrf table
+	echo "    Regular FIB info"
+
 	$IP addr del dev dummy2 172.16.104.11/24
 	$IP ro ls vrf red | grep -q 172.16.105.0/24
 	log_test $? 1 "Route removed from VRF when source address deleted"
@@ -1735,6 +1741,27 @@ ipv4_del_addr_test()
 	$IP ro ls vrf red | grep -q 172.16.105.0/24
 	log_test $? 0 "Route in VRF is not removed by address delete"
 
+	# removing address from device in vrf should only remove route from vrf
+	# table even when the associated fib info only differs in table ID
+	echo "    Identical FIB info with different table ID"
+
+	$IP addr del dev dummy2 172.16.104.12/24
+	$IP ro ls vrf red | grep -q 172.16.106.0/24
+	log_test $? 1 "Route removed from VRF when source address deleted"
+
+	$IP ro ls | grep -q 172.16.106.0/24
+	log_test $? 0 "Route in default VRF not removed"
+
+	$IP addr add dev dummy2 172.16.104.12/24
+	$IP route add vrf red 172.16.106.0/24 dev lo src 172.16.104.12
+
+	$IP addr del dev dummy1 172.16.104.12/24
+	$IP ro ls | grep -q 172.16.106.0/24
+	log_test $? 1 "Route removed in default VRF when source address deleted"
+
+	$IP ro ls vrf red | grep -q 172.16.106.0/24
+	log_test $? 0 "Route in VRF is not removed by address delete"
+
 	$IP li del dummy1
 	$IP li del dummy2
 	cleanup
-- 
2.37.3

[PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used

[Ido Schimmel]

Cited commit added the table ID to the FIB info structure, but did not
properly initialize it when table ID 0 is used. This can lead to a route
in the default VRF with a preferred source address not being flushed
when the address is deleted.

Consider the following example:

 # ip address add dev dummy1 192.0.2.1/28
 # ip address add dev dummy1 192.0.2.17/28
 # ip route add 198.51.100.0/24 via 192.0.2.2 src 192.0.2.17 metric 100
 # ip route add table 0 198.51.100.0/24 via 192.0.2.2 src 192.0.2.17 metric 200
 # ip route show 198.51.100.0/24
 198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 100
 198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 200

Both routes are installed in the default VRF, but they are using two
different FIB info structures. One with a metric of 100 and table ID of
254 (main) and one with a metric of 200 and table ID of 0. Therefore,
when the preferred source address is deleted from the default VRF,
the second route is not flushed:

 # ip address del dev dummy1 192.0.2.17/28
 # ip route show 198.51.100.0/24
 198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 200

Fix by storing a table ID of 254 instead of 0 in the route configuration
structure.

Add a test case that fails before the fix:

 # ./fib_tests.sh -t ipv4_del_addr

 IPv4 delete address route tests
     Regular FIB info
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Identical FIB info with different table ID
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Table ID 0
     TEST: Route removed in default VRF when source address deleted      [FAIL]

 Tests passed:   8
 Tests failed:   1

And passes after:

 # ./fib_tests.sh -t ipv4_del_addr

 IPv4 delete address route tests
     Regular FIB info
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Identical FIB info with different table ID
     TEST: Route removed from VRF when source address deleted            [ OK ]
     TEST: Route in default VRF not removed                              [ OK ]
     TEST: Route removed in default VRF when source address deleted      [ OK ]
     TEST: Route in VRF is not removed by address delete                 [ OK ]
     Table ID 0
     TEST: Route removed in default VRF when source address deleted      [ OK ]

 Tests passed:   9
 Tests failed:   0

Fixes: 5a56a0b3a45d ("net: Don't delete routes in different VRFs")
Reported-by: Donald Sharp <sharpd@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ipv4/fib_frontend.c                  |  3 +++
 tools/testing/selftests/net/fib_tests.sh | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index f361d3d56be2..b5736ef16ed2 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -841,6 +841,9 @@ static int rtm_to_fib_config(struct net *net, struct sk_buff *skb,
 		return -EINVAL;
 	}
 
+	if (!cfg->fc_table)
+		cfg->fc_table = RT_TABLE_MAIN;
+
 	return 0;
 errout:
 	return err;
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 11c89148b19f..5637b5dadabd 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -1712,11 +1712,13 @@ ipv4_del_addr_test()
 	$IP addr add dev dummy1 172.16.104.1/24
 	$IP addr add dev dummy1 172.16.104.11/24
 	$IP addr add dev dummy1 172.16.104.12/24
+	$IP addr add dev dummy1 172.16.104.13/24
 	$IP addr add dev dummy2 172.16.104.1/24
 	$IP addr add dev dummy2 172.16.104.11/24
 	$IP addr add dev dummy2 172.16.104.12/24
 	$IP route add 172.16.105.0/24 via 172.16.104.2 src 172.16.104.11
 	$IP route add 172.16.106.0/24 dev lo src 172.16.104.12
+	$IP route add table 0 172.16.107.0/24 via 172.16.104.2 src 172.16.104.13
 	$IP route add vrf red 172.16.105.0/24 via 172.16.104.2 src 172.16.104.11
 	$IP route add vrf red 172.16.106.0/24 dev lo src 172.16.104.12
 	set +e
@@ -1762,6 +1764,14 @@ ipv4_del_addr_test()
 	$IP ro ls vrf red | grep -q 172.16.106.0/24
 	log_test $? 0 "Route in VRF is not removed by address delete"
 
+	# removing address from device in default vrf should remove route from
+	# the default vrf even when route was inserted with a table ID of 0.
+	echo "    Table ID 0"
+
+	$IP addr del dev dummy1 172.16.104.13/24
+	$IP ro ls | grep -q 172.16.107.0/24
+	log_test $? 1 "Route removed in default VRF when source address deleted"
+
 	$IP li del dummy1
 	$IP li del dummy2
 	cleanup
-- 
2.37.3

Re: [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted

[David Ahern]

On 12/4/22 12:50 AM, Ido Schimmel wrote:
> Cited commit added the table ID to the FIB info structure, but did not
> prevent structures with different table IDs from being consolidated.
> This can lead to routes being flushed from a VRF when an address is
> deleted from a different VRF.
> 
> Fix by taking the table ID into account when looking for a matching FIB
> info. This is already done for FIB info structures backed by a nexthop
> object in fib_find_info_nh().
> 
> Add test cases that fail before the fix:
> 
>  # ./fib_tests.sh -t ipv4_del_addr
> 
>  IPv4 delete address route tests
>      Regular FIB info
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Identical FIB info with different table ID
>      TEST: Route removed from VRF when source address deleted            [FAIL]
>      TEST: Route in default VRF not removed                              [ OK ]
>  RTNETLINK answers: File exists
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [FAIL]
> 
>  Tests passed:   6
>  Tests failed:   2
> 
> And pass after:
> 
>  # ./fib_tests.sh -t ipv4_del_addr
> 
>  IPv4 delete address route tests
>      Regular FIB info
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Identical FIB info with different table ID
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
> 
>  Tests passed:   8
>  Tests failed:   0
> 
> Fixes: 5a56a0b3a45d ("net: Don't delete routes in different VRFs")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/fib_semantics.c                 |  1 +
>  tools/testing/selftests/net/fib_tests.sh | 27 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 

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

Re: [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used

[David Ahern]

On 12/4/22 12:50 AM, Ido Schimmel wrote:
> Cited commit added the table ID to the FIB info structure, but did not
> properly initialize it when table ID 0 is used. This can lead to a route
> in the default VRF with a preferred source address not being flushed
> when the address is deleted.
> 
> Consider the following example:
> 
>  # ip address add dev dummy1 192.0.2.1/28
>  # ip address add dev dummy1 192.0.2.17/28
>  # ip route add 198.51.100.0/24 via 192.0.2.2 src 192.0.2.17 metric 100
>  # ip route add table 0 198.51.100.0/24 via 192.0.2.2 src 192.0.2.17 metric 200
>  # ip route show 198.51.100.0/24
>  198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 100
>  198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 200
> 
> Both routes are installed in the default VRF, but they are using two
> different FIB info structures. One with a metric of 100 and table ID of
> 254 (main) and one with a metric of 200 and table ID of 0. Therefore,
> when the preferred source address is deleted from the default VRF,
> the second route is not flushed:
> 
>  # ip address del dev dummy1 192.0.2.17/28
>  # ip route show 198.51.100.0/24
>  198.51.100.0/24 via 192.0.2.2 dev dummy1 src 192.0.2.17 metric 200
> 
> Fix by storing a table ID of 254 instead of 0 in the route configuration
> structure.
> 
> Add a test case that fails before the fix:
> 
>  # ./fib_tests.sh -t ipv4_del_addr
> 
>  IPv4 delete address route tests
>      Regular FIB info
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Identical FIB info with different table ID
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Table ID 0
>      TEST: Route removed in default VRF when source address deleted      [FAIL]
> 
>  Tests passed:   8
>  Tests failed:   1
> 
> And passes after:
> 
>  # ./fib_tests.sh -t ipv4_del_addr
> 
>  IPv4 delete address route tests
>      Regular FIB info
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Identical FIB info with different table ID
>      TEST: Route removed from VRF when source address deleted            [ OK ]
>      TEST: Route in default VRF not removed                              [ OK ]
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
>      TEST: Route in VRF is not removed by address delete                 [ OK ]
>      Table ID 0
>      TEST: Route removed in default VRF when source address deleted      [ OK ]
> 
>  Tests passed:   9
>  Tests failed:   0
> 
> Fixes: 5a56a0b3a45d ("net: Don't delete routes in different VRFs")
> Reported-by: Donald Sharp <sharpd@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  net/ipv4/fib_frontend.c                  |  3 +++
>  tools/testing/selftests/net/fib_tests.sh | 10 ++++++++++
>  2 files changed, 13 insertions(+)
> 

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

Re: [PATCH net 0/2] ipv4: Two bug fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Sun,  4 Dec 2022 09:50:43 +0200 you wrote:
> Two small fixes for bugs in IPv4 routing code.
> 
> A variation of the second bug was reported by an FRR 5.0 (released
> 06/18) user as this version was setting a table ID of 0 for the default
> VRF, unlike iproute2 and newer FRR versions.
> 
> The first bug was discovered while fixing the second.
> 
> [...]

Here is the summary with links:
  - [net,1/2] ipv4: Fix incorrect route flushing when source address is deleted
    https://git.kernel.org/netdev/net/c/f96a3d74554d
  - [net,2/2] ipv4: Fix incorrect route flushing when table ID 0 is used
    https://git.kernel.org/netdev/net/c/c0d999348e01

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html