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

guest memory: place stack below text/code #950

Merged
merged 14 commits into from
Oct 7, 2023
Merged

guest memory: place stack below text/code #950

merged 14 commits into from
Oct 7, 2023

Conversation

SchmErik
Copy link
Contributor

@SchmErik SchmErik commented Oct 5, 2023

The previous memory layout was prone to a security vulnerability because the
stack could collide with the heap. This change places the stack below
TEXT_START with enough space for 2 MB of stack. This also results in increased
locality of memory which will result in less page faults for small programs.

@SchmErik
Copy link
Contributor Author

SchmErik commented Oct 5, 2023

@intoverflow I'm not sure if 2MB is large enough for zeth... Feel free to chime in and let us know if it's not enough!

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Benchmark for Linux-cuda 29d0914

Click to hide benchmark
Test Base PR %
fib/100/execute 5.2±0.11ms 5.2±0.08ms 0.00%
fib/100/prove 791.8±1.66ms 724.2±5.75ms -8.54%
fib/100/total 796.2±2.72ms 726.0±3.98ms -8.82%
fib/1000/execute 5.9±0.11ms 5.8±0.13ms -1.69%
fib/1000/prove 818.7±3.01ms 755.4±4.46ms -7.73%
fib/1000/total 824.5±2.27ms 759.6±5.03ms -7.87%
fib/10000/execute 12.1±0.11ms 11.7±0.15ms -3.31%
fib/10000/prove 2.8±0.01s 2.5±0.01s -10.71%
fib/10000/total 2.8±0.01s 2.5±0.01s -10.71%

Benchmark for Linux-default 29d0914

Click to hide benchmark
Test Base PR %
fib/100/execute 6.7±0.46ms 6.6±0.44ms -1.49%
fib/100/prove 5.1±0.03s 5.0±0.02s -1.96%
fib/100/total 5.1±0.03s 5.0±0.02s -1.96%
fib/1000/execute 7.8±0.51ms 7.6±0.48ms -2.56%
fib/1000/prove 5.1±0.03s 5.1±0.02s 0.00%
fib/1000/total 5.1±0.03s 5.1±0.03s 0.00%
fib/10000/execute 14.3±0.48ms 14.0±0.47ms -2.10%
fib/10000/prove 20.9±0.10s 20.9±0.08s 0.00%
fib/10000/total 21.0±0.10s 20.9±0.15s -0.48%

Benchmark for macOS-default

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

Benchmark for macOS-metal

    <details open>
      <summary>Click to hide benchmark</summary>
      Benchmarks have changed between the two branches, unable to diff.
    </details>

@Wollac
Copy link
Contributor

Wollac commented Oct 6, 2023

I'm not sure if 2MB is large enough for zeth... Feel free to chime in and let us know if it's not enough!

There is a test https://github.com/risc0/zeth/blob/main/testing/ef-tests/tests/executor.rs that attempts to run an artificial transaction that should require a lot of stack in the guest. It might be worth checking these changes against this test. If it passes, I think we are pretty sure that it should work for real Ethereum blocks as well.

@flaub flaub enabled auto-merge October 7, 2023 04:11
@flaub flaub added this pull request to the merge queue Oct 7, 2023
Merged via the queue into main with commit e37024b Oct 7, 2023
16 checks passed
@flaub flaub deleted the erik/new-mem-layout branch October 7, 2023 05:35
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.

None yet

3 participants