From 3e53d096287ce117a455f051d772d21aac63640e Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Wed, 15 Mar 2023 13:31:58 +0200 Subject: [PATCH 1/2] bgpd: Free previously dup'ed aspath attribute for aggregate routes Fixes memory leak: ``` ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233:Direct leak of 80 byte(s) in 2 object(s) allocated f rom: ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 0 0x7fcce641b037 in __interceptor_calloc ../../ ../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 1 0x7fcce601f874 in qcalloc lib/memory.c:105 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 2 0x55a9594c805e in aspath_dup bgpd/bgp_aspath.c:688 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 3 0x55a9594cf55b in bgp_aggr_aspath_prepare bgpd/bgp_aspath.c:2164 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 4 0x7fcce5fbc95a in hash_iterate lib/hash.c:252 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 5 0x55a9594cf82f in bgp_compute_aggregate_aspath_val bgpd/bgp_aspath.c:2223 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 6 0x55a9594cf5db in bgp_compute_aggregate_aspath bgpd/bgp_aspath.c:2179 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 7 0x55a9592740fb in bgp_add_route_to_aggregate bgpd/bgp_route.c:8035 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 8 0x55a9592750ec in bgp_aggregate_increment bgpd/bgp_route.c:8234 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 9 0x55a959263174 in bgp_update bgpd/bgp_route.c:4797 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 10 0x55a9592696c9 in bgp_nlri_parse_ip bgpd/bgp_route.c:6070 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 11 0x55a959210649 in bgp_nlri_parse bgpd/bgp_packet.c:339 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 12 0x55a95921a3a1 in bgp_update_receive bgpd/bgp_packet.c:2023 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 13 0x55a95922159b in bgp_process_packet bgpd/bgp_packet.c:2933 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 14 0x7fcce60eaa50 in thread_call lib/thread.c:1991 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 15 0x7fcce5fe2e54 in frr_run lib/libfrr.c:1185 ./bgp_local_asn_dot.test_bgp_local_asn_dot_agg/r3.bgpd.asan.653233- 16 0x55a9590d256d in main bgpd/bgp_main.c:505 ``` Signed-off-by: Donatas Abraitis --- bgpd/bgp_aspath.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 2ae693cd30..89e631d437 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -2169,11 +2169,15 @@ static void bgp_aggr_aspath_prepare(struct hash_bucket *hb, void *arg) { struct aspath *hb_aspath = hb->data; struct aspath **aggr_aspath = arg; + struct aspath *aspath = NULL; - if (*aggr_aspath) - *aggr_aspath = aspath_aggregate(*aggr_aspath, hb_aspath); - else + if (*aggr_aspath) { + aspath = aspath_aggregate(*aggr_aspath, hb_aspath); + aspath_free(*aggr_aspath); + *aggr_aspath = aspath; + } else { *aggr_aspath = aspath_dup(hb_aspath); + } } void bgp_aggr_aspath_remove(void *arg) From 223d11f5a4d3abb5f6b4b3d64747b997dc7073ef Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 13 Mar 2023 22:26:09 +0200 Subject: [PATCH 2/2] bgpd: Unlock dest if we return earlier for aggregate install If the bgp is in shutdown state or so, do not forget to unlock the dest node. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 17 +++++++++++------ bgpd/bgp_route.h | 3 ++- bgpd/bgp_routemap.c | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 747090dc3a..18a7b6ae22 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -7248,7 +7248,7 @@ static struct bgp_aggregate *bgp_aggregate_new(void) return XCALLOC(MTYPE_BGP_AGGREGATE, sizeof(struct bgp_aggregate)); } -static void bgp_aggregate_free(struct bgp_aggregate *aggregate) +void bgp_aggregate_free(struct bgp_aggregate *aggregate) { XFREE(MTYPE_ROUTE_MAP_NAME, aggregate->suppress_map_name); route_map_counter_decrement(aggregate->suppress_map); @@ -7648,7 +7648,7 @@ static void bgp_aggregate_med_update(struct bgp_aggregate *aggregate, } /* Update an aggregate as routes are added/removed from the BGP table */ -void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, +bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, safi_t safi, struct bgp_aggregate *aggregate) { struct bgp_table *table; @@ -7666,9 +7666,9 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, /* If the bgp instance is being deleted or self peer is deleted * then do not create aggregate route */ - if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS) - || (bgp->peer_self == NULL)) - return; + if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS) || + bgp->peer_self == NULL) + return false; /* Initialize and test routes for MED difference. */ if (aggregate->match_med) @@ -7860,6 +7860,8 @@ void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, bgp_aggregate_install(bgp, afi, safi, p, origin, aspath, community, ecommunity, lcommunity, atomic_aggregate, aggregate); + + return true; } void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, @@ -8450,7 +8452,10 @@ static int bgp_aggregate_set(struct vty *vty, const char *prefix_str, afi_t afi, bgp_dest_set_bgp_aggregate_info(dest, aggregate); /* Aggregate address insert into BGP routing table. */ - bgp_aggregate_route(bgp, &p, afi, safi, aggregate); + if (!bgp_aggregate_route(bgp, &p, afi, safi, aggregate)) { + bgp_aggregate_free(aggregate); + bgp_dest_unlock_node(dest); + } return CMD_SUCCESS; } diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 2afd97c4dd..fd6d0cfe79 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -780,7 +780,7 @@ extern void bgp_config_write_distance(struct vty *, struct bgp *, afi_t, extern void bgp_aggregate_delete(struct bgp *bgp, const struct prefix *p, afi_t afi, safi_t safi, struct bgp_aggregate *aggregate); -extern void bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, +extern bool bgp_aggregate_route(struct bgp *bgp, const struct prefix *p, afi_t afi, safi_t safi, struct bgp_aggregate *aggregate); extern void bgp_aggregate_increment(struct bgp *bgp, const struct prefix *p, @@ -889,6 +889,7 @@ extern void bgp_path_info_free_with_caller(const char *caller, extern void bgp_path_info_add_with_caller(const char *caller, struct bgp_dest *dest, struct bgp_path_info *pi); +extern void bgp_aggregate_free(struct bgp_aggregate *aggregate); #define bgp_path_info_add(A, B) \ bgp_path_info_add_with_caller(__func__, (A), (B)) #define bgp_path_info_free(B) bgp_path_info_free_with_caller(__func__, (B)) diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index 61cff27e9e..771e48b426 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -4211,8 +4211,8 @@ static void bgp_route_map_process_update(struct bgp *bgp, const char *rmap_name, inet_ntop(bn_p->family, &bn_p->u.prefix, buf, sizeof(buf))); - bgp_aggregate_route(bgp, bn_p, afi, safi, - aggregate); + (void)bgp_aggregate_route(bgp, bn_p, afi, safi, + aggregate); } } }