From 11eb040f24e020d05d65983d0f87f79b000c7b9f Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Mon, 6 Jul 2015 16:32:28 -0400 Subject: [PATCH] Fix: handle sys_futex() FUTEX_WAIT interrupted by signal We need to handle EINTR returned by sys_futex() FUTEX_WAIT, otherwise a signal interrupting this system call could make sys_futex return too early, and therefore cause a synchronization issue. Ensure that the futex compatibility layer returns meaningful errors and errno when using poll() or pthread cond variables. Reported-by: Gerd Gerats CC: Paul E. McKenney CC: Lai Jiangshan CC: Stephen Hemminger CC: Alan Stern CC: lttng-dev@lists.lttng.org CC: rp@svcs.cs.pdx.edu Signed-off-by: Mathieu Desnoyers --- compat_futex.c | 36 +++++++++++++++++++++++++++--------- urcu-call-rcu-impl.h | 24 +++++++++++++++++++----- urcu-defer-impl.h | 24 +++++++++++++++++++----- urcu-qsbr.c | 19 ++++++++++++++++--- urcu.c | 19 ++++++++++++++++--- urcu/futex.h | 3 +++ urcu/static/urcu-qsbr.h | 9 +++++++-- urcu/static/urcu.h | 9 +++++++-- 8 files changed, 114 insertions(+), 29 deletions(-) diff --git a/compat_futex.c b/compat_futex.c index 6ec0b39..a357134 100644 --- a/compat_futex.c +++ b/compat_futex.c @@ -54,7 +54,7 @@ pthread_cond_t __urcu_compat_futex_cond = PTHREAD_COND_INITIALIZER; int compat_futex_noasync(int32_t *uaddr, int op, int32_t val, const struct timespec *timeout, int32_t *uaddr2, int32_t val3) { - int ret, gret = 0; + int ret; /* * Check if NULL. Don't let users expect that they are taken into @@ -70,7 +70,11 @@ int compat_futex_noasync(int32_t *uaddr, int op, int32_t val, cmm_smp_mb(); ret = pthread_mutex_lock(&__urcu_compat_futex_lock); - assert(!ret); + if (ret) { + errno = ret; + ret = -1; + goto end; + } switch (op) { case FUTEX_WAIT: /* @@ -91,11 +95,16 @@ int compat_futex_noasync(int32_t *uaddr, int op, int32_t val, pthread_cond_broadcast(&__urcu_compat_futex_cond); break; default: - gret = -EINVAL; + errno = EINVAL; + ret = -1; } ret = pthread_mutex_unlock(&__urcu_compat_futex_lock); - assert(!ret); - return gret; + if (ret) { + errno = ret; + ret = -1; + } +end: + return ret; } /* @@ -107,6 +116,8 @@ int compat_futex_noasync(int32_t *uaddr, int op, int32_t val, int compat_futex_async(int32_t *uaddr, int op, int32_t val, const struct timespec *timeout, int32_t *uaddr2, int32_t val3) { + int ret = 0; + /* * Check if NULL. Don't let users expect that they are taken into * account. @@ -122,13 +133,20 @@ int compat_futex_async(int32_t *uaddr, int op, int32_t val, switch (op) { case FUTEX_WAIT: - while (CMM_LOAD_SHARED(*uaddr) == val) - poll(NULL, 0, 10); + while (CMM_LOAD_SHARED(*uaddr) == val) { + if (poll(NULL, 0, 10) < 0) { + ret = -1; + /* Keep poll errno. Caller handles EINTR. */ + goto end; + } + } break; case FUTEX_WAKE: break; default: - return -EINVAL; + errno = EINVAL; + ret = -1; } - return 0; +end: + return ret; } diff --git a/urcu-call-rcu-impl.h b/urcu-call-rcu-impl.h index e70789a..6495f46 100644 --- a/urcu-call-rcu-impl.h +++ b/urcu-call-rcu-impl.h @@ -235,9 +235,22 @@ static void call_rcu_wait(struct call_rcu_data *crdp) { /* Read call_rcu list before read futex */ cmm_smp_mb(); - if (uatomic_read(&crdp->futex) == -1) - futex_async(&crdp->futex, FUTEX_WAIT, -1, - NULL, NULL, 0); + if (uatomic_read(&crdp->futex) != -1) + return; + while (futex_async(&crdp->futex, FUTEX_WAIT, -1, + NULL, NULL, 0)) { + switch (errno) { + case EWOULDBLOCK: + /* Value already changed. */ + return; + case EINTR: + /* Retry if interrupted by signal. */ + break; /* Get out of switch. */ + default: + /* Unexpected error. */ + urcu_die(errno); + } + } } static void call_rcu_wake_up(struct call_rcu_data *crdp) @@ -246,8 +259,9 @@ static void call_rcu_wake_up(struct call_rcu_data *crdp) cmm_smp_mb(); if (caa_unlikely(uatomic_read(&crdp->futex) == -1)) { uatomic_set(&crdp->futex, 0); - futex_async(&crdp->futex, FUTEX_WAKE, 1, - NULL, NULL, 0); + if (futex_async(&crdp->futex, FUTEX_WAKE, 1, + NULL, NULL, 0) < 0) + urcu_die(errno); } } diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h index a7d0b2f..8f1e5a5 100644 --- a/urcu-defer-impl.h +++ b/urcu-defer-impl.h @@ -160,8 +160,9 @@ static void wake_up_defer(void) { if (caa_unlikely(uatomic_read(&defer_thread_futex) == -1)) { uatomic_set(&defer_thread_futex, 0); - futex_noasync(&defer_thread_futex, FUTEX_WAKE, 1, - NULL, NULL, 0); + if (futex_noasync(&defer_thread_futex, FUTEX_WAKE, 1, + NULL, NULL, 0) < 0) + urcu_die(errno); } } @@ -198,9 +199,22 @@ static void wait_defer(void) uatomic_set(&defer_thread_futex, 0); } else { cmm_smp_rmb(); /* Read queue before read futex */ - if (uatomic_read(&defer_thread_futex) == -1) - futex_noasync(&defer_thread_futex, FUTEX_WAIT, -1, - NULL, NULL, 0); + if (uatomic_read(&defer_thread_futex) != -1) + return; + while (futex_noasync(&defer_thread_futex, FUTEX_WAIT, -1, + NULL, NULL, 0)) { + switch (errno) { + case EWOULDBLOCK: + /* Value already changed. */ + return; + case EINTR: + /* Retry if interrupted by signal. */ + break; /* Get out of switch. */ + default: + /* Unexpected error. */ + urcu_die(errno); + } + } } } diff --git a/urcu-qsbr.c b/urcu-qsbr.c index 1a94efa..52cb04a 100644 --- a/urcu-qsbr.c +++ b/urcu-qsbr.c @@ -125,9 +125,22 @@ static void wait_gp(void) { /* Read reader_gp before read futex */ cmm_smp_rmb(); - if (uatomic_read(&gp_futex) == -1) - futex_noasync(&gp_futex, FUTEX_WAIT, -1, - NULL, NULL, 0); + if (uatomic_read(&gp_futex) != -1) + return; + while (futex_noasync(&gp_futex, FUTEX_WAIT, -1, + NULL, NULL, 0)) { + switch (errno) { + case EWOULDBLOCK: + /* Value already changed. */ + return; + case EINTR: + /* Retry if interrupted by signal. */ + break; /* Get out of switch. */ + default: + /* Unexpected error. */ + urcu_die(errno); + } + } } /* diff --git a/urcu.c b/urcu.c index 1d4bf7a..c5abeaf 100644 --- a/urcu.c +++ b/urcu.c @@ -224,9 +224,22 @@ static void wait_gp(void) { /* Read reader_gp before read futex */ smp_mb_master(RCU_MB_GROUP); - if (uatomic_read(&gp_futex) == -1) - futex_async(&gp_futex, FUTEX_WAIT, -1, - NULL, NULL, 0); + if (uatomic_read(&gp_futex) != -1) + return; + while (futex_async(&gp_futex, FUTEX_WAIT, -1, + NULL, NULL, 0)) { + switch (errno) { + case EWOULDBLOCK: + /* Value already changed. */ + return; + case EINTR: + /* Retry if interrupted by signal. */ + break; /* Get out of switch. */ + default: + /* Unexpected error. */ + urcu_die(errno); + } + } } /* diff --git a/urcu/futex.h b/urcu/futex.h index 906d9b7..b71563b 100644 --- a/urcu/futex.h +++ b/urcu/futex.h @@ -42,6 +42,9 @@ extern "C" { * * futex_async is signal-handler safe for the wakeup. It uses polling * on the wait-side in compatibility mode. + * + * BEWARE: sys_futex() FUTEX_WAIT may return early if interrupted + * (returns EINTR). */ #ifdef CONFIG_RCU_HAVE_FUTEX diff --git a/urcu/static/urcu-qsbr.h b/urcu/static/urcu-qsbr.h index e8cdfbe..bf97a40 100644 --- a/urcu/static/urcu-qsbr.h +++ b/urcu/static/urcu-qsbr.h @@ -144,8 +144,13 @@ static inline void wake_up_gp(void) if (uatomic_read(&gp_futex) != -1) return; uatomic_set(&gp_futex, 0); - futex_noasync(&gp_futex, FUTEX_WAKE, 1, - NULL, NULL, 0); + /* + * Ignoring return value until we can make this function + * return something (because urcu_die() is not publicly + * exposed). + */ + (void) futex_noasync(&gp_futex, FUTEX_WAKE, 1, + NULL, NULL, 0); } } diff --git a/urcu/static/urcu.h b/urcu/static/urcu.h index c517693..5b9ceec 100644 --- a/urcu/static/urcu.h +++ b/urcu/static/urcu.h @@ -234,8 +234,13 @@ static inline void wake_up_gp(void) { if (caa_unlikely(uatomic_read(&gp_futex) == -1)) { uatomic_set(&gp_futex, 0); - futex_async(&gp_futex, FUTEX_WAKE, 1, - NULL, NULL, 0); + /* + * Ignoring return value until we can make this function + * return something (because urcu_die() is not publicly + * exposed). + */ + (void) futex_async(&gp_futex, FUTEX_WAKE, 1, + NULL, NULL, 0); } } -- 2.34.1