seccomp: do not ignore SPEC_ALLOW flag #3580
Conversation
Commit 58ea21d added support for seccomp flags such as SPEC_ALLOW, but it does not work as expected, because since commit 7a8d716 we do not use libseccomp-golang's Load(), but handle flags separately in patchbfp. This fixes setting SPEC_ALLOW flag. Add a comment to not forget to amend filterFlags when adding new flags. Signed-off-by: Kir Kolyshkin <[email protected]>
|
Would it be possible to have a test? |
| @@ -641,8 +646,13 @@ func filterFlags(config *configs.Seccomp, filter *libseccomp.ScmpFilter) (flags | |||
| flags |= uint(C.C_FILTER_FLAG_LOG) | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Support seccomp flags not yet added to libseccomp-golang... | |||
There was a problem hiding this comment.
is this the only flag not supported by libseccomp-golang? If not, is this comment still relevant?
There was a problem hiding this comment.
Currently, libseccomp-golang is on par with the runtime-spec, thus I removed this comment.
There was a problem hiding this comment.
Currently correct, but I'd like support for SECCOMP_FILTER_FLAG_WAIT_KILLABLE_RECV (Linux 5.19, torvalds/linux@c2aa2dfef243).
opencontainers/runtime-spec#1161
Not yet in libseccomp: seccomp/libseccomp#387
There was a problem hiding this comment.
Right, but I guess we'll support it in libseccomp and libseccomp-golang first, when here in runc.
I can't think of a way to check if SPEC_ALLOW (or any other flag) is set in a particular ebpf loaded into the kernel, thus no way to test this. If anyone knows such a way, please share. |
Also, we already have a test, added in #3390. The problem is, it doesn't check whether the flags are actually set -- merely that they are accepted. Hmm, we can actually use strace to check which flags are being set! Let me see... |
|
It is still possible to use strace from the test, but I did something different -- modified the code to actually debug print the flags value, and amended the seccomp flags setting test to check if the flags value is as expected. @AkihiroSuda PTAL |
kolyshkin
changed the title
kolyshkin
changed the title
|
Thanks, LGTM but a couple of nits |
Add a debug print of seccomp flags value, so the test can check those (without using something like strace, that is). Amend the flags setting test with the numeric values expected, and the logic to check those. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
fix-seccomp-ssb
branch
from
edb41fc
to
26dc55e
Compare
Addressed. |
kolyshkin
requested review from
AkihiroSuda
and removed request for
haircommander
|
CentOS 7 failure is a flake (#3540); restarted. |
|
I am not a maintainer and I didn't test it but the patch looks good to me. |
Commit 58ea21d added support for seccomp flags such as
SPEC_ALLOW, but it does not work, as since commit 7a8d716
we do not use libseccomp-golang's Load(), but handle flags separately in patchbfp.
This fixes setting SPEC_ALLOW flag.
Add a comment to not forget to amend
filterFlagswhen adding new flags.Add a test case that the flags are set as expected (see the second commit for details).
Fixes: #3582