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

Only map with executable permissions when using code space #1176

Merged
merged 9 commits into from
Jul 26, 2024

Conversation

k-sareen
Copy link
Collaborator

@k-sareen k-sareen commented Jul 24, 2024

This PR closes #7. This PR

  • refactors MmapStrategy to include protection flags, and allows each space to pass Mmapstrategy to the mmapper.
  • removes exec permission from most spaces. Only code spaces will have exec permission.
  • adds a feature exec_permission_on_all_spaces for bindings that may allocate code into normal spaces.

@k-sareen k-sareen requested a review from qinsoon July 24, 2024 23:27
@qinsoon
Copy link
Member

qinsoon commented Jul 25, 2024

I pushed some changes. It refactors MmapStrategy and allows spaces to pass what protection flags they need for mmap. With the change, only code spaces are mmapped with exec.

@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Jul 25, 2024
@k-sareen
Copy link
Collaborator Author

k-sareen commented Jul 25, 2024

Thank you for the refactoring. It looks much better now. Jikes is broken for some reason. Does it depend on exec permissions on non-codespaces perhaps...?

@qinsoon
Copy link
Member

qinsoon commented Jul 25, 2024

Thank you for the refactoring. It looks much better now. Jikes is broken for some reason. Does it depend on exec permissions on non-codespaces perhaps...?

I am not sure. MMTK in JikesRVM does mmap all the spaces with exec. Our Julia binding currently allocates code into normal spaces.

I added a feature exec_permission_on_all_spaces which allows exec for all the spaces. I can try turn on the feature for those two bindings.

@qinsoon
Copy link
Member

qinsoon commented Jul 25, 2024

binding-refs
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-mmtk-core-pr-1176
V8_BINDING_REPO=qinsoon/mmtk-v8
V8_BINDING_REF=update-mmtk-core-pr-1176

@qinsoon
Copy link
Member

qinsoon commented Jul 25, 2024

It seems Julia does not need exec permission for normal spaces, but V8 (NoGC has no code space) and JikesRVM need.

@qinsoon qinsoon requested a review from wks July 25, 2024 08:26
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

Mostly OK. Only a small suggestion.

@@ -98,27 +144,27 @@ pub fn dzmmap_noreplace(start: Address, size: usize, strategy: MmapStrategy) ->
/// This function does not reserve swap space for this mapping, which means there is no guarantee that writes to the
/// mapping can always be successful. In case of out of physical memory, one may get a segfault for writing to the mapping.
/// We can use this to reserve the address range, and then later overwrites the mapping with dzmmap().
pub fn mmap_noreserve(start: Address, size: usize, strategy: MmapStrategy) -> Result<()> {
let prot = PROT_NONE;
pub fn mmap_noreserve(start: Address, size: usize, mut options: MmapStrategy) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have been using the variable name strategy in other functions for MmapStrategy. I think it's OK to use strategy here, too.

Copy link
Member

Choose a reason for hiding this comment

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

Done. That was a mistake.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM.

@qinsoon qinsoon added this pull request to the merge queue Jul 26, 2024
Merged via the queue into mmtk:master with commit 9a49d6a Jul 26, 2024
20 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PROT_EXEC only for certain spaces
3 participants