Skip to content

pkg/bpf: Protect each uintptr with runtime.KeepAlive #10168

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 17, 2020
Merged

pkg/bpf: Protect each uintptr with runtime.KeepAlive #10168

merged 1 commit into from
Feb 17, 2020

Conversation

brb
Copy link
Member

@brb brb commented Feb 12, 2020 •
edited
Loading

The Go's GC is unable to track objects referred by uintptr [1] [2].

A consequence of this is that the GC might collect such objects when a blocking syscall is executed. This might corrupt memory of other Go objects e.g. if the reclaimed memory is used for new objects and the kernel modifies the objects in the context of the blocking syscall.

To prevent from this, we protect each pointer to objects with runtime.KeepAlive() which marks the objects as alive at least until runtime.KeepAlive() has been called.

FOR REVIEWERS:

  • Please double-check (I might have missed, and I don't see any SA tool which can check this) that each reference to objects which references are converted to uintptr and then passed to Syscall*() is protected with runtime.KeepAlive().
  • For v1.6 and v1.5 I need to create an additional backport to fix perf_linux.go which in v1.7 was replaced by cilium/ebpf. I will do it once this PR gets backported.

[1]: golang/go#13372 (comment)
[2]: https://utcc.utoronto.ca/~cks/space/blog/programming/GoUintptrVsUnsafePointer

Developed together with @aanm and @borkmann.

This change is Reviewable

@brb brb requested a review from a team February 12, 2020 16:36
@brb brb added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. needs-backport/1.7 labels Feb 12, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

2 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@brb brb added wip release-note/bug This PR fixes an issue in a previous release of Cilium. and removed pending-review labels Feb 12, 2020
@brb
Copy link
Member Author

brb commented Feb 12, 2020

test-me-please

@aanm
Copy link
Member

aanm commented Feb 12, 2020 

"github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA="'  -o maptool
# github.com/cilium/cilium/pkg/bpf
../../pkg/bpf/map_linux.go:610:2: undefined: runtime
../../pkg/bpf/map_linux.go:611:2: undefined: runtime
../../pkg/bpf/map_linux.go:612:2: undefined: runtime
../../pkg/bpf/map_linux.go:613:2: undefined: runtime
../../pkg/bpf/map_linux.go:614:2: undefined: runtime
../../pkg/bpf/map_linux.go:751:2: undefined: runtime
../../pkg/bpf/map_linux.go:752:2: undefined: runtime
../../pkg/bpf/map_linux.go:753:2: undefined: runtime
../../pkg/bpf/map_linux.go:754:2: undefined: runtime
make[1]: *** [maptool] Error 2
make[1]: Leaving directory `/home/travis/gopath/src/github.com/cilium/cilium/tools/maptool'
make: *** [unit-tests] Error 2

@florianl
Copy link
Contributor

Maybe just an idea:
By default all bpf calls are using runtime.KeepAlive() in cilium/ebpf. So maybe, this can be used and pkg/bpf/ wraps this and adds on top only the cilium specific parts, like BPFSyscallDurationEnabled.

@brb brb force-pushed the pr/brb/fix-uintptr branch from c2282f8 to 42c3226 Compare February 12, 2020 17:55
@brb
Copy link
Member Author

brb commented Feb 12, 2020

test-me-please

@brb
Copy link
Member Author

brb commented Feb 12, 2020

@florianl This is just a temporary fix. We are planning to migrate to cilium/ebpf soonish.

@joestringer
Copy link
Member

Travis failed:

# github.com/cilium/cilium/pkg/bpf
pkg/bpf/map_linux.go:611:2: unreachable code

@aanm
Copy link
Member

aanm commented Feb 13, 2020

test-me-please

@aanm aanm force-pushed the pr/brb/fix-uintptr branch from 42c3226 to da0448c Compare February 13, 2020 07:58
@brb brb force-pushed the pr/brb/fix-uintptr branch from da0448c to 4fa0da5 Compare February 13, 2020 08:07
@brb
Copy link
Member Author

brb commented Feb 13, 2020

test-me-please

@coveralls
Copy link

coveralls commented Feb 13, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.005%) to 44.468% when pulling c9db470 on pr/brb/fix-uintptr into b3a0bf3 on master.

@brb brb changed the title bpf: Protect each uintpr/unsafe.Pointer by runtime.KeepAlive [WIP] bpf: Protect each uintpr/unsafe.Pointer by runtime.KeepAlive Feb 13, 2020
@brb brb force-pushed the pr/brb/fix-uintptr branch from 4fa0da5 to d1527b6 Compare February 13, 2020 14:09
@brb
Copy link
Member Author

brb commented Feb 13, 2020

test-me-please

@brb brb force-pushed the pr/brb/fix-uintptr branch from d1527b6 to f9f49fd Compare February 13, 2020 14:37
@brb
Copy link
Member Author

brb commented Feb 13, 2020

test-me-please

@aanm aanm merged commit 9f492a1 into master Feb 17, 2020
@aanm aanm deleted the pr/brb/fix-uintptr branch February 17, 2020 13:09
@nebril nebril mentioned this pull request Feb 17, 2020
Merged
borkmann pushed a commit that referenced this pull request Feb 17, 2020
@brb @borkmann
pkg/bpf: Protect attr in perf_linux.go with runtime.KeepAlive
14887a0 
Otherise, the Go's GC might collect it while the syscall is being
executed. See #10168 for more
context.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
aanm pushed a commit that referenced this pull request Feb 17, 2020
@brb @aanm
pkg/bpf: Protect attr in perf_linux.go with runtime.KeepAlive
e66b5e7 
Otherise, the Go's GC might collect it while the syscall is being
executed. See #10168 for more
context.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@nebril nebril mentioned this pull request Feb 17, 2020
Merged
jrfastab
jrfastab reviewed Feb 20, 2020
View reviewed changes
pkg/bpf/prog_linux.go
@@ -122,6 +125,8 @@ func GetProgInfoByFD(fd int) (ProgInfo, error) {
duration = spanstat.Start()
}
ret, _, err := unix.Syscall(unix.SYS_BPF, BPF_OBJ_GET_INFO_BY_FD, uintptr(unsafe.Pointer(&attr)), unsafe.Sizeof(attr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need, runtime.KeepAlive(&attr) here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrfastab Good catch, runtime.KeepAlive(&attrInfo) below should have been runtime.KeepAlive(&attr) instead. However, neither public functions from prog_linux.go are being used, so maybe it makes sense to backport #10187 to the v1.{5,6,7} instead of fixing the dead code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree backporting dead-code removal makes sense here. I'll rework the backport series as well.

This was referenced Feb 20, 2020
Closed
Merged
@nebril nebril mentioned this pull request Feb 20, 2020
Merged
@waynr waynr mentioned this pull request Apr 28, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfastab jrfastab jrfastab left review comments

@borkmann borkmann borkmann approved these changes

@aanm aanm aanm approved these changes

@joestringer joestringer Awaiting requested review from joestringer

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@brb @aanm @florianl @joestringer @coveralls @borkmann @jrfastab @nebril