Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ospfd: fix deferred shutdown handling #14870

Merged
merged 2 commits into from
Dec 5, 2023
Merged

ospfd: fix deferred shutdown handling #14870

merged 2 commits into from
Dec 5, 2023

Conversation

rwestphal
Copy link
Member

Fixes #14855

@donaldsharp
Copy link
Member 

ospfd: 
  ## showing active allocations in memory group libfrr
ospfd:     Link List                     :      2 *         40
ospfd: 
  ## showing active allocations in memory group logging subsystem
ospfd: 
  ## showing active allocations in memory group ospfd

looks much much better.

@rwestphal rwestphal force-pushed the ospfd-shutdown-leaks branch from 4027970 to f87d512 Compare November 24, 2023 00:48
@rwestphal
Copy link
Member Author

looks much much better.

I've just pushed an update clearing those two remaining lists that I missed in my first iteration :)

ton31337
ton31337 approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@donaldsharp
Copy link
Member

```### AddressSanitizer error in topotest test_ospf_gr_helper1.py, test `teardown_module`, router `r1`

ERROR: AddressSanitizer: heap-use-after-free on address 0x6040000395b0 at pc 0x564cf03999a5 bp 0x7ffccc24aaf0 sp 0x7ffccc24aae0
READ of size 8 at 0x6040000395b0 thread T0
    #0 0x564cf03999a4 in free_opaque_info_per_type ospfd/ospf_opaque.c:585
    #1 0x564cf0399c8f in ospf_delete_opaque_functab ospfd/ospf_opaque.c:428
    #2 0x564cf044ac36 in ospf_gr_helper_stop ospfd/ospf_gr_helper.c:216
    #3 0x564cf0448121 in ospf_terminate ospfd/ospfd.c:634
    #4 0x564cf035573e in sigint ospfd/ospf_main.c:91
    #5 0x7fa75ec8d60a in frr_sigevent_process lib/sigevent.c:115
    #6 0x7fa75ecb9146 in event_fetch lib/event.c:1745
    #7 0x7fa75ebfeddd in frr_run lib/libfrr.c:1213
    #8 0x564cf0355b49 in main ospfd/ospf_main.c:252
    #9 0x7fa75e217c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
    #10 0x564cf03554a9 in _start (/usr/lib/frr/ospfd+0x1814a9)

0x6040000395b0 is located 32 bytes inside of 40-byte region [0x604000039590,0x6040000395b8)
freed by thread T0 here:
    #0 0x7fa75f21d7a8 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xde7a8)
    #1 0x7fa75ec1c579 in qfree lib/memory.c:130
    #2 0x564cf0399b1c in free_opaque_info_per_type ospfd/ospf_opaque.c:617
    #3 0x564cf0399bc8 in free_opaque_info_per_type_del ospfd/ospf_opaque.c:623
    #4 0x7fa75ec00604 in list_delete_all_node lib/linklist.c:333
    #5 0x7fa75ec006dd in list_delete lib/linklist.c:343
    #6 0x564cf0397d7e in ospf_opaque_type9_lsa_term ospfd/ospf_opaque.c:142
    #7 0x564cf036a3b7 in ospf_if_free ospfd/ospf_interface.c:342
    #8 0x564cf0446734 in ospf_finish_final ospfd/ospfd.c:730
    #9 0x564cf0448000 in ospf_deferred_shutdown_check ospfd/ospfd.c:614
    #10 0x564cf0448000 in ospf_finish ospfd/ospfd.c:668
    #11 0x564cf03e9636 in no_router_ospf ospfd/ospf_vty.c:242
    #12 0x7fa75eb9eb51 in cmd_execute_command_real lib/command.c:978
    #13 0x7fa75eb9f015 in cmd_execute_command lib/command.c:1036
    #14 0x7fa75eb9f494 in cmd_execute lib/command.c:1203
    #15 0x7fa75ecc7cd5 in vty_command lib/vty.c:594
    #16 0x7fa75ecc8180 in vty_execute lib/vty.c:1357
    #17 0x7fa75ecd0a51 in vtysh_read lib/vty.c:2365
    #18 0x7fa75ecbba14 in event_call lib/event.c:1974
    #19 0x7fa75ebfedd2 in frr_run lib/libfrr.c:1214
    #20 0x564cf0355b49 in main ospfd/ospf_main.c:252
    #21 0x7fa75e217c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

previously allocated by thread T0 here:
    #0 0x7fa75f21dd28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
    #1 0x7fa75ec1bbdf in qcalloc lib/memory.c:105
    #2 0x564cf0399d77 in register_opaque_info_per_type ospfd/ospf_opaque.c:534
    #3 0x564cf039a4b4 in register_opaque_lsa ospfd/ospf_opaque.c:732
    #4 0x564cf039a4b4 in ospf_opaque_lsa_install ospfd/ospf_opaque.c:1571
    #5 0x564cf0383a8e in ospf_lsa_install ospfd/ospf_lsa.c:3102
    #6 0x564cf039c454 in ospf_opaque_self_originated_lsa_received ospfd/ospf_opaque.c:2131
    #7 0x564cf03ad7eb in ospf_ls_upd ospfd/ospf_packet.c:1876
    #8 0x564cf03ad7eb in ospf_read_helper ospfd/ospf_packet.c:2896
    #9 0x564cf03ad7eb in ospf_read ospfd/ospf_packet.c:2927
    #10 0x7fa75ecbba14 in event_call lib/event.c:1974
    #11 0x7fa75ebfedd2 in frr_run lib/libfrr.c:1214
    #12 0x564cf0355b49 in main ospfd/ospf_main.c:252
    #13 0x7fa75e217c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)```

