-
Notifications
You must be signed in to change notification settings - Fork 721
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
Prevent pprof from causing BPF verifier livelocks #805
Conversation
Signed-off-by: Timo Beckers <[email protected]>
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.
Very nice, one of those PRs where the change is tiny but the impact is big!
internal/unix/signals.go
Outdated
@@ -0,0 +1,41 @@ | |||
package unix |
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.
Can you move our own new types from internal/unix
to internal/sys
? The former should only ever contain wrappers to make macOS users happy.
- Makes it easier to get rid of
internal/unix
once I figure out how to get gopls to pass linux build tags when editing. - Avoids
signals_other.go
- Allows unexporting API
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.
once I figure out how to get gopls to pass linux build tags when editing.
In VSCode, I use:
"gopls": {
"build.buildFlags": [
"-tags=linux,amd64,integration"
],
},
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.
Avoids
signals_other.go
How, though? Package sys
also needs to build on macOS, so we'd still need a signals_other.go
there.
Edit: you mean to fold signals_other.go
into types_other.go
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.
I had forgotten about SIG_BLOCK and friends, sorry. I'll try to submit another CL that add them to x/sys, but let's keep SIG_BLOCK in internal/unix for now.
0bd797e
to
1caaad1
Compare
Signed-off-by: Timo Beckers <[email protected]>
This patch implements the sigsetAdd function to add a signal to a signal set. Care was taken to make this build and run correctly on different GOOS and GOARCH combinations. Signed-off-by: Timo Beckers <[email protected]>
sys.maskProfilerSignal() locks the calling goroutine to its underlying OS thread and blocks the SIGPROF signal from interrupting the thread. sys.unmaskProfilerSignal() reverses the operation and should be used in tandem to properly restore OS thread state afterwards. Add a wrapper around unix.PthreadSigmask added in a recent version of x/sys. Signed-off-by: Timo Beckers <[email protected]>
If a thread receives a signal while blocked in BPF_PROG_LOAD, the verifier can cooperatively interrupt itself by checking pending signals for the thread and return -EAGAIN from the syscall to request userspace to retry. When a Go program is built and run with pprof enabled, threads are routinely sent a SIGPROF to make them dump profiling information, which can lead to a runaway reaction if the program takes longer to verify than the interrupt frequency. To prevent this, mask SIGPROF before calling BPF_PROG_LOAD and remove it when done. Signed-off-by: Timo Beckers <[email protected]>
1caaad1
to
ef08703
Compare
unmaskProfilerSignal() | ||
|
||
var old unix.Sigset_t | ||
if err := unix.PthreadSigmask(0, nil, &old); err != nil { |
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.
Neat!
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.
🚢 it!
Read https://dxuuu.xyz/bpf-go-pprof.html for more context.
If a thread receives a signal while blocked in BPF_PROG_LOAD, the verifier can cooperatively interrupt itself by checking pending signals for the thread and return -EAGAIN from the syscall to request userspace to retry.
When a Go program is built and run with pprof enabled, threads are routinely sent a SIGPROF to make them dump profiling information, which can lead to a runaway reaction if the program takes longer to verify than the interrupt frequency. To prevent this, mask SIGPROF before calling BPF_PROG_LOAD and remove it when done.
@danobi