8000 register normalizeTimeArg processor only when proctree is on by geyslan · Pull Request #4332 · aquasecurity/tracee · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

register normalizeTimeArg processor only when proctree is on #4332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 10, 2024
Merged

register normalizeTimeArg processor only when proctree is on #4332

merged 2 commits into from
Oct 10, 2024

Conversation

geyslan
Copy link
Member
@geyslan geyslan commented Sep 26, 2024 •
edited
Loading

Close: #4330

1. Explain what the PR does

0b35baa fix(ebpf): register processor conditionally

Register SchedProcessFork processors, including normalizeTimeArg,
only if proctree is enabled.

206e160 fix(ebpf): try to assert only when it is not nil

The normalizeTimeArg function was not checking if the value was nil
before asserting it as an uint64.

This also adds the type to the error message.

2. Explain how to test it

sudo ./dist/tracee -e sched_process_fork
sudo ./dist/tracee --proctree source=both -e sched_process_fork to have related args filled.

3. Other comments

@geyslan geyslan self-assigned this Sep 26, 2024
NDStrahilevitz
NDStrahilevitz reviewed Sep 27, 2024
View reviewed changes
Copy link
Collaborator
@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

In what case will a time arg be nil?

@geyslan
Copy link
Member Author
geyslan commented Sep 27, 2024

In what case will a time arg be nil?

Currently, when not using proctree, see:

sudo ./dist/tracee -e sched_process_fork

17:35:12:005276  1000   IPDL Background  16017   16052   0                sched_process_fork        parent_tid: 16052, ..., **parent_process_start_time: <nil>**, leader_tid: <nil>, leader_ns_tid: <nil>, leader_pid: <nil>, leader_ns_pid: <nil>, leader_start_time: <nil>

sudo ./dist/tracee --proctree source=both -e sched_process_fork

17:37:29:009715  1000   firefox          16017   16017   0                sched_process_fork        parent_tid: 16017, ..., **parent_process_start_time: 1727389186434828788**, leader_tid: 16017, leader_ns_tid: 16017, leader_pid: 16017, leader_ns_pid: 16017, leader_start_time: 1727389260327760332

@NDStrahilevitz
Copy link
Collaborator

In what case will a time arg be nil?

Currently, when not using proctree, see:

sudo ./dist/tracee -e sched_process_fork

17:35:12:005276  1000   IPDL Background  16017   16052   0                sched_process_fork        parent_tid: 16052, ..., **parent_process_start_time: <nil>**, leader_tid: <nil>, leader_ns_tid: <nil>, leader_pid: <nil>, leader_ns_pid: <nil>, leader_start_time: <nil>

sudo ./dist/tracee --proctree source=both -e sched_process_fork

17:37:29:009715  1000   firefox          16017   16017   0                sched_process_fork        parent_tid: 16017, ..., **parent_process_start_time: 1727389186434828788**, leader_tid: 16017, leader_ns_tid: 16017, leader_pid: 16017, leader_ns_pid: 16017, leader_start_time: 1727389260327760332

That's odd. Sounds to me like that is the bug.

@geyslan
Copy link
Member Author
geyslan commented Sep 27, 2024

If you're not using proctree, it should have been fed by the control plane signal, right?

@NDStrahilevitz
Copy link
Collaborator
NDStrahilevitz commented Sep 27, 2024 •
edited
Loading

Ok so these arguments are sort of "metadata" arguments which are submitted on a need basis for the process tree (see here). I then question what merit they have as arguments. Arguments or as want to call it "event data" should reflect user informative data on the event. If this is not such data, that is, it is some sort of "metadata" argument, then that reflects an issue in the event format.
Further, it implies that the processing should be applied conditionally. WDYT?
@yanivagman @itaysk I think we need to take this usecase into account in #2870 (event specific "data" which is relevant for internal logic but not for the user)

@geyslan
Copy link
Member Author
geyslan commented Sep 27, 2024

Further, it implies that the processing should be applied conditionally. WDYT?

It is:

tracee/pkg/ebpf/processor.go

Lines 100 to 102 in 1a0065b

switch t.config.ProcTree.Source {
case proctree.SourceEvents, proctree.SourceBoth:
t.RegisterEventProcessor(events.SchedProcessFork, t.procTreeForkProcessor)

@NDStrahilevitz
Copy link
Collaborator
NDStrahilevitz commented Sep 27, 2024 •
edited
Loading

@geyslan

The relevant processor is here

tracee/pkg/ebpf/processor.go

Lines 87 to 93 in 1a0065b

// any later code has access to normalized time arguments.
t.RegisterEventProcessor(events.SchedProcessFork, t.normalizeTimeArg(
"start_time",
"parent_start_time",
"parent_process_start_time",
"leader_start_time",
))

@geyslan
Copy link
Member Author
geyslan commented Sep 27, 2024

Nice. This makes sense to me now. So yeah, I believe it should be in the condition below.

@geyslan geyslan changed the title fix(ebpf): assert as uint64 when value is not nil register normalizeTimeArg processor only when proctree is on Sep 27, 2024
NDStrahilevitz
NDStrahilevitz requested changes Oct 7, 2024
View reviewed changes
pkg/ebpf/processor.go
//
// Process Tree Processors
//

// Processors registered when proctree source "events" is enabled.
switch t.config.ProcTree.Source {
case proctree.SourceEvents, proctree.SourceBoth:
// Event Timestamps Normalization
Copy link
Collaborator

Choose a reason for hiding this comment

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

Logically this works. But I would rather have it in a separate section to convey the intention of all timestamp arguments being registered and processed first. I do think that an effort in another PR of yours may help us move this logic from the processing alltogether, I'll elaborate there.

@NDStrahilevitz NDStrahilevitz mentioned this pull request Oct 7, 2024
Merged
NDStrahilevitz
NDStrahilevitz approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator
@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

I want to resolve this properly with the changes to argument typing (and thus move this logic to decoding timestamp args).
So for now, lets merge this and more importantly cherry-pick to the v0.22.3 release so that users don't receive the repeated errors.

@geyslan
fix(ebpf): try to assert only when it is not nil
cf1562d 
The normalizeTimeArg function was not checking if the value was nil
before asserting it as an uint64.

This also adds the type to the error message.
@geyslan
fix(ebpf): register processor conditionally
30b33a4 
Register SchedProcessFork processors, including normalizeTimeArg,
only if proctree is enabled.
@geyslan geyslan force-pushed the 4330-fix-normalize branch from 0b35baa to 30b33a4 Compare October 10, 2024 12:19
@geyslan
Copy link
Member Author
geyslan commented Oct 10, 2024

/fast-forward

@github-actions github-actions bot merged commit 30b33a4 into aquasecurity:main Oct 10, 2024
31 checks passed
@geyslan geyslan deleted the 4330-fix-normalize branch October 10, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NDStrahilevitz NDStrahilevitz NDStrahilevitz approved these changes

Assignees

@geyslan geyslan

Labels
area/ebpf kind/bug milestone/v0.23.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sched_process_fork emits errors instead of events
2 participants
@geyslan @NDStrahilevitz
0