From fb8ae704615c5043ffe3209a14f51f7a701c307a Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Wed, 30 Jun 2021 10:52:29 +0200 Subject: [PATCH] bgpd: prevent routes loop through itself Some BGP updates received by BGP invite local router to install a route through itself. The system will not do it, and the route should be considered as not valid at the earliest. This case is detected on the zebra, and this detection prevents from trying to install this route to the local system. However, the nexthop tracking mechanism is called, and acts as if the route was valid, which is not the case. By detecting in BGP that use case, we avoid installing the invalid routes. Signed-off-by: Philippe Guibert (cherry picked from commit 654a5978f695087af062bfc9a382321fa2ccc4ae) --- bgpd/bgp_fsm.c | 6 +++--- bgpd/bgp_mplsvpn.c | 4 ++-- bgpd/bgp_nht.c | 13 ++++++++++++- bgpd/bgp_nht.h | 3 ++- bgpd/bgp_route.c | 10 ++++++---- .../test_bgp_basic_functionality.py | 12 ------------ 6 files changed, 25 insertions(+), 23 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 20c61fec57..a242510566 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -110,9 +110,9 @@ int bgp_peer_reg_with_nht(struct peer *peer) && !CHECK_FLAG(peer->bgp->flags, BGP_FLAG_DISABLE_NH_CONNECTED_CHK)) connected = 1; - return bgp_find_or_add_nexthop(peer->bgp, peer->bgp, - family2afi(peer->su.sa.sa_family), - SAFI_UNICAST, NULL, peer, connected); + return bgp_find_or_add_nexthop( + peer->bgp, peer->bgp, family2afi(peer->su.sa.sa_family), + SAFI_UNICAST, NULL, peer, connected, NULL); } static void peer_xfer_stats(struct peer *peer_dst, struct peer *peer_src) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index eb68d84c06..3ebe0538e0 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -602,7 +602,7 @@ leak_update(struct bgp *bgp, /* destination bgp instance */ * 'connected' parameter? */ nh_valid = bgp_find_or_add_nexthop( - bgp, bgp_nexthop, afi, safi, bpi, NULL, 0); + bgp, bgp_nexthop, afi, safi, bpi, NULL, 0, p); if (debug) zlog_debug("%s: nexthop is %svalid (in vrf %s)", @@ -678,7 +678,7 @@ leak_update(struct bgp *bgp, /* destination bgp instance */ * 'connected' parameter? */ nh_valid = bgp_find_or_add_nexthop(bgp, bgp_nexthop, afi, safi, - new, NULL, 0); + new, NULL, 0, p); if (debug) zlog_debug("%s: nexthop is %svalid (in vrf %s)", diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index e94f63541d..fe9c124f66 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -150,7 +150,8 @@ void bgp_unlink_nexthop_by_peer(struct peer *peer) */ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, afi_t afi, safi_t safi, struct bgp_path_info *pi, - struct peer *peer, int connected) + struct peer *peer, int connected, + const struct prefix *orig_prefix) { struct bgp_nexthop_cache_head *tree = NULL; struct bgp_nexthop_cache *bnc; @@ -182,6 +183,16 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, if (make_prefix(afi, pi, &p) < 0) return 1; + if (!is_bgp_static_route && orig_prefix + && prefix_same(&p, orig_prefix)) { + if (BGP_DEBUG(nht, NHT)) { + zlog_debug( + "%s(%pFX): prefix loops through itself", + __func__, &p); + } + return 0; + } + srte_color = pi->attr->srte_color; } else if (peer) { /* diff --git a/bgpd/bgp_nht.h b/bgpd/bgp_nht.h index 9268b225ca..29294a722a 100644 --- a/bgpd/bgp_nht.h +++ b/bgpd/bgp_nht.h @@ -42,7 +42,8 @@ extern void bgp_parse_nexthop_update(int command, vrf_id_t vrf_id); extern int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, afi_t a, safi_t safi, struct bgp_path_info *p, - struct peer *peer, int connected); + struct peer *peer, int connected, + const struct prefix *orig_prefix); /** * bgp_unlink_nexthop() - Unlink the nexthop object from the path structure. diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 366a53e26c..76c3b2d720 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -4111,7 +4111,8 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, nh_afi = BGP_ATTR_NH_AFI(afi, pi->attr); if (bgp_find_or_add_nexthop(bgp, bgp_nexthop, nh_afi, - safi, pi, NULL, connected) + safi, pi, NULL, connected, + p) || CHECK_FLAG(peer->flags, PEER_FLAG_IS_RFAPI_HD)) bgp_path_info_set_flag(dest, pi, BGP_PATH_VALID); @@ -4257,7 +4258,7 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, nh_afi = BGP_ATTR_NH_AFI(afi, new->attr); if (bgp_find_or_add_nexthop(bgp, bgp, nh_afi, safi, new, NULL, - connected) + connected, p) || CHECK_FLAG(peer->flags, PEER_FLAG_IS_RFAPI_HD)) bgp_path_info_set_flag(dest, new, BGP_PATH_VALID); else { @@ -5487,7 +5488,7 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p, if (bgp_find_or_add_nexthop(bgp, bgp_nexthop, afi, safi, pi, NULL, - 0)) + 0, p)) bgp_path_info_set_flag(dest, pi, BGP_PATH_VALID); else { @@ -5539,7 +5540,8 @@ void bgp_static_update(struct bgp *bgp, const struct prefix *p, /* Nexthop reachability check. */ if (CHECK_FLAG(bgp->flags, BGP_FLAG_IMPORT_CHECK) && (safi == SAFI_UNICAST || safi == SAFI_LABELED_UNICAST)) { - if (bgp_find_or_add_nexthop(bgp, bgp, afi, safi, new, NULL, 0)) + if (bgp_find_or_add_nexthop(bgp, bgp, afi, safi, new, NULL, 0, + p)) bgp_path_info_set_flag(dest, new, BGP_PATH_VALID); else { if (BGP_DEBUG(nht, NHT)) { diff --git a/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py b/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py index 374cce21f6..e33b906d6c 100644 --- a/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py +++ b/tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py @@ -1134,11 +1134,6 @@ def test_bgp_with_loopback_with_same_subnet_p1(request): dut = "r1" protocol = "bgp" for addr_type in ADDR_TYPES: - result = verify_rib(tgen, addr_type, dut, input_dict_r1, protocol=protocol) - assert result is True, "Testcase {} :Failed \n Error: {}".format( - tc_name, result - ) - result = verify_fib_routes(tgen, addr_type, dut, input_dict_r1, expected=False) assert result is not True, "Testcase {} : Failed \n" "Expected behavior: routes should not present in fib \n" @@ -1156,13 +1151,6 @@ def test_bgp_with_loopback_with_same_subnet_p1(request): dut = "r3" protocol = "bgp" for addr_type in ADDR_TYPES: - result = verify_rib( - tgen, addr_type, dut, input_dict_r3, protocol=protocol, fib=None - ) - assert result is True, "Testcase {} :Failed \n Error: {}".format( - tc_name, result - ) - result = verify_fib_routes(tgen, addr_type, dut, input_dict_r1, expected=False) assert result is not True, "Testcase {} : Failed \n" "Expected behavior: routes should not present in fib \n" -- 2.39.5