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

Add support for attaching uprobes and uretprobes to offsets #1419

Merged
merged 14 commits into from
Dec 6, 2024

Conversation

rafaelroquetto
Copy link
Contributor

@rafaelroquetto rafaelroquetto commented Nov 28, 2024

In its core, this PR adds support for attaching ebf programs to (start and return) offsets, in the same fashion done for "goprobes". The motivation is also the same: uretprobes attachments can be unreliable in contexts where the target process stack changes. In such scenarios, attaching uretprobes may cause the target process to crash.

Under the hood, we scan the target binary for the offsets of the symbols we want to attach to: the start offset, and the offset of every RET instruction present in that function. This approach is not flawless, and will fail if the the function/symbol in question does not posses any RET instruction due to compiler optimisations (see here for an example).

Due to the requirement of handling uprobe offsets, codewise the handling of uprobes and goprobes becomes less distinct. The ultimate goal would be to merge their corresponding code and remove considerable code duplication. A central piece for this is the repurposing of ebpfcommon.FuncPrograms into a more comprehensive data structure that carries the entire context (relevant to our use cases) of ebpf attachments: apart from the start and end programs, it now features the symbol name and the offsets. This also enables us to ditch a few maps that were being used for this purpose, and simplifies the code considerably.

@rafaelroquetto rafaelroquetto added the do-not-merge WIP or something that musn't be merged label Nov 28, 2024
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 77.04918% with 70 lines in your changes missing coverage. Please review.

Project coverage is 80.93%. Comparing base (21f9154) to head (1e50a27).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/internal/ebpf/instrumenter.go 71.06% 31 Missing and 15 partials ⚠️
pkg/internal/ebpf/generictracer/generictracer.go 87.64% 7 Missing and 4 partials ⚠️
pkg/internal/goexec/instructions.go 58.33% 3 Missing and 2 partials ⚠️
pkg/internal/ebpf/httptracer/httptracer.go 57.14% 3 Missing ⚠️
pkg/internal/ebpf/tctracer/tctracer.go 57.14% 3 Missing ⚠️
pkg/internal/ebpf/gotracer/gotracer.go 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
- Coverage   81.07%   80.93%   -0.14%     
==========================================
  Files         149      149              
  Lines       15130    15250     +120     
==========================================
+ Hits        12266    12342      +76     
- Misses       2272     2301      +29     
- Partials      592      607      +15     
Flag Coverage Δ
integration-test 59.83% <71.47%> (+0.08%) ⬆️
k8s-integration-test 59.35% <75.73%> (-1.24%) ⬇️
oats-test 33.92% <66.55%> (+0.28%) ⬆️
unittests 51.92% <24.26%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rafaelroquetto rafaelroquetto force-pushed the newretprobes2 branch 4 times, most recently from 720fc67 to bf7c80d Compare November 30, 2024 01:39
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Very cool refactor and so nice to see we use the same mechanism now for Go and non-Go.

@rafaelroquetto rafaelroquetto removed the do-not-merge WIP or something that musn't be merged label Dec 6, 2024
@rafaelroquetto rafaelroquetto marked this pull request as ready for review December 6, 2024 22:09
@rafaelroquetto rafaelroquetto changed the title WIP: Add support for attaching uprobes and uretprobes to offsets Add support for attaching uprobes and uretprobes to offsets Dec 6, 2024
@rafaelroquetto rafaelroquetto merged commit a6d91fd into main Dec 6, 2024
13 checks passed
@rafaelroquetto rafaelroquetto deleted the newretprobes2 branch December 6, 2024 22:52
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.

2 participants