On 11/30/2016 01:05 PM, Nadav Har'El wrote:
Our pthread_spin_lock implementation incorrectly used the kernel spinlock
implementation which disabled preemption, which is both inappropriate and
unnecessary for application code.
Ironically, after commit 930cb83d914796893746d5d7f7f9cf6df37db5e6,
pthread_spin_lock() became completely unusable: Any appliction will call
pthread_spin_unlock() while the spin-lock is held, and the first call
will require a symbol resolution which asserts preemption is not disabled,
and crashes.
This patch fixes the pthread_spin_lock implementation to not disable
preemption.
This patch also adds a test for this case, which crashes before this patch
(and succeeds with it).
Fixes #814
Looks good.
Signed-off-by: Nadav Har'El <n...@scylladb.com>
---
libc/pthread.cc | 39 +++++++++++++++++++++++++++------------
tests/tst-pthread.c | 14 ++++++++++++++
2 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/libc/pthread.cc b/libc/pthread.cc
index d78d18a..b66fed9 100644
--- a/libc/pthread.cc
+++ b/libc/pthread.cc
@@ -28,7 +28,6 @@
#include <osv/lazy_indirect.hh>
#include <api/time.h>
-#include <osv/spinlock.h>
#include <osv/rwlock.h>
#include "pthread.hh"
@@ -316,16 +315,24 @@ int pthread_getcpuclockid(pthread_t thread, clockid_t
*clock_id)
return 0;
}
-// pthread_spinlock_t and spinlock_t aren't really the same type. But since
-// spinlock_t is a boolean and pthread_spinlock_t is defined to be an integer,
-// just casting it like this is fine. As long as we are never operating more
-// than sizeof(int) at a time, we should be fine.
+// Note that for pthread_spin_lock() we cannot use the implementation
+// from <osv/spinlock.h> because it disables preemption, which is
+// inappropriate for application code, and also unnecessary (the kernel
+// version needs to defend against a deadlock when one of the lock holders
+// disables preemption - but an application cannot disable preemption).
+// So we repeat similar code here.
+inline bool* from_libc(pthread_spinlock_t* a) {
+ static_assert(sizeof(bool) <= sizeof(pthread_spinlock_t),
+ "pthread_spinlock_t cannot hold a bool");
+ return reinterpret_cast<bool*>(a);
+}
+
int pthread_spin_init(pthread_spinlock_t *lock, int pshared)
{
- static_assert(sizeof(spinlock_t) <= sizeof(pthread_spinlock_t),
- "OSv spinlock type doesn't match pthread's");
- // PTHREAD_PROCESS_SHARED and PTHREAD_PROCESS_PRIVATE are the same while
we have a single process.
- spinlock_init(reinterpret_cast<spinlock_t *>(lock));
+ // PTHREAD_PROCESS_SHARED and PTHREAD_PROCESS_PRIVATE are the same while
+ // we have a single process.
+ bool* b = from_libc(lock);
+ *b = false;
return 0;
}
@@ -336,13 +343,20 @@ int pthread_spin_destroy(pthread_spinlock_t *lock)
int pthread_spin_lock(pthread_spinlock_t *lock)
{
- spin_lock(reinterpret_cast<spinlock_t *>(lock));
+ bool* b = from_libc(lock);
+ while (__sync_lock_test_and_set(b, 1)) {
+ while (*b) {
+ barrier();
+ // FIXME: use "PAUSE" instruction here
+ }
+ }
return 0; // We can't really do deadlock detection
}
int pthread_spin_trylock(pthread_spinlock_t *lock)
{
- if (!spin_trylock(reinterpret_cast<spinlock_t *>(lock))) {
+ bool* b = from_libc(lock);
+ if (__sync_lock_test_and_set(b, 1)) {
return EBUSY;
}
return 0;
@@ -350,7 +364,8 @@ int pthread_spin_trylock(pthread_spinlock_t *lock)
int pthread_spin_unlock(pthread_spinlock_t *lock)
{
- spin_unlock(reinterpret_cast<spinlock_t *>(lock));
+ bool* b = from_libc(lock);
+ __sync_lock_release(b, 0);
return 0;
}
diff --git a/tests/tst-pthread.c b/tests/tst-pthread.c
index 8e27573..066fff7 100644
--- a/tests/tst-pthread.c
+++ b/tests/tst-pthread.c
@@ -158,6 +158,20 @@ int main(void)
printf("ts2 = %ld,%ld\n",ts2.tv_sec, ts2.tv_nsec);
printf("ns = %ld\n",ns);
+ // Test that pthread_spin_unlock() doesn't crash when it is resolved for
+ // the first time while a spinlock is taken (see issue #814)
+ pthread_spinlock_t spin;
+ pthread_spin_init(&spin, PTHREAD_PROCESS_PRIVATE);
+ pthread_spin_lock(&spin);
+ pthread_spin_unlock(&spin);
+ pthread_spin_destroy(&spin);
+ // Moreover, the application may even sleep while holding a spinlock
+ pthread_spin_init(&spin, PTHREAD_PROCESS_PRIVATE);
+ pthread_spin_lock(&spin);
+ usleep(1000);
+ pthread_spin_unlock(&spin);
+ pthread_spin_destroy(&spin);
+
printf("SUMMARY: %u tests / %u failures\n", tests_total, tests_failed);
return tests_failed == 0 ? 0 : 1;
}
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
