From 836f1cfc10776182aed664055953aeddea5d9fff Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 13 Mar 2024 10:26:58 -0400 Subject: [PATCH 1/3] bgpd: Ensure that the correct aspath is free'd Currently in subgroup_default_originate the attr.aspath is set in bgp_attr_default_set, which hashs the aspath and creates a refcount for it. If this is a withdraw the subgroup_announce_check and bgp_adj_out_set_subgroup is called which will intern the attribute. This will cause the the attr.aspath to be set to a new value finally at the bottom of the function it intentionally uninterns the aspath which is not the one that was created for this function. This reduces the other aspath's refcount by 1 and if a clear bgp * is issued fast enough the aspath for that will be removed and the system will crash. Signed-off-by: Donald Sharp (cherry picked from commit e613e12f12b9274520ff2a8650496bdeb97e011c) --- bgpd/bgp_updgrp_adv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index ccbb23ebb4d6..e4ef5f449b36 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -831,6 +831,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) struct bgp *bgp; struct attr attr; struct attr *new_attr = &attr; + struct aspath *aspath; struct prefix p; struct peer *from; struct bgp_dest *dest; @@ -868,6 +869,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) /* make coverity happy */ assert(attr.aspath); + aspath = attr.aspath; attr.med = 0; attr.flag |= ATTR_FLAG_BIT(BGP_ATTR_MULTI_EXIT_DISC); @@ -1023,7 +1025,7 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) } } - aspath_unintern(&attr.aspath); + aspath_unintern(&aspath); } /* From 52f4e550e13cfd23c23578e44e17f8f37472c25e Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 2 Mar 2024 09:42:30 -0500 Subject: [PATCH 2/3] bgpd: Include unsuppress-map as a valid outgoing policy If unsuppress-map is setup for outgoing peers, consider that policy is being applied as for RFC 8212. Signed-off-by: Donald Sharp (cherry picked from commit 6814401c47b948a50c6db93d60ac793a8cdce45f) --- bgpd/bgp_route.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index d9f1eda3e21b..442bba1539df 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -5958,10 +5958,10 @@ bool bgp_outbound_policy_exists(struct peer *peer, struct bgp_filter *filter) if (peer->sort == BGP_PEER_IBGP) return true; - if (peer->sort == BGP_PEER_EBGP - && (ROUTE_MAP_OUT_NAME(filter) || PREFIX_LIST_OUT_NAME(filter) - || FILTER_LIST_OUT_NAME(filter) - || DISTRIBUTE_OUT_NAME(filter))) + if (peer->sort == BGP_PEER_EBGP && + (ROUTE_MAP_OUT_NAME(filter) || PREFIX_LIST_OUT_NAME(filter) || + FILTER_LIST_OUT_NAME(filter) || DISTRIBUTE_OUT_NAME(filter) || + UNSUPPRESS_MAP_NAME(filter))) return true; return false; } From 20bd3821d3b77840f397802f28fb54cbb7c7a76c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sat, 2 Mar 2024 09:50:38 -0500 Subject: [PATCH 3/3] bgpd: Ensure community data is freed in some cases. Customer has this valgrind trace: Direct leak of 2829120 byte(s) in 70728 object(s) allocated from: 0 in community_new ../bgpd/bgp_community.c:39 1 in community_uniq_sort ../bgpd/bgp_community.c:170 2 in route_set_community ../bgpd/bgp_routemap.c:2342 3 in route_map_apply_ext ../lib/routemap.c:2673 4 in subgroup_announce_check ../bgpd/bgp_route.c:2367 5 in subgroup_process_announce_selected ../bgpd/bgp_route.c:2914 6 in group_announce_route_walkcb ../bgpd/bgp_updgrp_adv.c:199 7 in hash_walk ../lib/hash.c:285 8 in update_group_af_walk ../bgpd/bgp_updgrp.c:2061 9 in group_announce_route ../bgpd/bgp_updgrp_adv.c:1059 10 in bgp_process_main_one ../bgpd/bgp_route.c:3221 11 in bgp_process_wq ../bgpd/bgp_route.c:3221 12 in work_queue_run ../lib/workqueue.c:282 The above leak detected by valgrind was from a screenshot so I copied it by hand. Any mistakes in line numbers are purely from my transcription. Additionally this is against a slightly modified 8.5.1 version of FRR. Code inspection of 8.5.1 -vs- latest master shows the same problem exists. Code should be able to be followed from there to here. What is happening: There is a route-map being applied that modifes the outgoing community to a peer. This is saved in the attr copy created in subgroup_process_announce_selected. This community pointer is not interned. So the community->refcount is still 0. Normally when a prefix is announced, the attr and the prefix are placed on a adjency out structure where the attribute is interned. This will cause the community to be saved in the community hash list as well. In a non-normal operation when the decision to send is aborted after the route-map application, the attribute is just dropped and the pointer to the community is just dropped too, leading to situations where the memory is leaked. The usage of bgp suppress-fib would would be a case where the community is caused to be leaked. Additionally the previous commit where an unsuppress-map is used to modify the outgoing attribute but since unsuppress-map was not considered part of outgoing policy the attribute would be dropped as well. This pointer drop also extends to any dynamically allocated memory saved by the attribute pointer that was not interned yet as well. So let's modify the return case where the decision is made to not send the prefix to the peer to always just flush the attribute to ensure memory is not leaked. Fixes: #15459 Signed-off-by: Donald Sharp (cherry picked from commit addff17a555240a4ccb5d0c5733a780256837651) --- bgpd/bgp_conditional_adv.c | 5 +++-- bgpd/bgp_route.c | 26 +++++++++++++++----------- bgpd/bgp_updgrp.h | 2 +- bgpd/bgp_updgrp_adv.c | 37 ++++++++++++++++++++----------------- 4 files changed, 39 insertions(+), 31 deletions(-) diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c index 6ed0dd797b5b..2d96b444b6c5 100644 --- a/bgpd/bgp_conditional_adv.c +++ b/bgpd/bgp_conditional_adv.c @@ -122,8 +122,9 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi, if (update_type == UPDATE_TYPE_ADVERTISE && subgroup_announce_check(dest, pi, subgrp, dest_p, &attr, &advmap_attr)) { - bgp_adj_out_set_subgroup(dest, subgrp, &attr, - pi); + if (!bgp_adj_out_set_subgroup(dest, subgrp, + &attr, pi)) + bgp_attr_flush(&attr); } else { /* If default originate is enabled for * the peer, do not send explicit diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 442bba1539df..7ac14f593e8c 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2956,7 +2956,7 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, { const struct prefix *p; struct peer *onlypeer; - struct attr attr; + struct attr attr = { 0 }, *pattr = &attr; struct bgp *bgp; bool advertise; @@ -2984,26 +2984,30 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, advertise = bgp_check_advertise(bgp, dest, safi); if (selected) { - if (subgroup_announce_check(dest, selected, subgrp, p, &attr, + if (subgroup_announce_check(dest, selected, subgrp, p, pattr, NULL)) { /* Route is selected, if the route is already installed * in FIB, then it is advertised */ if (advertise) { if (!bgp_check_withdrawal(bgp, dest, safi)) { - struct attr *adv_attr = - bgp_attr_intern(&attr); - - bgp_adj_out_set_subgroup(dest, subgrp, - adv_attr, - selected); - } else + if (!bgp_adj_out_set_subgroup(dest, + subgrp, + pattr, + selected)) + bgp_attr_flush(pattr); + } else { bgp_adj_out_unset_subgroup( dest, subgrp, 1, addpath_tx_id); - } - } else + bgp_attr_flush(pattr); + } + } else + bgp_attr_flush(pattr); + } else { bgp_adj_out_unset_subgroup(dest, subgrp, 1, addpath_tx_id); + bgp_attr_flush(pattr); + } } /* If selected is NULL we must withdraw the path using addpath_tx_id */ diff --git a/bgpd/bgp_updgrp.h b/bgpd/bgp_updgrp.h index 0d866f3be7e4..3db079b928b3 100644 --- a/bgpd/bgp_updgrp.h +++ b/bgpd/bgp_updgrp.h @@ -442,7 +442,7 @@ extern struct bgp_adj_out *bgp_adj_out_alloc(struct update_subgroup *subgrp, extern void bgp_adj_out_remove_subgroup(struct bgp_dest *dest, struct bgp_adj_out *adj, struct update_subgroup *subgrp); -extern void bgp_adj_out_set_subgroup(struct bgp_dest *dest, +extern bool bgp_adj_out_set_subgroup(struct bgp_dest *dest, struct update_subgroup *subgrp, struct attr *attr, struct bgp_path_info *path); diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index e4ef5f449b36..077f04fa6dc4 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -497,7 +497,7 @@ bgp_advertise_clean_subgroup(struct update_subgroup *subgrp, return next; } -void bgp_adj_out_set_subgroup(struct bgp_dest *dest, +bool bgp_adj_out_set_subgroup(struct bgp_dest *dest, struct update_subgroup *subgrp, struct attr *attr, struct bgp_path_info *path) { @@ -517,7 +517,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, bgp = SUBGRP_INST(subgrp); if (DISABLE_BGP_ANNOUNCE) - return; + return false; /* Look for adjacency information. */ adj = adj_lookup( @@ -533,7 +533,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, bgp_addpath_id_for_peer(peer, afi, safi, &path->tx_addpath)); if (!adj) - return; + return false; subgrp->pscount++; } @@ -572,7 +572,7 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, * will never be able to coalesce the 3rd peer down */ subgrp->version = MAX(subgrp->version, dest->version); - return; + return false; } if (adj->adv) @@ -620,6 +620,8 @@ void bgp_adj_out_set_subgroup(struct bgp_dest *dest, bgp_adv_fifo_add_tail(&subgrp->sync->update, adv); subgrp->version = MAX(subgrp->version, dest->version); + + return true; } /* The only time 'withdraw' will be false is if we are sending @@ -829,7 +831,7 @@ void subgroup_announce_route(struct update_subgroup *subgrp) void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) { struct bgp *bgp; - struct attr attr; + struct attr attr = { 0 }; struct attr *new_attr = &attr; struct aspath *aspath; struct prefix p; @@ -970,18 +972,19 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) if (dest) { for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { - if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) - if (subgroup_announce_check( - dest, pi, subgrp, - bgp_dest_get_prefix(dest), - &attr, NULL)) { - struct attr *default_attr = - bgp_attr_intern(&attr); - - bgp_adj_out_set_subgroup( - dest, subgrp, - default_attr, pi); - } + if (!CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) + continue; + + if (subgroup_announce_check(dest, pi, subgrp, + bgp_dest_get_prefix( + dest), + &attr, NULL)) { + if (!bgp_adj_out_set_subgroup(dest, + subgrp, + &attr, pi)) + bgp_attr_flush(&attr); + } else + bgp_attr_flush(&attr); } bgp_dest_unlock_node(dest); }