Re: [PATCH v2 06/14] elf: Use relaxed atomics for racy accesses [BZ #19329]

[Carlos O'Donell <carlos@redhat.com>]

On 5/11/21 5:31 AM, Szabolcs Nagy wrote:
> The 05/10/2021 22:56, Carlos O'Donell wrote:
>> On 4/16/21 5:12 AM, Szabolcs Nagy via Libc-alpha wrote:
>>> The 04/15/2021 15:21, Adhemerval Zanella wrote:
>>>> On 13/04/2021 05:19, Szabolcs Nagy via Libc-alpha wrote:
>>>>> This is a follow up patch to the fix for bug 19329.  This adds
>>>>> relaxed MO atomics to accesses that are racy, but relaxed MO is
>>>>> enough.
>>>>
>>>> Could you extend a bit why relaxed MO should be suffice?
>>>
>>> is it ok to change the commit message to:
>>>
>>> This is a follow up patch to the fix for bug 19329.  This adds relaxed
>>> MO atomics to accesses that are racy, but relaxed MO is enough.
>>
>> Suggest:
>>
>> This is a follow up patch to the fix for bug 19329.  This adds relaxed
>> MO atomics to accesses that were previously data races but are now
>> race conditions, and where relaxed MO is sufficient.
>>  
>>> The racy accesses all follow the pattern that the write is behind the
>>
>> s/racy accesses/race conditions/g
>>
>>> dlopen lock, but a read can happen concurrently (e.g. during tls access)
>>> without holding the lock.  For slotinfo entries the read value only
>>> matters if it reads from a synchronized write in dlopen or dlclose,
>>> otherwise the related dtv entry is not valid to access so it is fine to
>>> leave it in inconsistent state. Same for GL(dl_tls_max_dtv_idx) and
>>
>> s/it in/it in an/g
>>
>> s/Same/The same applies for/g
>>
>>> GL(dl_tls_generation), but there we rely on the read value being larger
>>> than the last written value that was synchronized.
>>
>> Do you mean to imply that the synchronized writes all increase the generation
>> counter, and so any out of order reads rely on the value to be increasing?
> 
> yes, if the current thread is synchronized with a dlopen and reads
> GL(dl_tls_genertion) with relaxed MO then either the gen of the
> dlopened module is read or a larger value. So we can use relaxed MO
> value to see if the dtv needs update at tls access.
> 
> (This is the difficult bit in fixing bug 19924: we can use relaxed
> MO because we update the dtv upto the gen of the module. slotinfo
> entries with larger gen are ignored, but if we want to update upto
> the global gen then we need additional synchronization.)
> 
>> Suggested:
>> The same applies for GL(dl_tls_max_dtv_idx) and GL(dl_tls_generation), but
>> there the algorithm relies on the fact that the read of the last synchronized
>> write is an increasing value.
> 
> Thanks for the review, i attached the fixed patch i plan to commit
> later today if there are no further comments.
 
Thanks. I saw you commited this and it looked good to me!

Thanks for working through these issues!

-- 
Cheers,
Carlos.