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.

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

Fix memory leaks across the code #1687

Merged
merged 6 commits into from
Oct 4, 2022
Merged

Fix memory leaks across the code #1687

merged 6 commits into from
Oct 4, 2022

Conversation

s0nx
Copy link
Contributor

@s0nx s0nx commented Aug 30, 2022

Patch fixes several memory leaks:

  • A couple small ones in unit tests
  • A major one related to HTTP/2 streams processing
unreferenced object 0xffff8881394156c0 (size 704):
  comm "softirq", pid 0, jiffies 4295177155 (age 612.702s)
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 80 c4 90 34 80 88 ff ff  ...........4....
  backtrace:
    [<0000000035bfc8aa>] tfw_h2_add_stream+0xbe/0x410 [tempesta_fw]
    [<00000000ba7e15f7>] tfw_h2_frame_recv+0x3140/0x64f0 [tempesta_fw]
    [<000000004e5a3637>] ss_skb_process+0x3a3/0x5e0 [tempesta_fw]
    [<00000000aedeaca5>] tfw_h2_frame_process+0x16d/0x1200 [tempesta_fw]
    [<00000000e66f86b4>] tfw_connection_recv+0x100/0x1d0 [tempesta_fw]
    [<000000003ca45a93>] tfw_tls_connection_recv+0x538/0xbb0 [tempesta_fw]
    [<00000000b7f25f34>] ss_tcp_process_data+0x5d9/0xd90 [tempesta_fw]
    [<000000004dcf244d>] ss_tcp_data_ready+0xc4/0x1c0 [tempesta_fw]
    [<00000000832ba38a>] tcp_data_queue+0x1669/0x4cb0
    [<000000001c743974>] tcp_rcv_established+0x6ae/0x1d10
    [<00000000a6c79ebb>] tcp_v4_do_rcv+0x4fa/0x750
    [<0000000035a2b47c>] tcp_v4_rcv+0x2588/0x3530
    [<00000000158df7d6>] ip_protocol_deliver_rcu+0x6a/0x550
    [<0000000085c0e07f>] ip_local_deliver_finish+0x1a4/0x250
    [<000000006d5a9809>] ip_local_deliver+0x246/0x2a0
    [<00000000d925252d>] ip_rcv+0x15a/0x180
unreferenced object 0xffff888139416d80 (size 704):

  • Potential leak during http chains configuration

@s0nx s0nx requested review from krizhanovsky and const-t August 30, 2022 16:13
@s0nx s0nx self-assigned this Aug 30, 2022
@s0nx s0nx linked an issue Aug 30, 2022 that may be closed by this pull request
@s0nx s0nx marked this pull request as draft August 30, 2022 16:13
@s0nx s0nx linked an issue Aug 30, 2022 that may be closed by this pull request
@s0nx s0nx force-pushed the pv-mem-leak-fix branch 2 times, most recently from 58ff68d to 6b7fa4a Compare September 12, 2022 13:13
@s0nx s0nx marked this pull request as ready for review September 12, 2022 20:06
@s0nx
Copy link
Contributor Author

s0nx commented Sep 12, 2022

At the moment kmemleak still reports that several objects have leaked (http chains configuration):

unreferenced object 0xffff888123797400 (size 512):
  comm "sysctl", pid 71793, jiffies 4303451488 (age 6533.964s)
  hex dump (first 32 bytes):
    00 c5 20 3a 81 88 ff ff 34 00 00 00 00 00 00 00  .. :....4.......
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000000b8de27b>] tfw_cfgop_http_rule+0x95d/0x1990 [tempesta_fw]
    [<00000000ffef506a>] spec_handle_entry+0xe8/0x170 [tempesta_fw]
    [<000000008cc20cae>] tfw_cfg_handle_children+0x2a3/0x440 [tempesta_fw]
    [<00000000ffef506a>] spec_handle_entry+0xe8/0x170 [tempesta_fw]
    [<0000000012d943b2>] tfw_cfg_parse_mods+0x2c1/0x3d0 [tempesta_fw]
    [<00000000a3aaf719>] tfw_cfg_parse+0x78/0xb0 [tempesta_fw]
    [<000000005e3599a3>] tfw_ctlfn_state_io+0x1bd/0x5d0 [tempesta_fw]
    [<00000000fcb08e9e>] proc_sys_call_handler+0x293/0x420
    [<00000000cb31df24>] new_sync_write+0x364/0x5e0
    [<00000000446deaaa>] vfs_write+0x4d5/0x710
    [<00000000c8ee35ff>] ksys_write+0xdd/0x1a0
    [<00000000f9139a8e>] do_syscall_64+0x33/0x80
    [<00000000bb27a0eb>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
unreferenced object 0xffff88813a20c500 (size 64):

I've rechecked http chains configuration code and seems like these are false-positives. spec_cleanup() and the actual cleanup handler (tfw_cfgop_rules_cleanup()) deallocates all the objects correctly. More than that if one would unload tempesta_fw before the kmemleak scan, there won't be any leaks reported.

h2load test (h2load https://f35tfw.local -t 2 -c 1000 -D 60) passes without any issues.

Copy link
Contributor

@const-t const-t left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

The fixes look good. But please collect perf data for h2load from #1677 and let's have a look onto the streams allocation and freeing routines. It'd be good to use at least 8 CPU VM for the tests, more is better.

s0nx added 6 commits October 4, 2022 15:50
Signed-off-by: Petr Vyazovik <xen@f-m.fm>
Signed-off-by: Petr Vyazovik <xen@f-m.fm>
Signed-off-by: Petr Vyazovik <xen@f-m.fm>
Signed-off-by: Petr Vyazovik <xen@f-m.fm>
Erasing RB tree node might cause tree rebalancing and is not allowed while traversing the tree in post-order

Signed-off-by: Petr Vyazovik <xen@f-m.fm>
Signed-off-by: Petr Vyazovik <xen@f-m.fm>
@s0nx s0nx force-pushed the pv-mem-leak-fix branch from be591c6 to 8a1a794 Compare October 4, 2022 11:51
@krizhanovsky krizhanovsky merged commit 853937f into master Oct 4, 2022
@krizhanovsky krizhanovsky deleted the pv-mem-leak-fix branch October 4, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOM on h2load Possible memory leaks
3 participants