Merge pull request #8956 from pguibert6WIND/bgp_loop_through_itself

bgpd: prevent routes loop through itself

bgpd/bgp_evpn.c

@@ -2520,7 +2520,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)) {

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,
+	return bgp_find_or_add_nexthop(
-				       family2afi(peer->su.sa.sa_family),
+		peer->bgp, peer->bgp, family2afi(peer->su.sa.sa_family),
-				       SAFI_UNICAST, NULL, peer, connected);
+		SAFI_UNICAST, NULL, peer, connected, NULL);
 }
 
 static void peer_xfer_stats(struct peer *peer_dst, struct peer *peer_src)

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)",

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) {
 		/*

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.

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)) {

tests/topotests/bgp_basic_functionality_topo1/test_bgp_basic_functionality.py

@@ -774,9 +774,9 @@ def test_BGP_attributes_with_vrf_default_keyword_p0(request):
         }
 
         result = verify_bgp_rib(tgen, addr_type, dut, input_dict)
-        assert result is True, "Testcase  : Failed \n Error: {}".format(tc_name, result)
+        assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result)
         result = verify_rib(tgen, addr_type, dut, input_dict)
-        assert result is True, "Testcase  : Failed \n Error: {}".format(tc_name, result)
+        assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result)
 
     for addr_type in ADDR_TYPES:
         dut = "r4"
@@ -793,9 +793,9 @@ def test_BGP_attributes_with_vrf_default_keyword_p0(request):
         }
 
         result = verify_bgp_rib(tgen, addr_type, dut, input_dict)
-        assert result is True, "Testcase  : Failed \n Error: {}".format(tc_name, result)
+        assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result)
         result = verify_rib(tgen, addr_type, dut, input_dict)
-        assert result is True, "Testcase  : Failed \n Error: {}".format(tc_name, result)
+        assert result is True, "Testcase {} : Failed \n Error: {}".format(tc_name, result)
 
         input_dict_4 = {"largeCommunity": "500:500:500", "community": "500:500"}
 
@@ -1134,15 +1134,10 @@ 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)
+        result = verify_fib_routes(tgen, addr_type, dut, input_dict_r1)
-        assert result is True, "Testcase {} :Failed \n Error: {}".format(
+        assert result is not True, "Testcase {} : Failed \n".format(tc_name)
-            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"
-        "Error: {}".format(tc_name, result)
+        "Error: {}".format(result)
 
     step("Verify Ipv4 and Ipv6 network installed in r3 RIB but not in FIB")
     input_dict_r3 = {
@@ -1156,17 +1151,10 @@ def test_bgp_with_loopback_with_same_subnet_p1(request):
     dut = "r3"
     protocol = "bgp"
     for addr_type in ADDR_TYPES:
-        result = verify_rib(
+        result = verify_fib_routes(tgen, addr_type, dut, input_dict_r1)
-            tgen, addr_type, dut, input_dict_r3, protocol=protocol, fib=None
+        assert result is not True, "Testcase {} : Failed \n".format(tc_name)
-        )
-        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"
-        "Error: {}".format(tc_name, result)
+        "Error: {}".format(result)
 
     write_test_footer(tc_name)