[PATCH] Fix race in pthread_mutex_lock while promoting to PTHREAD_MUTEX_ELISION_NP [BZ #23275]

[Stefan Liebler]

On 09/17/2018 03:38 PM, Florian Weimer wrote:
> On 06/12/2018 04:24 PM, Stefan Liebler wrote:
> 
>> ChangeLog:
>>
>> Â Â Â Â Â [BZ #23275]
>> Â Â Â Â Â * nptl/tst-mutex10.c: New File.
>> Â Â Â Â Â * nptl/Makefile (tests): Add tst-mutex10.
>> Â Â Â Â Â (tst-mutex-ENV): New variable.
>> Â Â Â Â Â * sysdeps/unix/sysv/linux/s390/force-elision.h: (FORCE_ELISION):
>> Â Â Â Â Â Ensure that elision path is used if elision is available.
>> Â Â Â Â Â * sysdeps/unix/sysv/linux/powerpc/force-elision.h
>> Â Â Â Â Â (FORCE_ELISION): Likewise.
>> Â Â Â Â Â * sysdeps/unix/sysv/linux/x86/force-elision.h: (FORCE_ELISION):
>> Â Â Â Â Â Likewise.
>> Â Â Â Â Â * nptl/pthreadP.h (PTHREAD_MUTEX_TYPE,
>> Â Â Â Â Â PTHREAD_MUTEX_TYPE_ELISION, PTHREAD_MUTEX_PSHARED):
>> Â Â Â Â Â Use atomic_load_relaxed.
>> Â Â Â Â Â * nptl/pthread_mutex_consistent.c (pthread_mutex_consistent):
>> Â Â Â Â Â Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_getprioceiling.c
>> Â Â Â Â Â (pthread_mutex_getprioceiling): Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full,
>> Â Â Â Â Â __pthread_mutex_cond_lock_adjust): Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_setprioceiling.c
>> Â Â Â Â Â (pthread_mutex_setprioceiling): Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_timedlock.c (__pthread_mutex_timedlock):
>> Â Â Â Â Â Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock):
>> Â Â Â Â Â Likewise.
>> Â Â Â Â Â * nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full):
>> Â Â Â Â Â Likewise.
>> Â Â Â Â Â * sysdeps/nptl/bits/thread-shared-types.h
>> Â Â Â Â Â (struct __pthread_mutex_s): Add comments.
>> Â Â Â Â Â * nptl/pthread_mutex_destroy.c (__pthread_mutex_destroy):
>> Â Â Â Â Â Use atomic_load_relaxed and atomic_store_relaxed.
>> Â Â Â Â Â * nptl/pthread_mutex_init.c (__pthread_mutex_init):
>> Â Â Â Â Â Use atomic_store_relaxed.
> 
> I had another look at this.  I think the code changes are okay, but:
> 
> There is a reference to “pthread_mutex_destroy()” in the new test.  Per 
> GNU style, this should just be “pthread_mutex_destroy”.
done

> 
> There are three places where a comma is used before “that” (as a 
> conjunction).  This comma is no longer used in contemporary standard 
> English.
Changed it in the three force-elision.h files.

> 
> The comment in force-elision.h references 
> PTHREAD_MUTEX_TIMED_NO_ELISION_NP and not PTHREAD_MUTEX_NO_ELISION_NP. > Is this deliberate?
Yes, you are right. This should be PTHREAD_MUTEX_NO_ELISION_NP as 
PTHREAD_MUTEX_TIMED_NO_ELISION_NP is a more specialized flag as 
PTHREAD_MUTEX_NO_ELISION_NP. Changed it.

> I also find the second part of this comment 
> confusing (which contains the flag reference) a bit confusing.  I assume 
> that PTHREAD_MUTEX_NO_ELISION_NP is somehow checked before FORCE_ELISION 
> is called, and that check is not racy because 
> PTHREAD_MUTEX_NO_ELISION_NP is unchanged after mutex initialization.  So 
> for each particular mutex, we either always enable elision as part of 
> the first locking operation, or we never do.
The flag PTHREAD_MUTEX_NO_ELISION_NP is not explicitly checked before 
FORCE_ELISION is called. But only one of those two flags can be set.
Your remaining assumption is correct.
I've changed the comment in the three force-elision.h files. Is this okay?

I've attached the new version of the patch. Only comments are changed. 
The code is unchanged.

> 
> Thanks,
> Florian
> 

Thanks,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20181004_race_lock_elision.patch
Type: text/x-patch
Size: 31181 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20181004/ff62291b/attachment.bin>