From 3d30f6defb5d0c6bc4d016b8eca114c4198acbbe Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Wed, 27 Jan 2021 16:20:22 -0500 Subject: [PATCH 1/2] zebra: disallow resolution to duplicate nexthops Disallow the resolution to nexthops that are marked duplicate. When we are resolving to an ecmp group, it's possible this group has duplicates. I found this when I hit a bug where we can have groups resolving to each other and cause the resolved->next->next pointer to increase exponentially. Sufficiently large ecmp and zebra will grind to a hault. Like so: ``` D> 4.4.4.14/32 [150/0] via 1.1.1.1 (recursive), weight 1, 00:00:02 * via 1.1.1.1, dummy1 onlink, weight 1, 00:00:02 via 4.4.4.1 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.2 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.3 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.4 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.5 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.6 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.7 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.8 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.9 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.10 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.11 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.12 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.13 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.15 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1 onlink, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1 onlink, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 4.4.4.16 (recursive), weight 1, 00:00:02 via 1.1.1.1, dummy1 onlink, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 via 1.1.1.1, dummy1, weight 1, 00:00:02 D> 4.4.4.15/32 [150/0] via 1.1.1.1 (recursive), weight 1, 00:00:09 * via 1.1.1.1, dummy1 onlink, weight 1, 00:00:09 via 4.4.4.1 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.2 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.3 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.4 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.5 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.6 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.7 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.8 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.9 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.10 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.11 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.12 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.13 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.14 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 4.4.4.16 (recursive), weight 1, 00:00:09 via 1.1.1.1, dummy1 onlink, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 via 1.1.1.1, dummy1, weight 1, 00:00:09 D> 4.4.4.16/32 [150/0] via 1.1.1.1 (recursive), weight 1, 00:00:19 * via 1.1.1.1, dummy1 onlink, weight 1, 00:00:19 via 4.4.4.1 (recursive), weight 1, 00:00:19 via 1.1.1.1, dummy1, weight 1, 00:00:19 via 4.4.4.2 (recursive), weight 1, 00:00:19 ............... ................ and on... ``` You can repro the above via: ``` kernel routes: 1.1.1.1 dev dummy1 scope link 4.4.4.0/24 via 1.1.1.1 dev dummy1 ============================== config: nexthop-group doof nexthop 1.1.1.1 nexthop 4.4.4.1 nexthop 4.4.4.10 nexthop 4.4.4.11 nexthop 4.4.4.12 nexthop 4.4.4.13 nexthop 4.4.4.14 nexthop 4.4.4.15 nexthop 4.4.4.16 nexthop 4.4.4.2 nexthop 4.4.4.3 nexthop 4.4.4.4 nexthop 4.4.4.5 nexthop 4.4.4.6 nexthop 4.4.4.7 nexthop 4.4.4.8 nexthop 4.4.4.9 ! =========================== Then use sharpd to install 4.4.4.16 -> 4.4.4.1 pointing to that nexthop group in decending order. ``` With these changes it prevents the growing ecmp above by disallowing duplicates to be in the resolution decision. These nexthops are not installed anyways so why should we be resolving to them? Signed-off-by: Stephen Worley --- zebra/zebra_nhg.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 604480b68b..2864b96c83 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -1760,6 +1760,10 @@ static bool nexthop_valid_resolve(const struct nexthop *nexthop, if (!CHECK_FLAG(resolved->flags, NEXTHOP_FLAG_ACTIVE)) return false; + /* Must not be duplicate */ + if (CHECK_FLAG(resolved->flags, NEXTHOP_FLAG_DUPLICATE)) + return false; + switch (nexthop->type) { case NEXTHOP_TYPE_IPV4_IFINDEX: case NEXTHOP_TYPE_IPV6_IFINDEX: From 13f93db3288d845bdca47a77fe026db5605684d5 Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Fri, 29 Jan 2021 15:58:34 -0500 Subject: [PATCH 2/2] topotests: add test for infinite recursion Add a test for the infinite recursion case fixed with 0c4dbb5f8fe8fb188fa0e0aa8ce04764e893b79b See that commit for details of the problem. This test uses a simpler version of the repro found there as the test. Signed-off-by: Stephen Worley --- .../all-protocol-startup/r1/ip_nht.ref | 12 +++++ .../test_all_protocol_startup.py | 48 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/tests/topotests/all-protocol-startup/r1/ip_nht.ref b/tests/topotests/all-protocol-startup/r1/ip_nht.ref index 098e3bf387..1da4da4df5 100644 --- a/tests/topotests/all-protocol-startup/r1/ip_nht.ref +++ b/tests/topotests/all-protocol-startup/r1/ip_nht.ref @@ -39,6 +39,18 @@ 4.4.4.2 unresolved Client list: pbr(fd XX) +6.6.6.1 + unresolved + Client list: pbr(fd XX) +6.6.6.2 + unresolved + Client list: pbr(fd XX) +6.6.6.3 + unresolved + Client list: pbr(fd XX) +6.6.6.4 + unresolved + Client list: pbr(fd XX) 192.168.0.2 resolved via connected is directly connected, r1-eth0 diff --git a/tests/topotests/all-protocol-startup/test_all_protocol_startup.py b/tests/topotests/all-protocol-startup/test_all_protocol_startup.py index d4c831e1c4..5942aca71d 100644 --- a/tests/topotests/all-protocol-startup/test_all_protocol_startup.py +++ b/tests/topotests/all-protocol-startup/test_all_protocol_startup.py @@ -82,6 +82,7 @@ class NetworkTopo(Topo): ## ##################################################### + @pytest.mark.isis @pytest.mark.ospf @pytest.mark.rip @@ -537,6 +538,51 @@ def test_nexthop_groups(): verify_route_nexthop_group("5.5.5.1/32") + ## 4-way ECMP Routes Pointing to Each Other + + # This is to check for a bug with NH resolution where + # routes would infintely resolve to each other blowing + # up the resolved-> nexthop pointer. + + net["r1"].cmd( + 'vtysh -c "c t" -c "nexthop-group infinite-recursive" -c "nexthop 6.6.6.1" -c "nexthop 6.6.6.2" \ + -c "nexthop 6.6.6.3" -c "nexthop 6.6.6.4"' + ) + + # static route nexthops can recurse to + + net["r1"].cmd('vtysh -c "c t" -c "ip route 6.6.6.0/24 1.1.1.1"') + + # Make routes that point to themselves in ecmp + + net["r1"].cmd( + 'vtysh -c "sharp install routes 6.6.6.4 nexthop-group infinite-recursive 1"' + ) + + net["r1"].cmd( + 'vtysh -c "sharp install routes 6.6.6.3 nexthop-group infinite-recursive 1"' + ) + + net["r1"].cmd( + 'vtysh -c "sharp install routes 6.6.6.2 nexthop-group infinite-recursive 1"' + ) + + net["r1"].cmd( + 'vtysh -c "sharp install routes 6.6.6.1 nexthop-group infinite-recursive 1"' + ) + + # Get routes and test if has too many (duplicate) nexthops + nhg_id = route_get_nhg_id("6.6.6.1/32") + output = net["r1"].cmd('vtysh -c "show nexthop-group rib %d"' % nhg_id) + + dups = re.findall(r"(via 1\.1\.1\.1)", output) + + # Should find 3, itself is inactive + assert len(dups) == 3, ( + "Route 6.6.6.1/32 with Nexthop Group ID=%d has wrong number of resolved nexthops" + % nhg_id + ) + ##CLI(net) ## Remove all NHG routes @@ -548,6 +594,8 @@ def test_nexthop_groups(): net["r1"].cmd('vtysh -c "sharp remove routes 4.4.4.1 1"') net["r1"].cmd('vtysh -c "sharp remove routes 4.4.4.2 1"') net["r1"].cmd('vtysh -c "sharp remove routes 5.5.5.1 1"') + net["r1"].cmd('vtysh -c "sharp remove routes 6.6.6.1 4"') + net["r1"].cmd('vtysh -c "c t" -c "no ip route 6.6.6.0/24 1.1.1.1"') def test_rip_status():