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 <philippe.guibert@6wind.com>

diff --git a/bgpd/bgp_evpn.c b/bgpd/bgp_evpn.c
index c99f539c7b72ec6d9e157f610b0a12a22b7c61b5..f0c3707920b306ba8509fdc05cfa2aee56426ec6 100644 (file)
--- 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 79dc0aec9d0850000199c09bb79449a58732b480..c172968774d842dcccdfd312753669e0c5319520 100644 (file)
--- 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 0de48dcf78f5941f4267458ef1fb349cf7f73fb0..cab58b45b876fe6293dafde1681fbf8f17e6d1b0 100644 (file)
--- 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 eb00a4641c8393d6d43a2e3a288cfe94d2743dda..c77e240855a8f4ce0f43e949d2f09963d999a847 100644 (file)
--- 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 e006aa4469cddd745e379d066a101072e5a8c27b..43f0aa26cf24f0f681f4fca9d3afa1fd855f1297 100644 (file)
--- 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 bd45314350aaf71f41ea4d4d6800e76e38fa43b0..de75e9ea96270537b1c3745eea432dd6c2491645 100644 (file)
--- 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 374cce21f61bc463f347dc1512482e579fbc89bd..e33b906d6c4f45820e25113614975292acef8731 100644 (file)
--- 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"