8262952: [macos_aarch64] os::commit_memory failure #3865
Conversation
|
|
|
@gerard-ziemski The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
src/hotspot/os/bsd/os_bsd.cpp
Outdated
| MACOS_ONLY(| (exec ? MAP_JIT : 0)) | ||
| // On macOS aarch64 MAP_JIT is required if we want to commit an executable chunk | ||
| // later, so always reserve executable memory right from the start | ||
| MACOS_AARCH64_ONLY(| MAP_JIT); |
There was a problem hiding this comment.
Is this the right way to do it? Where do we ever map memory we want to use for code generation, but not ask for exec permission? Is that a bug?
There was a problem hiding this comment.
Inside the hotspot VM code we seem to be doing the right thing, however, in the test, i.e. test/hotspot/gtest/runtime/test_os.cpp we ask for regular memory, then try to commit a chunk with elevated exec privileges, which seems to work on all other platforms, except aarch64 macOS.
The alternative, would be to fix the test in question, but since this scenario works fine on all the other platforms, I figured the proposed way to handle it in the VM would be the preferable way to fix it, so future test writers do not need to know this particular platform behavior difference and risk getting it wrong again.
There was a problem hiding this comment.
I get that, but we're disabling a security feature by always mapping memory as (potentially) executable. So, we're trading runtime VM security for convenience in testing, and that strikes me as a bad bargain.
There was a problem hiding this comment.
Hi @gerard-ziemski,
please fix the test instead. This would be a security issue, especially since the locations of some of our mappings are very predictable (eg CDS archive).
If this is about the "multi-release" test in test_os.cpp (377ff), just disable the exec flag there. I use alternating exec
flags to prevent the kernel from folding neighboring mappings, but actually that part is rather linux specific. Arguably we may even completely exclude the test, like I do for AIX already. Its important for Windows, somewhat less important for Linux, and covers other platforms only for completeness sake.
Wrt to MAP_JIT , what we do now is the result of long discussions: https://git.openjdk.java.net/jdk/pull/294.
Cheers, Thomas
There was a problem hiding this comment.
I did struggle deciding whether it's a good compromise to make the testing more convenient and consistent among platforms by allowing memory on macOS aarch64 executable right from start, which is why I mentioned that we can (and do) remove that privilege later when we commit it, but after stepping away from this code for a moment and looking at it now, I do agree that it was bad choice.
I'll fix the test instead.
src/hotspot/os/bsd/os_bsd.cpp
Outdated
| MACOS_ONLY(| (exec ? MAP_JIT : 0)) | ||
| // On macOS aarch64 MAP_JIT is required if we want to commit an executable chunk | ||
| // later, so always reserve executable memory right from the start | ||
| MACOS_AARCH64_ONLY(| MAP_JIT); |
There was a problem hiding this comment.
Hi @gerard-ziemski,
please fix the test instead. This would be a security issue, especially since the locations of some of our mappings are very predictable (eg CDS archive).
If this is about the "multi-release" test in test_os.cpp (377ff), just disable the exec flag there. I use alternating exec
flags to prevent the kernel from folding neighboring mappings, but actually that part is rather linux specific. Arguably we may even completely exclude the test, like I do for AIX already. Its important for Windows, somewhat less important for Linux, and covers other platforms only for completeness sake.
Wrt to MAP_JIT , what we do now is the result of long discussions: https://git.openjdk.java.net/jdk/pull/294.
Cheers, Thomas
…r, fix the test intead to ask for exececutable memory as needed
|
Thank you theRealAph & Thomas for your feedback! Lesson learned. I fixed the test instead. |
|
@gerard-ziemski This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 191 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
…t exec memory or not
|
Talking about "exec" permission of the memory in
I'm sure there must be a reason, that I don't know, but why do we need it to be of "PROT_READ"? Java2Demo, for example, seems to run fine without the "read" permission on it, i.e.:
|
|
@theRealAph Could you please sign off on this change if you are OK with my change incorporating your feedback? |
|
/integrate |
openjdk
bot
added
integrated
|
@gerard-ziemski Since your change was applied there have been 196 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit fadf580. |
On x86_64 macOS the following sequence works just fine:
attempt_reserve_memory_at(executable=false)
commit_memory(executable=true)
however, on aarch64 macOS it fails.
We fix the test itself to explicitly ask for executable memory when reserving it since it later commits it as executable.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3865/head:pull/3865$ git checkout pull/3865Update a local copy of the PR:
$ git checkout pull/3865$ git pull https://git.openjdk.java.net/jdk pull/3865/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3865View PR using the GUI difftool:
$ git pr show -t 3865Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3865.diff