fix(userspace/libsinsp): keep event thread after execve #2212

erthalion · 2024-12-20T10:34:48Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Currently when a thread from a thread group is doing execve, we expect that the kernel will reassign its tid at the end to the group leader, and simulate this behavior in the parser accordingly. The final result is all the threads in the thread group, except the leader, are removed from the cache.

But looks like under certain circumstances it's possible to end up in a situation when the kernel is not doing the reassignment, yet the syscall ends successfully. This leads to a crash, since the parser removes the thread associated with the execve_x event, which will be accessed later during post processing -- and everything is explode in use-after-free. It's hard to reproduce artificially, but there are crash reports from the field, demonstrating the problem and confirming the patch fixes the crash. So far the issue was discovered only on ppc64le (Power10 to be more precise).

To handle this, keep the event thread in place. Note, that tid here comes from the BPF probe directly, where it's captured via bpf_get_current_task/_btf. This means that the tid is the one really reported by the kernel, so keeping it represents the current state precisely.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

github-actions · 2024-12-20T13:31:15Z

Perf diff from master - unit tests

     3.29%     -0.67%  [.] sinsp_thread_manager::find_thread
     8.04%     +0.53%  [.] sinsp_evt::get_type
    11.04%     -0.52%  [.] sinsp::next
     1.59%     +0.43%  [.] scap_event_decode_params
     5.84%     +0.43%  [.] next_event_from_file
     2.92%     -0.37%  [.] is_conversion_needed
    10.25%     +0.36%  [.] sinsp_parser::reset
     2.08%     -0.29%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     1.52%     -0.23%  [.] libsinsp::sinsp_suppress::process_event
     0.78%     -0.22%  [.] libsinsp::events::is_unknown_event

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0280         +0.0280           146           150           146           150
BM_sinsp_split_median                                          +0.0265         +0.0265           146           150           146           150
BM_sinsp_split_stddev                                          +0.3178         +0.3189             1             1             1             1
BM_sinsp_split_cv                                              +0.2819         +0.2830             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0936         +0.0936            55            61            55            61
BM_sinsp_concatenate_paths_relative_path_median                +0.0914         +0.0914            56            61            56            61
BM_sinsp_concatenate_paths_relative_path_stddev                -0.6214         -0.6215             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.6538         -0.6539             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0165         +0.0165            23            24            23            24
BM_sinsp_concatenate_paths_empty_path_median                   +0.0164         +0.0164            23            24            23            24
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.0398         -0.0360             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.0554         -0.0516             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.1264         +0.1264            56            63            56            63
BM_sinsp_concatenate_paths_absolute_path_median                +0.1247         +0.1247            56            63            56            63
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.7981         -0.7993             1             0             1             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.8208         -0.8218             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0129         +0.0129           390           395           390           395
BM_sinsp_split_container_image_median                          +0.0107         +0.0107           390           395           390           395
BM_sinsp_split_container_image_stddev                          +0.4410         +0.4422             3             4             3             4
BM_sinsp_split_container_image_cv                              +0.4227         +0.4238             0             0             0             0

codecov · 2024-12-20T13:35:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.43%. Comparing base (5b95bc8) to head (52b88b0).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2212   +/-   ##
=======================================
  Coverage   75.42%   75.43%           
=======================================
  Files         265      265           
  Lines       34048    34044    -4     
  Branches     5803     5801    -2     
=======================================
- Hits        25681    25680    -1     
+ Misses       8367     8364    -3

Flag	Coverage Δ
libsinsp	`75.43% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

FedeDP

Thanks, great fix!
/approve

poiana · 2024-12-20T14:29:50Z

LGTM label has been added.

Git tree hash: ba32ab7aa14c9782719acd3037e5b4e78fc68c8d

FedeDP · 2024-12-20T14:29:53Z

/milestone 0.20.0

FedeDP · 2024-12-20T14:30:43Z

Thank you @erthalion! I updated the PR body to be compliant with our semantic commit message (otherwise it can't be parsed by the release workflow :D )!

LucaGuerra · 2024-12-20T15:11:28Z

userspace/libsinsp/parsers.cpp

+			 *
+			 * Also make sure the thread to be removed is not the one
+			 * associated with the event. Under normal conditions this
+			 * should not happen, since the kernel will reassing tid before


Suggested change

* should not happen, since the kernel will reassing tid before

* should not happen, since the kernel will reassign tid before

(sorry about the suggestion for a typo in comment but it sounded bad 😅)

Thanks! In fact, I think the word with typo perfectly describes what kernel does, when the tid doesn't change :)

Currently when a thread from a thread group is doing execve, we expect that the kernel will reassign its tid at the end to the group leader, and simulate this behavior in the parser accordingly. The final result is all the threads in the thread group, except the leader, are removed from the cache. But looks like under certain circumstances it's possible to end up in a situation when the kernel is not doing the reassignment, yet the syscall ends successfully. This leads to a crash, since the parser removes the thread associated with the execve_x event, which will be accessed later during post processing -- and everything is expose in use-after-free. It's hard to reproduce artificially, but there are crash reports from the field, demonstrating the problem and confirming the patch fixes the crash. So far the issue was discovered only on ppc64le (Power10 to be more precise). To handle this, keep the event thread in place. Note, that tid here comes from the BPF probe directly, where it's captured via bpf_get_current_task/_btf. This means that the tid is the one really reported by the kernel, so keeping it represents the current state precisely. Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>

erthalion · 2024-12-20T16:18:23Z

Thank you @erthalion! I updated the PR body to be compliant with our semantic commit message (otherwise it can't be parsed by the release workflow :D )!

Thanks, @FedeDP ! There was one silly typo I've fixed, so I'm afraid the PR needs another lgtm.

FedeDP

/approve

poiana · 2024-12-20T16:26:06Z

LGTM label has been added.

Git tree hash: 0aeb9047510577ac29e412dc741ff40db8d209c9

poiana · 2024-12-20T16:26:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erthalion, FedeDP

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added kind/bug Something isn't working release-note-none dco-signoff: no area/libsinsp size/S labels Dec 20, 2024

poiana requested review from hbrueckner and leogr December 20, 2024 10:35

erthalion force-pushed the fix/keep-event-thread-on-exec branch from 4b592d7 to 3beda1a Compare December 20, 2024 10:35

poiana added dco-signoff: yes and removed dco-signoff: no labels Dec 20, 2024

leogr requested review from FedeDP, Andreagit97 and gnosek December 20, 2024 13:25

erthalion force-pushed the fix/keep-event-thread-on-exec branch from 3beda1a to b07ae70 Compare December 20, 2024 14:24

FedeDP previously approved these changes Dec 20, 2024

View reviewed changes

poiana assigned FedeDP Dec 20, 2024

poiana added the lgtm label Dec 20, 2024

poiana added the approved label Dec 20, 2024

poiana added this to the 0.20.0 milestone Dec 20, 2024

FedeDP changed the title ~~Keep event thread after execve~~ fix(userspace/libsinsp): keep event thread after execve Dec 20, 2024

LucaGuerra reviewed Dec 20, 2024

View reviewed changes

erthalion dismissed FedeDP’s stale review via 52b88b0 December 20, 2024 16:15

erthalion force-pushed the fix/keep-event-thread-on-exec branch from b07ae70 to 52b88b0 Compare December 20, 2024 16:15

poiana removed the lgtm label Dec 20, 2024

poiana requested a review from FedeDP December 20, 2024 16:15

FedeDP approved these changes Dec 20, 2024

View reviewed changes

poiana added the lgtm label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(userspace/libsinsp): keep event thread after execve #2212

fix(userspace/libsinsp): keep event thread after execve #2212

erthalion commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

FedeDP left a comment

poiana commented Dec 20, 2024

FedeDP commented Dec 20, 2024

FedeDP commented Dec 20, 2024

LucaGuerra Dec 20, 2024

LucaGuerra Dec 20, 2024

erthalion Dec 20, 2024

erthalion commented Dec 20, 2024

FedeDP left a comment

poiana commented Dec 20, 2024

poiana commented Dec 20, 2024

	* should not happen, since the kernel will reassing tid before
	* should not happen, since the kernel will reassign tid before

fix(userspace/libsinsp): keep event thread after execve #2212

Are you sure you want to change the base?

fix(userspace/libsinsp): keep event thread after execve #2212

Conversation

erthalion commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 • edited Loading

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 20, 2024

FedeDP commented Dec 20, 2024

FedeDP commented Dec 20, 2024

LucaGuerra Dec 20, 2024

Choose a reason for hiding this comment

LucaGuerra Dec 20, 2024

Choose a reason for hiding this comment

erthalion Dec 20, 2024

Choose a reason for hiding this comment

erthalion commented Dec 20, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Dec 20, 2024

poiana commented Dec 20, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading