Submitter | Stefan Liebler |
---|---|
Date | Feb. 6, 2019, 11:25 a.m. |
Message ID | <c60e8de1-704a-9ff1-6ec9-474e47cc4686@linux.ibm.com> |
Download | mbox | patch |
Permalink | /patch/31321/ |
State | New |
Headers | show |
Comments
On 2/6/19 6:25 AM, Stefan Liebler wrote: > Hi Carlos, > I've updated the patch with three additional comments and I've mentioned the filed bug. > Please review it once again before I commit it to master and cherry pick it to the release branches. Thank you! Reviewed. >> Yes, I did that backport to RHEL 7.6. These fixes are just "further" >> fixes right? I'll work on getting this fixed in RHEL 7.7, and RHEL 8 >> for all arches. > Sounds great. > That's the same fix for pthread_mutex_trylock as previously done for pthread_mutex_lock and pthread_mutex_timedlock. I've filed these: https://bugzilla.redhat.com/show_bug.cgi?id=1672771 https://bugzilla.redhat.com/show_bug.cgi?id=1672773 Feel free to comment or verify if they are going to be needed in RHEL7.7 or RHEL8. I haven't done the analysis of the disassembly yet. If you could have a look that would help. > 20190206_pthread_mutex_trylock_barriers.patch > > commit b4c6ee19e804b0e90c117ec353ce67d321f0319b > Author: Stefan Liebler <stli@linux.ibm.com> > Date: Wed Feb 6 11:27:03 2019 +0100 > > Add compiler barriers around modifications of the robust mutex list for pthread_mutex_trylock. [BZ #24180] > > While debugging a kernel warning, Thomas Gleixner, Sebastian Sewior and > Heiko Carstens found a bug in pthread_mutex_trylock due to misordered > instructions: > 140: a5 1b 00 01 oill %r1,1 > 144: e5 48 a0 f0 00 00 mvghi 240(%r10),0 <--- THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > 14a: e3 10 a0 e0 00 24 stg %r1,224(%r10) <--- last THREAD_SETMEM of ENQUEUE_MUTEX_PI > > vs (with compiler barriers): > 140: a5 1b 00 01 oill %r1,1 > 144: e3 10 a0 e0 00 24 stg %r1,224(%r10) > 14a: e5 48 a0 f0 00 00 mvghi 240(%r10),0 > > Please have a look at the discussion: > "Re: WARN_ON_ONCE(!new_owner) within wake_futex_pi() triggerede" > (https://lore.kernel.org/lkml/20190202112006.GB3381@osiris/) > > This patch is introducing the same compiler barriers and comments > for pthread_mutex_trylock as introduced for pthread_mutex_lock and > pthread_mutex_timedlock by commit 8f9450a0b7a9e78267e8ae1ab1000ebca08e473e > "Add compiler barriers around modifications of the robust mutex list." > > ChangeLog: > > [BZ #24180] > * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock): > Add compiler barriers and comments. OK for master. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c > index 8fe43b8f0f..bf2869eca2 100644 > --- a/nptl/pthread_mutex_trylock.c > +++ b/nptl/pthread_mutex_trylock.c > @@ -94,6 +94,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > case PTHREAD_MUTEX_ROBUST_ADAPTIVE_NP: > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, > &mutex->__data.__list.__next); > + /* We need to set op_pending before starting the operation. Also > + see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 1/10 barriers. > > oldval = mutex->__data.__lock; > do > @@ -119,7 +122,12 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > /* But it is inconsistent unless marked otherwise. */ > mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT; > > + /* We must not enqueue the mutex before we have acquired it. > + Also see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 2/10 barriers. > ENQUEUE_MUTEX (mutex); > + /* We need to clear op_pending after we enqueue the mutex. */ > + __asm ("" ::: "memory"); OK. 3/10 barriers. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > /* Note that we deliberately exist here. If we fall > @@ -135,6 +143,8 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > int kind = PTHREAD_MUTEX_TYPE (mutex); > if (kind == PTHREAD_MUTEX_ROBUST_ERRORCHECK_NP) > { > + /* We do not need to ensure ordering wrt another memory > + access. Also see comments at ENQUEUE_MUTEX. */ OK. 1/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, > NULL); > return EDEADLK; > @@ -142,6 +152,8 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > > if (kind == PTHREAD_MUTEX_ROBUST_RECURSIVE_NP) > { > + /* We do not need to ensure ordering wrt another memory > + access. */ OK. 2/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, > NULL); > > @@ -160,6 +172,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > id, 0); > if (oldval != 0 && (oldval & FUTEX_OWNER_DIED) == 0) > { > + /* We haven't acquired the lock as it is already acquired by > + another owner. We do not need to ensure ordering wrt another > + memory access. */ OK. 3/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > return EBUSY; > @@ -173,13 +188,20 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > if (oldval == id) > lll_unlock (mutex->__data.__lock, > PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); > + /* FIXME This violates the mutex destruction requirements. See > + __pthread_mutex_unlock_full. */ OK. 4/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > return ENOTRECOVERABLE; > } > } > while ((oldval & FUTEX_OWNER_DIED) != 0); > > + /* We must not enqueue the mutex before we have acquired it. > + Also see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 4/10 barriers. > ENQUEUE_MUTEX (mutex); > + /* We need to clear op_pending after we enqueue the mutex. */ > + __asm ("" ::: "memory"); OK. 5/10 barriers. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > mutex->__data.__owner = id; > @@ -211,10 +233,15 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > } > > if (robust) > - /* Note: robust PI futexes are signaled by setting bit 0. */ > - THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, > - (void *) (((uintptr_t) &mutex->__data.__list.__next) > - | 1)); > + { > + /* Note: robust PI futexes are signaled by setting bit 0. */ > + THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, > + (void *) (((uintptr_t) &mutex->__data.__list.__next) > + | 1)); > + /* We need to set op_pending before starting the operation. Also > + see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 6/10 barriers. > + } > > oldval = mutex->__data.__lock; > > @@ -223,12 +250,16 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > { > if (kind == PTHREAD_MUTEX_ERRORCHECK_NP) > { > + /* We do not need to ensure ordering wrt another memory > + access. */ OK. 5/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > return EDEADLK; > } > > if (kind == PTHREAD_MUTEX_RECURSIVE_NP) > { > + /* We do not need to ensure ordering wrt another memory > + access. */ OK. 6/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > /* Just bump the counter. */ > @@ -250,6 +281,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > { > if ((oldval & FUTEX_OWNER_DIED) == 0) > { > + /* We haven't acquired the lock as it is already acquired by > + another owner. We do not need to ensure ordering wrt another > + memory access. */ OK. 7/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > return EBUSY; > @@ -270,6 +304,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > if (INTERNAL_SYSCALL_ERROR_P (e, __err) > && INTERNAL_SYSCALL_ERRNO (e, __err) == EWOULDBLOCK) > { > + /* The kernel has not yet finished the mutex owner death. > + We do not need to ensure ordering wrt another memory > + access. */ OK. 8/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > return EBUSY; > @@ -287,7 +324,12 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > /* But it is inconsistent unless marked otherwise. */ > mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT; > > + /* We must not enqueue the mutex before we have acquired it. > + Also see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 7/10 barriers. > ENQUEUE_MUTEX (mutex); > + /* We need to clear op_pending after we enqueue the mutex. */ > + __asm ("" ::: "memory"); OK. 8/10 barriers. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > > /* Note that we deliberately exit here. If we fall > @@ -310,13 +352,20 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) > PTHREAD_ROBUST_MUTEX_PSHARED (mutex)), > 0, 0); > > + /* To the kernel, this will be visible after the kernel has > + acquired the mutex in the syscall. */ OK. 9/9 comments. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > return ENOTRECOVERABLE; > } > > if (robust) > { > + /* We must not enqueue the mutex before we have acquired it. > + Also see comments at ENQUEUE_MUTEX. */ > + __asm ("" ::: "memory"); OK. 9/10 barriers. > ENQUEUE_MUTEX_PI (mutex); > + /* We need to clear op_pending after we enqueue the mutex. */ > + __asm ("" ::: "memory"); OK. 10/10 barriers. > THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); > } >
On 02/06/2019 04:59 PM, Carlos O'Donell wrote: > On 2/6/19 6:25 AM, Stefan Liebler wrote: >> Hi Carlos, >> I've updated the patch with three additional comments and I've mentioned the filed bug. >> Please review it once again before I commit it to master and cherry pick it to the release branches. > > Thank you! Reviewed. Committed to master (2.29.9000): "Add compiler barriers around modifications of the robust mutex list for pthread_mutex_trylock. [BZ #24180]" (https://sourceware.org/git/?p=glibc.git;a=commit;h=823624bdc47f1f80109c9c52dee7939b9386d708) and backported it to glibc 2.25 ... 2.29 release branches. Thanks. Stefan > >>> Yes, I did that backport to RHEL 7.6. These fixes are just "further" >>> fixes right? I'll work on getting this fixed in RHEL 7.7, and RHEL 8 >>> for all arches. >> Sounds great. >> That's the same fix for pthread_mutex_trylock as previously done for pthread_mutex_lock and pthread_mutex_timedlock. > > I've filed these: > https://bugzilla.redhat.com/show_bug.cgi?id=1672771 > https://bugzilla.redhat.com/show_bug.cgi?id=1672773 > > Feel free to comment or verify if they are going to be needed in RHEL7.7 > or RHEL8. I haven't done the analysis of the disassembly yet. If you > could have a look that would help. >
On 2/7/19 10:05 AM, Stefan Liebler wrote: > On 02/06/2019 04:59 PM, Carlos O'Donell wrote: >> On 2/6/19 6:25 AM, Stefan Liebler wrote: >>> Hi Carlos, >>> I've updated the patch with three additional comments and I've mentioned the filed bug. >>> Please review it once again before I commit it to master and cherry pick it to the release branches. >> >> Thank you! Reviewed. > Committed to master (2.29.9000): > "Add compiler barriers around modifications of the robust mutex list for pthread_mutex_trylock. [BZ #24180]" > (https://sourceware.org/git/?p=glibc.git;a=commit;h=823624bdc47f1f80109c9c52dee7939b9386d708) > > and backported it to glibc 2.25 ... 2.29 release branches. Thank you! Now if only we could fix the final robust mutex design flaw[1] ;-)
Patch
commit b4c6ee19e804b0e90c117ec353ce67d321f0319b Author: Stefan Liebler <stli@linux.ibm.com> Date: Wed Feb 6 11:27:03 2019 +0100 Add compiler barriers around modifications of the robust mutex list for pthread_mutex_trylock. [BZ #24180] While debugging a kernel warning, Thomas Gleixner, Sebastian Sewior and Heiko Carstens found a bug in pthread_mutex_trylock due to misordered instructions: 140: a5 1b 00 01 oill %r1,1 144: e5 48 a0 f0 00 00 mvghi 240(%r10),0 <--- THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); 14a: e3 10 a0 e0 00 24 stg %r1,224(%r10) <--- last THREAD_SETMEM of ENQUEUE_MUTEX_PI vs (with compiler barriers): 140: a5 1b 00 01 oill %r1,1 144: e3 10 a0 e0 00 24 stg %r1,224(%r10) 14a: e5 48 a0 f0 00 00 mvghi 240(%r10),0 Please have a look at the discussion: "Re: WARN_ON_ONCE(!new_owner) within wake_futex_pi() triggerede" (https://lore.kernel.org/lkml/20190202112006.GB3381@osiris/) This patch is introducing the same compiler barriers and comments for pthread_mutex_trylock as introduced for pthread_mutex_lock and pthread_mutex_timedlock by commit 8f9450a0b7a9e78267e8ae1ab1000ebca08e473e "Add compiler barriers around modifications of the robust mutex list." ChangeLog: [BZ #24180] * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock): Add compiler barriers and comments. diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c index 8fe43b8f0f..bf2869eca2 100644 --- a/nptl/pthread_mutex_trylock.c +++ b/nptl/pthread_mutex_trylock.c @@ -94,6 +94,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) case PTHREAD_MUTEX_ROBUST_ADAPTIVE_NP: THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, &mutex->__data.__list.__next); + /* We need to set op_pending before starting the operation. Also + see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); oldval = mutex->__data.__lock; do @@ -119,7 +122,12 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) /* But it is inconsistent unless marked otherwise. */ mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT; + /* We must not enqueue the mutex before we have acquired it. + Also see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); ENQUEUE_MUTEX (mutex); + /* We need to clear op_pending after we enqueue the mutex. */ + __asm ("" ::: "memory"); THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); /* Note that we deliberately exist here. If we fall @@ -135,6 +143,8 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) int kind = PTHREAD_MUTEX_TYPE (mutex); if (kind == PTHREAD_MUTEX_ROBUST_ERRORCHECK_NP) { + /* We do not need to ensure ordering wrt another memory + access. Also see comments at ENQUEUE_MUTEX. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return EDEADLK; @@ -142,6 +152,8 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) if (kind == PTHREAD_MUTEX_ROBUST_RECURSIVE_NP) { + /* We do not need to ensure ordering wrt another memory + access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); @@ -160,6 +172,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) id, 0); if (oldval != 0 && (oldval & FUTEX_OWNER_DIED) == 0) { + /* We haven't acquired the lock as it is already acquired by + another owner. We do not need to ensure ordering wrt another + memory access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return EBUSY; @@ -173,13 +188,20 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) if (oldval == id) lll_unlock (mutex->__data.__lock, PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); + /* FIXME This violates the mutex destruction requirements. See + __pthread_mutex_unlock_full. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return ENOTRECOVERABLE; } } while ((oldval & FUTEX_OWNER_DIED) != 0); + /* We must not enqueue the mutex before we have acquired it. + Also see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); ENQUEUE_MUTEX (mutex); + /* We need to clear op_pending after we enqueue the mutex. */ + __asm ("" ::: "memory"); THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); mutex->__data.__owner = id; @@ -211,10 +233,15 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) } if (robust) - /* Note: robust PI futexes are signaled by setting bit 0. */ - THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, - (void *) (((uintptr_t) &mutex->__data.__list.__next) - | 1)); + { + /* Note: robust PI futexes are signaled by setting bit 0. */ + THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, + (void *) (((uintptr_t) &mutex->__data.__list.__next) + | 1)); + /* We need to set op_pending before starting the operation. Also + see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); + } oldval = mutex->__data.__lock; @@ -223,12 +250,16 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) { if (kind == PTHREAD_MUTEX_ERRORCHECK_NP) { + /* We do not need to ensure ordering wrt another memory + access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return EDEADLK; } if (kind == PTHREAD_MUTEX_RECURSIVE_NP) { + /* We do not need to ensure ordering wrt another memory + access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); /* Just bump the counter. */ @@ -250,6 +281,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) { if ((oldval & FUTEX_OWNER_DIED) == 0) { + /* We haven't acquired the lock as it is already acquired by + another owner. We do not need to ensure ordering wrt another + memory access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return EBUSY; @@ -270,6 +304,9 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) if (INTERNAL_SYSCALL_ERROR_P (e, __err) && INTERNAL_SYSCALL_ERRNO (e, __err) == EWOULDBLOCK) { + /* The kernel has not yet finished the mutex owner death. + We do not need to ensure ordering wrt another memory + access. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return EBUSY; @@ -287,7 +324,12 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) /* But it is inconsistent unless marked otherwise. */ mutex->__data.__owner = PTHREAD_MUTEX_INCONSISTENT; + /* We must not enqueue the mutex before we have acquired it. + Also see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); ENQUEUE_MUTEX (mutex); + /* We need to clear op_pending after we enqueue the mutex. */ + __asm ("" ::: "memory"); THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); /* Note that we deliberately exit here. If we fall @@ -310,13 +352,20 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex) PTHREAD_ROBUST_MUTEX_PSHARED (mutex)), 0, 0); + /* To the kernel, this will be visible after the kernel has + acquired the mutex in the syscall. */ THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); return ENOTRECOVERABLE; } if (robust) { + /* We must not enqueue the mutex before we have acquired it. + Also see comments at ENQUEUE_MUTEX. */ + __asm ("" ::: "memory"); ENQUEUE_MUTEX_PI (mutex); + /* We need to clear op_pending after we enqueue the mutex. */ + __asm ("" ::: "memory"); THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL); }