-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bgpd: fix various memory leaks in dampening #9217
Conversation
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9U18AMD64-20615/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO9U18AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9U18I386-20615/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO9U18I386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 arm8 part 3: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 3: No useful log foundTopotests Ubuntu 18.04 amd64 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP3U1804AMD64-20615/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 3:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TP3U1804AMD64/ErrorLog/log_topotests.txt Topotests debian 10 amd64 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3DEB10AMD64-20615/test Topology Tests failed for Topotests debian 10 amd64 part 3:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO3DEB10AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3U18I386-20615/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 3:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO3U18I386/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
Current code is a complete misuse of SLIST structure. Instead of just adding a SLIST_ENTRY to struct bgp_damp_info, it allocates a separate structure to be a node in the list. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
bdi->path is never NULL, therefore the structure was never freed. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
bgp_damp_config, afi and safi are never used. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When bgp_damp_info_free is called from bgp_route.c, we were never deleting the BDI from the list. Move the deletion inside bgp_damp_info_free to cover all cases. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
This code always used double-linked list before this rework in 8.0 that introduced all these crashes and memory leaks. Using single-linked list is actually a performance regression, because there are frequent removes here and single-linked list obviously handles removes much worse. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-20647/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20647/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20653/ This is a comment from an automated CI system. |
@Mergifyio backport stable/8.0 |
Command
|
Command
|
Overall this PR aims to fix multiple memory leaks in the dampening code. All the leaks are regressions in 8.0 and I want this code to be backported even though there are a couple of cleanup-only commits. It is difficult to fix the leaks without doing the cleanup.
Please, check individual commits for descriptions.
The last commit changes the SLIST to a LIST. The dampening code was always using a double-linked list before the rework in 8.0 that introduced all those crashes and memory leaks. As there are frequent removals from the list, using a single-linked list is actually a real performance regression so changing it back to double-linked list should be considered as a fix.