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

refactor: Improve API used by ebpf programs #3982

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

yanivagman
Copy link
Collaborator

@yanivagman yanivagman commented Apr 16, 2024

1. Explain what the PR does

As preparation for migrating event scope filters into the kernel and matching filters by event id, we now save the event id in the program_data_t structure during initialization instead of when submitting the event to userspace.

By knowing the event id during program init, we can also retrieve the bitmap of the policies that requested this event, removing the need to call should_submit() within the program.

Next, should_trace() is renamed to evaluate_context_filters() to better represent its function and accommodate potential future argument filters. This function considers both event submit bits and matched context filters. Its return value is now boolean to hide the internal policy matching mechanism, ensuring future-proofing against changes in bitmap size or implementation.

For programs with logic for multiple events, we introduce event_is_selected() to determine if the event is selected by any policies. We also add reset_event() to reset matched_policies and argument buffer for a new event id, and reset_event_args_buf() for cases where the same event is submitted multiple times from the same program, addressing a bug where submitting multiple events may yield incorrect matched_policies bitmap.

Additionally, policies_matched() is added to return the result of matched_policies.

2. Explain how to test it

3. Other comments

@yanivagman yanivagman force-pushed the refactor_ebpf_programs_api branch 2 times, most recently from 70ce608 to bebf27e Compare April 16, 2024 21:04
@yanivagman yanivagman requested a review from geyslan April 16, 2024 21:59
Both mnt_ns_filter and pid_ns_filter maps had a key of 64 bits in size.
This key size is wrong, and should be set to 32 bits.
@yanivagman yanivagman force-pushed the refactor_ebpf_programs_api branch from bebf27e to 2dab95a Compare April 17, 2024 05:48
@geyslan
Copy link
Member

geyslan commented Apr 17, 2024

I'll be on this today 👍🏼.

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM, I put some doubts and suggestions.

Comment on lines +242 to +250
event_config_t *event_config = get_event_config(event_id, event->context.policies_version);
if (event_config == NULL)
return false;
Copy link
Member

Choose a reason for hiding this comment

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

As this is the only point that reset_event can fail (and all callers return/abort when this returns false), shouldn't we anticipate its check in the beginning of the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean here

Copy link
Member

Choose a reason for hiding this comment

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

I meant: only touch event fields if there actually is a config for that event, like:

statfunc bool reset_event(event_data_t *event, u32 event_id)
{
    // check first
    event_config_t *event_config = get_event_config(event_id, event->context.policies_version);
    if (event_config == NULL)
        return false;

    // reset if there's a config for it.
    event->context.eventid = event_id;
    reset_event_args_buf(event);
    event->config.submit_for_policies = ~0ULL;
    ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know about this change.
These fields not depend on the event config, and cost almost nothing (just simple assignments).
In case the caller don't check the return value, returning from reset_event after doing these 4 assignments makes the reset valid, and matched_policies is set to match all policies.

pkg/ebpf/c/common/context.h Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c 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 Outdated Show resolved Hide resolved
@yanivagman yanivagman force-pushed the refactor_ebpf_programs_api branch from 2dab95a to d78f011 Compare April 18, 2024 17:13
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
As preparation for migrating event scope filters into the kernel and matching
filters by event id, we now save the event id in the program_data_t structure
during initialization instead of when submitting the event to userspace.

By knowing the event id during program init, we can also retrieve the bitmap of
the policies that requested this event, removing the need to call
should_submit() within the program.

Next, should_trace() is renamed to evaluate_context_filters() to better represent
its function and accommodate potential future argument filters. This function
considers both event submit bits and matched context filters. Its return value is
now boolean to hide the internal policy matching mechanism, ensuring
future-proofing against changes in bitmap size or implementation.

For programs with logic for multiple events, we introduce event_is_selected()
to determine if the event is selected by any policies. We also add reset_event()
to reset matched_policies and argument buffer for a new event id, and
reset_event_args_buf() for cases where the same event is submitted multiple
times from the same program, addressing a bug where submitting multiple events
may yield incorrect matched_policies bitmap.

Additionally, policies_matched() is added to return the result of matched_policies.
@yanivagman yanivagman merged commit 9c79fea into aquasecurity:main Apr 18, 2024
32 checks passed
@yanivagman yanivagman deleted the refactor_ebpf_programs_api branch April 18, 2024 20:17
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.

2 participants