commit 86013ef5cea322b8f4b9c22f230c22cce369e947 Author: Carlos O'Donell Date: Mon Jan 21 22:50:12 2019 -0500 nptl: Fix pthread_rwlock_try*lock stalls (Bug 23844) For a full analysis of both the pthread_rwlock_tryrdlock() stall and the pthread_rwlock_trywrlock() stall see: https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14 In the pthread_rwlock_trydlock() function we fail to inspect for PTHREAD_RWLOCK_FUTEX_USED in __wrphase_futex and wake the waiting readers. In the pthread_rwlock_trywrlock() function we write 1 to __wrphase_futex and loose the setting of the PTHREAD_RWLOCK_FUTEX_USED bit, again failing to wake waiting readers during unlock. The fix in the case of pthread_rwlock_trydlock() is to check for PTHREAD_RWLOCK_FUTEX_USED and wake the readers. The fix in the case of pthread_rwlock_trywrlock() is to only write 1 to __wrphase_futex if we installed the write phase, since all other readers would be spinning waiting for this step. We add two new tests, one exercises the stall for pthread_rwlock_trywrlock() which is easy to exercise, and one exercises the stall for pthread_rwlock_trydlock() which is harder to exercise. The pthread_rwlock_trywrlock() test fails consistently without the fix, and passes after. The pthread_rwlock_tryrdlock() test fails roughly 5-10% of the time without the fix, and passes all the time after. Signed-off-by: Carlos O'Donell Signed-off-by: Torvald Riegel Signed-off-by: Rik Prohaska Co-authored-by: Torvald Riegel Co-authored-by: Rik Prohaska (cherry picked from commit 5fc9ed4c4058bfbdf51ad6e7aac7d209b580e8c4) diff --git a/ChangeLog b/ChangeLog index 59dab18463..adb4e719a6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,20 @@ +2019-01-31 Carlos O'Donell + Torvald Riegel + Rik Prohaska + + [BZ# 23844] + * nptl/Makefile (tests): Add tst-rwlock-tryrdlock-stall, and + tst-rwlock-trywrlock-stall. + * nptl/pthread_rwlock_tryrdlock.c (__pthread_rwlock_tryrdlock): + Wake waiters if PTHREAD_RWLOCK_FUTEX_USED is set. + * nptl/pthread_rwlock_trywrlock.c (__pthread_rwlock_trywrlock): + Set __wrphase_fute to 1 only if we started the write phase. + * nptl/tst-rwlock-tryrdlock-stall.c: New file. + * nptl/tst-rwlock-trywrlock-stall.c: New file. + * support/Makefile (libsupport-routines): Add xpthread_rwlock_destroy. + * support/xpthread_rwlock_destroy.c: New file. + * support/xthread.h: Declare xpthread_rwlock_destroy. + 2019-01-31 Siddhesh Poyarekar * version.h (RELEASE): Set to "stable". diff --git a/nptl/Makefile b/nptl/Makefile index 340282c6cb..0e316edfac 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -319,7 +319,8 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \ tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \ tst-cnd-timedwait tst-thrd-detach tst-mtx-basic tst-thrd-sleep \ tst-mtx-recursive tst-tss-basic tst-call-once tst-mtx-timedlock \ - tst-rwlock-pwn + tst-rwlock-pwn \ + tst-rwlock-tryrdlock-stall tst-rwlock-trywrlock-stall tests-internal := tst-rwlock19 tst-rwlock20 \ tst-sem11 tst-sem12 tst-sem13 \ diff --git a/nptl/pthread_rwlock_tryrdlock.c b/nptl/pthread_rwlock_tryrdlock.c index 368862ff07..2f94f17f36 100644 --- a/nptl/pthread_rwlock_tryrdlock.c +++ b/nptl/pthread_rwlock_tryrdlock.c @@ -94,15 +94,22 @@ __pthread_rwlock_tryrdlock (pthread_rwlock_t *rwlock) /* Same as in __pthread_rwlock_rdlock_full: We started the read phase, so we are also responsible for updating the write-phase futex. Relaxed MO is sufficient. - Note that there can be no other reader that we have to wake - because all other readers will see the read phase started by us - (or they will try to start it themselves); if a writer started - the read phase, we cannot have started it. Furthermore, we - cannot discard a PTHREAD_RWLOCK_FUTEX_USED flag because we will - overwrite the value set by the most recent writer (or the readers - before it in case of explicit hand-over) and we know that there - are no waiting readers. */ - atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0); + We have to do the same steps as a writer would when handing over the + read phase to use because other readers cannot distinguish between + us and the writer. + Note that __pthread_rwlock_tryrdlock callers will not have to be + woken up because they will either see the read phase started by us + or they will try to start it themselves; however, callers of + __pthread_rwlock_rdlock_full just increase the reader count and then + check what state the lock is in, so they cannot distinguish between + us and a writer that acquired and released the lock in the + meantime. */ + if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0) + & PTHREAD_RWLOCK_FUTEX_USED) != 0) + { + int private = __pthread_rwlock_get_private (rwlock); + futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private); + } } return 0; diff --git a/nptl/pthread_rwlock_trywrlock.c b/nptl/pthread_rwlock_trywrlock.c index fd37a71ce4..fae475cc70 100644 --- a/nptl/pthread_rwlock_trywrlock.c +++ b/nptl/pthread_rwlock_trywrlock.c @@ -46,8 +46,15 @@ __pthread_rwlock_trywrlock (pthread_rwlock_t *rwlock) &rwlock->__data.__readers, &r, r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED)) { + /* We have become the primary writer and we cannot have shared + the PTHREAD_RWLOCK_FUTEX_USED flag with someone else, so we + can simply enable blocking (see full wrlock code). */ atomic_store_relaxed (&rwlock->__data.__writers_futex, 1); - atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1); + /* If we started a write phase, we need to enable readers to + wait. If we did not, we must not change it because other threads + may have set the PTHREAD_RWLOCK_FUTEX_USED in the meantime. */ + if ((r & PTHREAD_RWLOCK_WRPHASE) == 0) + atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1); atomic_store_relaxed (&rwlock->__data.__cur_writer, THREAD_GETMEM (THREAD_SELF, tid)); return 0; diff --git a/nptl/tst-rwlock-tryrdlock-stall.c b/nptl/tst-rwlock-tryrdlock-stall.c new file mode 100644 index 0000000000..5e476da2b8 --- /dev/null +++ b/nptl/tst-rwlock-tryrdlock-stall.c @@ -0,0 +1,355 @@ +/* Bug 23844: Test for pthread_rwlock_tryrdlock stalls. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* For a full analysis see comment: + https://sourceware.org/bugzilla/show_bug.cgi?id=23844#c14 + + Provided here for reference: + + --- Analysis of pthread_rwlock_tryrdlock() stall --- + A read lock begins to execute. + + In __pthread_rwlock_rdlock_full: + + We can attempt a read lock, but find that the lock is + in a write phase (PTHREAD_RWLOCK_WRPHASE, or WP-bit + is set), and the lock is held by a primary writer + (PTHREAD_RWLOCK_WRLOCKED is set). In this case we must + wait for explicit hand over from the writer to us or + one of the other waiters. The read lock threads are + about to execute: + + 341 r = (atomic_fetch_add_acquire (&rwlock->__data.__readers, + 342 (1 << PTHREAD_RWLOCK_READER_SHIFT)) + 343 + (1 << PTHREAD_RWLOCK_READER_SHIFT)); + + An unlock beings to execute. + + Then in __pthread_rwlock_wrunlock: + + 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers); + ... + 549 while (!atomic_compare_exchange_weak_release + 550 (&rwlock->__data.__readers, &r, + 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED) + 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0 + 553 : PTHREAD_RWLOCK_WRPHASE)))) + 554 { + ... + 556 } + + We clear PTHREAD_RWLOCK_WRLOCKED, and if there are + no readers so we leave the lock in PTHRAD_RWLOCK_WRPHASE. + + Back in the read lock. + + The read lock adjusts __readres as above. + + 383 while ((r & PTHREAD_RWLOCK_WRPHASE) != 0 + 384 && (r & PTHREAD_RWLOCK_WRLOCKED) == 0) + 385 { + ... + 390 if (atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers, &r, + 391 r ^ PTHREAD_RWLOCK_WRPHASE)) + 392 { + + And then attemps to start the read phase. + + Assume there happens to be a tryrdlock at this point, noting + that PTHREAD_RWLOCK_WRLOCKED is clear, and PTHREAD_RWLOCK_WRPHASE + is 1. So the try lock attemps to start the read phase. + + In __pthread_rwlock_tryrdlock: + + 44 if ((r & PTHREAD_RWLOCK_WRPHASE) == 0) + 45 { + ... + 49 if (((r & PTHREAD_RWLOCK_WRLOCKED) != 0) + 50 && (rwlock->__data.__flags + 51 == PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP)) + 52 return EBUSY; + 53 rnew = r + (1 << PTHREAD_RWLOCK_READER_SHIFT); + 54 } + ... + 89 while (!atomic_compare_exchange_weak_acquire (&rwlock->__data.__readers, + 90 &r, rnew)); + + And succeeds. + + Back in the write unlock: + + 557 if ((r >> PTHREAD_RWLOCK_READER_SHIFT) != 0) + 558 { + ... + 563 if ((atomic_exchange_relaxed (&rwlock->__data.__wrphase_futex, 0) + 564 & PTHREAD_RWLOCK_FUTEX_USED) != 0) + 565 futex_wake (&rwlock->__data.__wrphase_futex, INT_MAX, private); + 566 } + + We note that PTHREAD_RWLOCK_FUTEX_USED is non-zero + and don't wake anyone. This is OK because we handed + over to the trylock. It will be the trylock's responsibility + to wake any waiters. + + Back in the read lock: + + The read lock fails to install PTHRAD_REWLOCK_WRPHASE as 0 because + the __readers value was adjusted by the trylock, and so it falls through + to waiting on the lock for explicit handover from either a new writer + or a new reader. + + 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, + 449 1 | PTHREAD_RWLOCK_FUTEX_USED, + 450 abstime, private); + + We use PTHREAD_RWLOCK_FUTEX_USED to indicate the futex + is in use. + + At this point we have readers waiting on the read lock + to unlock. The wrlock is done. The trylock is finishing + the installation of the read phase. + + 92 if ((r & PTHREAD_RWLOCK_WRPHASE) != 0) + 93 { + ... + 105 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 0); + 106 } + + The trylock does note that we were the one that + installed the read phase, but the comments are not + correct, the execution ordering above shows that + readers might indeed be waiting, and they are. + + The atomic_store_relaxed throws away PTHREAD_RWLOCK_FUTEX_USED, + and the waiting reader is never worken becuase as noted + above it is conditional on the futex being used. + + The solution is for the trylock thread to inspect + PTHREAD_RWLOCK_FUTEX_USED and wake the waiting readers. + + --- Analysis of pthread_rwlock_trywrlock() stall --- + + A write lock begins to execute, takes the write lock, + and then releases the lock... + + In pthread_rwlock_wrunlock(): + + 547 unsigned int r = atomic_load_relaxed (&rwlock->__data.__readers); + ... + 549 while (!atomic_compare_exchange_weak_release + 550 (&rwlock->__data.__readers, &r, + 551 ((r ^ PTHREAD_RWLOCK_WRLOCKED) + 552 ^ ((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0 ? 0 + 553 : PTHREAD_RWLOCK_WRPHASE)))) + 554 { + ... + 556 } + + ... leaving it in the write phase with zero readers + (the case where we leave the write phase in place + during a write unlock). + + A write trylock begins to execute. + + In __pthread_rwlock_trywrlock: + + 40 while (((r & PTHREAD_RWLOCK_WRLOCKED) == 0) + 41 && (((r >> PTHREAD_RWLOCK_READER_SHIFT) == 0) + 42 || (prefer_writer && ((r & PTHREAD_RWLOCK_WRPHASE) != 0)))) + 43 { + + The lock is not locked. + + There are no readers. + + 45 if (atomic_compare_exchange_weak_acquire ( + 46 &rwlock->__data.__readers, &r, + 47 r | PTHREAD_RWLOCK_WRPHASE | PTHREAD_RWLOCK_WRLOCKED)) + + We atomically install the write phase and we take the + exclusive write lock. + + 48 { + 49 atomic_store_relaxed (&rwlock->__data.__writers_futex, 1); + + We get this far. + + A reader lock begins to execute. + + In pthread_rwlock_rdlock: + + 437 for (;;) + 438 { + 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex)) + 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED)) + 441 { + 442 int private = __pthread_rwlock_get_private (rwlock); + 443 if (((wpf & PTHREAD_RWLOCK_FUTEX_USED) == 0) + 444 && (!atomic_compare_exchange_weak_relaxed + 445 (&rwlock->__data.__wrphase_futex, + 446 &wpf, wpf | PTHREAD_RWLOCK_FUTEX_USED))) + 447 continue; + 448 int err = futex_abstimed_wait (&rwlock->__data.__wrphase_futex, + 449 1 | PTHREAD_RWLOCK_FUTEX_USED, + 450 abstime, private); + + We are in a write phase, so the while() on line 439 is true. + + The value of wpf does not have PTHREAD_RWLOCK_FUTEX_USED set + since this is the first reader to lock. + + The atomic operation sets wpf with PTHREAD_RELOCK_FUTEX_USED + on the expectation that this reader will be woken during + the handoff. + + Back in pthread_rwlock_trywrlock: + + 50 atomic_store_relaxed (&rwlock->__data.__wrphase_futex, 1); + 51 atomic_store_relaxed (&rwlock->__data.__cur_writer, + 52 THREAD_GETMEM (THREAD_SELF, tid)); + 53 return 0; + 54 } + ... + 57 } + + We write 1 to __wrphase_futex discarding PTHREAD_RWLOCK_FUTEX_USED, + and so in the unlock we will not awaken the waiting reader. + + The solution to this is to realize that if we did not start the write + phase we need not write 1 or any other value to __wrphase_futex. + This ensures that any readers (which saw __wrphase_futex != 0) can + set PTHREAD_RWLOCK_FUTEX_USED and this can be used at unlock to + wake them. + + If we installed the write phase then all other readers are looping + here: + + In __pthread_rwlock_rdlock_full: + + 437 for (;;) + 438 { + 439 while (((wpf = atomic_load_relaxed (&rwlock->__data.__wrphase_futex)) + 440 | PTHREAD_RWLOCK_FUTEX_USED) == (1 | PTHREAD_RWLOCK_FUTEX_USED)) + 441 { + ... + 508 } + + waiting for the write phase to be installed or removed before they + can begin waiting on __wrphase_futex (part of the algorithm), or + taking a concurrent read lock, and thus we can safely write 1 to + __wrphase_futex. + + If we did not install the write phase then the readers may already + be waiting on the futex, the original writer wrote 1 to __wrphase_futex + as part of starting the write phase, and we cannot also write 1 + without loosing the PTHREAD_RWLOCK_FUTEX_USED bit. + + --- + + Summary for the pthread_rwlock_tryrdlock() stall: + + The stall is caused by pthread_rwlock_tryrdlock failing to check + that PTHREAD_RWLOCK_FUTEX_USED is set in the __wrphase_futex futex + and then waking the futex. + + The fix for bug 23844 ensures that waiters on __wrphase_futex are + correctly woken. Before the fix the test stalls as readers can + wait forever on __wrphase_futex. */ + +#include +#include +#include +#include +#include +#include + +/* We need only one lock to reproduce the issue. We will need multiple + threads to get the exact case where we have a read, try, and unlock + all interleaving to produce the case where the readers are waiting + and the try fails to wake them. */ +pthread_rwlock_t onelock; + +/* The number of threads is arbitrary but empirically chosen to have + enough threads that we see the condition where waiting readers are + not woken by a successful tryrdlock. */ +#define NTHREADS 32 + +_Atomic int do_exit; + +void * +run_loop (void *arg) +{ + int i = 0, ret; + while (!do_exit) + { + /* Arbitrarily choose if we are the writer or reader. Choose a + high enough ratio of readers to writers to make it likely + that readers block (and eventually are susceptable to + stalling). + + If we are a writer, take the write lock, and then unlock. + If we are a reader, try the lock, then lock, then unlock. */ + if ((i % 8) != 0) + xpthread_rwlock_wrlock (&onelock); + else + { + if ((ret = pthread_rwlock_tryrdlock (&onelock)) != 0) + { + if (ret == EBUSY) + xpthread_rwlock_rdlock (&onelock); + else + exit (EXIT_FAILURE); + } + } + /* Thread does some work and then unlocks. */ + xpthread_rwlock_unlock (&onelock); + i++; + } + return NULL; +} + +int +do_test (void) +{ + int i; + pthread_t tids[NTHREADS]; + xpthread_rwlock_init (&onelock, NULL); + for (i = 0; i < NTHREADS; i++) + tids[i] = xpthread_create (NULL, run_loop, NULL); + /* Run for some amount of time. Empirically speaking exercising + the stall via pthread_rwlock_tryrdlock is much harder, and on + a 3.5GHz 4 core x86_64 VM system it takes somewhere around + 20-200s to stall, approaching 100% stall past 200s. We can't + wait that long for a regression test so we just test for 20s, + and expect the stall to happen with a 5-10% chance (enough for + developers to see). */ + sleep (20); + /* Then exit. */ + printf ("INFO: Exiting...\n"); + do_exit = 1; + /* If any readers stalled then we will timeout waiting for them. */ + for (i = 0; i < NTHREADS; i++) + xpthread_join (tids[i]); + printf ("INFO: Done.\n"); + xpthread_rwlock_destroy (&onelock); + printf ("PASS: No pthread_rwlock_tryrdlock stalls detected.\n"); + return 0; +} + +#define TIMEOUT 30 +#include diff --git a/nptl/tst-rwlock-trywrlock-stall.c b/nptl/tst-rwlock-trywrlock-stall.c new file mode 100644 index 0000000000..14d27cbcbc --- /dev/null +++ b/nptl/tst-rwlock-trywrlock-stall.c @@ -0,0 +1,108 @@ +/* Bug 23844: Test for pthread_rwlock_trywrlock stalls. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* For a full analysis see comments in tst-rwlock-tryrdlock-stall.c. + + Summary for the pthread_rwlock_trywrlock() stall: + + The stall is caused by pthread_rwlock_trywrlock setting + __wrphase_futex futex to 1 and loosing the + PTHREAD_RWLOCK_FUTEX_USED bit. + + The fix for bug 23844 ensures that waiters on __wrphase_futex are + correctly woken. Before the fix the test stalls as readers can + wait forever on __wrphase_futex. */ + +#include +#include +#include +#include +#include +#include + +/* We need only one lock to reproduce the issue. We will need multiple + threads to get the exact case where we have a read, try, and unlock + all interleaving to produce the case where the readers are waiting + and the try clears the PTHREAD_RWLOCK_FUTEX_USED bit and a + subsequent unlock fails to wake them. */ +pthread_rwlock_t onelock; + +/* The number of threads is arbitrary but empirically chosen to have + enough threads that we see the condition where waiting readers are + not woken by a successful unlock. */ +#define NTHREADS 32 + +_Atomic int do_exit; + +void * +run_loop (void *arg) +{ + int i = 0, ret; + while (!do_exit) + { + /* Arbitrarily choose if we are the writer or reader. Choose a + high enough ratio of readers to writers to make it likely + that readers block (and eventually are susceptable to + stalling). + + If we are a writer, take the write lock, and then unlock. + If we are a reader, try the lock, then lock, then unlock. */ + if ((i % 8) != 0) + { + if ((ret = pthread_rwlock_trywrlock (&onelock)) != 0) + { + if (ret == EBUSY) + xpthread_rwlock_wrlock (&onelock); + else + exit (EXIT_FAILURE); + } + } + else + xpthread_rwlock_rdlock (&onelock); + /* Thread does some work and then unlocks. */ + xpthread_rwlock_unlock (&onelock); + i++; + } + return NULL; +} + +int +do_test (void) +{ + int i; + pthread_t tids[NTHREADS]; + xpthread_rwlock_init (&onelock, NULL); + for (i = 0; i < NTHREADS; i++) + tids[i] = xpthread_create (NULL, run_loop, NULL); + /* Run for some amount of time. The pthread_rwlock_tryrwlock stall + is very easy to trigger and happens in seconds under the test + conditions. */ + sleep (10); + /* Then exit. */ + printf ("INFO: Exiting...\n"); + do_exit = 1; + /* If any readers stalled then we will timeout waiting for them. */ + for (i = 0; i < NTHREADS; i++) + xpthread_join (tids[i]); + printf ("INFO: Done.\n"); + xpthread_rwlock_destroy (&onelock); + printf ("PASS: No pthread_rwlock_tryrwlock stalls detected.\n"); + return 0; +} + +#include diff --git a/support/Makefile b/support/Makefile index 432cf2fe6c..c15b93647c 100644 --- a/support/Makefile +++ b/support/Makefile @@ -129,6 +129,7 @@ libsupport-routines = \ xpthread_mutexattr_settype \ xpthread_once \ xpthread_rwlock_init \ + xpthread_rwlock_destroy \ xpthread_rwlock_rdlock \ xpthread_rwlock_unlock \ xpthread_rwlock_wrlock \ diff --git a/support/xpthread_rwlock_destroy.c b/support/xpthread_rwlock_destroy.c new file mode 100644 index 0000000000..6d6e953569 --- /dev/null +++ b/support/xpthread_rwlock_destroy.c @@ -0,0 +1,26 @@ +/* pthread_rwlock_destroy with error checking. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include + +void +xpthread_rwlock_destroy (pthread_rwlock_t *rwlock) +{ + xpthread_check_return ("pthread_rwlock_destroy", + pthread_rwlock_destroy (rwlock)); +} diff --git a/support/xthread.h b/support/xthread.h index 47c23235f3..9fe1f68b3b 100644 --- a/support/xthread.h +++ b/support/xthread.h @@ -84,6 +84,7 @@ void xpthread_rwlockattr_setkind_np (pthread_rwlockattr_t *attr, int pref); void xpthread_rwlock_wrlock (pthread_rwlock_t *rwlock); void xpthread_rwlock_rdlock (pthread_rwlock_t *rwlock); void xpthread_rwlock_unlock (pthread_rwlock_t *rwlock); +void xpthread_rwlock_destroy (pthread_rwlock_t *rwlock); __END_DECLS commit 726a78867b3144e9b9da10197bcf59bde3d8b2a4 Author: H.J. Lu Date: Mon Feb 4 08:55:52 2019 -0800 x86-64 memcmp: Use unsigned Jcc instructions on size [BZ #24155] Since the size argument is unsigned. we should use unsigned Jcc instructions, instead of signed, to check size. Tested on x86-64 and x32, with and without --disable-multi-arch. [BZ #24155] CVE-2019-7309 * NEWS: Updated for CVE-2019-7309. * sysdeps/x86_64/memcmp.S: Use RDX_LP for size. Clear the upper 32 bits of RDX register for x32. Use unsigned Jcc instructions, instead of signed. * sysdeps/x86_64/x32/Makefile (tests): Add tst-size_t-memcmp-2. * sysdeps/x86_64/x32/tst-size_t-memcmp-2.c: New test. (cherry picked from commit 3f635fb43389b54f682fc9ed2acc0b2aaf4a923d) diff --git a/ChangeLog b/ChangeLog index adb4e719a6..e0969afd80 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2019-02-04 H.J. Lu + + [BZ #24155] + CVE-2019-7309 + * NEWS: Updated for CVE-2019-7309. + * sysdeps/x86_64/memcmp.S: Use RDX_LP for size. Clear the + upper 32 bits of RDX register for x32. Use unsigned Jcc + instructions, instead of signed. + * sysdeps/x86_64/x32/Makefile (tests): Add tst-size_t-memcmp-2. + * sysdeps/x86_64/x32/tst-size_t-memcmp-2.c: New test. + 2019-01-31 Carlos O'Donell Torvald Riegel Rik Prohaska diff --git a/NEWS b/NEWS index 912a9bdc0f..1751ed118a 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,23 @@ See the end for copying conditions. Please send GNU C library bug reports via using `glibc' in the "product" field. +Version 2.29.1 + +The following bugs are resolved with this release: + + [24155] x32 memcmp can treat positive length as 0 (if sign bit in RDX is set) (CVE-2019-7309) + +Security related changes: + + CVE-2019-7309: x86-64 memcmp used signed Jcc instructions to check + size. For x86-64, memcmp on an object size larger than SSIZE_MAX + has undefined behavior. On x32, the size_t argument may be passed + in the lower 32 bits of the 64-bit RDX register with non-zero upper + 32 bits. When it happened with the sign bit of RDX register set, + memcmp gave the wrong result since it treated the size argument as + zero. Reported by H.J. Lu. + + Version 2.29 Major new features: diff --git a/sysdeps/x86_64/memcmp.S b/sysdeps/x86_64/memcmp.S index 1fc487caa5..1322bb3b92 100644 --- a/sysdeps/x86_64/memcmp.S +++ b/sysdeps/x86_64/memcmp.S @@ -21,14 +21,18 @@ .text ENTRY (memcmp) - test %rdx, %rdx +#ifdef __ILP32__ + /* Clear the upper 32 bits. */ + movl %edx, %edx +#endif + test %RDX_LP, %RDX_LP jz L(finz) cmpq $1, %rdx - jle L(finr1b) + jbe L(finr1b) subq %rdi, %rsi movq %rdx, %r10 cmpq $32, %r10 - jge L(gt32) + jae L(gt32) /* Handle small chunks and last block of less than 32 bytes. */ L(small): testq $1, %r10 @@ -156,7 +160,7 @@ L(A32): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) /* Pre-unroll to be ready for unrolled 64B loop. */ testq $32, %rdi jz L(A64) @@ -178,7 +182,7 @@ L(A64): movq %r11, %r10 andq $-64, %r10 cmpq %r10, %rdi - jge L(mt32) + jae L(mt32) L(A64main): movdqu (%rdi,%rsi), %xmm0 @@ -216,7 +220,7 @@ L(mt32): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) L(A32main): movdqu (%rdi,%rsi), %xmm0 @@ -254,7 +258,7 @@ L(ATR): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) testq $16, %rdi jz L(ATR32) @@ -325,7 +329,7 @@ L(ATR64main): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) L(ATR32res): movdqa (%rdi,%rsi), %xmm0 diff --git a/sysdeps/x86_64/x32/Makefile b/sysdeps/x86_64/x32/Makefile index 1557724b0c..8748956563 100644 --- a/sysdeps/x86_64/x32/Makefile +++ b/sysdeps/x86_64/x32/Makefile @@ -8,7 +8,8 @@ endif ifeq ($(subdir),string) tests += tst-size_t-memchr tst-size_t-memcmp tst-size_t-memcpy \ tst-size_t-memrchr tst-size_t-memset tst-size_t-strncasecmp \ - tst-size_t-strncmp tst-size_t-strncpy tst-size_t-strnlen + tst-size_t-strncmp tst-size_t-strncpy tst-size_t-strnlen \ + tst-size_t-memcmp-2 endif ifeq ($(subdir),wcsmbs) diff --git a/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c b/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c new file mode 100644 index 0000000000..d8ae1a0813 --- /dev/null +++ b/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c @@ -0,0 +1,79 @@ +/* Test memcmp with size_t in the lower 32 bits of 64-bit register. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#define TEST_MAIN +#ifdef WIDE +# define TEST_NAME "wmemcmp" +#else +# define TEST_NAME "memcmp" +#endif + +#include "test-size_t.h" + +#ifdef WIDE +# include +# include + +# define MEMCMP wmemcmp +# define CHAR wchar_t +#else +# define MEMCMP memcmp +# define CHAR char +#endif + +IMPL (MEMCMP, 1) + +typedef int (*proto_t) (const CHAR *, const CHAR *, size_t); + +static int +__attribute__ ((noinline, noclone)) +do_memcmp (parameter_t a, parameter_t b) +{ + return CALL (&b, a.p, b.p, a.len); +} + +static int +test_main (void) +{ + test_init (); + + parameter_t dest = { { page_size / sizeof (CHAR) }, buf1 }; + parameter_t src = { { 0 }, buf2 }; + + memcpy (buf1, buf2, page_size); + + CHAR *p = (CHAR *) buf1; + p[page_size / sizeof (CHAR) - 1] = (CHAR) 1; + + int ret = 0; + FOR_EACH_IMPL (impl, 0) + { + src.fn = impl->fn; + int res = do_memcmp (dest, src); + if (res >= 0) + { + error (0, 0, "Wrong result in function %s: %i >= 0", + impl->name, res); + ret = 1; + } + } + + return ret ? EXIT_FAILURE : EXIT_SUCCESS; +} + +#include commit 2de15ac95713a238dc258eb8977ecdfca811fc19 Author: Florian Weimer Date: Tue Feb 5 13:49:02 2019 +0100 arm: Use "nr" constraint for Systemtap probes [BZ #24164] With the default "nor" constraint, current GCC will use the "o" constraint for constants, after emitting the constant to memory. That results in unparseable Systemtap probe notes such as "-4@.L1052". Removing the "o" alternative and using "nr" instead avoids this. (cherry picked from commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6) diff --git a/ChangeLog b/ChangeLog index e0969afd80..9517ff9e82 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2019-02-05 Florian Weimer + + [BZ #24164] + arm: Use "nr" constraint for Systemtap probes, to avoid the + compiler using memory operands for constants, due to the "o" + alternative in the default "nor" constraint. + * include/stap-probe.h [USE_STAP_PROBE]: Include + + * sysdeps/generic/stap-probe-machine.h: New file. + * sysdeps/arm/stap-probe-machine.h: Likewise. + 2019-02-04 H.J. Lu [BZ #24155] diff --git a/NEWS b/NEWS index 1751ed118a..1f0fc4b3cb 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,7 @@ Version 2.29.1 The following bugs are resolved with this release: [24155] x32 memcmp can treat positive length as 0 (if sign bit in RDX is set) (CVE-2019-7309) + [24164] Systemtap probes need to use "nr" constraint on 32-bit Arm Security related changes: diff --git a/include/stap-probe.h b/include/stap-probe.h index c53dd86592..8c26292edd 100644 --- a/include/stap-probe.h +++ b/include/stap-probe.h @@ -21,6 +21,7 @@ #ifdef USE_STAP_PROBE +# include # include /* Our code uses one macro LIBC_PROBE (name, n, arg1, ..., argn). diff --git a/sysdeps/arm/stap-probe-machine.h b/sysdeps/arm/stap-probe-machine.h new file mode 100644 index 0000000000..d27ca22040 --- /dev/null +++ b/sysdeps/arm/stap-probe-machine.h @@ -0,0 +1,22 @@ +/* Macros for customizing Systemtap . Arm version. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* The default "nor" constraint produces unparseable memory references + for constants. Omit the problematic "o" constraint. See bug 24164 + and GCC PR 89146. */ +#define STAP_SDT_ARG_CONSTRAINT nr diff --git a/sysdeps/generic/stap-probe-machine.h b/sysdeps/generic/stap-probe-machine.h new file mode 100644 index 0000000000..2e5790c3b2 --- /dev/null +++ b/sysdeps/generic/stap-probe-machine.h @@ -0,0 +1,19 @@ +/* Macros for customizing Systemtap . Generic version. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* By default, there are no customizations. */ commit 44113a8ba24af23d7bbb174f9087a6b83a76289a Author: Stefan Liebler Date: Thu Feb 7 15:18:36 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. (cherry picked from commit 823624bdc47f1f80109c9c52dee7939b9386d708) diff --git a/ChangeLog b/ChangeLog index 9517ff9e82..7a8895bcc6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2019-02-07 Stefan Liebler + + [BZ #24180] + * nptl/pthread_mutex_trylock.c (__pthread_mutex_trylock): + Add compiler barriers and comments. + 2019-02-05 Florian Weimer [BZ #24164] 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); }