From 74c9d89aaf3df1b583de341169c4cb77eaa1b3b4 Mon Sep 17 00:00:00 2001 From: Enke Chen Date: Wed, 8 Jan 2025 09:12:56 -0800 Subject: [PATCH 1/3] Revert "bgpd: Reinstall aggregated routes if using route-maps and it was changed" This reverts commit ee1986f1b5ae6b94b446b12e1b77cc30d8f5f46d. The fix is incomplete, and is no longer needed with the fix that applies the route-map for an aggregate and then compares the attribute. Signed-off-by: Enke Chen --- bgpd/bgp_route.c | 6 ++---- bgpd/bgp_route.h | 1 - bgpd/bgp_routemap.c | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 0124b12cc37c..8c3b5e97ed7d 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7799,9 +7799,8 @@ static void bgp_aggregate_install( * If the aggregate information has not changed * no need to re-install it again. */ - if (pi && (!aggregate->rmap.changed && - bgp_aggregate_info_same(pi, origin, aspath, community, - ecommunity, lcommunity))) { + if (pi && bgp_aggregate_info_same(pi, origin, aspath, community, + ecommunity, lcommunity)) { bgp_dest_unlock_node(dest); if (aspath) @@ -8796,7 +8795,6 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, aggregate->rmap.name = XSTRDUP(MTYPE_ROUTE_MAP_NAME, rmap); aggregate->rmap.map = route_map_lookup_by_name(rmap); - aggregate->rmap.changed = true; route_map_counter_increment(aggregate->rmap.map); } diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index d71bfd3ebc62..4dcd035d793b 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -446,7 +446,6 @@ struct bgp_aggregate { struct { char *name; struct route_map *map; - bool changed; } rmap; /* Suppress-count. */ diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index 61586ea922f5..7a96f0cf6e2f 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -4612,7 +4612,6 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, route_map_counter_increment(map); aggregate->rmap.map = map; - aggregate->rmap.changed = true; matched = true; } From 22d95f4ba8444171944eab29e99dfa6087813d6f Mon Sep 17 00:00:00 2001 From: Enke Chen Date: Wed, 8 Jan 2025 17:34:29 -0800 Subject: [PATCH 2/3] bgpd: apply route-map for aggregate before attribute comparison Currently when re-evaluating an aggregate route, the full attribute of the aggregate route is not compared with the existing one in the BGP table. That can result in unnecessary churns (un-install and then install) of the aggregate route when a more specific route is added or deleted, or when the route-map for the aggregate changes. The churn would impact route installation and route advertisement. The fix is to apply the route-map for the aggregate first and then compare the attribute. Here is an example of the churn: debug bgp aggregate prefix 5.5.5.0/24 ! route-map set-comm permit 10 set community 65004:200 ! router bgp 65001 address-family ipv4 unicast redistribute static aggregate-address 5.5.5.0/24 route-map set-comm ! Step 1: ip route 5.5.5.1/32 Null0 Jan 8 10:28:49 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 1 Jan 8 10:28:49 enke-vm1 bgpd[285786]: [Y444T-HEVNG] aggregate 5.5.5.0/24: installed Step 2: ip route 5.5.5.2/32 Null0 Jan 8 10:29:03 enke-vm1 bgpd[285786]: [J7PXJ-A7YA2] bgp_aggregate_install: aggregate 5.5.5.0/24, count 2 Jan 8 10:29:03 enke-vm1 bgpd[285786]: [S2EH5-EQSX6] aggregate 5.5.5.0/24: existing, removed Jan 8 10:29:03 enke-vm1 bgpd[285786]: [Y444T-HEVNG] aggregate 5.5.5.0/24: installed --- Signed-off-by: Enke Chen --- bgpd/bgp_route.c | 98 ++++++++++-------------------------------------- 1 file changed, 20 insertions(+), 78 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 8c3b5e97ed7d..93d27aa167c6 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7729,44 +7729,6 @@ static bool aggr_unsuppress_path(struct bgp_aggregate *aggregate, return false; } -static bool bgp_aggregate_info_same(struct bgp_path_info *pi, uint8_t origin, - struct aspath *aspath, - struct community *comm, - struct ecommunity *ecomm, - struct lcommunity *lcomm) -{ - static struct aspath *ae = NULL; - enum asnotation_mode asnotation; - - asnotation = bgp_get_asnotation(NULL); - - if (!aspath) - ae = aspath_empty(asnotation); - - if (!pi) - return false; - - if (origin != pi->attr->origin) - return false; - - if (!aspath_cmp(pi->attr->aspath, (aspath) ? aspath : ae)) - return false; - - if (!community_cmp(bgp_attr_get_community(pi->attr), comm)) - return false; - - if (!ecommunity_cmp(bgp_attr_get_ecommunity(pi->attr), ecomm)) - return false; - - if (!lcommunity_cmp(bgp_attr_get_lcommunity(pi->attr), lcomm)) - return false; - - if (!CHECK_FLAG(pi->flags, BGP_PATH_VALID)) - return false; - - return true; -} - static void bgp_aggregate_install( struct bgp *bgp, afi_t afi, safi_t safi, const struct prefix *p, uint8_t origin, struct aspath *aspath, struct community *community, @@ -7775,14 +7737,14 @@ static void bgp_aggregate_install( { struct bgp_dest *dest; struct bgp_table *table; - struct bgp_path_info *pi, *orig, *new; + struct bgp_path_info *pi, *new; struct attr *attr; table = bgp->rib[afi][safi]; dest = bgp_node_get(table, p); - for (orig = pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) + 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_AGGREGATE) break; @@ -7799,19 +7761,21 @@ static void bgp_aggregate_install( * If the aggregate information has not changed * no need to re-install it again. */ - if (pi && bgp_aggregate_info_same(pi, origin, aspath, community, - ecommunity, lcommunity)) { + attr = bgp_attr_aggregate_intern(bgp, origin, aspath, community, ecommunity, + lcommunity, aggregate, atomic_aggregate, p); + if (!attr) { + aspath_free(aspath); + community_free(&community); + ecommunity_free(&ecommunity); + lcommunity_free(&lcommunity); bgp_dest_unlock_node(dest); + bgp_aggregate_delete(bgp, p, afi, safi, aggregate); + return; + } - if (aspath) - aspath_free(aspath); - if (community) - community_free(&community); - if (ecommunity) - ecommunity_free(&ecommunity); - if (lcommunity) - lcommunity_free(&lcommunity); - + if (pi && CHECK_FLAG(pi->flags, BGP_PATH_VALID) && attrhash_cmp(pi->attr, attr)) { + bgp_attr_unintern(&attr); + bgp_dest_unlock_node(dest); return; } @@ -7823,22 +7787,6 @@ static void bgp_aggregate_install( bgp_process(bgp, dest, pi, afi, safi); } - attr = bgp_attr_aggregate_intern( - bgp, origin, aspath, community, ecommunity, lcommunity, - aggregate, atomic_aggregate, p); - - if (!attr) { - aspath_free(aspath); - community_free(&community); - ecommunity_free(&ecommunity); - lcommunity_free(&lcommunity); - bgp_dest_unlock_node(dest); - bgp_aggregate_delete(bgp, p, afi, safi, aggregate); - if (BGP_DEBUG(update_groups, UPDATE_GROUPS)) - zlog_debug("%s: %pFX null attribute", __func__, - p); - return; - } new = info_make(ZEBRA_ROUTE_BGP, BGP_ROUTE_AGGREGATE, 0, bgp->peer_self, attr, dest); @@ -7849,17 +7797,11 @@ static void bgp_aggregate_install( bgp_process(bgp, dest, new, afi, safi); } else { uninstall_aggregate_route: - for (pi = orig; pi; pi = pi->next) - if (pi->peer == bgp->peer_self - && pi->type == ZEBRA_ROUTE_BGP - && pi->sub_type == BGP_ROUTE_AGGREGATE) - break; - - /* Withdraw static BGP route from routing table. */ - if (pi) { - bgp_path_info_delete(dest, pi); - bgp_process(bgp, dest, pi, afi, safi); - } + /* Withdraw the aggregate route from routing table. */ + if (pi) { + bgp_path_info_delete(dest, pi); + bgp_process(bgp, dest, pi, afi, safi); + } } bgp_dest_unlock_node(dest); From 94ca6ddfae959a08e84a7a5a070f44ddba70f156 Mon Sep 17 00:00:00 2001 From: Enke Chen Date: Thu, 9 Jan 2025 14:48:35 -0800 Subject: [PATCH 3/3] bgpd: fix memory leak in bgp_aggregate_install() Potential memory leak with as-set and matching-MED-only config. Signed-off-by: Enke Chen Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 93d27aa167c6..a87cd21f22e3 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7753,8 +7753,13 @@ static void bgp_aggregate_install( * If we have paths with different MEDs, then don't install * (or uninstall) the aggregate route. */ - if (aggregate->match_med && aggregate->med_mismatched) + if (aggregate->match_med && aggregate->med_mismatched) { + aspath_free(aspath); + community_free(&community); + ecommunity_free(&ecommunity); + lcommunity_free(&lcommunity); goto uninstall_aggregate_route; + } if (aggregate->count > 0) { /*