Fix: handle sys_futex() FUTEX_WAIT interrupted by signal
authorMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Mon, 6 Jul 2015 20:32:28 +0000 (16:32 -0400)
committerMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Wed, 8 Jul 2015 18:43:01 +0000 (14:43 -0400)
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 <geg@ngncc.de>
CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
CC: Lai Jiangshan <laijs@cn.fujitsu.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: lttng-dev@lists.lttng.org
CC: rp@svcs.cs.pdx.edu
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
compat_futex.c
urcu-call-rcu-impl.h
urcu-defer-impl.h
urcu-qsbr.c
urcu-wait.h
urcu.c
urcu/futex.h
urcu/static/urcu-qsbr.h
urcu/static/urcu.h

index 6ec0b39..a357134 100644 (file)
@@ -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;
 }
index f02405d..d33c731 100644 (file)
@@ -256,9 +256,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)
@@ -267,8 +280,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);
        }
 }
 
@@ -276,9 +290,22 @@ static void call_rcu_completion_wait(struct call_rcu_completion *completion)
 {
        /* Read completion barrier count before read futex */
        cmm_smp_mb();
-       if (uatomic_read(&completion->futex) == -1)
-               futex_async(&completion->futex, FUTEX_WAIT, -1,
-                     NULL, NULL, 0);
+       if (uatomic_read(&completion->futex) != -1)
+               return;
+       while (futex_async(&completion->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_completion_wake_up(struct call_rcu_completion *completion)
@@ -287,8 +314,9 @@ static void call_rcu_completion_wake_up(struct call_rcu_completion *completion)
        cmm_smp_mb();
        if (caa_unlikely(uatomic_read(&completion->futex) == -1)) {
                uatomic_set(&completion->futex, 0);
-               futex_async(&completion->futex, FUTEX_WAKE, 1,
-                     NULL, NULL, 0);
+               if (futex_async(&completion->futex, FUTEX_WAKE, 1,
+                               NULL, NULL, 0) < 0)
+                       urcu_die(errno);
        }
 }
 
index 2cff807..f1dca8f 100644 (file)
@@ -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);
+                       }
+               }
        }
 }
 
index 685efb5..619df60 100644 (file)
@@ -121,9 +121,22 @@ static void wait_gp(void)
 {
        /* Read reader_gp before read futex */
        cmm_smp_rmb();
-       if (uatomic_read(&rcu_gp.futex) == -1)
-               futex_noasync(&rcu_gp.futex, FUTEX_WAIT, -1,
-                     NULL, NULL, 0);
+       if (uatomic_read(&rcu_gp.futex) != -1)
+               return;
+       while (futex_noasync(&rcu_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);
+               }
+       }
 }
 
 /*
index d00842a..94f3e35 100644 (file)
@@ -25,6 +25,7 @@
 
 #include <urcu/uatomic.h>
 #include <urcu/wfstack.h>
+#include "urcu-die.h"
 
 /*
  * Number of busy-loop attempts before waiting on futex for grace period
@@ -122,8 +123,11 @@ void urcu_adaptative_wake_up(struct urcu_wait_node *wait)
        cmm_smp_mb();
        assert(uatomic_read(&wait->state) == URCU_WAIT_WAITING);
        uatomic_set(&wait->state, URCU_WAIT_WAKEUP);
-       if (!(uatomic_read(&wait->state) & URCU_WAIT_RUNNING))
-               futex_noasync(&wait->state, FUTEX_WAKE, 1, NULL, NULL, 0);
+       if (!(uatomic_read(&wait->state) & URCU_WAIT_RUNNING)) {
+               if (futex_noasync(&wait->state, FUTEX_WAKE, 1,
+                               NULL, NULL, 0) < 0)
+                       urcu_die(errno);
+       }
        /* Allow teardown of struct urcu_wait memory. */
        uatomic_or(&wait->state, URCU_WAIT_TEARDOWN);
 }
@@ -144,8 +148,20 @@ void urcu_adaptative_busy_wait(struct urcu_wait_node *wait)
                        goto skip_futex_wait;
                caa_cpu_relax();
        }
-       futex_noasync(&wait->state, FUTEX_WAIT,
-               URCU_WAIT_WAITING, NULL, NULL, 0);
+       while (futex_noasync(&wait->state, FUTEX_WAIT, URCU_WAIT_WAITING,
+                       NULL, NULL, 0)) {
+               switch (errno) {
+               case EWOULDBLOCK:
+                       /* Value already changed. */
+                       goto skip_futex_wait;
+               case EINTR:
+                       /* Retry if interrupted by signal. */
+                       break;  /* Get out of switch. */
+               default:
+                       /* Unexpected error. */
+                       urcu_die(errno);
+               }
+       }
 skip_futex_wait:
 
        /* Tell waker thread than we are running. */
diff --git a/urcu.c b/urcu.c
index 94d1131..e773065 100644 (file)
--- a/urcu.c
+++ b/urcu.c
@@ -236,9 +236,22 @@ static void wait_gp(void)
 {
        /* Read reader_gp before read futex */
        smp_mb_master(RCU_MB_GROUP);
-       if (uatomic_read(&rcu_gp.futex) == -1)
-               futex_async(&rcu_gp.futex, FUTEX_WAIT, -1,
-                     NULL, NULL, 0);
+       if (uatomic_read(&rcu_gp.futex) != -1)
+               return;
+       while (futex_async(&rcu_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);
+               }
+       }
 }
 
 /*
index bb270c2..2be3bb6 100644 (file)
@@ -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
index 8f2ca32..143d75a 100644 (file)
@@ -106,8 +106,13 @@ static inline void wake_up_gp(void)
                if (uatomic_read(&rcu_gp.futex) != -1)
                        return;
                uatomic_set(&rcu_gp.futex, 0);
-               futex_noasync(&rcu_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(&rcu_gp.futex, FUTEX_WAKE, 1,
+                               NULL, NULL, 0);
        }
 }
 
index b5fc09f..af8eee4 100644 (file)
@@ -168,8 +168,13 @@ static inline void wake_up_gp(void)
 {
        if (caa_unlikely(uatomic_read(&rcu_gp.futex) == -1)) {
                uatomic_set(&rcu_gp.futex, 0);
-               futex_async(&rcu_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(&rcu_gp.futex, FUTEX_WAKE, 1,
+                               NULL, NULL, 0);
        }
 }
 
This page took 0.041783 seconds and 4 git commands to generate.