-
Notifications
You must be signed in to change notification settings - Fork 1.2k
PVF worker: Maintain lists of used syscalls #1663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
f635302
9d6d172
9d822ab
f1e9d15
6994f5d
1edf424
9c1aeae
4bd6362
7f3e387
355ab24
5882013
96a1500
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| #!/bin/sh | ||
|
|
||
| # Wrapper for building with musl. | ||
| # | ||
| # See comments for musl-gcc in this repo. | ||
|
|
||
| g++ "$@" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #!/bin/sh | ||
|
|
||
| # Wrapper for building with musl. | ||
| # | ||
| # musl unfortunately requires a musl-enabled C compiler (musl-gcc) to be | ||
| # installed, which can be kind of a pain to get installed depending on the | ||
| # distro. That's not a very good user experience. | ||
| # | ||
| # The real musl-gcc wrapper sets the correct system include paths for linking | ||
| # with musl libc library. Since this is not actually used to link any binaries | ||
| # it should most likely work just fine. | ||
|
|
||
| gcc "$@" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -504,3 +504,18 @@ cargo-hfuzz: | |
| - cargo hfuzz build | ||
| - for target in $(cargo read-manifest | jq -r '.targets | .[] | .name'); do | ||
| cargo hfuzz run "$target" || { printf "fuzzing failure for %s\n" "$target"; exit 1; }; done | ||
|
|
||
| # cf https://github.com/paritytech/polkadot-sdk/issues/1652 | ||
| test-syscalls: | ||
| stage: test | ||
| extends: | ||
| - .docker-env | ||
| - .common-refs | ||
| - .run-immediately | ||
| variables: | ||
| SKIP_WASM_BUILD: 1 | ||
| script: | ||
| - cargo build --locked --profile production --target x86_64-unknown-linux-musl --bin polkadot-execute-worker --bin polkadot-prepare-worker | ||
| - cd polkadot/scripts/list-syscalls | ||
| - ./list-syscalls.rb ../../../target/x86_64-unknown-linux-musl/production/polkadot-execute-worker --only-used-syscalls | diff -u execute-worker-syscalls - | ||
| - ./list-syscalls.rb ../../../target/x86_64-unknown-linux-musl/production/polkadot-prepare-worker --only-used-syscalls | diff -u prepare-worker-syscalls - | ||
|
Comment on lines
+520
to
+521
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a lot @altaua, it works! I'm just curious why there is a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We got some confirmation that it works. 😂 https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3941675
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @altaua It would be ideal if we had an informative message on failure, something like |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| 0 (read) | ||
| 1 (write) | ||
| 2 (open) | ||
| 3 (close) | ||
| 4 (stat) | ||
| 5 (fstat) | ||
| 7 (poll) | ||
| 8 (lseek) | ||
| 9 (mmap) | ||
| 10 (mprotect) | ||
| 11 (munmap) | ||
| 12 (brk) | ||
| 13 (rt_sigaction) | ||
| 14 (rt_sigprocmask) | ||
| 15 (rt_sigreturn) | ||
| 16 (ioctl) | ||
| 19 (readv) | ||
| 20 (writev) | ||
| 24 (sched_yield) | ||
| 25 (mremap) | ||
| 28 (madvise) | ||
| 39 (getpid) | ||
| 41 (socket) | ||
| 42 (connect) | ||
| 45 (recvfrom) | ||
| 46 (sendmsg) | ||
| 53 (socketpair) | ||
| 55 (getsockopt) | ||
| 56 (clone) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see particularly dangerous syscalls here, like Is there a possibility of further tailoring this list to the ones required strictly during PVF execution, after all the wasmtime setup is done? We only really need to install the filter right before executing the PVF, not during the prerequisite setup, which should enable us to tighten this list
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had an idea of having a "blacklist for the whitelist", that is, pick out all the I/O syscalls from this list and additionally block those. Makes sense to implement that at the same time as logging, and we'll see if any I/O calls get through the filter. For now, as we discussed, I just want some reasonable assurance that |
||
| 60 (exit) | ||
| 61 (wait4) | ||
| 62 (kill) | ||
| 72 (fcntl) | ||
| 79 (getcwd) | ||
| 82 (rename) | ||
| 83 (mkdir) | ||
| 87 (unlink) | ||
| 89 (readlink) | ||
| 96 (gettimeofday) | ||
| 97 (getrlimit) | ||
| 99 (sysinfo) | ||
| 102 (getuid) | ||
| 110 (getppid) | ||
| 131 (sigaltstack) | ||
| 140 (getpriority) | ||
| 141 (setpriority) | ||
| 144 (sched_setscheduler) | ||
| 157 (prctl) | ||
| 158 (arch_prctl) | ||
| 200 (tkill) | ||
| 202 (futex) | ||
| 204 (sched_getaffinity) | ||
| 213 (epoll_create) | ||
| 217 (getdents64) | ||
| 218 (set_tid_address) | ||
| 228 (clock_gettime) | ||
| 230 (clock_nanosleep) | ||
| 231 (exit_group) | ||
| 232 (epoll_wait) | ||
| 233 (epoll_ctl) | ||
| 257 (openat) | ||
| 262 (newfstatat) | ||
| 263 (unlinkat) | ||
| 273 (set_robust_list) | ||
| 281 (epoll_pwait) | ||
| 284 (eventfd) | ||
| 290 (eventfd2) | ||
| 291 (epoll_create1) | ||
| 302 (prlimit64) | ||
| 318 (getrandom) | ||
| 319 (memfd_create) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need those extra wrappers, do we? We're only calling gcc/g++ with no extra flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, compiling with the
x86_64-unknown-linux-musltarget will fail otherwise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it enough to set
CC_x86_64_unknown_linux_musl = { value = "gcc", force = true, relative = true }?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, looks like that works (without
relative = true), though we lose the explanatory comments in the files. For that reason I'd rather keep themusl-gccfiles.