-
Notifications
You must be signed in to change notification settings - Fork 490
feat(sensors): support 38+ override on lsm funcs #4244
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
base: main
Are you sure you want to change the base?
Conversation
|
@olsajiri may I have your feedback on this one? Thanks a lot. |
c0067a5 to
23c75cf
Compare
olsajiri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, left few comments, thanks
| if fmodret, ok = fmodretMap[attachFunc]; !ok { | ||
|
|
||
| fmodret = program.Builder( | ||
| path.Join(option.Config.HubbleLib, loadProgName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we already add new program for this, could we move override programs into separated object?
that might speed up the load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think it makes a lot of sense to do this. Thanks!
| "github.com/cilium/tetragon/pkg/sensors/program" | ||
| ) | ||
|
|
||
| var fmodretMap map[string]*program.Program |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about something similar but bit more generic to cover also standard kprobe override programs.. there's no limitation for attached kprobe programs, but it would at least benefit from having just single copy of the program and reduce the footprint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea. Let me look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue I noticed is that, if I'm not mistaken, for kprobe override programs, when multi-kprobe is enabled, each kprobe attach points actually share the same override_maps. Do you think we should change that and use per-hookpoint map instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum not sure what you mean..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wasn't aware that the creation and loading of maps and programs in multi kprobe flows are based on the data type of load.LoaderData. That makes sense now. Thanks a lot!
| fmodret.PinPath = "fmod_ret/" + attachFunc | ||
|
|
||
| fmodretmap := program.MapBuilder("override_tasks", fmodret) | ||
| fmodretmap.PinPath = path.Join("fmod_ret/", attachFunc, "override_tasks") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather place it in some visibly special place like we do for base sensor, perhaps something like:
__override__/fmodret/security_xxx
__override__/fmodret/security_yyy
...
__override__/kprobe/ksys_read
__override__/kprobe/ksys_write
...
__override__/override_tasks
pkg/sensors/tracing/generickprobe.go
Outdated
| logger.GetLogger().Info("loading generic fmod ret program", "prog", load) | ||
|
|
||
| unload := func() { | ||
| deleteFmodRetProg(load.Attach) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use program.Program.unloaderOverride or add something in there so it gets removed during program's unload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't notice this function exists when I wrote the code. From its name it's definitely more suitable. Let me look into this.
23c75cf to
acfea29
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Changes since v1:
|
|
I still keep this as draft because of an item that I'd like to discuss first. Say we have a scenario is like the below (keep in mind that in v2 we use a shared override_tasks map):
I can think of a few directions:
@olsajiri do you think this is a real issue that we should address? I'd love to know your thoughts on this. |
yea, that seems like a problem.. so at the moment override_task is program's map, so each override program has its own copy, I'd suggest to have some state of this change doing the same, and adding a change to single map on top of that perhaps we could have policy id as part of the override_task value and have sensor unload to cleanup its records before it unloads the override program.. something like you suggest in 2) but not sure what's benefit of inner map I'll check on your change in more detail but on first glance please try to split the change into more logical changes/commits, it's easier to review, thanks |
acfea29 to
99bf162
Compare
Thanks. I think that makes sense. Let me see if I can add an extra field to the key or value of override_tasks map.
I've split them into multiple commits. Please feel free to let me know if anything is not clear! |
olsajiri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, left few comments, please split the change into more commits in next version
pkg/sensors/program/loader_linux.go
Outdated
| if !ok { | ||
| return fmt.Errorf("progName %s not in collecition spec programs: %+v", progName, coll.Programs) | ||
| } | ||
| progSpec.AttachTo = load.Attach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed for kprobe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question with new version..
pkg/sensors/program/loader_linux.go
Outdated
| } | ||
| if unloadFunc != nil { | ||
| load.unloaderOverride = &unloader.CustomUnloader{ | ||
| UnloadFunc: unloadFunc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just set load.unloaderOverride in getOverrideProg ? then we could leave LoadFmodRetProgram as is
|
Hi @olsajiri thanks for your patience. I've updated v3 in this PR and changed this PR as ready for review. The change since last time:
I'll appreciate any feedback on this. |
@holyspectral I'm checking on that, would you mind to rebase it? it'd be easier for me to run it, thanks |
Move bpf functions regarding overrides into bpf_generic_override.o Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Provide a custom CustomUnloader, so custom actions can be performed when a ebpf program is unloaded and unloaderOverride is called. Signed-off-by: Sam Wang (holyspectral) <[email protected]>
1. Allow policies to share override programs as long as they share the same attached functions. This includes both programs based on kprobe and fmod_ret. 2. The pinned path of ebpf programs and maps are moved to below locations: /bpffs/tetragon/__override__ /bpffs/tetragon/__override__/kprobe /bpffs/tetragon/__override__/kprobe/__x64_sys_symlinkat /bpffs/tetragon/__override__/kprobe/__x64_sys_symlinkat/link_override /bpffs/tetragon/__override__/kprobe/__x64_sys_symlinkat/prog_override /bpffs/tetragon/__override__/kprobe/__x64_sys_execve /bpffs/tetragon/__override__/kprobe/__x64_sys_execve/link_override /bpffs/tetragon/__override__/kprobe/__x64_sys_execve/prog_override /bpffs/tetragon/__override__/override_tasks /bpffs/tetragon/__override__/fmod_ret /bpffs/tetragon/__override__/fmod_ret/security_bprm_creds_for_exec /bpffs/tetragon/__override__/fmod_ret/security_bprm_creds_for_exec/prog Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
For multi kprobe, the id is sent as part of cookies, so each kprobe program can have their own id. For single kprobe, the id is retrieved from EventConfig. Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
In the previous version, override_tasks is a shared map created via MapBuilder. This caused a problem during cleanup because these maps can't track reference count across policies. In this commit, the override_tasks map is changed to a global map. Signed-off-by: Sam Wang (holyspectral) <[email protected]>
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
464d08b to
0828a7f
Compare
Signed-off-by: Sam Wang (holyspectral) <[email protected]>
|
Looks like there are issues from vmtests on older kernels. I will take a look. |
olsajiri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first few changes look good but then I fail to see the reason for some of the following changes, I left some comments, please check
I think last 3 commits are fixes of your previous commits and should be squash where they belong
pkg/sensors/program/loader_linux.go
Outdated
| if !ok { | ||
| return fmt.Errorf("progName %s not in collecition spec programs: %+v", progName, coll.Programs) | ||
| } | ||
| progSpec.AttachTo = load.Attach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question with new version..
pkg/sensors/program/loader_linux.go
Outdated
| } | ||
|
|
||
| func LoadFmodRetProgram(bpfDir string, load *Program, maps []*Map, progName string, verbose int) error { | ||
| func LoadKProbeOverrideProgram(bpfDir string, load *Program, maps []*Map, progName string, verbose int, unloadFunc func(bool) error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this added when it's completely removed in one of the following commit?
also the commit log mentions only unloader, not this function
| progs, maps = createKProbeOverrideProgramFromEntry(load, gk.funcName, progs, maps) | ||
| } | ||
| } else { | ||
| overrideTasksMap := program.MapBuilderProgram("override_tasks", load) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems wrong.. if we have load.override we want to ping the override_tasks map, right? you're doing it for the !load.override case
pkg/sensors/tracing/generickprobe.go
Outdated
| } | ||
| gk.data = &genericKprobeData{} | ||
|
|
||
| progs, maps = createKProbeOverrideProgramFromEntry(load, gk.funcName, progs, maps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have the load.OverrideFmodRet check in here and call createFmodRetOverrideProgramFromEntry?
| return loadGenericFmodRetProgram(args.BPFDir, args.Load, args.Maps, args.Verbose) | ||
| } | ||
|
|
||
| func (k *kprobeOverrideProgram) LoadProbe(args sensors.LoadProbeArgs) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so how do we know this gets called before the main program load to ensure it's called in the right order?
|
|
||
| if overrideProgMap == nil { | ||
| overrideProgMap = make(map[string]*Program) | ||
| overrideProgMap = make(map[string]*genericOverride) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs more explanation on why is this needed.. please use commit log message to describe what you are doing and why.. for now and for future ;-)
| FUNC_INLINE __u32 get_index(void *ctx) | ||
| { | ||
| return (__u32)get_attach_cookie(ctx); | ||
| return (__u32)(get_attach_cookie(ctx) & 0x0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's real scary ;-)
again, please explain in the commit log message what you are doing and why.. I fail to see why this is needed
| : "+r"(x)); \ | ||
| }) | ||
|
|
||
| FUNC_INLINE int try_override(void *ctx, struct bpf_map_def *override_tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make the change frst before moving the function, it's hard to review what changed
| load.MapLoad = append(load.MapLoad, config) | ||
|
|
||
| // 0-3: index, 4-11: override_id, others: reserved | ||
| cookies := index&0x0f | (gk.loadArgs.overrideID << 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the use case for this? please explain
| return nil | ||
| } | ||
|
|
||
| func cleanupPendingDeletionOverrideMap(id int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be part of some of the previous commits right?
|
Could you reorganize your PR commits, squash the things that need to be squashed (lints, back and forth change in the code), separate the others correctly? It's a bit hard to review as of now, I see Jiri proposed some ideas. Also some commits seems misstitled ("kprobe: support 38+ override on lsm funcs"). It should appear in the patch set how we should merge it to the main branch. Thanks a lot! |
Fixes #4204
Description
As mentioned in #4204, this PR makes multiple kprobe sensors to share their
fmod_retprograms, as long as they attach to the same function, so we can allow more than 38+ overrides on lsm functions.(update Dec. 11, 2025) The changes since draft PR
The changes:
override_tasksbecomes a shared global map.Changelog