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

feat(caps): base ebpf capabilities #4178

Merged

Conversation

NDStrahilevitz
Copy link
Collaborator

1. Explain what the PR does

6bd0b39 feat(caps): base ebpf capabilities

Add the ability to add the ebpf capabilities to the base ring, instead
of allocating a specific ring. When the feature is enabled, requests for
the eBPF ring will seamlessly avoid a ring switch, and the callback will
be called as is.

2. Explain how to test it

Stack addresses should remain working.

3. Other comments

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.

I don't see we removed the ebpf ring request from the hot path in the pipeline - shouldn't we do that?

pkg/capabilities/capabilities.go Outdated Show resolved Hide resolved
pkg/capabilities/capabilities.go Outdated Show resolved Hide resolved
Comment on lines +278 to +280
useBaseEbpf := func(cfg config.Config) bool {
return cfg.Output.StackAddresses
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we simply add the required bpf capability to the base ring if StackAddresses is selected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So if in the future we add other features which require a similar approach, we can add the logic here instead of directly coupling to stackaddresses.

Copy link
Collaborator

@yanivagman yanivagman Jul 9, 2024

Choose a reason for hiding this comment

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

Why coupling?
I mean, to expose some method to add more capabilities to the base ring, and then we can remove all the other changes in this PR, and just add the capability here using this method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There already is such a method, but the eBPF caps specifically aren't constant across systems. So I thought initializing a specific bypass for eBPF in the code location where we do the calculation of what are these eBPF caps, would be easier.
About coupling, I meant that this is slightly less coupled, or at least that it makes the intention that this condition coud change in the future more clear, then doing ebpfBase: cfg.Output.StackAddresses.

@NDStrahilevitz
Copy link
Collaborator Author

I don't see we removed the ebpf ring request from the hot path in the pipeline - shouldn't we do that?

Now there's no need, because the EBPF callback automatically skips the capability switch if we include EBPF caps in the base. So its essentially like a mini-bypass.

Add the ability to add the ebpf capabilities to the base ring, instead
of allocating a specific ring. When the feature is enabled, requests for
the eBPF ring will seamlessly avoid a ring switch, and the callback will
be called as is.
@yanivagman
Copy link
Collaborator

I don't see we removed the ebpf ring request from the hot path in the pipeline - shouldn't we do that?

Now there's no need, because the EBPF callback automatically skips the capability switch if we include EBPF caps in the base. So its essentially like a mini-bypass.

But this method still tries to acquire the capabilities lock in the hot path

@NDStrahilevitz
Copy link
Collaborator Author

NDStrahilevitz commented Jul 9, 2024

I don't see we removed the ebpf ring request from the hot path in the pipeline - shouldn't we do that?

Now there's no need, because the EBPF callback automatically skips the capability switch if we include EBPF caps in the base. So its essentially like a mini-bypass.

But this method still tries to acquire the capabilities lock in the hot path

It wouldn't be the first lock acquisition we have inside the pipeline. The cgroups tracking for example does that. In any case, the performance overhead from capability switching was resolved. If there is lock contention because of this, we can reconsider, I'm not sure there will be (seeing as in similar situations so far we haven't).
Though if the lock contention is an issue, we may as well make the change for the full bypass as well.

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.

Ok, we can continue with this change for now.
In the future, we can rethink how capabilities are handled.

@NDStrahilevitz NDStrahilevitz merged commit 5c09519 into aquasecurity:main Jul 9, 2024
32 checks passed
@NDStrahilevitz NDStrahilevitz deleted the stack_addresses_ebpf_caps branch July 9, 2024 14:07
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