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(driver/kmod): don't truncate path at 256 #1954

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

therealbobo
Copy link
Contributor

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:

The kmod truncates the path at 256. The other two drivers don't have the same behaviour.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Copy link

Please double check driver/API_VERSION file. See versioning.

/hold

Copy link

Perf diff from master - unit tests

     2.15%     +0.92%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     4.09%     +0.79%  [.] sinsp_evt::get_type
     6.54%     +0.73%  [.] sinsp::next
     4.20%     -0.65%  [.] gzfile_read
     1.01%     +0.57%  [.] scap_event_encode_params_v
     4.84%     -0.53%  [.] sinsp_parser::process_event
     2.03%     -0.51%  [.] scap_event_decode_params
     0.27%     +0.49%  [.] libsinsp::state::stl_container_table_adapter<std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, libsinsp::state::value_table_entry_adapter<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::dynamic_fields_t>::stl_container_table_adapter
     4.07%     +0.42%  [.] sinsp_evt::load_params
     1.75%     -0.36%  [.] sinsp_parser::event_cleanup

Perf diff from master - scap file

     4.34%     +5.79%  [.] sinsp_evt_formatter::tostring_withformat
     4.44%     +5.78%  [.] sinsp_filter_check::rawval_to_string
    17.68%     -4.34%  [.] sinsp_filter_check_thread::extract_single
    12.68%     -4.00%  [.] libsinsp::runc::match_container_id
     4.05%     +3.85%  [.] 0x00000000000a6f60
     8.87%     -2.20%  [.] sinsp_filter_check::get_transformed_field_info
     4.43%     +2.10%  [.] sinsp_filter_check::tostring
     8.64%     -2.09%  [.] main
     4.28%     +1.82%  [.] 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
    17.66%     -0.53%  [.] sinsp_filter_check::extract_nocache

Heap diff from master - unit tests

