Skip to content

Conversation

fanquake
Copy link
Member

After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e firejail.

There is more related discussion in #24771.

Note that given where it's used, the sandbox also gets dragged into the kernel.

If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.

Closes #24771.

After initially being merged in bitcoin#20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.

Note that given where it's used, the sandbox also gets dragged into the
kernel.

There is some related discussion in bitcoin#24771.

This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.

Closes bitcoin#24771.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 16, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge, davidgumberg, achow101
Concept ACK TheCharlatan, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27861 (kernel: Add interrupt class by TheCharlatan)
  • #27854 ([WIP] add a stratum v2 template provider by ccdle12)
  • #27607 (index: make startup more efficient by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@dergoegge
Copy link
Member

Concept ACK 🟥🟥🟥

@hebasto
Copy link
Member

hebasto commented Jun 24, 2023

Concept ACK.

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 32e2ffc

@davidgumberg
Copy link
Contributor

crACK 32e2ffc

Nit: We may want to remove BCLog::Util as well, since SetSyscallSandboxPolicy was its only user.

@DrahtBot
Copy link
Contributor

Guix builds

File commit 296735f
(master)
commit 91706d1
(master and this pull)
SHA256SUMS.part c65f335e7527365f... 47a237496379779c...
*-aarch64-linux-gnu-debug.tar.gz 5a0fe9c9d1efc225... 97fe9e1014216595...
*-aarch64-linux-gnu.tar.gz 05170c9b821d3d15... 1e8b8c30ec0c799c...
*-arm-linux-gnueabihf-debug.tar.gz f7e39f68b22907e5... 634a0a685ce46367...
*-arm-linux-gnueabihf.tar.gz e674a6f14867b5d4... 34dffe5b68e10ba4...
*-arm64-apple-darwin-unsigned.dmg 8380f18bd9bf08a5... 0592cad94c0701b8...
*-arm64-apple-darwin-unsigned.tar.gz f4e10c4baf370d3c... 24470e1acdaf64e8...
*-arm64-apple-darwin.tar.gz 10683ff96986df42... 228517d5a4ec2cdc...
*-powerpc64-linux-gnu-debug.tar.gz 62ad0ba588090ff1... 14597f966dcf36d7...
*-powerpc64-linux-gnu.tar.gz 972edd6915d01cf1... 6848ed76574cea79...
*-powerpc64le-linux-gnu-debug.tar.gz d49ee3a837144285... ccdae1099b66398c...
*-powerpc64le-linux-gnu.tar.gz 189b13c7a4df3f82... d374f8da1798a525...
*-riscv64-linux-gnu-debug.tar.gz 767845df189ec4e8... c7ff5e0f08a7790f...
*-riscv64-linux-gnu.tar.gz 3a49a981a56cbc6b... 74e5aec8ef960691...
*-x86_64-apple-darwin-unsigned.dmg 946d54e9a8754e18... 696a73f6cf4b1908...
*-x86_64-apple-darwin-unsigned.tar.gz 9ee286d80a9134b4... 178caa484bcc938d...
*-x86_64-apple-darwin.tar.gz af85c63c8384691d... 693f164d6235f0ea...
*-x86_64-linux-gnu-debug.tar.gz 9eaab59941d2989c... 8fb8ad47f07bc2cd...
*-x86_64-linux-gnu.tar.gz 448ab0cd5f64b0ba... 59a62838ee5c3181...
*.tar.gz e55d3332038ad8e6... a805c237d32cb819...
guix_build.log d94426f4f609f4f9... fd5013e8161d2c1c...
guix_build.log.diff 18742188c7b716ba...

@achow101
Copy link
Member

ACK 32e2ffc

The syscall sandbox has a rather significant maintenance burden for rather limited benefit.

@achow101 achow101 merged commit caff95a into bitcoin:master Jun 27, 2023
@fanquake fanquake deleted the remove_syscall_sandbox branch June 28, 2023 10:15
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 30, 2023
fanquake added a commit to fanquake/oss-fuzz that referenced this pull request Nov 7, 2023
DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Nov 7, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outcome of the syscall sandbox experiment
8 participants