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: Add support for multi kprobe override #1218

Merged
merged 7 commits into from
Jul 28, 2023

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jul 12, 2023

Adding support to load override helper for kprobe.multi attached
kprobes.

@olsajiri olsajiri force-pushed the multi_kprobe_override branch 4 times, most recently from a028c22 to d0f9e19 Compare July 13, 2023 13:35
@olsajiri olsajiri changed the title tetragon: Add support fomr multi kprobe override tetragon: Add support for multi kprobe override Jul 24, 2023
@olsajiri olsajiri force-pushed the multi_kprobe_override branch 3 times, most recently from 57ab859 to 26d2862 Compare July 25, 2023 12:53
@olsajiri olsajiri marked this pull request as ready for review July 25, 2023 13:27
@olsajiri olsajiri requested a review from a team as a code owner July 25, 2023 13:27
Copy link
Member

@tpapagian tpapagian left a comment

Choose a reason for hiding this comment

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

Except for some minor comments, overall LGTM, thanks!

pkg/sensors/tracing/kprobe_test.go Show resolved Hide resolved
pkg/sensors/tracing/kprobe_test.go Outdated Show resolved Hide resolved
pkg/sensors/program/loader.go Outdated Show resolved Hide resolved
In case the override helper is not supported we can get verifier
failure when loading kprobe multi object.

Adding the code to skip the loading of override program the same
way we do for normal kprobes.

Fixes: 840b12d ("tetragon: Move override setup into kprobe open/attach functions")
Signed-off-by: Jiri Olsa <[email protected]>
We will need to load override program with kprobe.multi
interface in following changes.

Signed-off-by: Jiri Olsa <[email protected]>
Adding support to load override helper for kprobe.multi attached
kprobes.

Signed-off-by: Jiri Olsa <[email protected]>
Adding override support for kprobes attached with kprobe.multi interface.

Signed-off-by: Jiri Olsa <[email protected]>
Let's use multi object even for single kprobe, the kprobe-multi
interface should be faster than generic kprobe attach.

Signed-off-by: Jiri Olsa <[email protected]>
Adding verbose out about call being overridden for both
single and multi kprobes, like:

    logcapture.go:25: time="2023-07-25T12:22:44Z" level=info msg="Added multi kprobe" function=__x64_sys_openat override=true return=true
    logcapture.go:25: time="2023-07-25T12:22:44Z" level=info msg="Added multi kprobe" function=__x64_sys_linkat override=true return=false
    logcapture.go:25: time="2023-07-25T12:22:44Z" level=info msg="Added multi kprobe" function=__x64_sys_symlinkat override=true return=false
    logcapture.go:25: time="2023-07-25T12:22:44Z" level=info msg="Added multi kprobe" function=__x64_sys_renameat override=false return=false

Signed-off-by: Jiri Olsa <[email protected]>
Add override test for multiple kprobes to verify the
kprobe multi interface handles override properly.

The test hooks on 4 syscalls and override 3 of them.

  sys_openat        override with -1
  sys_linkat        override with -2
  sys_symlinkat     override with -3
  sys_renameat      no override

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

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

There is small rant below, as I was trying to understand the code, but nothing obvious to be addressed in this PR.

pkg/sensors/tracing/generickprobe.go Show resolved Hide resolved
@kkourt kkourt merged commit 8d3cdcb into cilium:main Jul 28, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants