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

tetragon: Setup execve_map max entries #3279

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jan 7, 2025

No description provided.

@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Jan 7, 2025
@olsajiri olsajiri force-pushed the pr/olsajiri/harden branch 3 times, most recently from 7b664dd to ab3513b Compare January 7, 2025 13:31
Comment on lines 109 to 110
threads := readFileDefault("/proc/sys/kernel/threads-max", 32768)
ExecveMap.SetMaxEntries(int(threads))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? It seems it will make the size of the execve_map even larger as threads-max is often well above 32K. This will take significant space while running threads-max threads is pretty rare nop?

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 tried this as mitigation for https://github.com/isovalent/security/issues/88 .. I think we need some combination of this change (with some reasonable size for execve_map) and other ways mentioned in the issue

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, maybe this is one of the cases where NO_PREALLOC could help so that we can dynamically size to a very large map. But I could see how this can lead to memory issues in the future. That's not an easy problem :/

Move '!enter' early in the function which will help following
changes to be more readable.

There's no functional change.

Signed-off-by: Jiri Olsa <[email protected]>
Passing execve_map_value directly to match_binaries to
eliminate superfluous event_find_curr in it.

Signed-off-by: Jiri Olsa <[email protected]>
Moving SizeWithSuffix to strutils package, so we can
use it from other places.

Signed-off-by: Jiri Olsa <[email protected]>
Copy link

netlify bot commented Jan 13, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit fcfa0a9
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/678523219e6d5e0008c8b839
😎 Deploy Preview https://deploy-preview-3279--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -416,4 +419,6 @@ func AddFlags(flags *pflag.FlagSet) {
flags.Int(KeyEventCacheRetryDelay, defaults.DefaultEventCacheRetryDelay, "Delay in seconds between event cache retries")

flags.Bool(KeyCompatibilitySyscall64SizeType, false, "syscall64 type will produce output of type size (compatibility flag, will be removed in v1.4)")

flags.String(KeyExecveMapEntries, "", "Set entries for execve_map table (default 32768)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have the default from the actual default value here? If we ever change it, the change should be reflected in help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants