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

register normalizeTimeArg processor only when proctree is on #4332

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Sep 26, 2024

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

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

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:

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

@geyslan

The relevant processor is here

// 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
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.
Register SchedProcessFork processors, including normalizeTimeArg,
only if proctree is enabled.
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.

sched_process_fork emits errors instead of events
2 participants