[202305] update frr patch for bgpd-Ensure-community-data-is-freed-in-some-cases #18246
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,246 @@ | ||
| From 6b4ca0f4385399c7b06491efb2b79b39ce435028 Mon Sep 17 00:00:00 2001 | ||
| From: Donald Sharp <sharpd@nvidia.com> | ||
| Date: Sat, 2 Mar 2024 09:50:38 -0500 | ||
| Subject: [PATCH] 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 <sharpd@nvidia.com> | ||
|
|
||
| update | ||
| --- | ||
| bgpd/bgp_conditional_adv.c | 5 +++-- | ||
| bgpd/bgp_route.c | 20 ++++++++++++++------ | ||
| bgpd/bgp_updgrp.h | 2 +- | ||
| bgpd/bgp_updgrp_adv.c | 35 ++++++++++++++++++++++------------- | ||
| 4 files changed, 40 insertions(+), 22 deletions(-) | ||
|
|
||
| diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c | ||
| index 4ad00ed121..89ea85ff46 100644 | ||
| --- a/bgpd/bgp_conditional_adv.c | ||
| +++ b/bgpd/bgp_conditional_adv.c | ||
| @@ -134,8 +134,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 2a1ca2b2e8..7414524fe3 100644 | ||
| --- a/bgpd/bgp_route.c | ||
| +++ b/bgpd/bgp_route.c | ||
| @@ -2917,16 +2917,24 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, | ||
| * in FIB, then it is advertised | ||
| */ | ||
| if (advertise) { | ||
| - if (!bgp_check_withdrawal(bgp, dest)) | ||
| - bgp_adj_out_set_subgroup( | ||
| - dest, subgrp, &attr, selected); | ||
| - else | ||
| + if (!bgp_check_withdrawal(bgp, dest)) { | ||
| + if (!bgp_adj_out_set_subgroup(dest, | ||
| + subgrp, | ||
| + &attr, | ||
| + selected)) | ||
| + bgp_attr_flush(&attr); | ||
| + } else { | ||
| bgp_adj_out_unset_subgroup( | ||
| dest, subgrp, 1, addpath_tx_id); | ||
| - } | ||
| - } else | ||
| + bgp_attr_flush(&attr); | ||
| + } | ||
| + } else | ||
| + bgp_attr_flush(&attr); | ||
| + } else { | ||
| bgp_adj_out_unset_subgroup(dest, subgrp, 1, | ||
| addpath_tx_id); | ||
| + bgp_attr_flush(&attr); | ||
| + } | ||
| } | ||
|
|
||
| /* 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 e27c1e7b67..b7b6aa07e9 100644 | ||
| --- a/bgpd/bgp_updgrp.h | ||
| +++ b/bgpd/bgp_updgrp.h | ||
| @@ -458,7 +458,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 af8ef751da..22af326230 100644 | ||
| --- a/bgpd/bgp_updgrp_adv.c | ||
| +++ b/bgpd/bgp_updgrp_adv.c | ||
| @@ -454,7 +454,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) | ||
| { | ||
| @@ -474,7 +474,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( | ||
| @@ -490,7 +490,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++; | ||
| } | ||
| @@ -529,7 +529,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) | ||
| @@ -576,6 +576,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 | ||
| @@ -668,7 +670,7 @@ void subgroup_announce_table(struct update_subgroup *subgrp, | ||
| { | ||
| struct bgp_dest *dest; | ||
| struct bgp_path_info *ri; | ||
| - struct attr attr; | ||
| + struct attr attr = {0}; | ||
| struct peer *peer; | ||
| afi_t afi; | ||
| safi_t safi; | ||
| @@ -715,17 +717,20 @@ void subgroup_announce_table(struct update_subgroup *subgrp, | ||
| &attr, NULL)) { | ||
| /* Check if route can be advertised */ | ||
| if (advertise) { | ||
| - if (!bgp_check_withdrawal(bgp, dest)) | ||
| - bgp_adj_out_set_subgroup( | ||
| + if (!bgp_check_withdrawal(bgp, dest)) { | ||
| + if (!bgp_adj_out_set_subgroup( | ||
| dest, subgrp, &attr, | ||
| - ri); | ||
| - else | ||
| + ri)) | ||
| + bgp_attr_flush(&attr); | ||
| + } else { | ||
| bgp_adj_out_unset_subgroup( | ||
| dest, subgrp, 1, | ||
| bgp_addpath_id_for_peer( | ||
| peer, afi, | ||
| safi_rib, | ||
| &ri->tx_addpath)); | ||
| + bgp_attr_flush(&attr); | ||
| + } | ||
| } | ||
| } else { | ||
| /* If default originate is enabled for | ||
| @@ -745,6 +750,7 @@ void subgroup_announce_table(struct update_subgroup *subgrp, | ||
| bgp_addpath_id_for_peer( | ||
| peer, afi, safi_rib, | ||
| &ri->tx_addpath)); | ||
| + bgp_attr_flush(&attr); | ||
| } | ||
| } | ||
| } | ||
| @@ -947,7 +953,7 @@ 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)) | ||
|
There was a problem hiding this comment. Can you please check the original PR for this file as well? The diff is little bit different There was a problem hiding this comment.
Yes, this diff was based on our understanding. we will sync with donaldsharp first, and push the latest update if necessary later, thanks. |
||
| + if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) { | ||
| if (subgroup_announce_check( | ||
| dest, pi, subgrp, | ||
| bgp_dest_get_prefix(dest), | ||
| @@ -956,9 +962,12 @@ void subgroup_default_originate(struct update_subgroup *subgrp, int withdraw) | ||
| bgp_attr_intern(&attr); | ||
|
|
||
| bgp_adj_out_set_subgroup( | ||
| - dest, subgrp, | ||
| - default_attr, pi); | ||
| - } | ||
| + dest, subgrp, | ||
| + default_attr, pi); | ||
| + bgp_attr_unintern(&default_attr); | ||
| + } else | ||
| + bgp_attr_flush(&attr); | ||
| + } | ||
| } | ||
| bgp_dest_unlock_node(dest); | ||
| } | ||
| -- | ||
| 2.25.1 | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please check the latest update on FRRouting/frr#15466 ? The diff has changed in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI, @dgsudharsan thanks for reviewing the code. This part of the code should be the same, the difference should be related to the difference between 202305 branch and master. thanks.