Bgp filter fun (backport #15466) #15550
Closed
Conversation
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
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
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 <sharpd@nvidia.com> (cherry picked from commit e613e12)
If unsuppress-map is setup for outgoing peers, consider that policy is being applied as for RFC 8212. Signed-off-by: Donald Sharp <sharpd@nvidia.com> (cherry picked from commit 6814401)
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> (cherry picked from commit addff17) # Conflicts: # bgpd/bgp_route.c
|
Cherry-pick of addff17 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Merged
|
Closing this, created a manual backport here #15568. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
see individual commits
a) Allows unsuppress-maps to be part of the filtering system that would allow outgoing data to a peer.
b) Fix memory leak associated with late decision not to send data to peer in bgp
This is an automatic backport of pull request #15466 done by Mergify.