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 <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>

diff --git a/compat_futex.c b/compat_futex.c
index 6ec0b3912b34afbda58c873371f83aec55d35f68..a3571343ca37d086e77620da1ed0dd5e9fd71778 100644 (file)
--- 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 f02405ddd698ca684901a3d68c2534b9e9295b9e..d33c731ad1e9eae6437ae95595ecf0c825f3e6e8 100644 (file)
--- a/urcu-call-rcu-impl.h
+++ b/urcu-call-rcu-impl.h
@@ -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);
        }
 }
 

diff --git a/urcu-defer-impl.h b/urcu-defer-impl.h
index 2cff80792b0a4f6fecc7016e49667a809e36918b..f1dca8ff85c2289d300cc92a77e9d623cbb39216 100644 (file)
--- 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 685efb5297dca06632697d810f5f078bc0aa835d..619df60511b67ce9f5408d13e5ef33f7326ee7f5 100644 (file)
--- a/urcu-qsbr.c
+++ b/urcu-qsbr.c
@@ -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);
+               }
+       }
 }
 
 /*

diff --git a/urcu-wait.h b/urcu-wait.h
index d00842a04b26484d67bdde4ed9a7550baf926039..94f3e3559ba8c32492234f911911db7faa1337a9 100644 (file)
--- a/urcu-wait.h
+++ b/urcu-wait.h
@@ -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 94d1131e3dba6df7982a33def88f6ec3908c2bf2..e77306584d77f0fc012f6bfbae466452b3556b74 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);
+               }
+       }
 }
 
 /*

diff --git a/urcu/futex.h b/urcu/futex.h
index bb270c2933fcaf174a67207858addbc35e09982c..2be3bb6c1dc96e5ee71d932f9a2ae7f5221d901b 100644 (file)
--- 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 8f2ca32a2f111c4b1469c010c5f1fcb4669039a5..143d75a7e1e047f0ec2f81709f3ca39175debdab 100644 (file)
--- a/urcu/static/urcu-qsbr.h
+++ b/urcu/static/urcu-qsbr.h
@@ -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);
        }
 }
 

diff --git a/urcu/static/urcu.h b/urcu/static/urcu.h
index b5fc09f93553578ae32f660b9c65a49a9ea3517f..af8eee442d3849f5756423d7337d16c93f712f34 100644 (file)
--- a/urcu/static/urcu.h
+++ b/urcu/static/urcu.h
@@ -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);
        }
 }