[FRR]Adding fix for memory leak seen with BGP community #18272
Conversation
| - if (!bgp_check_withdrawal(bgp, dest)) | ||
| + if (!bgp_check_withdrawal(bgp, dest)) { | ||
| + struct attr *adv_attr = | ||
| + bgp_attr_intern(&attr); |
There was a problem hiding this comment.
hi @dgsudharsan by comparing the diff, we think there is one issue in the patch. after line 186 else, we think in that part, we also need a bgp_attr_flush(attr). because the leaked community allocated from subgroup_announce_check function, after this function, need to consider the flush. but in this patch at line 715, it only flush after !bgp_check_withdrawal, it may not enough. Could you help to check below snippet from Liping's patch?
There was a problem hiding this comment.
Based on our understanding and test results, we still believe that using bgp_attr_unintern here is the better solution.
After shutdown all BGP neighbors, it would be more reasonable if the output of "show bgp community-info" is empty.
There was a problem hiding this comment.
Hi @donaldsharp Can you please check this?
| bgp_attr_intern(&attr); | ||
|
|
||
| - bgp_adj_out_set_subgroup( | ||
| + if (!bgp_adj_out_set_subgroup( |
There was a problem hiding this comment.
@dgsudharsan as we discussed,
- No need to check the return value of this function, either true or false, we need to free.
- In the line 376, to replace bgp_attr_flush with bgp_attr_unintern, which is coupled with bgp_attr_intern.
- In the else branch, to add the bgp_attr_flush.
There was a problem hiding this comment.
Seems the latest patch still checking the function and no flush in the else patch, Could you please help to double confirm, thanks a lot.
|
@StormLiangMS @lipxu Can you please review and signoff? |
Sorry for late response, will verify it in these two days, and let you know if any update, thanks. |
| + 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))) |
There was a problem hiding this comment.
Hi, @dgsudharsan , seems there was UNSUPPRESS_MAP_NAME(filter) in the first commit, but removed in the final version, is it expected? thanks.
There was a problem hiding this comment.
Hi @donaldsharp can you please help answer this?
There was a problem hiding this comment.
it is missing, but the fix it came from 6814401c47b948a50 is a fix that is not related to the actual memory leak. It is related to allowing a unsuppress-map as a valid outgoing policy. Where it was not allowed before.
|
For information, why does it require a specific patch instead of Sonic that could consume an official maintenance branch of FRR ? |
SONiC's methodology is to consume official release tags from FRR and then integrate any required fixes on top of it. In this case the last updated version was 8.5.1. This patch was made created only recently and can only be added as patch on top of 8.5.1. Later when SONiC picks in a stable official version of FRR which contains the patch, the patch would be removed. |
|
@lipxu @StormLiangMS Can you please help prioritize and close the review? |
|
Thank you @lipxu for testing and approving this PR. @StormLiangMS Can you please help to merge this PR? I will create one for 202311 and will have this patch for master in 8.5.4 upgrade |
|
hi @dgsudharsan do you have PR for 202311? |
|
Hi @StormLiangMS Here is PR for 202311 #18606 |
Porting PR #18272 to 202311. Why I did it Porting fix for FRRouting/frr#15459 Adding patch for PR FRRouting/frr#15466 Work item tracking Microsoft ADO (number only): How I did it Ported the fix FRRouting/frr#15466 to 8.5.1 How to verify it Running the test_stress_route and ensure no memory leak
Why I did it
Porting fix for FRRouting/frr#15459
Adding patch for PR FRRouting/frr#15466
Work item tracking
How I did it
Ported the fix FRRouting/frr#15466 to 8.5.1
How to verify it
Running the test_stress_route and ensure no memory leak
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)