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

mpk: restore PKRU state when a fiber resumes execution #7789

Merged
merged 3 commits into from
Feb 15, 2024

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Jan 18, 2024

Previously, when a fiber was suspended, other computation could change the PKRU state on the current CPU. This means that the fiber could be resumed with a different PKRU state. This could be bad, resulting in situations in which the fiber can access more memory slots than it should or cannot even access its own memory slots.

This change saves the PKRU state prior to a fiber being suspended. When the fiber resumes execution, that PKRU state is restored.

@abrown
Copy link
Contributor Author

abrown commented Jan 18, 2024

This probably needs an async_functions.rs test but I'm still struggling with how to craft a scenario that checks for this issue.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jan 18, 2024
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

With a test that fails without this change I think it's good-to-go.

Previously, when a fiber was suspended, other computation could change
the PKRU state on the current CPU. This means that the fiber could be
resumed with a different PKRU state. This could be bad, resulting in
situations in which the fiber can access more memory slots than it
should or cannot even access its own memory slots.

This change saves the PKRU state prior to a fiber being suspended. When
the fiber resumes execution, that PKRU state is restored.
This adds a test that alternately polls two Wasm instances in a loop.
Since the instances are async, we can set up epochs to suspend each
fiber as we iterate over a loop. Because we alternate between the two
instances, it checks that `AsyncCx::block_on` has correctly restored the
PKRU bits; otherwise we should see test failures. In the process of
writing this test I discovered bytecodealliance#7942, which can be solved separately
(it has to do with the interaction between memory images, _not_ used
here, and MPK).

prtest:full
Not all stores have protection keys and MPK is not always enabled. This
change checks for these conditions before context-switching the PKRU
bits.
@abrown abrown marked this pull request as ready for review February 15, 2024 02:29
@abrown abrown requested a review from a team as a code owner February 15, 2024 02:29
@abrown abrown requested review from fitzgen and alexcrichton and removed request for a team February 15, 2024 02:29
@alexcrichton alexcrichton added this pull request to the merge queue Feb 15, 2024
Merged via the queue into bytecodealliance:main with commit 2aaeddb Feb 15, 2024
43 checks passed
@abrown abrown deleted the pku-async branch February 15, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants