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

[Tobias Hartmann]

On Mon, 7 Oct 2024 23:30:03 GMT, Dean Long <dlong 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 there can be multiple patch sides in one nmethod (for example, one for each field access in `TestConcurrentPatching::test`) and multiple threads executing that nmethod can trigger patching concurrently. Although the patch body is not executed, one `Thread A` 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 B` is done with patching another patch side and already walks the nmethod oops via `Universe::heap()->register_nmethod(nm)` to notify the GC. `Thread B` might then encounter a half-written oop from `Thread A` 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
>> 
>> In short:
>> - `Thread A`: Patches location 1 in an nmethod and executes `register_nmethod()`, walking all immediate oops.
>> - `Thread B`: Patches location 2 in the same nmethod and just wrote half of the oop immediate of an `mov` in the patch body because the store is not atomic.
>> - `Thread A`: Crashes when walking the immediate oops of the nmethod and encountering the oop just partially written by `Thread B` concurrently.
>> 
>> Updating the oop immediate is not atomic on x86_64 because the address of the immediate is not 8-byte aligned.
>> I propose to simply align it in `PatchingStub::emit_code` to guarantee atomicity.
>> 
>> 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) t...
>
> Wouldn't it be better to get rid of the concurrency?  We could grab CodeCache_lock and Patching_lock in the same block, so we serialize the patching and register_nmethod.

@dean-long I discussed this with @tschatzl and, on his request, improved the PR description a bit. He would also prefer the alignment solution because it does not increase the scope of the lock (and we already rely on word-aligned word-sized memory accesses being atomic in many other places).

What do you think?

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

PR Comment: https://git.openjdk.org/jdk/pull/21389#issuecomment-2406908303