Add available LinuxSeccompFlags
#1138
Conversation
saschagrunert
requested review from
crosbymichael,
cyphar,
dqminh,
giuseppe,
hqhq,
mrunalp,
tianon and
vbatts
as code owners
specs-go/config.go
Outdated
| // LinuxSeccompFlagTsync is the filter to synchronize all other threads of | ||
| // the calling process to the same seccomp filter tree. A "filter tree" is | ||
| // the ordered list of filters attached to a thread. (Attaching identical | ||
| // filters in separate seccomp() calls results in different filters from | ||
| // this perspective.) | ||
| LinuxSeccompFlagTsync LinuxSeccompFlag = "SECCOMP_FILTER_FLAG_TSYNC" |
There was a problem hiding this comment.
With @rata we were discussing whether to remove tsync from the spec. In the context of container runtimes, the process will call seccomp() before exec() to the entrypoint, so all other threads, if any, will be terminated anyway, so tsync does not make a difference.
Examples:
- AFAIU crun is not multithreaded, so tsync is meaningless
- runc is written in Go so multithreaded, but uses libseccomp-golang that always set tsync: it's not configurable because threads and Go routine are not necessarily tightly coupled.
There was a problem hiding this comment.
The spec still lists all three as available:
https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#seccomp
I'm a bit concerned that it will cause confusion if we remove it.
There was a problem hiding this comment.
I've added it without thinking if it made any sense as I've copied all the existing flags. I agree that it is pointless in the OCI specs, and we could drop it.
There was a problem hiding this comment.
I don't think it should be removed, I think it should be marked as deprecated.
See this: #1077
There was a problem hiding this comment.
Good, removed it from the code now.
saschagrunert
force-pushed
the
seccomp-filter-flags
branch
from
a8ee163 to
b668209
Compare
specs-go/config.go
Outdated
| // LinuxSeccompFlagTsync is the filter to synchronize all other threads of | ||
| // the calling process to the same seccomp filter tree. A "filter tree" is | ||
| // the ordered list of filters attached to a thread. (Attaching identical | ||
| // filters in separate seccomp() calls results in different filters from | ||
| // this perspective.) | ||
| LinuxSeccompFlagTsync LinuxSeccompFlag = "SECCOMP_FILTER_FLAG_TSYNC" |
There was a problem hiding this comment.
I don't think it should be removed, I think it should be marked as deprecated.
See this: #1077
specs-go/config.go
Outdated
| // LinuxSeccompFlagTsync is the filter to synchronize all other threads of | ||
| // the calling process to the same seccomp filter tree. A "filter tree" is | ||
| // the ordered list of filters attached to a thread. (Attaching identical | ||
| // filters in separate seccomp() calls results in different filters from | ||
| // this perspective.) |
There was a problem hiding this comment.
As I pointed out in another PR doing the same, I don't think we should add a const for this: #1108 (comment)
We now list the available `LinuxSeccompFlag` values as part of the runtime spec. Signed-off-by: Sascha Grunert <[email protected]> Co-authored-by: Alban Crequy <[email protected]> Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
seccomp-filter-flags
branch
from
b668209 to
e78a3c3
Compare
|
@saschagrunert thanks, this SGTM. It is also handling the comments that were raised by @tianon in the other PR. I'd cc @thaJeztah and @tianon just in case. I don't mind which PR ends up being merged, but IMO there should be some coordination with authors of the other PR :) |
| // override this filter flag by preventing specific actions from being | ||
| // logged via the /proc/sys/kernel/seccomp/actions_logged file. (since | ||
| // Linux 4.14) | ||
| LinuxSeccompFlagLog LinuxSeccompFlag = "SECCOMP_FILTER_FLAG_LOG" |
There was a problem hiding this comment.
@thaJeztah since the name of these variables is different than in your PR (FlagLog and FlagAllow) I'm wondering if Docker/moby has already used your variables?
I'm inclined to merge this PR given the robust comments and that it drops the TS_SYNC flag that @rata called out
|
ping @opencontainers/runtime-spec-maintainers |
We now list the available
LinuxSeccompFlagvalues as part of the runtime spec.cc @alban