pkg/seccomp: simplify and speed up isEnabled#5256
Conversation
|
Hi @kolyshkin. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
There was a problem hiding this comment.
Do we potentially need to retry on EINTR?
There was a problem hiding this comment.
(at least) according to documentation, prctl never returns EINTR.
ea8f5ed to
39babd0
Compare
|
Added a second commit to use sync.Once |
|
#5267 may fix the test failure. |
mikebrow
left a comment
There was a problem hiding this comment.
LGTM
I note that you already changed https://github.com/containers/common/blob/master/pkg/seccomp/supported.go and runc.. and hound indicates those two will cover all of the k8s stack once these changes are vendored in.
Current implementation of seccomp.IsEnabled (rooted in runc) is not too good. First, it parses the whole /proc/self/status, adding each key: value pair into the map (lots of allocations and future work for garbage collector), when using a single key from that map. Second, the presence of "Seccomp" key in /proc/self/status merely means that kernel option CONFIG_SECCOMP is set, but there is a need to _also_ check for CONFIG_SECCOMP_FILTER (the code for which exists but never executed in case /proc/self/status has Seccomp key). Replace all this with a single call to prctl; see the long comment in the code for details. While at it, improve the IsEnabled documentation. NOTE historically, parsing /proc/self/status was added after a concern was raised in opencontainers/runc#471 that prctl(PR_GET_SECCOMP, ...) can result in the calling process being killed with SIGKILL. This is a valid concern, so the new code here does not use PR_GET_SECCOMP at all. Signed-off-by: Kir Kolyshkin <[email protected]>
It does not make sense to check if seccomp is supported by the kernel more than once per runtime, so let's use sync.Once to speed it up. A quick benchmark (old implementation, before this commit, after): BenchmarkIsEnabledOld-4 37183 27971 ns/op BenchmarkIsEnabled-4 1252161 947 ns/op BenchmarkIsEnabledOnce-4 666274008 2.14 ns/op Signed-off-by: Kir Kolyshkin <[email protected]>
39babd0 to
3292ea5
Compare
|
Rebased to current master in an attempt to fix CI failure |
|
Build succeeded.
|
1. pkg/seccomp: simplify and speed up isEnabled
Current implementation of
seccomp.isEnabled(rooted in runc) is nottoo good.
First, it parses the whole /proc/self/status, adding each key: value
pair into the map (lots of allocations and future work for garbage
collector), when using a single key from that map.
Second, the presence of "Seccomp" key in /proc/self/status merely means
that kernel option CONFIG_SECCOMP is set, but there is a need to also
check for CONFIG_SECCOMP_FILTER (the code for which exists but never
executed in case /proc/self/status has Seccomp key).
Replace all this with with one liner. Here is the entire code (copy-pasted here
mostly for the explanation it provides):
While at it, improve the IsEnabled documentation.
NOTE historically, parsing /proc/self/status was added after a concern
was raised in opencontainers/runc#471 that
prctl(PR_GET_SECCOMP, ...) can result in the calling process being
killed with SIGKILL. This is a valid concern, so the new code here
does not use PR_GET_SECCOMP at all.
2. pkg/seccomp: use sync.Once to further speed up IsEnabled
It does not make sense to check if seccomp is supported by the kernel
more than once per runtime, so let's use sync.Once to speed it up.
A quick benchmark (old implementation, before this commit, after):