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 e70789a5906d6dec6f707025ec3db5895ea34b0f..6495f465e1fade0477179be441694751cbc8c473 100644 (file)
--- 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 a7d0b2f7c45aa0a1054d70aac989293b63c87466..8f1e5a52c7a9bf769ef0f2224c8d0872a7a89648 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 1a94efa61a47fc48e12f9fc671b7c147d29382b4..52cb04aee9ff1b1b5a7b2caef45d6d5b5317ba77 100644 (file)
--- 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 1d4bf7afb04030155f7cb88d4dcb2cf8e2518a66..c5abeafa06d3bd796a2148a1effdf90c2c959dd1 100644 (file)
--- 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 906d9b723ae88481fc7874e1409dc24dd80f35eb..b71563b7c90c5f3136d4e2517e4de95bb8576202 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 e8cdfbe34d9d26b537fa07d9bae26483f7cc7d68..bf97a40c1a9b5345d7110f60b958b63dd0af61f0 100644 (file)
--- 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 c517693ff1cf187982882bf30f412e454ae8d26f..5b9ceecdca5f291dbc2fff96ae7be25113a3750f 100644 (file)
--- 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);
        }
 }