Update seccompiler to use libseccomp #4926
Open
+849
−4,300
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShadowCurse
force-pushed
the
seccomp2
branch
13 times, most recently
from
6bdce02 to
b44d926
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4926 +/- ##
==========================================
- Coverage 83.95% 83.07% -0.89%
==========================================
Files 248 244 -4
Lines 27839 26634 -1205
==========================================
- Hits 23371 22125 -1246
- Misses 4468 4509 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ShadowCurse
force-pushed
the
seccomp2
branch
from
b44d926 to
6c5f77b
Compare
ShadowCurse
force-pushed
the
seccomp2
branch
5 times, most recently
from
3fb90d6 to
4ef05b8
Compare
roypat
reviewed
Nov 28, 2024
ShadowCurse
force-pushed
the
seccomp2
branch
2 times, most recently
from
50ac4e6 to
919390b
Compare
I'm curious, what does this mean? and how was this evaluated? |
ShadowCurse
force-pushed
the
seccomp2
branch
5 times, most recently
from
775c84f to
1b253fb
Compare
ShadowCurse
force-pushed
the
seccomp2
branch
from
1fc051b to
c411a78
Compare
pb8o
reviewed
Dec 12, 2024
pb8o
previously approved these changes
Dec 12, 2024
pb8o
force-pushed
the
seccomp2
branch
3 times, most recently
from
0e3f083 to
93cc0a2
Compare
roypat
previously approved these changes
Dec 16, 2024
pb8o
force-pushed
the
seccomp2
branch
3 times, most recently
from
7b1ef1c to
42787fa
Compare
roypat
previously approved these changes
Dec 17, 2024
|
(just resolved merge conflict so there's a chance my approval will survive me going on vacation) |
ShadowCurse
force-pushed
the
seccomp2
branch
4 times, most recently
from
ea82c60 to
c334e39
Compare
libseccomp provides a better quality compiler for BPF seccomp programs than our current implementation. In our testing it produces BPF code with ~65% less instructions which makes final binaries smaller which in turn makes Firecracker binary smaller because we include them into Firecracker at build time. For this transition we create a minimal set of bindings for `libseccomp` in order to simplify maintenance and avoid adding additional dependencies. The only tricky issue with this transition is the way `ioctl` and other syscalls are checked with libseccomp. It always adds a check for the high bits of the request to be 0. Unfortunately when we build with `musl`, some syscalls like `ioctl` have upper bits set to 1. Because of this, we replace `Eq` with `MaskedEq` with mask `0x00000000FFFFFFFF` when the argument is 32bits. This commit also removes dependency of firecracker and vmm crates on the seccompiler crate. Co-authored-by: Pablo Barbáchano <[email protected]> Signed-off-by: Egor Lazarchuk <[email protected]>
Since we depend on libseccomp in the previous commit, these commands to update the syscall table are no longer needed. Signed-off-by: Pablo Barbáchano <[email protected]>
According to https://www.man7.org/linux/man-pages/man2/PR_SET_SECCOMP.2const.html using `prctl` for setting seccomp filer is deprecated, so switch to using `syscall` instead. Signed-off-by: Egor Lazarchuk <[email protected]>
Replace __errno_location() with std::io::Error::last_os_error() as a more standard of getting errno value. Signed-off-by: Egor Lazarchuk <[email protected]>
The error enum had only 1 element and we can replace it with alias for simplicity. Signed-off-by: Egor Lazarchuk <[email protected]>
Add a note about updating backend for seccompiler to libseccomp. Signed-off-by: Egor Lazarchuk <[email protected]>
We need to compile it from source because version provided by the distribution is not compiled with musl-gcc. Signed-off-by: Egor Lazarchuk <[email protected]>
Kani on x86 for some reason cannot find libseccomp by default, so we add additional path to the build.rs Signed-off-by: Egor Lazarchuk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Replace our custom implementation of a seccompiler with libseccomp.
By out test,
libseccompproduces ~65% smaller binaries which is very beneficial as we embed the combination of those into the Firecracker.In order to interact with
libseccompwe add custom bindings which contain only the needed set of methods, constants needed for our use case. This simplifies the maintenance and avoids adding dependencies.Reason
libseccompprovides better quality compiler for BPF seccomp programs than our current implementation.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.