Fix ksymbols mem consumption #4095

yanivagman · 2024-06-02T09:44:31Z

1. Explain what the PR does

fix: add missing dependency for hooked_syscalls event
The hooked_syscalls event requires CAP_SYSLOG in order to
refresh its symbol table when reading /proc/kallsyms.
Add this missing dependency.

feat(ksymbols): restore lazy ksyms implementation
Use modified ksymbols implementation. The new implementation may take
a list of required symbols and addresses to track. If the list is given,
symbol scanning will only save those symbols or addresses which were
given in the list. If a new symbol is queried, then a rescan is needed.

Refactor tracee initialization to find all necessary symbols to track
ahead of runtime.

2. Explain how to test it

3. Other comments

NDStrahilevitz

Note the last comment in particular, that is an oversight I made in the original implementation.

pkg/ebpf/probes/probes.go

pkg/ebpf/probes/trace.go

NDStrahilevitz · 2024-06-03T09:30:14Z

pkg/ebpf/tracee.go

@@ -862,6 +890,104 @@ func (t *Tracee) newConfig(cfg *policy.PoliciesConfig, version uint16) *Config {
 	}
 }

+func (t *Tracee) initKsymTableRequiredSyms() error {


We need to make sure we always initialize the text segment in relevant events. Perhaps we may want it as a symbol always present. WDYT?

Shouldn't this be handled by event dependencies?

It should, that's why the first part was about double-checking that.

pkg/utils/environment/kernel_symbols.go

geyslan · 2024-06-03T11:50:02Z

Tested and the args from hooked_syscall are being populated correctly.

Related to the memory footprint this clearly brings a smaller initial mem consumption (RSS): 249496 (PR) < 456036 (main)

This PR

ps -p `pgrep tracee` -o pid,vsz,rss,cmd
    PID    VSZ   RSS CMD
 412217 4201868 249496 ./dist/tracee -e hooked_syscall

main branch

ps -p `pgrep tracee` -o pid,vsz,rss,cmd
    PID    VSZ   RSS CMD
 423066 4918396 456036 ./dist/tracee -e hooked_syscall

geyslan · 2024-06-03T12:50:51Z

@NDStrahilevitz I addressed some of your comments. The remainder is left for further consideration.

NDStrahilevitz · 2024-06-03T12:55:46Z

@NDStrahilevitz I addressed some of your comments. The remainder is left for further consideration.

My last comment still seems rather critical IMO, @yanivagman WDYT? I fear that if we don't address it we will introduce a performance regression while a hooked_symbol event is triggered.

Dismissing for +1

NDStrahilevitz

LGTM once e2e fixes are resolved.

The hooked_syscalls and do_init_module events require CAP_SYSLOG in order to refresh its symbol table when reading /proc/kallsyms. Add these missing dependencies.

geyslan · 2024-06-03T23:15:50Z

@yanivagman @NDStrahilevitz ignore the wip (3cacd03). It can be overridden.

NDStrahilevitz · 2024-06-04T18:25:17Z

pkg/ebpf/c/tracee.bpf.c

        kernel_new_mod_t new_mod = {.insert_time = insert_time};
        u64 mod_addr = (u64) mod;
        // new_module_map - must be after the module is added to modules list,
        // otherwise there's a risk for race condition
-        bpf_map_update_elem(&new_module_map, &mod_addr, &new_mod, BPF_ANY);


@OriGlassman Proposed this internally afaik, should we keep this?

Use modified ksymbols implementation. The new implementation may take a list of required symbols and addresses to track. If the list is given, symbol scanning will only save those symbols or addresses which were given in the list. If a new symbol is queried, then a rescan is needed. Refactor tracee initialization to find all necessary symbols to track ahead of runtime. Co-authored-by: Geyslan Gregório <geyslan@gmail.com> Co-authored-by: Yaniv Agman <yanivagman@gmail.com>

This commit refactors the refresh logic of KernelSymbolTable to be sequential, removing the use of goroutines and channels. The decision to simplify the code was made for the following reasons: 1. Simplicity: The sequential approach simplifies the codebase, making it easier to understand, maintain, and debug. 2. Less Relevant Concurrency: With the introduction of "required symbols", the need for concurrency has been reduced. We now skip memory allocation for symbols that are not required, which constitutes the majority of the symbols in /proc/kallsyms.

github-actions bot assigned yanivagman Jun 2, 2024

github-actions bot added area/ebpf area/testing labels Jun 2, 2024

yanivagman force-pushed the fix_ksymbols_mem_consumption_2 branch from fa45261 to 16e19b5 Compare June 2, 2024 14:00

github-actions bot added the area/events label Jun 2, 2024

yanivagman force-pushed the fix_ksymbols_mem_consumption_2 branch from 16e19b5 to b210980 Compare June 2, 2024 14:24

This was referenced Jun 2, 2024

Fix ksymbols mem consumption #4093

Closed

[backport 0.21.0] Fix ksymbols mem consumption backport #4100

Merged

yanivagman requested review from geyslan and NDStrahilevitz June 2, 2024 18:40

NDStrahilevitz previously requested changes Jun 3, 2024

View reviewed changes

geyslan force-pushed the fix_ksymbols_mem_consumption_2 branch from b210980 to c111214 Compare June 3, 2024 12:48

geyslan force-pushed the fix_ksymbols_mem_consumption_2 branch 2 times, most recently from 0636aa9 to ef12be7 Compare June 3, 2024 16:20

NDStrahilevitz self-requested a review June 3, 2024 19:04

NDStrahilevitz approved these changes Jun 3, 2024

View reviewed changes

fix: add missing cap_syslog dependencies

d922dc5

The hooked_syscalls and do_init_module events require CAP_SYSLOG in order to refresh its symbol table when reading /proc/kallsyms. Add these missing dependencies.

yanivagman force-pushed the fix_ksymbols_mem_consumption_2 branch from ef12be7 to 6913d6b Compare June 3, 2024 19:35

yanivagman force-pushed the fix_ksymbols_mem_consumption_2 branch from 3cacd03 to c81d337 Compare June 4, 2024 11:33

NDStrahilevitz force-pushed the fix_ksymbols_mem_consumption_2 branch from 86bd02a to 29b75af Compare June 4, 2024 14:29

geyslan mentioned this pull request Jun 4, 2024

feat(ksymbols): reintroduce lazy symbol queries aquasecurity/libbpfgo#437

Closed

NDStrahilevitz force-pushed the fix_ksymbols_mem_consumption_2 branch 2 times, most recently from 2e1871a to 646578f Compare June 4, 2024 18:01

NDStrahilevitz reviewed Jun 4, 2024

View reviewed changes

NDStrahilevitz force-pushed the fix_ksymbols_mem_consumption_2 branch from 646578f to 425af51 Compare June 4, 2024 18:54

NDStrahilevitz and others added 2 commits June 4, 2024 22:40

yanivagman force-pushed the fix_ksymbols_mem_consumption_2 branch 2 times, most recently from f436739 to 3879a21 Compare June 5, 2024 10:22

yanivagman merged commit 625f393 into aquasecurity:main Jun 5, 2024
32 checks passed

yanivagman deleted the fix_ksymbols_mem_consumption_2 branch June 5, 2024 11:41

yanivagman linked an issue Jun 25, 2024 that may be closed by this pull request

Increased memory usage compared to v0.19.0 #4098

Closed

yanivagman mentioned this pull request Jun 25, 2024

Increased memory usage compared to v0.19.0 #4098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ksymbols mem consumption #4095

Fix ksymbols mem consumption #4095

yanivagman commented Jun 2, 2024 •

edited

Loading

NDStrahilevitz left a comment

NDStrahilevitz Jun 3, 2024

yanivagman Jun 3, 2024

NDStrahilevitz Jun 3, 2024

geyslan commented Jun 3, 2024 •

edited

Loading

geyslan commented Jun 3, 2024

NDStrahilevitz commented Jun 3, 2024 •

edited

Loading

NDStrahilevitz left a comment

geyslan commented Jun 3, 2024 •

edited

Loading

NDStrahilevitz Jun 4, 2024

Fix ksymbols mem consumption #4095

Fix ksymbols mem consumption #4095

Conversation

yanivagman commented Jun 2, 2024 • edited Loading

1. Explain what the PR does

2. Explain how to test it

3. Other comments

NDStrahilevitz left a comment

Choose a reason for hiding this comment

NDStrahilevitz Jun 3, 2024

Choose a reason for hiding this comment

yanivagman Jun 3, 2024

Choose a reason for hiding this comment

NDStrahilevitz Jun 3, 2024

Choose a reason for hiding this comment

geyslan commented Jun 3, 2024 • edited Loading

This PR

main branch

geyslan commented Jun 3, 2024

NDStrahilevitz commented Jun 3, 2024 • edited Loading

NDStrahilevitz left a comment

Choose a reason for hiding this comment

geyslan commented Jun 3, 2024 • edited Loading

NDStrahilevitz Jun 4, 2024

Choose a reason for hiding this comment

yanivagman commented Jun 2, 2024 •

edited

Loading

geyslan commented Jun 3, 2024 •

edited

Loading

NDStrahilevitz commented Jun 3, 2024 •

edited

Loading

geyslan commented Jun 3, 2024 •

edited

Loading