8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature #14095

shqking · 2023-05-23T05:20:58Z

When revisiting the behavior of UseBranchProtection [1], we get one SIGILL error when running with -XX:UseBranchProtection=pac-ret on hardware without PAC.

Problem:

We build and run java --version with the following configuration matrix Config X VMoption X Machine.

  Config = {--enable-branch-protection, null}
  VMoption = {-XX:UseBranchProtection=pac-ret, -XX:UseBranchProtection=standard}
  Machine = {w/ PAC, w/o PAC}

VM crashes with SIGILL error for configure Config=null, VMoption=pac-ret, Machine=w/o PAC. The unrecognized instruction is pacia x30, x29, i.e. pacia(lr, rfp) generated by function MacroAssembler::protect_return_address(). [2]

Root cause:

Instruction pacia is not in the NOP space. That's why Config=null, VMoption=pac-ret passes on hardware w/ PAC, but fails on hardware w/o PAC.
-XX:UseBranchProtection=pac-ret behaves differently from the document [3], i.e.

  In order to use Branch Protection features in the VM,
  --enable-branch-protection must be used

_rop_protection is not turned off for Config=null. That's why VMoption=pac-ret, Machine=w/o PAC passes with
Config=--enable-branch-protection but fails with Config=null.

Fix:

This patch refines the parsing of -XX:UseBranchProtection=pac-ret:

We handle "pac-ret" and "standard" in the same way, since only one type of branch protection is implemented for now, i.e. "pac-ret". We may update "standard" in the future if "bti" protection is added.
_rop_protection is not turned on unless all the three conditions are satisfied [4]. Otherwise, it's kept off and one warning message is emitted.

// Enable PAC if this code has been built with branch-protection, the
// CPU/OS supports it, and incompatible preview features aren't enabled.

[1] https://bugs.openjdk.org/browse/JDK-8287325?focusedCommentId=14581099&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581099
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L5976
[3] https://github.com/openjdk/jdk/blob/master/doc/building.md#branch-protection
[4] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp#L457

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature

Reviewers

Andrew Haley (@theRealAph - Reviewer) ⚠️ Review applies to fbbf86a8
Nick Gasson (@nick-arm - Reviewer)
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14095/head:pull/14095
$ git checkout pull/14095

Update a local copy of the PR:
$ git checkout pull/14095
$ git pull https://git.openjdk.org/jdk.git pull/14095/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14095

View PR using the GUI difftool:
$ git pr show -t 14095

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14095.diff

Webrev

Link to Webrev Comment

…c-ret on hardware without PAC feature When revisiting the behavior of UseBranchProtection [1], we get one SIGILL error when running with -XX:UseBranchProtection=pac-ret on hardware without PAC. Problem: We build and run `java --version` with the following configuration matrix `Config X VMoption X Machine`. ``` Config = {--enable-branch-protection, null} VMoption = {-XX:UseBranchProtection=pac-ret, -XX:UseBranchProtection=standard} Machine = {w/ PAC, w/o PAC} ``` VM crashes with SIGILL error for configure `Config=null, VMoption=pac-ret, Machine=w/o PAC`. The unrecognized instruction is `pacia x30, x29`, i.e. `pacia(lr, rfp)` generated by function `MacroAssembler::protect_return_address()`. [2] Root cause: 1. Instruction `pacia` is not in the NOP space. That's why `Config=null, VMoption=pac-ret` passes on hardware w/ PAC, but fails on hardware w/o PAC. 2. -XX:UseBranchProtection=pac-ret behaves differently from the document [3], i.e. ``` In order to use Branch Protection features in the VM, --enable-branch-protection must be used ``` `_rop_protection` is not turned off for `Config=null`. That's why `VMoption=pac-ret, Machine=w/o PAC` passes with `Config=--enable-branch-protection` but fails with `Config=null`. Fix: This patch refines the parsing of -XX:UseBranchProtection=pac-ret: 1. We handle "pac-ret" and "standard" in the same way, since only one type of branch protection is implemented for now, i.e. "pac-ret". We may update "standard" in the future if "bti" protection is added. 2. `_rop_protection` is not turned on unless all the three conditions are satisfied [4]. Otherwise, it's kept off and one warning message is emitted. ``` // Enable PAC if this code has been built with branch-protection, the // CPU/OS supports it, and incompatible preview features aren't enabled. ``` [1] https://bugs.openjdk.org/browse/JDK-8287325?focusedCommentId=14581099&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581099 [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L5976 [3] https://github.com/openjdk/jdk/blob/master/doc/building.md#branch-protection [4] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp#L457

bridgekeeper · 2023-05-23T05:22:33Z

👋 Welcome back haosun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-05-23T05:24:11Z

@shqking The following label will be automatically applied to this pull request:

hotspot

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.

mlbridge · 2023-05-23T05:28:07Z

Webrevs

theRealAph

This looks good to me, but please read https://bugs.openjdk.org/browse/JDK-8287325 before you commit anything.

shqking · 2023-05-23T12:35:25Z

This looks good to me, but please read https://bugs.openjdk.org/browse/JDK-8287325 before you commit anything.

Thanks for reviewing this patch.

I personally thought they are two different issues, and should be fixed separately.

JDK-8287325 is the incompatible issue between pac-ret and virtual threads, whereas
this issue is that pac-ret is enabled even on hardware without the support of PAC feature, leading to SIGILL error.

Regarding JDK-8287325, we have proposed the zero modifier solution (See #13322). But we currently set that PR as draft, as we're trying to implement another solution of using "relative sp" as the modifier (which was suggested by Dean Long). We just finished one prototype of relative sp modifier, but there're still several jtreg failures. We're trying to fix them. We will upload our prototype for review once it's ready.

As for your comment, do you mean we should fix these two issues in one patch? Thanks.

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

theRealAph · 2023-05-23T15:08:58Z

This looks good to me, but please read https://bugs.openjdk.org/browse/JDK-8287325 before you commit anything.

Thanks for reviewing this patch.

As for your comment, do you mean we should fix these two issues in one patch? Thanks.

No, but you do need to align. I added a suggestion above, for clarity.

Virtual threads are proposed to be a permanent feature in JDK 21, and Arguments:enable_preview check no longer works. As an alternative, we check if VMContinuations is on. In this way, ROP protection is enabled only with VM options `-XX:UseBranchProtection=standard|pac-ret -XX:+UnlockExperimentalVMOptions -XX:-VMContinuations` on hardware with the support of PAC feature.

shqking · 2023-05-25T08:33:40Z

In the new commit, we disable ROP-protection if VMContinuations is on.
Hope this can address your comments.

Test for the new commit:

I built and ran java --version with the following configuration matrix Config X VMoption X Machine.
_rop_protection behaves as expected.

  Config = {--enable-branch-protection, null}
  VMoption = {-XX:UseBranchProtection=pac-ret, -XX:UseBranchProtection=pac-ret -XX:-VMContinuations}
  Machine = {w/ PAC, w/o PAC}

I further ran tier1~3 with Config=--enable-branch-protection, VMoption=-XX:UseBranchProtection=pac-ret -XX:-VMContinuations, Machine=w/ PAC, and only got one jtreg failiure, i.e.jdk/incubator/concurrent/ScopedValue/StressStackOverflow.java.
I don't think it's related to PAC-RET. See https://bugs.openjdk.org/browse/JDK-8308609

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

openjdk · 2023-05-25T09:09:00Z

@shqking This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature

Reviewed-by: aph, ngasson, dlong

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 145 new commits pushed to the master branch:

0951474: 8309150: Need to escape " inside attribute values
0119969: 8309171: Test vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java fails after JDK-8308341
f8a924a: 8308975: Fix signed integer overflow in compiler code, part 2
5531f6b: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn
e42a4b6: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
8dbd384: 8308678: (fs) UnixPath::toRealPath needs additional permissions when running with SM (macOS)
c3cd481: 8304914: Use OperatingSystem, Architecture, and Version in jpackage
eae1f59: 8309159: Some minor comment and code cleanup in jdk/com/sun/jdi/PopFramesTest.java
45473ef: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
78aa5f3: 8299505: findVirtual on array classes incorrectly restricts the receiver type
... and 135 more: https://git.openjdk.org/jdk/compare/5d8ba938bef162b74816147eb1002a0620a419ba...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

shqking · 2023-05-31T08:39:46Z

Can anyone else help take a look at this patch?
Maybe @nick-arm @dean-long or @AlanBateman Thanks.

shqking · 2023-05-31T23:48:00Z

Thanks for your review!
GHA tests are passed. I'm integrating it.

/integrate

openjdk · 2023-05-31T23:49:18Z

Going to push as commit a46b5ac.
Since your change was applied there have been 147 commits pushed to the master branch:

f9ad7df: 8300865: C2: product reduction in ProdRed_Double is not vectorized
8eda97d: 8305320: DbgStrings and AsmRemarks are leaking
0951474: 8309150: Need to escape " inside attribute values
0119969: 8309171: Test vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java fails after JDK-8308341
f8a924a: 8308975: Fix signed integer overflow in compiler code, part 2
5531f6b: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn
e42a4b6: 8309236: ProblemList java/util/concurrent/locks/Lock/OOMEInAQS.java with ZGC and Generational ZGC again
8dbd384: 8308678: (fs) UnixPath::toRealPath needs additional permissions when running with SM (macOS)
c3cd481: 8304914: Use OperatingSystem, Architecture, and Version in jpackage
eae1f59: 8309159: Some minor comment and code cleanup in jdk/com/sun/jdi/PopFramesTest.java
... and 137 more: https://git.openjdk.org/jdk/compare/5d8ba938bef162b74816147eb1002a0620a419ba...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-31T23:49:30Z

@shqking Pushed as commit a46b5ac.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label May 23, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label May 23, 2023

theRealAph reviewed May 23, 2023

View reviewed changes

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp Outdated Show resolved Hide resolved

theRealAph reviewed May 25, 2023

View reviewed changes

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp Outdated Show resolved Hide resolved

theRealAph approved these changes May 25, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 25, 2023

theRealAph approved these changes May 25, 2023

View reviewed changes

Update the warning message

b22fa23

nick-arm approved these changes May 31, 2023

View reviewed changes

dean-long approved these changes May 31, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 31, 2023

openjdk bot closed this May 31, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 31, 2023

shqking deleted the jdk-8308503 branch May 31, 2023 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature #14095

8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature #14095

shqking commented May 23, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 23, 2023

openjdk bot commented May 23, 2023

mlbridge bot commented May 23, 2023 •

edited

Loading

theRealAph left a comment

shqking commented May 23, 2023

theRealAph commented May 23, 2023

shqking commented May 25, 2023

openjdk bot commented May 25, 2023 •

edited

Loading

shqking commented May 31, 2023

shqking commented May 31, 2023

openjdk bot commented May 31, 2023

openjdk bot commented May 31, 2023

8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature #14095

8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature #14095

Conversation

shqking commented May 23, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 23, 2023

openjdk bot commented May 23, 2023

mlbridge bot commented May 23, 2023 • edited Loading

Webrevs

theRealAph left a comment

Choose a reason for hiding this comment

shqking commented May 23, 2023

theRealAph commented May 23, 2023

shqking commented May 25, 2023

openjdk bot commented May 25, 2023 • edited Loading

shqking commented May 31, 2023

shqking commented May 31, 2023

openjdk bot commented May 31, 2023

openjdk bot commented May 31, 2023

shqking commented May 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 23, 2023 •

edited

Loading

openjdk bot commented May 25, 2023 •

edited

Loading