Stream: git-wasmtime

Topic: wasmtime / PR #2977 Add guard pages to the front of linea...

Wasmtime GitHub notifications bot (Jun 10 2021 at 17:58):

alexcrichton opened PR #2977 from pre-guard to main:

This commit implements a safety feature for Wasmtime to place guard
pages before the allocation of all linear memories. Guard pages placed
after linear memories are typically present for performance (at least)
because it can help elide bounds checks. Guard pages before a linear
memory, however, are never strictly needed for performance or features.
The intention of a preceding guard page is to help insulate against bugs
in Cranelift or other code generators, such as CVE-2021-32629.

This commit adds a Config::guard_before_linear_memory configuration
option, defaulting to true, which indicates whether guard pages should
be present both before linear memories as well as afterwards. Guard
regions continue to be controlled by
{static,dynamic}_memory_guard_size methods.

The implementation here affects both on-demand allocated memories as
well as the pooling allocator for memories. For on-demand memories this
adjusts the size of the allocation as well as adjusts the calculations
for the base pointer of the wasm memory. For the pooling allocator this
will place a singular extra guard region at the very start of the
allocation for memories. Since linear memories in the pooling allocator
are contiguous every memory already had a preceding guard region in
memory, it was just the previous memory's guard region afterwards. Only
the first memory needed this extra guard.

I've attempted to write some tests to help test all this, but this is
all somewhat tricky to test because the settings are pretty far away
from the actual behavior. I think, though, that the tests added here
should help cover various use cases and help us have confidence in
tweaking the various Config settings beyond their defaults.

Note that this also contains a semantic change where
InstanceLimits::memory_reservation_size has been removed. Instead this
field is now inferred from the static_memory_maximum_size and guard
size settings. This should hopefully remove some duplication in these
settings, canonicalizing on the guard-size/static-size settings as the
way to control memory sizes and virtual reservations.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 10 2021 at 18:10):

alexcrichton updated PR #2977 from pre-guard to main.

Wasmtime GitHub notifications bot (Jun 10 2021 at 18:47):

alexcrichton updated PR #2977 from pre-guard to main.

Wasmtime GitHub notifications bot (Jun 10 2021 at 19:50):

alexcrichton updated PR #2977 from pre-guard to main.

Wasmtime GitHub notifications bot (Jun 10 2021 at 20:54):

alexcrichton updated PR #2977 from pre-guard to main.

Wasmtime GitHub notifications bot (Jun 17 2021 at 14:59):

alexcrichton requested peterhuene for a review on PR #2977.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:18):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:18):

peterhuene created PR review comment:

This default value of 1 GiB for 32-bit hosts was the reason the pooling allocator didn't use static_memory_bound, as there isn't enough address space for a default of 1000 instances (and 2 seems like a really silly default for the count as an alternative).

Thoughts on this?

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:32):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:32):

peterhuene created PR review comment:

This name is a little confusing to me (specifically "unguarded"); perhaps simply mapping_start?

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:40):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:40):

peterhuene created PR review comment:

Nit: this looks good, but while we're here can we rename mapped_bytes to accessible_bytes (seemingly incorrectly named prior to these changes)?

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene created PR review comment:

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene created PR review comment:

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:46):

peterhuene created PR review comment:

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:48):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 17:48):

peterhuene edited PR review comment.

Wasmtime GitHub notifications bot (Jun 18 2021 at 03:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2021 at 03:53):

alexcrichton created PR review comment:

AFAIK wasmtime doesn't actually even run on any 32-bit platforms right now, or at least we definitely don't test it. Any objection to changing the default in Tunables to 10MB like it is here? Only being able to have 2 instances is also a little silly

Wasmtime GitHub notifications bot (Jun 18 2021 at 04:41):

peterhuene created PR review comment:

I have no objections; worth it certainly if it removes a duplicate configuration point.

Wasmtime GitHub notifications bot (Jun 18 2021 at 04:41):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2021 at 14:17):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2021 at 14:17):

alexcrichton created PR review comment:

Looking back at this I have no idea what I was thinking

Wasmtime GitHub notifications bot (Jun 18 2021 at 14:27):

alexcrichton updated PR #2977 from pre-guard to main.

Wasmtime GitHub notifications bot (Jun 18 2021 at 14:57):

alexcrichton merged PR #2977.

Wasmtime GitHub notifications bot (Jun 21 2021 at 17:27):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Jun 21 2021 at 17:27):

uweigand created PR review comment:

This unconditionally adds a SIMD operation, which we don't support on s390x yet, causing the test to fail.

Can we make this part architecture-dependent for now?

Wasmtime GitHub notifications bot (Jun 21 2021 at 17:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 21 2021 at 17:28):

alexcrichton created PR review comment:

Oh sure yeah, it's also fine to just comment this out since it's not really the most critical part of the test anyway

Last updated: Oct 23 2024 at 20:03 UTC