new(drivers): add `pgid` field #2077

Andreagit97 · 2024-09-20T13:37:36Z

What type of PR is this?

/kind bug

/kind feature

Any specific area of the project related to this PR?

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap

/area libsinsp

Does this PR require a change in the driver versions?

/version driver-SCHEMA-version-minor

What this PR does / why we need it:

As explained in #2076 this PR introduces the pgid field taken directly from the kernel. This ID is referred to the host pid namespace so it should help us writing more reliable rules

Which issue(s) this PR fixes:

Mitigation for #2076

Today we have an issue

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(drivers): add pgid field

github-actions · 2024-09-20T13:38:05Z

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

Andreagit97 · 2024-09-20T13:48:16Z

To solve #2076 we shouldn't rely on any virtual ID in our filter checks. So at a certain point, we should also collect the sid field directly from the kernel. While adding the new field in our drivers is not an issue we need to find a way to avoid the repetitions of filter-checks in userspace, for example, we should add a generic filter-check like proc[*].* (e.g. proc[sid].name) that should replace all the followings:

proc.vpgid.name
proc.vpgid.exe
proc.vpgid.exepath
proc.pgid.name
proc.sid.name
and all the others...

That's the reason why I avoided adding the new sid field in this PR, I would like to avoid filter check proliferations

codecov · 2024-09-20T13:48:33Z

Codecov Report

Attention: Patch coverage is 99.21260% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.70%. Comparing base (aeb8793) to head (ba74004).
Report is 24 commits behind head on master.

Files with missing lines	Patch %	Lines
userspace/libsinsp/threadinfo.cpp	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2077      +/-   ##
==========================================
+ Coverage   73.58%   73.70%   +0.11%     
==========================================
  Files         253      253              
  Lines       31869    31911      +42     
  Branches     5649     5605      -44     
==========================================
+ Hits        23452    23519      +67     
+ Misses       8416     8361      -55     
- Partials        1       31      +30

Flag	Coverage Δ
libsinsp	`73.70% <99.21%> (+0.11%)`	⬆️

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.

Andreagit97 · 2024-09-20T13:53:26Z

userspace/libsinsp/sinsp_filtercheck_thread.cpp

-		// group_leader has been updated to the highest process that has the same process group id.
-		// group_leader's comm is considered the process group leader.
-		m_tstr = group_leader->get_comm();
+	case TYPE_VPGID_NAME:


since this filter-check is unreliable by design as explained here #2076 we could simply call TYPE_PGID_NAME under the hood and modify the filter check description saying that it is deprecated

Same of course for the vsid when we will add the sid field

Makes sense to me.

github-actions · 2024-09-20T13:59:10Z

Perf diff from master - unit tests

     6.14%     -2.32%  [.] sinsp_evt::get_type
     6.19%     -1.78%  [.] next
     1.52%     +1.19%  [.] sinsp::fetch_next_event
     3.95%     +0.76%  [.] sinsp_evt::load_params
     1.14%     +0.73%  [.] sinsp_evt::get_ts
     1.19%     -0.64%  [.] scap_next
     1.67%     +0.61%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     6.12%     +0.60%  [.] sinsp::next
     0.92%     +0.57%  [.] sinsp_parser::parse_context_switch
     0.16%     +0.40%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_find_before_node

Heap diff from master - unit tests

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

Heap diff from master - scap file

peak heap memory consumption: -3.65K
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.0255         +0.0255           149           153           149           153
BM_sinsp_split_median                                          +0.0332         +0.0332           148           153           148           153
BM_sinsp_split_stddev                                          -0.3448         -0.3448             1             1             1             1
BM_sinsp_split_cv                                              -0.3611         -0.3611             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0111         -0.0111            61            60            61            60
BM_sinsp_concatenate_paths_relative_path_median                +0.0026         +0.0026            60            60            60            60
BM_sinsp_concatenate_paths_relative_path_stddev                -0.7679         -0.7679             1             0             1             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.7653         -0.7652             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     -0.0120         -0.0120            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_median                   -0.0110         -0.0111            25            25            25            25
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.2522         -0.2513             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.2431         -0.2422             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0106         -0.0106            63            62            63            62
BM_sinsp_concatenate_paths_absolute_path_median                -0.0061         -0.0062            63            62            63            62
BM_sinsp_concatenate_paths_absolute_path_stddev                +1.6190         +1.6198             0             1             0             1
BM_sinsp_concatenate_paths_absolute_path_cv                    +1.6471         +1.6480             0             0             0             0
BM_sinsp_split_container_image_mean                            -0.0194         -0.0194           401           393           401           393
BM_sinsp_split_container_image_median                          -0.0203         -0.0203           401           393           401           393
BM_sinsp_split_container_image_stddev                          -0.3930         -0.3921             3             2             3             2
BM_sinsp_split_container_image_cv                              -0.3810         -0.3801             0             0             0             0

github-actions · 2024-09-20T14:41:10Z

X64 kernel testing matrix

KERNEL	CMAKE-CONFIGURE	KMOD BUILD	KMOD SCAP-OPEN	BPF-PROBE BUILD	BPF-PROBE SCAP-OPEN	MODERN-BPF SCAP-OPEN
amazonlinux2-4.19	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2-5.10	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2-5.15	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2-5.4	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2022-5.15	🟢	🟢	🟢	🟢	🟢	🟢
amazonlinux2023-6.1	🟢	🟢	🟢	🟢	🟢	🟢
archlinux-6.0	🟢	🟢	🟢	🟢	🟢	🟢
archlinux-6.7	🟢	🟢	🟢	🟢	🟢	🟢
centos-3.10	🟢	🟢	🟢	🟡	🟡	🟡
centos-4.18	🟢	🟢	🟢	🟢	🟢	🟢
centos-5.14	🟢	🟢	🟢	🟢	🟢	🟢
fedora-5.17	🟢	🟢	🟢	🟢	🟢	🟢
fedora-5.8	🟢	🟢	🟢	🟢	🟢	🟢
fedora-6.2	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-3.10	🟢	🟢	🟢	🟡	🟡	🟡
oraclelinux-4.14	🟢	🟢	🟢	🟢	🟢	🟡
oraclelinux-5.15	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-5.4	🟢	🟢	🟢	🟢	🟢	🟡
ubuntu-4.15	🟢	🟢	🟢	🟢	🟢	🟡
ubuntu-5.8	🟢	🟢	🟢	🟢	🟢	🟡
ubuntu-6.5	🟢	🟢	🟢	🟢	🟢	🟢

ARM64 kernel testing matrix

KERNEL	CMAKE-CONFIGURE	KMOD BUILD	KMOD SCAP-OPEN	BPF-PROBE BUILD	BPF-PROBE SCAP-OPEN	MODERN-BPF SCAP-OPEN
amazonlinux2-5.4	🟢	🟢	🟢	🟢	🟢	🟡
amazonlinux2022-5.15	🟢	🟢	🟢	🟢	🟢	🟢
fedora-6.2	🟢	🟢	🟢	🟢	🟢	🟢
oraclelinux-4.14	🟢	🟢	🟢	🟡	🟡	🟡
oraclelinux-5.15	🟢	🟢	🟢	🟢	🟢	🟢
ubuntu-6.5	🟢	🟢	🟢	🟢	🟢	🟢

FedeDP · 2024-09-25T13:00:44Z

/milestone next-driver

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

driver/event_table.c

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 · 2024-10-01T09:50:30Z

This should be ready for review

driver/bpf/configure/TASK_PIDS_FIELD/test.c

driver/bpf/fillers.h

driver/ppm_fillers.c

FedeDP · 2024-10-01T15:32:22Z

test/e2e/tests/test_event_generator/test_db_program_spawned_process.py