total runtime: -0.24s.
calls to allocation functions: -974 (3975/s)
temporary memory allocations: -235 (959/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.02s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: 2 (86/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@Andreagit97
Copy link
Member

Can we add a test for it in the driver tests?

@poiana poiana added size/L and removed size/XS labels Jul 12, 2024
Copy link

Perf diff from master - unit tests

     2.83%     -1.14%  [.] scap_event_decode_params
     0.79%     +0.87%  [.] libsinsp::sinsp_suppress::process_event
     5.36%     -0.50%  [.] next
     4.35%     -0.50%  [.] sinsp_thread_manager::get_thread_ref
     0.05%     +0.43%  [.] 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_emplace<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >
     0.73%     +0.41%  [.] 0x00000000000e8394
     4.88%     -0.40%  [.] sinsp_parser::process_event
     0.47%     +0.39%  [.] scap_next
     7.46%     -0.39%  [.] sinsp::next
     0.68%     +0.38%  [.] sinsp_threadinfo::~sinsp_threadinfo

Perf diff from master - scap file

     6.12%     +4.31%  [.] sinsp_evt_formatter::tostring_withformat
    12.40%     -4.14%  [.] sinsp_filter_check_event::extract_single
     2.66%     +3.02%  [.] libsinsp::runc::match_container_id
     2.93%     +3.01%  [.] sinsp_filter_check::rawval_to_string
     6.26%     -2.10%  [.] main
     6.21%     -2.07%  [.] next
     8.05%     -1.78%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     3.12%     +1.34%  [.] sinsp_filter_check::get_field_info
     3.13%     +1.25%  [.] sinsp_evt::get_thread_info
     3.13%     +1.14%  [.] sinsp_threadinfo::~sinsp_threadinfo

Heap diff from master - unit tests

total runtime: 0.04s.
calls to allocation functions: -612 (-14571/s)
temporary memory allocations: -339 (-8071/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.00s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: 8 (-2000/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@FedeDP
Copy link
Contributor

FedeDP commented Jul 15, 2024

/milestone next-driver

@poiana poiana added this to the next-driver milestone Jul 15, 2024
@poiana poiana added size/XL and removed size/L labels Jul 16, 2024
Copy link

Perf diff from master - unit tests

     6.94%     -1.24%  [.] sinsp_thread_manager::find_thread
     2.26%     -0.78%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     2.13%     +0.70%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     1.38%     -0.70%  [.] std::vector<sinsp_evt_param, std::allocator<sinsp_evt_param> >::emplace_back<sinsp_evt*, unsigned int&, char const*, unsigned long&>
     5.19%     -0.67%  [.] next
     0.53%     +0.66%  [.] scap_next
     5.82%     +0.66%  [.] sinsp::next
     1.15%     -0.57%  [.] scap_event_encode_params_v
     1.11%     +0.51%  [.] libsinsp::sinsp_suppress::process_event
     0.79%     +0.51%  [.] sinsp_parser::event_cleanup

Perf diff from master - scap file

     2.94%     +6.74%  [.] libsinsp::runc::match_container_id
     6.73%     +6.62%  [.] sinsp_evt_formatter::tostring_withformat
     3.29%     +5.11%  [.] sinsp_filter_check::tostring
    13.05%     -5.09%  [.] next
     3.40%     +3.38%  [.] sinsp_evt::get_param_as_str
     7.31%     -2.07%  [.] sinsp_thread_manager::find_thread
     6.79%     -1.94%  [.] sinsp::fetch_next_event
    10.11%     -1.84%  [.] sinsp_filter_check_event::extract_single
     6.80%     -1.81%  [.] sinsp_filter_check::get_field_info
     3.39%     +1.40%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>

Heap diff from master - unit tests

total runtime: -0.26s.
calls to allocation functions: -694 (2638/s)
temporary memory allocations: -260 (988/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.01s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: 10 (-769/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

FedeDP
FedeDP previously approved these changes Jul 16, 2024
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jul 16, 2024

LGTM label has been added.

Git tree hash: 20df5236ad716cb414c39321a47c7b177ef50302

Copy link

Perf diff from master - unit tests

     6.99%     -1.60%  [.] sinsp_thread_manager::find_thread
     5.86%     +1.27%  [.] sinsp::next
     2.14%     +1.05%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     3.25%     +1.05%  [.] sinsp_evt::load_params
     2.28%     -1.05%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     0.90%     +0.89%  [.] sinsp_parser::parse_context_switch
     4.04%     +0.84%  [.] sinsp_parser::process_event
     1.38%     -0.80%  [.] std::vector<sinsp_evt_param, std::allocator<sinsp_evt_param> >::emplace_back<sinsp_evt*, unsigned int&, char const*, unsigned long&>
     9.64%     +0.58%  [.] sinsp_parser::reset
     0.32%     +0.48%  [.] sinsp_container_manager::resolve_container

Perf diff from master - scap file

     7.92%    +10.92%  [.] sinsp_evt_formatter::tostring_withformat
     7.92%     +4.24%  [.] sinsp_filter_check::extract_nocache
     8.60%     -2.02%  [.] sinsp_thread_manager::find_thread
     3.46%     +2.00%  [.] libsinsp::runc::match_container_id
    11.90%     +1.91%  [.] sinsp_filter_check_event::extract_single
     3.87%     +1.83%  [.] sinsp_filter_check::tostring
     3.99%     +1.80%  [.] rawstring_check::extract_single
     4.00%     +1.78%  [.] 0x00000000000a7d10
     8.01%     -1.75%  [.] sinsp_filter_check::get_field_info
     8.84%     +1.70%  [.] sinsp_filter_check_thread::extract_single

Heap diff from master - unit tests

total runtime: -0.06s.
calls to allocation functions: -1091 (19140/s)
temporary memory allocations: -381 (6684/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.02s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: 1 (55/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@therealbobo therealbobo force-pushed the kmod-dont-truncate-path branch from 90403d8 to 4c2ed07 Compare July 16, 2024 14:09
Copy link

Perf diff from master - unit tests

     6.91%     -1.76%  [.] sinsp_thread_manager::find_thread
     2.25%     -1.01%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     3.21%     +0.96%  [.] sinsp_evt::load_params
     2.12%     +0.87%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     3.00%     +0.82%  [.] sinsp_thread_manager::get_thread_ref
     3.27%     -0.73%  [.] gzfile_read
     1.79%     +0.68%  [.] scap_event_decode_params
     5.26%     -0.67%  [.] sinsp_evt::get_type
     0.53%     +0.54%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Identity, 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, true, true> >::find
     9.54%     +0.51%  [.] sinsp_parser::reset

Perf diff from master - scap file

    10.83%     -5.22%  [.] next
     2.87%     +4.54%  [.] sinsp_filter_check::rawval_to_string
     7.81%     -3.27%  [.] sinsp_filter_check_gen_event::extract_single
     2.73%     +3.14%  [.] sinsp_filter_check::tostring
     2.82%     +2.96%  [.] sinsp_evt::get_param_as_str
     5.81%     -2.06%  [.] sinsp_parser::process_event
     8.39%     -2.03%  [.] sinsp_filter_check_event::extract_single
     5.64%     -1.94%  [.] sinsp::fetch_next_event
     5.58%     -1.91%  [.] sinsp_evt_formatter::tostring_withformat
     2.73%     +1.52%  [.] sinsp_parser::reset

Heap diff from master - unit tests

total runtime: -0.07s.
calls to allocation functions: -1604 (22277/s)
temporary memory allocations: -702 (9750/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.00s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -1 (1000/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@therealbobo therealbobo force-pushed the kmod-dont-truncate-path branch from 4c2ed07 to 43bb44b Compare July 16, 2024 14:44
@poiana poiana removed the lgtm label Jul 16, 2024
@poiana poiana requested a review from FedeDP July 16, 2024 14:44
FedeDP
FedeDP previously approved these changes Jul 16, 2024
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Jul 16, 2024
@poiana
Copy link
Contributor

poiana commented Jul 16, 2024

LGTM label has been added.

Git tree hash: f20eb5f6e9218ed56d9f02aa46fac41f88bed3be

Copy link

Perf diff from master - unit tests

     3.98%     -1.15%  [.] sinsp_thread_manager::get_thread_ref
     9.99%     -0.69%  [.] sinsp_parser::reset
     1.88%     -0.67%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     1.64%     +0.65%  [.] sinsp::fetch_next_event
     5.31%     -0.55%  [.] next
     1.41%     -0.53%  [.] 0x00000000000e83b0
     0.67%     +0.51%  [.] libsinsp::events::is_unknown_event
     1.08%     -0.43%  [.] sinsp_evt::get_direction
     0.87%     -0.43%  [.] scap_next
     0.51%     +0.42%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Identity, 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, true, true> >::find

Perf diff from master - scap file

     7.93%    +17.16%  [.] sinsp_filter_check::extract_nocache
    18.68%     -7.83%  [.] sinsp_evt_formatter::tostring_withformat
    10.17%     -4.64%  [.] 0x00000000000a77e0
     2.65%     +3.94%  [.] sinsp_filter_check_event::extract_single
     5.32%     +3.70%  [.] sinsp_thread_manager::find_thread
     7.97%     -3.03%  [.] sinsp_filter_check::tostring
     7.87%     -3.01%  [.] gzfile_read
     5.34%     -1.45%  [.] sinsp_evt::get_param_as_str
    10.61%     -1.35%  [.] sinsp_filter_check_thread::extract_single
     5.21%     +0.46%  [.] sinsp_filter_check::get_transformed_field_info

Heap diff from master - unit tests

total runtime: 0.08s.
calls to allocation functions: -874 (-10530/s)
temporary memory allocations: -336 (-4048/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.00s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -11 (5500/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@therealbobo therealbobo force-pushed the kmod-dont-truncate-path branch from 43bb44b to dd12a37 Compare July 17, 2024 09:46
@poiana poiana removed the lgtm label Jul 17, 2024
@poiana poiana requested a review from FedeDP July 17, 2024 09:46
test/drivers/event_class/event_class.h Outdated Show resolved Hide resolved
test/libsinsp_e2e/sys_call_test.cpp Outdated Show resolved Hide resolved
Signed-off-by: Roberto Scolaro <[email protected]>
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

I have added a commit to remove the latest usages of #include <driver/ppm_events_public.h>

/approve

@poiana
Copy link
Contributor

poiana commented Jul 17, 2024

LGTM label has been added.

Git tree hash: c91a049aeb268f8a7b2a58b9a8f3fdd23bac4312

Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jul 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, therealbobo

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:

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

1 similar comment
@poiana
Copy link
Contributor

poiana commented Jul 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, therealbobo

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:

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

@Andreagit97
Copy link
Member

/unhold

@poiana poiana merged commit 4529462 into falcosecurity:master Jul 17, 2024
49 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants