Integrated: 8272170: Missing memory barrier when checking active state for regions

[Thomas Schatzl]

On Wed, 10 Nov 2021 08:37:24 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

> Hi all,
> 
>   can I have reviews for this fix to memory visibility race where we miss to make sure that the `HeapRegion` pointer in `HeapRegionManager::_regions` is visible before the "active"-check for that `HeapRegion` can return true?
> 
> I.e. the problem is this (according to my understanding):
> 
> void HeapRegionManager::expand(uint start, uint num_regions, WorkerThreads* pretouch_workers) {
>   commit_regions(start, num_regions, pretouch_workers);
>   for (uint i = start; i < start + num_regions; i++) {
>     HeapRegion* hr = _regions.get_by_index(i);
>     if (hr == NULL) {
>       hr = new_heap_region(i);
>       OrderAccess::storestore();
>       _regions.set_by_index(i, hr);
>       _allocated_heapregions_length = MAX2(_allocated_heapregions_length, i + 1);
>     }
>     G1CollectedHeap::heap()->hr_printer()->commit(hr);
>   }
>   activate_regions(start, num_regions);
> }
> 
> E.g. we first commit the memory, then create a `HeapRegion` instance which we properly guard with a `StoreStore` before assigning it to the `_regions` array. After that at the end we activate the regions (via `activate_regions`), meaning that the contents of the new `HeapRegion*` in the region table are valid.
> These bits in the `_active_regions` bitmap are put with memory barriers (via `par_set_range`).
> 
> In `HeapRegionManager::par_iterate`, if we iterate over the region map, we first check whether the region is available in the `_active_regions` bitmap, and then get and pass on the corresponding `HeapRegion*` to the given closure.
> 
> However there is no memory ordering between reading the available-bit in the `_active_regions` bitmap, so that bit could become visible to the thread iterating over the regions before the contents of the `_region` map itself, in effect passing a `nullptr` to the closure which isn't allowed. (Some details about the debugging session in the CR).
> 
> 
> void HeapRegionManager::par_iterate(HeapRegionClosure* blk, HeapRegionClaimer* hrclaimer, const uint start_index) const {
>   // Every worker will actually look at all regions, skipping over regions that
>   // are currently not committed.
>   // This also (potentially) iterates over regions newly allocated during GC. This
>   // is no problem except for some extra work.
>   const uint n_regions = hrclaimer->n_regions();
>   for (uint count = 0; count < n_regions; count++) {
>     const uint index = (start_index + count) % n_regions;
>     assert(index < n_regions, "sanity");
>     // Skip over unavailable regions
>     if (!is_available(index)) {
>       continue;
>     }
>     HeapRegion* r = _regions.get_by_index(index);
> [...]
>     bool res = blk->do_heap_region(r);
>     if (res) {
>       return;
>     }
>   }
> }
> 
> 
> The suggested fix is to ensure proper memory ordering in `is_available`, i.e. the loads must be ordered correctly.
> 
> I did check a bit around the usage of this method and `is_unavailable`, but I do not think there is a similar issue.
> 
> Testing: test crashed with a frequency of around 1/500, now passing 5k runs
> 
> Thanks,
>   Thomas

This pull request has now been integrated.

Changeset: 35a831d5
Author:    Thomas Schatzl <tschatzl at openjdk.org>
URL:       https://git.openjdk.java.net/jdk/commit/35a831d5a755de8f3c71653bd0a37190adddb8ae
Stats:     1 line in 1 file changed: 0 ins; 0 del; 1 mod

8272170: Missing memory barrier when checking active state for regions

Reviewed-by: sjohanss, ayang

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

PR: https://git.openjdk.java.net/jdk/pull/6324