@@ -53,7 +53,7 @@ def test_db_program_spawned_process(sinsp, run_containers: dict):
        },
        {
            "container.id": generator_id,
-            "evt.args": SinspField.regex_field(r'^res=0 exe=/bin/ls args=NULL tid=\d+\(ls\) pid=\d+\(ls\) ptid=\d+\(mysqld\) .* tty=0 pgid=1\(systemd\) loginuid=-1\(\<NONE\>\) flags=9\(EXE_WRITABLE\|EXE_LOWER_LAYER\) cap_inheritable=0'),
+            "evt.args": SinspField.regex_field(r'^res=0 exe=/bin/ls args=NULL tid=\d+\(ls\) pid=\d+\(ls\) ptid=\d+\(mysqld\) .* tty=0 vpgid=1\(systemd\) loginuid=-1\(\<NONE\>\) flags=9\(EXE_WRITABLE\|EXE_LOWER_LAYER\) cap_inheritable=0'),


IMHO this is already a somewhat breaking change :/ i mean, if anybody was trusting execve args to have pgid, and perhaps parsing it somehow, we are now breaking it.

evt.args is changed this is true, pgid will be the last rendered field instead of being rendered before loginuid, but i don't think anybody is relying on the position of this field inside evt.args to get it... i can imagine a rule with evt. args contains "pgid=..." but in this case everything should work as before. We have issues if someone rely on the order of these fields to do something, but it seems strange... BTW if there are better ideas i'm happy to change it

I agree with you, the "breaking change" is very unlikely to be a problem.

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Co-authored-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP

/approve

poiana · 2024-10-07T06:44:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, 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~~ [Andreagit97,FedeDP]

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

poiana · 2024-10-07T06:44:16Z

LGTM label has been added.

Git tree hash: 1606217d485790c7186cda19749bdb721ad5843e

FedeDP · 2024-10-07T08:05:35Z

/unhold

poiana added release-note-none dco-signoff: yes kind/bug Something isn't working kind/feature New feature or request area/driver-kmod area/driver-bpf approved area/driver-modern-bpf area/libscap area/libsinsp labels Sep 20, 2024

poiana requested review from hbrueckner and mstemm September 20, 2024 13:37

poiana added size/XXL release-note and removed release-note-none labels Sep 20, 2024

poiana added the do-not-merge/hold label Sep 20, 2024

Andreagit97 force-pushed the add_pgid branch from 59072cf to 44a8106 Compare September 20, 2024 13:49

Andreagit97 commented Sep 20, 2024

View reviewed changes

poiana added this to the next-driver milestone Sep 25, 2024

Andreagit97 added 5 commits September 30, 2024 18:19

new(drivers): add a new pgid field

c7ee6a7

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

new(userspace): add support for new pgid kernel field

64acbde

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

new(tests): add some tests for the new pgid field

2bc45ec

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

cleanup(sinsp): remove duplicated code

de0a678

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

test(sinsp): fix a regression test

7f880f5

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

chore: bump schema version

f6f83a1

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 force-pushed the add_pgid branch from 44a8106 to e80bc31 Compare September 30, 2024 17:32

Andreagit97 commented Sep 30, 2024

View reviewed changes

driver/event_table.c Show resolved Hide resolved

Andreagit97 added 2 commits September 30, 2024 21:57

fix(drivers): fix pgid extraction in kmod and legacy ebpf

f2f1eea

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

fix(tests): fix e2e tests using vpgid

50f3fd7

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>

Andreagit97 force-pushed the add_pgid branch from e80bc31 to 50f3fd7 Compare September 30, 2024 19:58

FedeDP reviewed Oct 1, 2024

View reviewed changes

driver/bpf/configure/TASK_PIDS_FIELD/test.c Outdated Show resolved Hide resolved

FedeDP reviewed Oct 1, 2024

View reviewed changes

driver/bpf/fillers.h Outdated Show resolved Hide resolved

FedeDP reviewed Oct 1, 2024

View reviewed changes

driver/ppm_fillers.c Outdated Show resolved Hide resolved

FedeDP reviewed Oct 1, 2024

View reviewed changes

cleanup(drivers): use helper methods

ba74004

Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Co-authored-by: Federico Di Pierro <nierro92@gmail.com>

Andreagit97 force-pushed the add_pgid branch from 18f0501 to ba74004 Compare October 2, 2024 06:19

FedeDP approved these changes Oct 7, 2024

View reviewed changes

poiana assigned FedeDP Oct 7, 2024

poiana added the lgtm label Oct 7, 2024

jasondellaluce approved these changes Oct 7, 2024

View reviewed changes

poiana assigned jasondellaluce Oct 7, 2024

poiana removed the do-not-merge/hold label Oct 7, 2024

poiana merged commit e25d0f0 into master Oct 7, 2024
60 checks passed

poiana deleted the add_pgid branch October 7, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(drivers): add `pgid` field #2077

new(drivers): add `pgid` field #2077

Andreagit97 commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024

Andreagit97 commented Sep 20, 2024

codecov bot commented Sep 20, 2024 •

edited

Loading

Andreagit97 Sep 20, 2024

Andreagit97 Sep 20, 2024

FedeDP Oct 1, 2024

github-actions bot commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading

FedeDP commented Sep 25, 2024

Andreagit97 commented Oct 1, 2024

FedeDP Oct 1, 2024

Andreagit97 Oct 2, 2024

FedeDP Oct 7, 2024

FedeDP left a comment

poiana commented Oct 7, 2024

poiana commented Oct 7, 2024

FedeDP commented Oct 7, 2024

new(drivers): add pgid field #2077

new(drivers): add pgid field #2077

Conversation

Andreagit97 commented Sep 20, 2024 • edited Loading

github-actions bot commented Sep 20, 2024

Andreagit97 commented Sep 20, 2024

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

Andreagit97 Sep 20, 2024

Choose a reason for hiding this comment

Andreagit97 Sep 20, 2024

Choose a reason for hiding this comment

FedeDP Oct 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 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

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

X64 kernel testing matrix

ARM64 kernel testing matrix

FedeDP commented Sep 25, 2024

Andreagit97 commented Oct 1, 2024

FedeDP Oct 1, 2024

Choose a reason for hiding this comment

Andreagit97 Oct 2, 2024

Choose a reason for hiding this comment

FedeDP Oct 7, 2024

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 7, 2024

poiana commented Oct 7, 2024

FedeDP commented Oct 7, 2024

new(drivers): add `pgid` field #2077

new(drivers): add `pgid` field #2077

Andreagit97 commented Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading