RFR: 8340313: Crash due to invalid oop in nmethod after C1 patching

[Vladimir Kozlov]

On Mon, 7 Oct 2024 13:39:28 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> C1 patching (explained in detail [here](https://github.com/openjdk/jdk/blob/212e32931cafe446d94219d6c3ffd92261984dff/src/hotspot/share/c1/c1_Runtime1.cpp#L835)) works by rewriting the [patch body](https://github.com/openjdk/jdk/blob/212e32931cafe446d94219d6c3ffd92261984dff/src/hotspot/share/c1/c1_Runtime1.cpp#L860), usually a move,  and then copying it into place over top of the jmp instruction being careful to flush caches and doing it in an MP-safe way.
> 
> Now the problem is that although the patch body is not executed, one thread can update the oop immediate of the `mov` in the patch body:
> https://github.com/openjdk/jdk/blob/212e32931cafe446d94219d6c3ffd92261984dff/src/hotspot/share/c1/c1_Runtime1.cpp#L1185
> 
> While another thread walks the nmethod oops via `Universe::heap()->register_nmethod(nm)` to notify the GC and then encounters a half-written oop if the immediate crosses a page or cache-line boundary:
> https://github.com/openjdk/jdk/blob/212e32931cafe446d94219d6c3ffd92261984dff/src/hotspot/share/c1/c1_Runtime1.cpp#L1282
> 
> Updating the oop immediate is not atomic because the address of the immediate is not 8-byte aligned.
> 
> I propose to simply align it in `PatchingStub::emit_code`.
> 
> The new regression test triggers the issue reliably for the `load_mirror_patching_id` case but unfortunately, I was not able to trigger the `load_appendix_patching_id` case which should be affected as well:
> https://github.com/openjdk/jdk/blob/212e32931cafe446d94219d6c3ffd92261984dff/src/hotspot/share/c1/c1_Runtime1.cpp#L1197
> 
> I still added a corresponding test case `testIndy` and a [new assert that checks proper alignment](https://github.com/openjdk/jdk/commit/f418bc01c946b4c76f4bceac1ad503dabe182df7) triggers in both scenarios. I had to remove them again because they are x64 specific and I don't think it's worth the effort of adding a platform independent way of alignment checking.
> 
> AArch64 is not affected because we always deopt instead of patching but other platforms might be affected as well. Should this fix be accepted, I'll ping the maintainers of the respective platforms.
> 
> Thanks,
> Tobias

src/hotspot/cpu/x86/c1_CodeStubs_x86.cpp line 334:

> 332:     // 8-byte align the address of the oop immediate to guarantee atomicity
> 333:     // when patching since the GC might walk nmethod oops concurrently.
> 334:     __ align(8, __ offset() + NativeMovConstReg::data_offset_rex);

In 32-bit VM oops are 4 bytes so 8 bytes is overkill but I am fine with unified alignment.
Should we align mov_metadata() too or it is guarantee aligned already?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21389#discussion_r1790750290