From 6de0cd9bdfbdac6a38ac2206d4257d951422aff6 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 28 Apr 2022 18:32:20 +0200 Subject: [PATCH 1/5] bgpd: fix VRF leaking with 'network import-check' (1/4) If 'bgp network import-check' is defined on the source BGP session, prefixes that are defined with the network command cannot be leaked to the other VRFs BGP table even if they are present in the origin VRF RIB. Always validate the nexthop of BGP static routes (i.e. defined with the network statement) if 'network import-check' is defined on the source BGP session and the prefix is present in source RIB. Signed-off-by: Louis Scalbert --- bgpd/bgp_mplsvpn.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 5aa752d6e7..2353910ead 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -1034,13 +1034,19 @@ static bool leak_update_nexthop_valid(struct bgp *to_bgp, struct bgp_dest *bn, else if (bpi_ultimate->type == ZEBRA_ROUTE_BGP && bpi_ultimate->sub_type == BGP_ROUTE_STATIC && table && (table->safi == SAFI_UNICAST || - table->safi == SAFI_LABELED_UNICAST) && - !CHECK_FLAG(bgp_nexthop->flags, BGP_FLAG_IMPORT_CHECK)) { - /* if the route is defined with the "network " command - * and "no bgp network import-check" is set, - * then mark the nexthop as valid. - */ - nh_valid = true; + table->safi == SAFI_LABELED_UNICAST)) { + /* the route is defined with the "network " command */ + + if (CHECK_FLAG(bgp_nexthop->flags, BGP_FLAG_IMPORT_CHECK)) + nh_valid = bgp_find_or_add_nexthop(to_bgp, bgp_nexthop, + afi, SAFI_UNICAST, + bpi_ultimate, NULL, + 0, p); + else + /* if "no bgp network import-check" is set, + * then mark the nexthop as valid. + */ + nh_valid = true; } else /* * TBD do we need to do anything about the From bb71bc02fd9c6b0aac66fafaef0ca9737e501dfb Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Tue, 5 Jul 2022 15:22:12 +0200 Subject: [PATCH 2/5] bgpd: fix VRF leaking with 'network import-check' (2/4) "if not XX else" statements are confusing. Replace two "if not XX else" statements by "if XX else" to prepare next commits. The patch is only cosmetic. Signed-off-by: Louis Scalbert --- bgpd/bgp_nht.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 05fd0dc4e7..dbf1fd2ea5 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -918,24 +918,22 @@ void bgp_nexthop_update(struct vrf *vrf, struct prefix *match, tree = &bgp->nexthop_cache_table[afi]; bnc_nhc = bnc_find(tree, match, nhr->srte_color, 0); - if (!bnc_nhc) { - if (BGP_DEBUG(nht, NHT)) - zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for nexthop cache", - &nhr->prefix, nhr->srte_color, - bgp->name_pretty); - } else + if (bnc_nhc) bgp_process_nexthop_update(bnc_nhc, nhr, false); + else if (BGP_DEBUG(nht, NHT)) + zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for nexthop cache", + &nhr->prefix, nhr->srte_color, + bgp->name_pretty); tree = &bgp->import_check_table[afi]; bnc_import = bnc_find(tree, match, nhr->srte_color, 0); - if (!bnc_import) { - if (BGP_DEBUG(nht, NHT)) - zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for import check", - &nhr->prefix, nhr->srte_color, - bgp->name_pretty); - } else + if (bnc_import) bgp_process_nexthop_update(bnc_import, nhr, true); + else if (BGP_DEBUG(nht, NHT)) + zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for import check", + &nhr->prefix, nhr->srte_color, + bgp->name_pretty); /* * HACK: if any BGP route is dependant on an SR-policy that doesn't From 879bfc01c827ef9aa1be97f8b5ad22f8cf6984ed Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 29 Apr 2022 14:26:04 +0200 Subject: [PATCH 3/5] bgpd: fix VRF leaking with 'network import-check' (3/4) If 'bgp network import-check' is defined on the source BGP session, prefixes that are defined with the network command cannot be leaked to the other VRFs BGP table even if they are present in the origin VRF RIB if the 'rt import' statement is defined after the 'network ' ones. When a prefix nexthop is updated, update the prefix route leaking. The current state of nexthop validation is now stored in the attributes of the bgp path info. Attributes are compared with the previous ones at route leaking update so that a nexthop validation change now triggers the update of destination VRF BGP table. Signed-off-by: Louis Scalbert --- bgpd/bgp_attr.c | 1 + bgpd/bgp_attr.h | 3 +++ bgpd/bgp_mplsvpn.c | 17 +++++++++++++++-- bgpd/bgp_nht.c | 24 +++++++++++++++++++++--- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/bgpd/bgp_attr.c b/bgpd/bgp_attr.c index 75aa2ac7cc..edfbc6c835 100644 --- a/bgpd/bgp_attr.c +++ b/bgpd/bgp_attr.c @@ -864,6 +864,7 @@ bool attrhash_cmp(const void *p1, const void *p2) attr1->df_alg == attr2->df_alg && attr1->nh_ifindex == attr2->nh_ifindex && attr1->nh_lla_ifindex == attr2->nh_lla_ifindex && + attr1->nh_flags == attr2->nh_flags && attr1->distance == attr2->distance && srv6_l3vpn_same(attr1->srv6_l3vpn, attr2->srv6_l3vpn) && srv6_vpn_same(attr1->srv6_vpn, attr2->srv6_vpn) && diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h index d30155e6db..5a39f734d3 100644 --- a/bgpd/bgp_attr.h +++ b/bgpd/bgp_attr.h @@ -155,6 +155,9 @@ struct attr { uint32_t med; uint32_t local_pref; ifindex_t nh_ifindex; + uint8_t nh_flags; + +#define BGP_ATTR_NH_VALID 0x01 /* Path origin attribute */ uint8_t origin; diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c index 2353910ead..32436861f4 100644 --- a/bgpd/bgp_mplsvpn.c +++ b/bgpd/bgp_mplsvpn.c @@ -2090,6 +2090,7 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ uint32_t num_labels = 0; int nexthop_self_flag = 1; struct bgp_path_info *bpi_ultimate = NULL; + struct bgp_path_info *bpi; int origin_local = 0; struct bgp *src_vrf; struct interface *ifp; @@ -2179,6 +2180,20 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ community_strip_accept_own(&static_attr); + bn = bgp_afi_node_get(to_bgp->rib[afi][safi], afi, safi, p, NULL); + + for (bpi = bgp_dest_get_bgp_path_info(bn); bpi; bpi = bpi->next) { + if (bpi->extra && bpi->extra->vrfleak && + bpi->extra->vrfleak->parent == path_vpn) + break; + } + + if (bpi && leak_update_nexthop_valid(to_bgp, bn, &static_attr, afi, safi, + path_vpn, bpi, src_vrf, p, debug)) + SET_FLAG(static_attr.nh_flags, BGP_ATTR_NH_VALID); + else + UNSET_FLAG(static_attr.nh_flags, BGP_ATTR_NH_VALID); + /* * Nexthop: stash and clear * @@ -2271,8 +2286,6 @@ static void vpn_leak_to_vrf_update_onevrf(struct bgp *to_bgp, /* to */ new_attr = bgp_attr_intern(&static_attr); bgp_attr_flush(&static_attr); - bn = bgp_afi_node_get(to_bgp->rib[afi][safi], afi, safi, p, NULL); - /* * ensure labels are copied * diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index dbf1fd2ea5..b6697568c6 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -903,7 +903,10 @@ void bgp_nexthop_update(struct vrf *vrf, struct prefix *match, { struct bgp_nexthop_cache_head *tree = NULL; struct bgp_nexthop_cache *bnc_nhc, *bnc_import; - struct bgp *bgp; + struct bgp *bgp, *bgp_default; + struct bgp_path_info *pi; + struct bgp_dest *dest; + safi_t safi; afi_t afi; if (!vrf->info) { @@ -928,9 +931,24 @@ void bgp_nexthop_update(struct vrf *vrf, struct prefix *match, tree = &bgp->import_check_table[afi]; bnc_import = bnc_find(tree, match, nhr->srte_color, 0); - if (bnc_import) + if (bnc_import) { bgp_process_nexthop_update(bnc_import, nhr, true); - else if (BGP_DEBUG(nht, NHT)) + + bgp_default = bgp_get_default(); + safi = nhr->safi; + if (bgp != bgp_default && bgp->rib[afi][safi]) { + dest = bgp_afi_node_get(bgp->rib[afi][safi], afi, safi, + match, NULL); + + for (pi = bgp_dest_get_bgp_path_info(dest); pi; + pi = pi->next) + if (pi->peer == bgp->peer_self && + pi->type == ZEBRA_ROUTE_BGP && + pi->sub_type == BGP_ROUTE_STATIC) + vpn_leak_from_vrf_update(bgp_default, + bgp, pi); + } + } else if (BGP_DEBUG(nht, NHT)) zlog_debug("parse nexthop update %pFX(%u)(%s): bnc info not found for import check", &nhr->prefix, nhr->srte_color, bgp->name_pretty); From 14e51be394b38431693662ccc600a1407ad1729b Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Thu, 5 May 2022 18:06:24 +0200 Subject: [PATCH 4/5] bgpd: fix VRF leaking with 'network import-check' (4/4) The following configuration creates an infinite routing leaking loop because 'rt vpn both' parameters are the same in both VRFs. > router bgp 5227 vrf r1-cust4 > no bgp network import-check > bgp router-id 192.168.1.1 > address-family ipv4 unicast > network 28.0.0.0/24 > rd vpn export 10:12 > rt vpn both 52:100 > import vpn > export vpn > exit-address-family > ! > router bgp 5227 vrf r1-cust5 > no bgp network import-check > bgp router id 192.168.1.1 > address-family ipv4 unicast > network 29.0.0.0/24 > rd vpn export 10:13 > rt vpn both 52:100 > import vpn > export vpn > exit-address-family The previous commit has added a routing leak update when a nexthop update is received from zebra. It indirectly calls bgp_find_or_add_nexthop() in which a static route triggers a nexthop cache entry registration that triggers a nexthop update from zebra. Do not register again the nexthop cache entry if the BGP_STATIC_ROUTE is already set. Signed-off-by: Louis Scalbert --- bgpd/bgp_nht.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index b6697568c6..e2c103bb52 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -406,7 +406,7 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, if (pi && is_route_parent_evpn(pi)) bnc->is_evpn_gwip_nexthop = true; - if (is_bgp_static_route) { + if (is_bgp_static_route && !CHECK_FLAG(bnc->flags, BGP_STATIC_ROUTE)) { SET_FLAG(bnc->flags, BGP_STATIC_ROUTE); /* If we're toggling the type, re-register */ From fb777555d3d45e58cd943ea776d124698c45c8a0 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 26 Jan 2024 13:30:24 +0100 Subject: [PATCH 5/5] topotests: vpnv4 route leaking with import-check Test vpnv4 route leaking with import-check Signed-off-by: Louis Scalbert --- .../topotests/bgp_l3vpn_to_bgp_vrf/customize.py | 2 ++ .../topotests/bgp_l3vpn_to_bgp_vrf/r1/bgpd.conf | 16 ++++++++++++++-- .../topotests/bgp_l3vpn_to_bgp_vrf/r1/zebra.conf | 4 ++++ .../bgp_l3vpn_to_bgp_vrf/scripts/check_routes.py | 16 ++++++++++++---- .../bgp_l3vpn_to_bgp_vrf/scripts/scale_down.py | 2 +- 5 files changed, 33 insertions(+), 7 deletions(-) diff --git a/tests/topotests/bgp_l3vpn_to_bgp_vrf/customize.py b/tests/topotests/bgp_l3vpn_to_bgp_vrf/customize.py index 23ab90794c..45c44ba5db 100644 --- a/tests/topotests/bgp_l3vpn_to_bgp_vrf/customize.py +++ b/tests/topotests/bgp_l3vpn_to_bgp_vrf/customize.py @@ -165,6 +165,8 @@ def ltemplatePreRouterStartHook(): cmds = [ "ip link add {0}-cust4 type vrf table 30", "ip link set dev {0}-cust4 up", + "ip link add {0}-cust5 type vrf table 40", + "ip link set dev {0}-cust5 up", ] rtr = "r1" for cmd in cmds: diff --git a/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/bgpd.conf b/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/bgpd.conf index b389eb1013..0d652dac07 100644 --- a/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/bgpd.conf +++ b/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/bgpd.conf @@ -64,5 +64,17 @@ router bgp 5227 vrf r1-cust4 import vpn export vpn exit-address-family -! -end + +router bgp 5227 vrf r1-cust5 + bgp router-id 192.168.1.1 + + address-family ipv4 unicast + network 172.16.1.1/32 + + label vpn export 105 + rd vpn export 10:15 + rt vpn both 52:100 + + import vpn + export vpn + exit-address-family diff --git a/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/zebra.conf b/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/zebra.conf index 221bc7a839..9a5b0a605e 100644 --- a/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/zebra.conf +++ b/tests/topotests/bgp_l3vpn_to_bgp_vrf/r1/zebra.conf @@ -18,6 +18,10 @@ interface r1-eth4 ip address 192.168.1.1/24 no link-detect +interface r1-cust5 + ip address 172.16.1.1/32 + no link-detect + ip forwarding diff --git a/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/check_routes.py b/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/check_routes.py index 3ab9b3f46e..28047f7b2d 100644 --- a/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/check_routes.py +++ b/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/check_routes.py @@ -60,6 +60,7 @@ want_r1_cust1_routes = [ {"p": "6.0.1.0/24", "n": "99.0.0.1"}, {"p": "6.0.2.0/24", "n": "99.0.0.1"}, {"p": "172.16.0.0/24", "n": "0.0.0.0", "bp": True}, + {"p": "172.16.1.1/32", "n": "0.0.0.0", "bp": True}, {"p": "99.0.0.1/32", "n": "192.168.1.2"}, ] bgpribRequireUnicastRoutes( @@ -73,6 +74,13 @@ bgpribRequireUnicastRoutes( "r1", "ipv4", "r1-cust4", "Customer 4 routes in r1 vrf", want_r1_cust4_routes ) +want_r1_cust5_routes = [ + {"p": "172.16.1.1/32", "n": "0.0.0.0", "bp": True}, +] +bgpribRequireUnicastRoutes( + "r1", "ipv4", "r1-cust5", "Customer 5 routes in r1 vrf", want_r1_cust5_routes +) + want_r3_cust1_routes = [ {"p": "5.1.0.0/24", "n": "99.0.0.2"}, {"p": "5.1.1.0/24", "n": "99.0.0.2"}, @@ -675,7 +683,7 @@ bgpribRequireUnicastRoutes( luCommand( "ce1", 'vtysh -c "show bgp ipv4 uni"', - "13 routes and 13", + "14 routes and 14", "wait", "Local and remote routes", 10, @@ -697,7 +705,7 @@ bgpribRequireUnicastRoutes( luCommand( "ce2", 'vtysh -c "show bgp ipv4 uni"', - "13 routes and 16", + "14 routes and 17", "wait", "Local and remote routes", 10, @@ -729,7 +737,7 @@ luCommand("r4", 'vtysh -c "show ip route vrf r4-cust2"') luCommand( "ce3", 'vtysh -c "show bgp ipv4 uni"', - "13 routes and 14", + "14 routes and 15", "wait", "Local and remote routes", 10, @@ -751,7 +759,7 @@ bgpribRequireUnicastRoutes( luCommand( "ce4", 'vtysh -c "show bgp vrf ce4-cust2 ipv4 uni"', - "13 routes and 15", + "14 routes and 16", "wait", "Local and remote routes", 10, diff --git a/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/scale_down.py b/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/scale_down.py index 43a5245d0f..190879cc27 100644 --- a/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/scale_down.py +++ b/tests/topotests/bgp_l3vpn_to_bgp_vrf/scripts/scale_down.py @@ -49,7 +49,7 @@ if ret != False and found != None: luCommand( rtr, 'vtysh -c "show bgp ipv4 uni" | grep Display', - " 13 route", + " 14 route", "wait", "BGP routes removed", wait,