Skip to content
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

Disable gVisor's DirectFS feature #898

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

EtiennePerot
Copy link
Contributor

@EtiennePerot EtiennePerot commented Aug 10, 2024

DirectFS is enabled by default in gVisor to improve I/O performance, but comes at the cost of enabling the openat(2) syscall (with severe restrictions, but still). As Dangerzone is not performance-sensitive, and that it is desirable to guarantee for the document conversion process to not open any files (to mimic some of what SELinux provides), might as well disable it by default.

See #226.

@apyrgio
Copy link
Contributor

apyrgio commented Aug 20, 2024

Thanks for the catch Etienne. I had seen this feature in a gVisor blog post, but I didn't have time to look to deep into it. I've taken a look at the rest of the gVisor flags, just to make sure we don't miss an important option, but I don't see something else we can tweak.

I'll merge this PR as soon as I run some benchmarks to ensure that the performance impact is indeed minimal for our use case (I don't expect something different to be honest).

@apyrgio apyrgio added this to the 0.8.0 milestone Sep 10, 2024
DirectFS is enabled by default in gVisor to improve I/O performance,
but comes at the cost of enabling the `openat(2)` syscall (with severe
restrictions, but still). As Dangerzone is not performance-sensitive,
and that it is desirable to guarantee for the document conversion
process to not open any files (to mimic some of what SELinux provides),
might as well disable it by default.

See freedomofpress#226.
@apyrgio
Copy link
Contributor

apyrgio commented Sep 10, 2024

I managed to run a quick benchmark for this PR. I ran our test suite on our 26 sample documents and took the following measurements:

  • Elapsed time (in seconds) with DirectFS disabled (3 runs)
  • Elapsed time (in seconds) with DirectFS enabled (3 runs)

The command I used is:

time poetry run pytest -v tests/test_cli.py::TestCliConversion::test_formats

Here are the results:

Without DirectFS With DirectFS
Run 1 84.7s 81.7s
Run 2 84.8s 80.8s
Run 3 84.0s 80.6s
Average 84.5s 81.0s
stdev 0.36s 0.48s

For this particular dataset, we notice a 5.3% speedup with DirectFS disabled. Because the second container is involved in this benchmark, which doesn't use gVisor, we can assume that the speedup is even larger. This would match the 12% speedup in the workloads that were measured in the gVisor blog post.

I think we can live with this performance penalty, if it means more security benefits in the case of a container escape.

@apyrgio
Copy link
Contributor

apyrgio commented Sep 10, 2024

Oh, btw, tested on Docker Desktop on macOS and it works as well. That's to be expected, since we remove syscalls, not add them.

@apyrgio apyrgio merged commit 73b0f8b into freedomofpress:main Sep 10, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants