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

Fix mprotect failures by enabling cranelift-jit selinux-fix #5204

Merged
merged 1 commit into from Nov 4, 2022

Conversation

uweigand
Copy link
Member

@uweigand uweigand commented Nov 4, 2022

The sample program in cranelift/filetests/src/function_runner.rs would abort with an mprotect failure under certain circumstances, see #4453 (comment)

Root cause was that enabling PROT_EXEC on the main process heap may be prohibited, depending on Linux distro and version.

This only shows up in the doc test sample program because the main clif-util is multi-threaded and therefore allocations will happen on glibc's per-thread heap, which is allocated via mmap, and not the main process heap.

Work around the problem by enabling the "selinux-fix" feature of the cranelift-jit crate dependency in the filetests. Note that this didn't compile out of the box, so a separate fix is also required and provided as part of this PR.

Going forward, it would be preferable to always use mmap to allocate the backing memory for JITted code.

CC @cfallin @afonso360

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Nov 4, 2022
@bjorn3
Copy link
Contributor

bjorn3 commented Nov 4, 2022

See #4986

The sample program in cranelift/filetests/src/function_runner.rs
would abort with an mprotect failure under certain circumstances,
see bytecodealliance#4453 (comment)

Root cause was that enabling PROT_EXEC on the main process heap
may be prohibited, depending on Linux distro and version.

This only shows up in the doc test sample program because the main
clif-util is multi-threaded and therefore allocations will happen
on glibc's per-thread heap, which is allocated via mmap, and not
the main process heap.

Work around the problem by enabling the "selinux-fix" feature of
the cranelift-jit crate dependency in the filetests.  Note that
this didn't compile out of the box, so a separate fix is also
required and provided as part of this PR.

Going forward, it would be preferable to always use mmap to allocate
the backing memory for JITted code.
@uweigand
Copy link
Member Author

uweigand commented Nov 4, 2022

Updated cfg checks to prevent breaking windows builds with selinux-fix.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

This PR is equivalent to fixing #4986 by turning the selinux-fix feature on for everybody. I think this is a good idea, but if we're going to do that, I think we should do two things:

  1. Benchmark the runtime performance impact of this change on Linux.
  2. Remove all the cfg attributes for selinux-fix and just use that path unconditionally (except on Windows, I guess).

@uweigand
Copy link
Member Author

uweigand commented Nov 4, 2022

This PR is equivalent to fixing #4986 by turning the selinux-fix feature on for everybody.

The intent of the PR is to turn on selinux-fix for the filetests crate only, which should not be performance-critical. It's limited to filetests because only there I'm seeing an actual immediate issue (failing tests) right now.

I agree that we should move to mmap everywhere, but that is performance-critical, and there may be better ways than the selinux-fix (I haven't looked into the implementation in detail, and I'm sure others are more familiar with that ...)

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Oh, yeah, I mis-read this. Okay, looks good to me!

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

I have also been bitten by this error in the past... thanks for fixing it!

@abrown abrown merged commit fba2287 into bytecodealliance:main Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants