RFR: 8318462: [GenShen] Prevent unsafe access to displaced mark-word [v3]

[Kelvin Nilsen]

On Thu, 26 Oct 2023 10:36:30 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> We must be very careful when we access (load or update) the object age concurrently because the age is stored in the object's header and that header can be 'displaced'. This means that the header is overloaded with a pointer (stack or ObjectMonitor*) and the original header is stored at that location. The header can also be in INFLATING state, which means that a stack-lock is currently in the process to be inflated to an ObjectMonitor. Let's consider the cases separately:
>> 
>> Loading object age:
>> - INFLATING: we can not access the header, and thus not the age.
>> - Stack-locked: the thread which locks the object can unlock at any time, concurrently. It is not safe to access the header, and thus the age.
>> - Monitor-locked: monitors don't go away without coordinating with Java threads and possibly GC threads. This coordination is done by handshaking the thread - a Java thread would be brought to its next safepoint, and GC threads, which are typically participating in handshakes by joining the SuspendibleThreadSet, can opt to handshake at a safe point by calling SuspendibleThreadSet::yield(). If 'our' thread participates in handshakes in one way or the other, it is safe to access an ObjectMonitor once we got a valid ObjectMonitor* from an object's header, but only until that thread handshakes. Long story short: it is typically safe to access ObjectMonitor :-)
>> 
>> Updating object age:
>> Updating object age only happens during evacuation, and only on a new copy of an object, before that copy has been published. Access to the object header is therefore fully thread-local and not problematic. What *is* problematic is when we have to deal with displaced headers, because displaced headers are *not* thread local, and must be considered a shared resource. A competing thread may succeed to evacuate the object and publish its copy before 'our' thread can do so. If 'our' thread would update the displaced header, it may over-write whatever the other thread has done.
>> - INFLATING: We cannot access the header at all. However, that should not happen: a thread can only inflate once it has successfully evacuated an object, and 
>> - Stack-locked: the other thread may succeed evacuation and continue to unlock the object, at which point the stack-pointer would be 'dangling' and we may over-write random information on the foreign thread stack.
>> - Monitor-locked: even though monitor deflation is coordinated (see above), updating the displaced header in a monitor might over-...
>
> Roman Kennke has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Fix inflating check with LW locking
>  - Merge branch 'master' into JDK-8318462
>  - Accept all-zero header with LW locking
>  - Handle forwarded headers
>  - Assert that no inflation happens
>  - 8318462: [Shenandoah] Prevent unsafe access to displaced mark-word

src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 544:

> 542:   // is thread-local and therefore safe to access. However, when the mark is
> 543:   // displaced (i.e. stack-locked or monitor-locked), then it must be considered
> 544:   //a shared memory location. It can/ be accessed by other threads.

Minor formatting issues in this line of comment.

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp line 123:

> 121:       assert(region->is_young(), "Only for young objects");
> 122:       uint age = ShenandoahHeap::get_object_age(obj);
> 123:       CENSUS_NOISE(heap->age_census()->add(age, region->age(), region->youth(), size, worker_id);)

I'm thinking if get_object_age() returns max_age+1, the CENSUS_NOISE macros might end up with invalid data.  Don't we need a check here?

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/343#discussion_r1373409115
PR Review Comment: https://git.openjdk.org/shenandoah/pull/343#discussion_r1373435503