Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

link: support for kprobe session #1623

Merged
merged 2 commits into from
Jan 13, 2025
Merged

link: support for kprobe session #1623

merged 2 commits into from
Jan 13, 2025

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Dec 10, 2024

kprobe session support plus 6.11 kernel update

@olsajiri olsajiri requested review from ti-mo, dylandreimerink and a team as code owners December 10, 2024 14:24
@ti-mo ti-mo changed the title Session update-kernel-deps: fetch objects from local Linux checkout Dec 10, 2024
@olsajiri olsajiri marked this pull request as draft December 10, 2024 19:46
@olsajiri olsajiri changed the title update-kernel-deps: fetch objects from local Linux checkout add support for kprobe session Dec 29, 2024
@olsajiri olsajiri force-pushed the session branch 2 times, most recently from 2a2f948 to d02c6b1 Compare December 29, 2024 10:35
@olsajiri olsajiri marked this pull request as ready for review December 29, 2024 11:15
@olsajiri olsajiri requested a review from mmat11 as a code owner December 29, 2024 11:15
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

I find it a bit of a missed opportunity that this requires user space to specify a different attach type to the link as it invites unnecessary complexity, adding corner cases to otherwise straightforward logic.

The kernel could attach the prog to entry and return if prog->expected_attach_type is KPROBE_SESSION. What was the rationale here? Backwards compat?

link/kprobe_multi.go Show resolved Hide resolved
link/kprobe_multi.go Show resolved Hide resolved
link/kprobe_multi.go Outdated Show resolved Hide resolved
@ti-mo ti-mo changed the title add support for kprobe session link: support for kprobe session Jan 8, 2025
@olsajiri
Copy link
Contributor Author

olsajiri commented Jan 8, 2025

Thanks for the patch!

I find it a bit of a missed opportunity that this requires user space to specify a different attach type to the link as it invites unnecessary complexity, adding corner cases to otherwise straightforward logic.

The kernel could attach the prog to entry and return if prog->expected_attach_type is KPROBE_SESSION. What was the rationale here? Backwards compat?

I think the reason was to keep it similar to other links with having link->attach_type == prog->expected_attach_type,
I recall discussion about that, but can't find :-\ sry

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@ti-mo ti-mo force-pushed the session branch 3 times, most recently from 450704e to 638d6b6 Compare January 13, 2025 12:44
…sion)

Adding support to attach kprobe multi link as session [1] by adding
Session bool to KprobeMultiOptions. When set true it attaches the
link with BPF_TRACE_KPROBE_SESSION attach_type, which means:

  - program is attached to both function entry and return
  - entry program can decided if the return program gets executed
  - entry program can share u64 cookie value with return program

[1] https://lore.kernel.org/bpf/20240430112830.1184228-1-jolsa@kernel.org/

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Co-authored-by: Timo Beckers <timo@isovalent.com>
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

@olsajiri I've made a few changes:

  • Session landed in 6.10, not 6.9.
  • TestKprobeSession is now gated on haveBPFLinkKprobeMulti() (instead of the new haveBPFLinkKprobeSession()) so it's also exercised on kernels with multi but without session. This makes sure KprobeMulti correctly returns ErrNotSupported on those kernels, which was previously broken, see next:
  • Feature probes are run before the blanket EINVAL check, which would previously always return the missing kernel symbol or prog... error instead of returning ErrNotSupported in the error path.
  • Added TestHaveBPFLinkKprobeSession to test the feature probe.

Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Latest version looks good to me

@ti-mo ti-mo merged commit 17797ff into cilium:main Jan 13, 2025
17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants