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

[Florian Weimer <fweimer@redhat.com>]

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”.

There are three places where a comma is used before “that” (as a 
conjunction).  This comma is no longer used in contemporary standard 
English.

The comment in force-elision.h references 
PTHREAD_MUTEX_TIMED_NO_ELISION_NP and not PTHREAD_MUTEX_NO_ELISION_NP. 
Is this deliberate?  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.

Thanks,
Florian