From 654a5978f695087af062bfc9a382321fa2ccc4ae 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 --- bgpd/bgp_evpn.c | 2 +- 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 ------------ 7 files changed, 26 insertions(+), 24 deletions(-) diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c index c99f539c7b..f0c3707920 100644 --- a/bgpd/bgp_evpn.c +++ b/bgpd/bgp_evpn.c @@ -2536,7 +2536,7 @@ static int install_evpn_route_entry_in_vrf(struct bgp *bgp_vrf, /* Gateway IP nexthop should be resolved */ if (attr.evpn_overlay.type == OVERLAY_INDEX_GATEWAY_IP) { if (bgp_find_or_add_nexthop(bgp_vrf, bgp_vrf, afi, safi, pi, - NULL, 0)) + NULL, 0, NULL)) bgp_path_info_set_flag(dest, pi, BGP_PATH_VALID); else { if (BGP_DEBUG(nht, NHT)) { diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 79dc0aec9d..c172968774 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 0de48dcf78..cab58b45b8 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -845,7 +845,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)", @@ -931,7 +931,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 eb00a4641c..c77e240855 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -160,7 +160,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; @@ -192,6 +193,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 e006aa4469..43f0aa26cf 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 bd45314350..de75e9ea96 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -4103,7 +4103,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); @@ -4244,7 +4245,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 { @@ -5683,7 +5684,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 { @@ -5735,7 +5736,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"