From 9f002fa5dd34e7d901b501e7d0306027d85b531a Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 24 Apr 2022 16:52:46 -0400 Subject: [PATCH] bgpd: Fix import check removal Fix: 06e4e90132ad23815c6f288dd7e6be334f5ab233 Modified BGP to pay more attention the prefix returned from zebra to ensure that a LPM wasn't accidently causing BGP import checks to think it had a match when it did not. This unfortunately removed the check to handle the route removal. This sequence of config and events would leave BGP in a bad state: ip route 100.100.100.0/24 Null0 router bgp 32932 bgp network import-check address-family ipv4 uni network 100.100.100.0/24 Then if you removed the static route the import check would still think the route existed: donatas-pc(config)# ip route 100.100.100.0/24 Null0 donatas-pc(config)# do sh ip bgp import-check-table Current BGP import check cache: 100.100.100.0 valid [IGP metric 0], #paths 1 blackhole Last update: Sat Apr 23 22:51:34 2022 donatas-pc(config)# do sh ip nht 100.100.100.0 resolved via static is directly connected, Null0 Client list: bgp(fd 17) donatas-pc(config)# do sh ip bgp neighbors 192.168.10.123 advertised-routes | include 100.100.100.0 *> 100.100.100.0/24 0.0.0.0 0 32768 i donatas-pc(config)# no ip route 100.100.100.0/24 Null0 donatas-pc(config)# do sh ip nht 100.100.100.0 resolved via kernel via 192.168.10.1, enp3s0 Client list: bgp(fd 17) donatas-pc(config)# do sh ip bgp import-check-table Current BGP import check cache: 100.100.100.0 valid [IGP metric 0], #paths 1 blackhole Last update: Sat Apr 23 22:51:34 2022 donatas-pc(config)# do sh ip bgp neighbors 192.168.10.123 advertised-routes | include 100.100.100.0 *> 100.100.100.0/24 0.0.0.0 0 32768 i donatas-pc(config)# Fix this by moving the code to handle the prefix check to the evaluation function and mark the bnc as not matching and actually evaluate the bnc. Signed-off-by: Donald Sharp --- bgpd/bgp_nht.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index dc4f30a906..3433e1471c 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -390,7 +390,8 @@ void bgp_delete_connected_nexthop(afi_t afi, struct peer *peer) } static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc, - struct zapi_route *nhr) + struct zapi_route *nhr, + bool import_check) { struct nexthop *nexthop; struct nexthop *oldnh; @@ -421,7 +422,21 @@ static void bgp_process_nexthop_update(struct bgp_nexthop_cache *bnc, if (nhr->nexthop_num != bnc->nexthop_num) bnc->change_flags |= BGP_NEXTHOP_CHANGED; - if (nhr->nexthop_num) { + if (import_check && (nhr->type == ZEBRA_ROUTE_BGP || + !prefix_same(&bnc->prefix, &nhr->prefix))) { + SET_FLAG(bnc->change_flags, BGP_NEXTHOP_CHANGED); + UNSET_FLAG(bnc->flags, BGP_NEXTHOP_VALID); + UNSET_FLAG(bnc->flags, BGP_NEXTHOP_LABELED_VALID); + UNSET_FLAG(bnc->flags, BGP_NEXTHOP_EVPN_INCOMPLETE); + + bnc_nexthop_free(bnc); + bnc->nexthop = NULL; + + if (BGP_DEBUG(nht, NHT)) + zlog_debug( + "%s: Import Check does not resolve to the same prefix for %pFX received %pFX or matching route is BGP", + __func__, &bnc->prefix, &nhr->prefix); + } else if (nhr->nexthop_num) { struct peer *peer = bnc->nht_info; /* notify bgp fsm if nbr ip goes from invalid->valid */ @@ -695,7 +710,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id) "parse nexthop update(%pFX(%u)(%s)): bnc info not found for nexthop cache", &nhr.prefix, nhr.srte_color, bgp->name_pretty); } else - bgp_process_nexthop_update(bnc_nhc, &nhr); + bgp_process_nexthop_update(bnc_nhc, &nhr, false); tree = &bgp->import_check_table[afi]; @@ -706,17 +721,8 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id) "parse nexthop update(%pFX(%u)(%s)): bnc info not found for import check", &nhr.prefix, nhr.srte_color, bgp->name_pretty); return; - } else { - if (nhr.type == ZEBRA_ROUTE_BGP - || !prefix_same(&bnc_import->prefix, &nhr.prefix)) { - if (BGP_DEBUG(nht, NHT)) - zlog_debug( - "%s: Import Check does not resolve to the same prefix for %pFX received %pFX", - __func__, &bnc_import->prefix, &nhr.prefix); - return; - } - bgp_process_nexthop_update(bnc_import, &nhr); } + bgp_process_nexthop_update(bnc_import, &nhr, true); /* * HACK: if any BGP route is dependant on an SR-policy that doesn't @@ -739,7 +745,7 @@ void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id) || CHECK_FLAG(bnc_iter->flags, BGP_NEXTHOP_VALID)) continue; - bgp_process_nexthop_update(bnc_iter, &nhr); + bgp_process_nexthop_update(bnc_iter, &nhr, false); } } } -- 2.39.5