RFR: 8264393: JDK-8258284 introduced dangling TLH race [v2]

[Daniel D.Daugherty]

On Wed, 31 Mar 2021 16:18:24 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Hi Dan,
>> 
>> As you describe it and how it look to me, isn't the issue just that we decrement before before reinstating the old list?
>> So if we do first publish the previous list as current list and then decrement the nested handle count it should be okay?
>> E.g.:
>> _thread->set_threads_hazard_ptr(_previous->_list);
>> _list->dec_nested_handle_cnt();
>> 
>> So you just need to move the "if (_has_ref_count) {" piece of code after the potential reinstating of the previous list.
>> 
>> Or am I missing something?
>> 
>> Thanks for finding it!
>
> @robehn - Thanks for reviewing the fix. Yes, I think you have missed something. :-)
> 
> I modeled the analysis of this race after one of your favorite race techniques
> in my analysis.ThreadsList.for_JBS attachment to the bug report: since there
> is nothing to force the two threads to interact with each in a particular order,
> I posited delays at various points in the execution of each thread. This quote
> from the analysis.ThreadsList.for_JBS attachment describes scenario:
> 
> Race note:
>     THR-1 is the thread calling the TLH destructors.
>     THR-2 is the exiting thread calling ThreadsSMRSupport::free_list.
> 
>     If THR-2's ThreadsSMRSupport::free_list() call finishes its scan of of
>     the active Threads _threads_hazard_ptr values BEFORE the TLH-2
>     destructor code sequence updates THR-1->_threads_hazard_ptr from TL-2
>     to TL-1, then TL-2 and not TL-1 will be on the list of in-use
>     ThreadsLists:
> 
>       // Gather a hash table of the current hazard ptrs:
>       ThreadScanHashtable *scan_table = new ThreadScanHashtable(hash_table_size);
>       ScanHazardPtrGatherThreadsListClosure scan_cl(scan_table);
>       threads_do(&scan_cl);
> 
>     At this point, THR-2's ThreadsSMRSupport::free_list() call stalls and
>     THR-1 not only finishes the TLH-2 destructor, it also finishes its use
>     of TLH-1 as described in the next section and starts to run the TLH-1
>     destructor.
> 
> After the first ThreadsListHandle is released for THR-1:
> 
> +----------------------------------+
> | THR-1                            |
> +----------------------------------+
> | _threads_hazard_ptr=0            |
> | _threads_list_ptr=0              |
> | _nested_threads_hazard_ptr_cnt=0 |
> +----------------------------------+
> 
>                                          +----------------------+
>                                          | TL-1                 |
>                                          +----------------------+
>                                          | _length=XXXXXXXXXXXX |
>                                          | _next_list=XXXXXXXXX |
>                                          | _threads[5]=XXXXXXXX |
>                                          | _nested_handle_cnt=X |
>                                          +----------------------+
> 
> Race note:
>     THR-1 is running the TLH-1 destructor and has decremented the TL-1
>     _nested_handle_cnt, but stalls before it clears _threads_hazard_ptr.
> 
>     The THR-2's ThreadsSMRSupport::free_list() call continues executing and
>     checks the _to_delete_list ThreadsLists and if they are not in the
>     scan_table and have a _nested_handle_cnt == 0 then, they are freed.
> 
>     This is how TL-1 is freed, but still remains in THR-1's
>     _threads_hazard_ptr field and can be observed later by THR-2 as a valid
>     hazard ptr in its call to smr_delete() on itself or by another thread
>     perusing the system ThreadsList. This is especially true after
>     ThreadsSMRSupport::free_list() has finished its work and released the
>     Threads_lock which will allow another thread to walk the set of hazard
>     ptrs.
> 
>     THR-1 resumes running again and clears _threads_hazard_ptr. However,
>     the other thread walking the set of hazard ptrs has the stale TL-1
>     value and tries to use it. Boom!
> 
> Switching the decrement:
> `_list->dec_nested_handle_cnt()`
> to happen after the:
> `_thread->set_threads_hazard_ptr(_previous->_list)`
> doesn't help because THR-2 observed TL-2 before we
> reached that code and then THR-2 stalled until after all
> the updates were made. THR-2 recorded TL-2 in the
> collection of current hazard ptrs and THR-2 knows nothing
> about TL-1 being a valid hazard ptr so THR-2 can free it.

I tested JDK-8264123 together with this fix (JDK-8264393) in Mach5 Tier[1-7]
and there are no regressions.

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

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