* [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted
2022-12-04 7:50 [PATCH net 0/2] ipv4: Two bug fixes Ido Schimmel
@ 2022-12-04 7:50 ` Ido Schimmel
2022-12-04 16:25 ` David Ahern
2022-12-04 7:50 ` [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used Ido Schimmel
2022-12-07 4:40 ` [PATCH net 0/2] ipv4: Two bug fixes patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2022-12-04 7:50 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, pabeni, edumazet, dsahern, mark.tomlinson, sharpd,
mlxsw, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted
2022-12-04 7:50 ` [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted Ido Schimmel
@ 2022-12-04 16:25 ` David Ahern
0 siblings, 0 replies; 6+ messages in thread
From: David Ahern @ 2022-12-04 16:25 UTC (permalink / raw)
To: Ido Schimmel, netdev
Cc: davem, kuba, pabeni, edumazet, mark.tomlinson, sharpd, mlxsw
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used
2022-12-04 7:50 [PATCH net 0/2] ipv4: Two bug fixes Ido Schimmel
2022-12-04 7:50 ` [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted Ido Schimmel
@ 2022-12-04 7:50 ` Ido Schimmel
2022-12-04 16:27 ` David Ahern
2022-12-07 4:40 ` [PATCH net 0/2] ipv4: Two bug fixes patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2022-12-04 7:50 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, pabeni, edumazet, dsahern, mark.tomlinson, sharpd,
mlxsw, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used
2022-12-04 7:50 ` [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used Ido Schimmel
@ 2022-12-04 16:27 ` David Ahern
0 siblings, 0 replies; 6+ messages in thread
From: David Ahern @ 2022-12-04 16:27 UTC (permalink / raw)
To: Ido Schimmel, netdev
Cc: davem, kuba, pabeni, edumazet, mark.tomlinson, sharpd, mlxsw
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 0/2] ipv4: Two bug fixes
2022-12-04 7:50 [PATCH net 0/2] ipv4: Two bug fixes Ido Schimmel
2022-12-04 7:50 ` [PATCH net 1/2] ipv4: Fix incorrect route flushing when source address is deleted Ido Schimmel
2022-12-04 7:50 ` [PATCH net 2/2] ipv4: Fix incorrect route flushing when table ID 0 is used Ido Schimmel
@ 2022-12-07 4:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-12-07 4:40 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev, davem, kuba, pabeni, edumazet, dsahern, mark.tomlinson,
sharpd, mlxsw
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
^ permalink raw reply [flat|nested] 6+ messages in thread