Stream: git-wasmtime

Topic: wasmtime / PR #5229 wiggle: adapt Wiggle guest slices for...

Wasmtime GitHub notifications bot (Nov 08 2022 at 21:29):

abrown opened PR #5229 from shmem-in-wiggle-copy-and-unsafe to main:

When multiple threads can concurrently modify a WebAssembly shared memory, the underlying data for a Wiggle GuestSlice and GuestSliceMut could change due to access from other threads. This breaks Rust guarantees when &[T] and &mut [T] slices are handed out. This change modifies GuestPtr to make as_slice and as_slice_mut return an Option which is None when the underlying WebAssembly memory is shared.

But WASI implementations still need access to the underlying WebAssembly memory, both to read to it and write from it. This change adds new APIs:

• GuestPtr::to_vec copies the bytes from WebAssembly memory (from which we can safely take a &[T])
• GuestPtr::as_unsafe_slice_mut returns a wrapper struct from which we can unsafe-ly return a mutable slice (users must accept the unsafety of concurrently modifying a &mut [T])

This approach allows us to maintain Wiggle's borrow-checking infrastructure, which enforces the guarantee that Wiggle will not modify overlapping regions, e.g. This is important because the underlying system calls may expect this. Though other threads may modify the same underlying region, this is impossible to prevent; at least Wiggle will not be able to do so.

Finally, the changes to Wiggle's API are propagated to all WASI implementations in Wasmtime. For now, code locations that attempt to get a guest slice will panic if the underlying memory is shared. Note that Wiggle is not enabled for shared memory (that will come later in something like #5054), but when it is, these panics will be clear indicators of locations that must be re-implemented in a thread-safe way.

<!--

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 (Nov 08 2022 at 21:30):

abrown edited PR #5229 from shmem-in-wiggle-copy-and-unsafe to main:

When multiple threads can concurrently modify a WebAssembly shared memory, the underlying data for a Wiggle GuestSlice and GuestSliceMut could change due to access from other threads. This breaks Rust guarantees when &[T] and &mut [T] slices are handed out. This change modifies GuestPtr to make as_slice and as_slice_mut return an Option which is None when the underlying WebAssembly memory is shared.

But WASI implementations still need access to the underlying WebAssembly memory, both to read to it and write from it. This change adds new APIs:

• GuestPtr::to_vec copies the bytes from WebAssembly memory (from which we can safely take a &[T])
• GuestPtr::as_unsafe_slice_mut returns a wrapper struct from which we can unsafe-ly return a mutable slice (users must accept the unsafety of concurrently modifying a &mut [T])

This approach allows us to maintain Wiggle's borrow-checking infrastructure, which enforces the guarantee that Wiggle will not modify overlapping regions, e.g. This is important because the underlying system calls may expect this. Other threads may modify the same underlying region and this is impossible to prevent; at least Wiggle will not be able to do so.

Finally, the changes to Wiggle's API are propagated to all WASI implementations in Wasmtime. For now, code locations that attempt to get a guest slice will panic if the underlying memory is shared. Note that Wiggle is not enabled for shared memory (that will come later in something like #5054), but when it is, these panics will be clear indicators of locations that must be re-implemented in a thread-safe way.

<!--

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 (Nov 08 2022 at 23:03):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Could the expect messages here be replaced with something along the lines of a TODO: .. to clarify that there's nothing a user running into this can do to fix things, it requires source changes here.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

double as *mut T cast?

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Could this have a comment, or a reference, for why shared memory is rejected here?

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Could you update the documentation of this function to indicate what the Option return means?

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

We'll need to be careful here because the mere presence of &mut [T], however ephemeral it is, will be UB for Rust that we need to avoid. Basically the raw-pointer-ness needs to get plumbed all the way down to the std::ptr::copy.

Also, can you be sure to tag this std::ptr::copy in the threads issue as "please audit at some point" along the lines of the other copies already in runtime/src/instances.rs

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Or sort of more generally I think it's worth writing down somewhere the general idea behind shared memory and how it works in wiggle, and then link to that as part of various comments.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Could this perhaps be refactored with the result or such to be usable as an internal implementation detail of the two methods above? Deduplicating the bounds checks would be pretty convenient and the "Unsafe" version could then have a methods {shared,mut}_borrow to give the safe versions, returning None for shared memories.

basically as_slice() could be a small wrapper around Ok(self.as_unsafe_slice().shared_borrow()) or something like that.

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Similar requests as above for doc updates and comments on the test for shared memory here

Wasmtime GitHub notifications bot (Nov 08 2022 at 23:18):

alexcrichton created PR review comment:

Also, given the sheer quantity of these, I think it might be good to have some sort of greppable string in the message here so an entry can be added to the threads tracking issue of how to find places to resolve.

Wasmtime GitHub notifications bot (Nov 09 2022 at 19:30):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 19:33):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:02):

alexcrichton created PR review comment:

I don't think that this is doing what you might expect, I think this'll need to be written with std::slice::from_raw_parts. That being said I think it's best to drop as_slice and as_slice_mut here for now since nothing needs it. I think these would be better modeled as a Deref<Target = [UnsafeCell<T>]> in the future if it's necessary.

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:02):

alexcrichton created PR review comment:

Could this method be implemented with as_unsafe_slice_mut to deduplicate the bits internally?

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:02):

alexcrichton created PR review comment:

For this this will still need to be implemented with an actual call to std::ptr::copy. I know for a fact that anything other than that is inherently UB, the only question to me is whether std::ptr::copy is UB-or-not in this situation.

Wasmtime GitHub notifications bot (Nov 09 2022 at 20:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 09 2022 at 22:36):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 23:17):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 09 2022 at 23:19):

abrown has marked PR #5229 as ready for review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 00:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 00:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 00:59):

alexcrichton created PR review comment:

I believe this will need to do a guest_slice.mut_borrow()?; before doing this copy to return an error if this is already borrowed for unshared memories.

Wasmtime GitHub notifications bot (Nov 10 2022 at 15:54):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 15:54):

abrown created PR review comment:

I have UnsafeGuestSlice::mut_borrow(self) which then makes it difficult to either use the mutable borrow or use the unsafe slice for std::ptr::copy. Should it take &mut self instead?

Wasmtime GitHub notifications bot (Nov 10 2022 at 16:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 16:00):

alexcrichton created PR review comment:

While no longer compatible with ? how about:

enum BorrowResult<T,S> {
    Ok(T),
    Shared(S),
    Err(GuestError),
}

as the return value? So that way given a Shared error you can still get at the internal pointer.

While you could change mut_burrow to &mut self it would require still having the self version to get the lifetimes on as_slice_mut to work.

Wasmtime GitHub notifications bot (Nov 10 2022 at 18:41):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 10 2022 at 19:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 10 2022 at 19:36):

alexcrichton has enabled auto merge for PR #5229.

Wasmtime GitHub notifications bot (Nov 10 2022 at 20:13):

abrown updated PR #5229 from shmem-in-wiggle-copy-and-unsafe to main.

Wasmtime GitHub notifications bot (Nov 10 2022 at 21:54):

alexcrichton merged PR #5229.

Last updated: Nov 22 2024 at 16:03 UTC