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(proctree): allow regular events to create proctree nodes #3498

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

rafaeldtinoco
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco commented Sep 22, 2023

commit 0ac5861 (HEAD -> proctreestuff, rafaeldtinoco/proctreestuff)
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Thu Sep 21 23:37:10 2023

fix(proctree): allow regular events to create proctree nodes

It is not impossible, although it is unlikely, that a fork/exec/exit
event arrives first in the regular events pipeline. There is also a
possibility that the signal event is lost.

This change allows tracee to choose which source of events the process
tree should be fed with. Using the control plane (and the fork, exec and
exit signal events) is preferrable, but user can also opt to use regular
sched events OR even both.

There is also another need for this: the analyze mode. If the signatures
rely in data source entries to work, when running tracee analyze mode
there will be a need to "process events" from the regular pipeline in a
way that those data sources exist in memory for the signature processing
as well (TBD later).

There is also the case where the pipeline is almost empty, and the
userland (and go runtime) context switching may cause one event
processor to need proc tree data that hasn't yet been saved. Having
"both" as the source for proc tree is needed in this case.

This commit also:

- creates a generic way to normalize timing args to relative time (or not)

commit 05bedbc
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Tue Sep 19 08:38:11 2023

chore(events_processor): create a file for processor functions

... preparing terrain for refactoring and for process tree processors.

pkg/ebpf/processor.go Show resolved Hide resolved
pkg/ebpf/processor_funcs.go Show resolved Hide resolved
pkg/ebpf/processor_funcs.go Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/controlplane/processes.go Show resolved Hide resolved
pkg/ebpf/events_pipeline.go Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
pkg/proctree/proctree_output.go Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/processor.go Outdated Show resolved Hide resolved
pkg/ebpf/processor_funcs.go Show resolved Hide resolved
pkg/ebpf/processor_funcs.go Show resolved Hide resolved
... preparing terrain for refactoring and for process tree processors.
Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

Overall LGTM,
please see below questions before merging this

pkg/cmd/flags/proctree.go Show resolved Hide resolved
save_to_submit_buf(&p.event->args_buf, (void *) &child_start_time, sizeof(u64), 9);

// Process tree information (if needed).
if (p.config->options & OPT_FORK_PROCTREE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Do we need this information now that @AlonZivony added the start time of all the three in every event?
  2. If you think we still need it, I don't think it needs to be gated with OPT_FORK_PROCTREE - this will cause inconsistencies in the event output between when the proc tree is enabled or not, so better to always send it as part of the event output

Copy link
Collaborator

Choose a reason for hiding this comment

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

We still need it.
My thought here is the following - if we want to have different arguments than the normal event, we should create a new event and submit it with the same first arguments and add on top of it the additional arguments (we do the same with magic_write I think).
We already have the concept of internal events, so I think it would be cleaner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we still need it? Those are just extra fields - we shouldn't care about sending them since we don't remove any of the existing fields.
In the future, we are going to have process_fork user-friendly event anyway, so having more arguments here shouldn't be a problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

We need the arguments because I don't pass the start times to the event, only to the decode step.
I don't think that increasing the size of the event drastically for an internal use and exposing all the confusing terms of up_parent and the likes is good for us. I believe that if this is only for an internal use, than it should be an internal event.
However, the question here is what do we do with the analyze mode?
The analyze mode will need this arguments, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that increasing the size of the event drastically for an internal use and exposing all the confusing terms of up_parent and the likes is good for us. I believe that if this is only for an internal use, than it should be an internal event.

That was the moto (for having the args, send the values only when needed, and remove the args before submitting to end user). Yes, we can create another event, but it would be the 3rd sched_process_fork event.

However, the question here is what do we do with the analyze mode?
The analyze mode will need this arguments, no?

Analyze mode nowadays don't do any kind of "process event" approach, so it wouldn't be ready even if we had something different. I consider this as "preparing the terrain" but changes will be needed in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was agreed that the analyze mode will be dealt in v0.20 most likely (and an issue will be opened for that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm keeping the OPT_FORK_PROCTREE because there is extra processing in getting leader and up_parent information that isn't really needed if process tree is not using events source.

I'm keeping the argument removal not to create a 3rd sched_process_fork event (besides the regular and the signal one). With that, the argument removal logic will be kept so we don't expose a big amount of arguments for SchedProcessFork event to the user (and up_parent wouldn't make much sense in that regard).

With that, I'm either solving, mitigating or workarounding all the concerns brought and yet being functional in time for the release. We can always change things later.

Copy link
Collaborator

@AlonZivony AlonZivony left a comment

Choose a reason for hiding this comment

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

The main issue is the sched_process_fork which is not the best approach in my opinion.
Overall looks very good except for it.
Most of my comments are about names and comments.

pkg/cmd/flags/proctree.go Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/controlplane/processes.go Show resolved Hide resolved
save_to_submit_buf(&p.event->args_buf, (void *) &child_start_time, sizeof(u64), 9);

// Process tree information (if needed).
if (p.config->options & OPT_FORK_PROCTREE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still need it.
My thought here is the following - if we want to have different arguments than the normal event, we should create a new event and submit it with the same first arguments and add on top of it the additional arguments (we do the same with magic_write I think).
We already have the concept of internal events, so I think it would be cleaner.

pkg/ebpf/processor_proctree.go Show resolved Hide resolved
It is not impossible, although it is unlikely, that a fork/exec/exit
event arrives first in the regular events pipeline. There is also a
possibility that the signal event is lost.

This change allows tracee to choose which source of events the process
tree should be fed with. Using the control plane (and the fork, exec and
exit signal events) is preferrable, but user can also opt to use regular
sched events OR even both.

There is also another need for this: the analyze mode. If the signatures
rely in data source entries to work, when running tracee analyze mode
there will be a need to "process events" from the regular pipeline in a
way that those data sources exist in memory for the signature processing
as well (TBD later).

There is also the case where the pipeline is almost empty, and the
userland (and go runtime) context switching may cause one event
processor to need proc tree data that hasn't yet been saved. Having
"both" as the source for proc tree is needed in this case.

This commit also:

- creates a generic way to normalize timing args to relative time (or not)
@rafaeldtinoco rafaeldtinoco merged commit 36853f5 into aquasecurity:main Sep 28, 2023
2 of 3 checks passed
@rafaeldtinoco rafaeldtinoco deleted the proctreestuff branch September 28, 2023 13:35
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.

Process Tree: Create a process tree fed by events that works as a datasource
3 participants