util: Fix GUIX build with syscall sandbox #23178
Merged
Conversation
This file contains hidden or 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
Member
Author
|
Thinking of it, I'm not sure this is the right solution. Might be better to do …instead Edit: pushed this new solution |
Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers, as is the case for the GUIX build on this platform.
03518b6 to
5f626ac
Compare
5f626ac to
1685d12
Compare
maflcko
pushed a commit
that referenced
this pull request
Oct 5, 2021
…d filesystem syscalls 44d77d2 sandbox: add copy_file_range to allowed filesystem syscalls (fanquake) ee08741 sandbox: add newfstatat to allowed filesystem syscalls (fanquake) Pull request description: Similar to #23178, this is a follow up to #20487, which has broken running the unit tests for some developers. Fix this by adding `newfstatat` to the list of allowed filesystem related calls. ACKs for top commit: achow101: ACK 44d77d2 laanwj: Code review ACK 44d77d2 practicalswift: cr ACK 44d77d2 Tree-SHA512: ce9d1b441ebf25bd2cf290566e05864223c1418dab315c962e1094ad877db5dd9fcab94ab98a46da8b712a8f5f46675d62ca3349215d8df46ec5b3c4d72dbaa6
practicalswift
approved these changes
Oct 5, 2021
Contributor
practicalswift
left a comment
There was a problem hiding this comment.
cr ACK 1685d1221e7e605ff073df94e223420691afa079
This might be helpful for fellow reviewers:
$ grep -E '(statx|getrandom|membarrier)$' linux/arch/x86/entry/syscalls/syscall_64.tbl
318 common getrandom sys_getrandom
324 common membarrier sys_membarrier
332 common statx sys_statx
$ grep SECCOMP_RET_KILL_PROCESS linux/include/uapi/linux/seccomp.h
#define SECCOMP_RET_KILL_PROCESS 0x80000000U /* kill the process */
Member
Contributor
|
@MarcoFalke Yes, I think these two no longer need to be conditional: #if defined(__NR_getrandom)
{__NR_getrandom, "getrandom"},
#endif // defined(__NR_getrandom)
…
#if defined(__NR_membarrier)
{__NR_membarrier, "membarrier"},
#endif // defined(__NR_membarrier) |
Member
Author
I'm still not decided what I want to do with that table, but i i's supposed to be more or less platform-independent (see also discussion here: #20487 (comment) ). I left the conditional like this for platforms that really don't have the getrandom/membarrier call. |
Define the following syscall numbers for x86_64, so that the profile will be the same no matter what kernel is built against, including kernels that don't have `__NR_statx`: ```c++ #define __NR_statx 332 #define __NR_getrandom 318 #define __NR_membarrier 324 ```
1685d12 to
2d02799
Compare
Member
Author
|
Anyhow, removed them and force-pushed. I agree it's also somewhat confusing and they can always be added again. |
Contributor
|
cr ACK 2d02799 Thanks for quickly resolving this! |
Contributor
|
FWIW this is how |
Member
Author
|
Thanks—we could do the same when we support sandboxing on multiple platforms. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 5, 2021
… allowed filesystem syscalls 44d77d2 sandbox: add copy_file_range to allowed filesystem syscalls (fanquake) ee08741 sandbox: add newfstatat to allowed filesystem syscalls (fanquake) Pull request description: Similar to bitcoin#23178, this is a follow up to bitcoin#20487, which has broken running the unit tests for some developers. Fix this by adding `newfstatat` to the list of allowed filesystem related calls. ACKs for top commit: achow101: ACK 44d77d2 laanwj: Code review ACK 44d77d2 practicalswift: cr ACK 44d77d2 Tree-SHA512: ce9d1b441ebf25bd2cf290566e05864223c1418dab315c962e1094ad877db5dd9fcab94ab98a46da8b712a8f5f46675d62ca3349215d8df46ec5b3c4d72dbaa6
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 5, 2021
2d02799 util: Make sure syscall numbers used in profile are defined (W. J. van der Laan) 8289d19 util: Define SECCOMP_RET_KILL_PROCESS if not provided by the headers (W. J. van der Laan) Pull request description: Looks like we've broke the GUIX build in bitcoin#20487. This attempts to fix it: - Define `__NR_statx` `__NR_getrandom` `__NR_membarrier` as some kernel headers lack them, and it's important to have the same profile independent on what kernel is used for building. - Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers. ACKs for top commit: practicalswift: cr ACK 2d02799 Tree-SHA512: c264c66f90af76bf364150e44d0a31876c2ef99f05777fcdd098a23f1e80efef43028f54bf9b3dad016110056d303320ed9741b0cb4c6266175fa9d5589b4277
Contributor
Guix builds
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Looks like we've broke the GUIX build in #20487. This attempts to fix it:
__NR_statx__NR_getrandom__NR_membarrieras some kernel headers lack them, and it's important to have the same profile independent on what kernel is used for building.SECCOMP_RET_KILL_PROCESSas it isn't defined in the headers.