Stream: git-wasmtime

Topic: wasmtime / PR #6031 cranelift-wasm: Add a bounds-checking...

Wasmtime GitHub notifications bot (Mar 16 2023 at 19:26):

fitzgen opened PR #6031 from dynamic-memories-with-guard-regions to main:

This is a new special case for when we know that there are enough guard pages to cover the memory access's offset and access size.

The precise should-we-trap condition is

index + offset + access_size > bound

However, if we instead check only the partial condition

index > bound

then the most out of bounds that the access can be, while that partial check still succeeds, is offset + access_size.

However, when we have a guard region that is at least as large as offset + access_size, we can rely on the virtual memory subsystem handling these out-of-bounds errors at runtime. Therefore, the partial index > bound check is sufficient for this heap configuration.

Additionally, this has the advantage that a series of Wasm loads that use the same dynamic index operand but different static offset immediates -- which is a common code pattern when accessing multiple fields in the same struct that is in linear memory -- will all emit the same index > bound check, which we can GVN.

Partially.

The bounds check comparison is GVN'd but we still branch on values we should know will always be true if we get this far in the code. This is actual br_ifs in the non-Spectre code and select_spectre_guards that we should know will always go a certain way if we have Spectre mitigations enabled. See the second commit for examples.

Improving the non-Spectre case is pretty straightforward: walk the dominator tree and remember which values we've already branched on at this point, and therefore we can simplify any further conditional branches on those same values into direct jumps.

Improving the Spectre case requires something that is morally the same, but has a couple snags:

• We don't have actual br_ifs to determine whether the bounds checking condition succeeded or not. We need to instead reason about dominating select_spectre_guard; {load, store} instruction pairs.

• We have to be SUPER careful about reasoning "through" select_spectre_guards. Our general rule is never to do that, since it could break the speculative execution sandboxing that the instruction is designed for.

This pull request leaves implementing these new optimization passes for follow ups.

Wasmtime GitHub notifications bot (Mar 16 2023 at 19:26):

fitzgen requested alexcrichton for a review on PR #6031.

Wasmtime GitHub notifications bot (Mar 16 2023 at 19:26):

fitzgen requested cfallin for a review on PR #6031.

Wasmtime GitHub notifications bot (Mar 16 2023 at 21:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 21:06):

alexcrichton created PR review comment:

I know at the top of this match it says that some duplication is necessary, but I personally found it a bit confusing to have the spectre/non-spectre cases split apart. Would it be possible, for dynamic heaps, to fuse these two arms together with a helper?

Wasmtime GitHub notifications bot (Mar 16 2023 at 21:06):

alexcrichton created PR review comment:

I think that this comment isn't actually correct in the case that a spectre guard is used, right? Without spectre guards the previously-trapping condition prevents overflow, but with spectre guards this could overflow. I think the saving grace, though, is that the overflowed value isn't used due to the spectre guard.

Wasmtime GitHub notifications bot (Mar 16 2023 at 21:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 21:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 22:03):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 22:03):

fitzgen created PR review comment:

Yeah I was being a bit loose and fudgy with my wording here, will be more precise.

Wasmtime GitHub notifications bot (Mar 16 2023 at 22:03):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 16 2023 at 22:03):

fitzgen created PR review comment:

Yeah I was thinking of doing this in a follow up. That cool?

Wasmtime GitHub notifications bot (Mar 17 2023 at 02:11):

fitzgen updated PR #6031 from dynamic-memories-with-guard-regions to main.

Wasmtime GitHub notifications bot (Mar 17 2023 at 02:11):

fitzgen requested alexcrichton for a review on PR #6031.

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:26):

alexcrichton created PR review comment:

Could a second version of this test be executed which asserts this? One without any guard pages on dynamic memories?

Wasmtime GitHub notifications bot (Mar 17 2023 at 14:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2023 at 18:02):

fitzgen updated PR #6031 from dynamic-memories-with-guard-regions to main.

Wasmtime GitHub notifications bot (Mar 17 2023 at 18:02):

fitzgen has enabled auto merge for PR #6031.

Wasmtime GitHub notifications bot (Mar 17 2023 at 18:44):

fitzgen updated PR #6031 from dynamic-memories-with-guard-regions to main.

Wasmtime GitHub notifications bot (Mar 17 2023 at 19:57):

fitzgen merged PR #6031.

Last updated: Dec 23 2024 at 13:07 UTC