@rwestphal
Copy link
Member Author

@donaldsharp #14886 fixes a problem in the opaque LSA code responsible for this heap-use-after-free. Once that PR is merged, I'll rebase this one and the CI should pass.

riw777
riw777 approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@rwestphal
ospfd: improve memory cleanup during shutdown
37f861f 
* On ospf_terminate(), proceed to clear the ospfd global variables even
  when no OSPF instance is configured
* Remove double call to route_map_finish()
* Call ospf_opaque_term() to clear the opaque LSA infrastructure
* Clear the `OspfRI.area_info` and `om->ospf` global lists.

Signed-off-by: Renato Westphal <[email protected]>
@rwestphal
ospfd: fix deferred shutdown handling
3e2f053 
The ospfd cleanup code is relatively complicated given the need to
appropriately handle the "max-metric router-lsa on-shutdown (5-100)"
command. When that command is configured and an OSPF instance is
unconfigured, the removal of the instance should be deferred to allow
other routers sufficient time to find alternate paths before the
local Router-LSAs are flushed. When ospfd is killed, however, deferred
shutdown shouldn't take place and all instances should be cleared
immediately.

This commit fixes a problem where ospf_deferred_shutdown_finish()
was prematurely exiting the daemon when no instances were left,
inadvertently preventing ospf_terminate() from clearing the ospfd
global variables. Additionally, the commit includes code refactoring
to enhance readability and maintainability.

Fixes FRRouting#14855.

Signed-off-by: Renato Westphal <[email protected]>
@rwestphal rwestphal force-pushed the ospfd-shutdown-leaks branch from f87d512 to 3e2f053 Compare December 1, 2023 12:25
@github-actions github-actions bot removed the conflicts label Dec 1, 2023
@rwestphal
Copy link
Member Author

CI:rerun

1 similar comment
@rwestphal
Copy link
Member Author

CI:rerun

@riw777 riw777 merged commit 03e3b34 into FRRouting:master Dec 5, 2023
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ton31337 ton31337 ton31337 approved these changes

@riw777 riw777 riw777 approved these changes

Assignees
No one assigned
Labels
bugfix master ospf size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OSPF does not fully free up MTYPE memory on shutdown as it should
4 participants
@rwestphal @donaldsharp @riw777 @ton31337