From 4974ad5f21d2cb17d313504f8d1252738526d588 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Wed, 22 Jun 2022 16:49:11 -0400 Subject: [PATCH] Fix: urcu-qsbr: futex wait: handle spurious futex wakeups MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Observed issue ============== The urcu-qsbr wait_gp() implements a futex wait/wakeup scheme identical to the workqueue code, which has an issue with spurious wakeups. A spurious wakeup on wait_gp can cause wait_gp to return with a urcu_qsbr_gp.futex state of -1, which is unexpected. It would cause the following loops in wait_for_readers() to decrement the urcu_qsbr_gp.futex to values below -1, thus actively using CPU as values will be decremented to very low negative values until it reaches 0 through underflow, or until the input_readers list is found to be empty. The state is restored to 0 when the input_readers list is found to be empty, which restores the futex state to a correct state for the following calls to wait_for_readers(). This issue will cause spurious unexpected high CPU use, but will not lead to data corruption. Cause ===== From futex(5): FUTEX_WAIT Returns 0 if the caller was woken up. Note that a wake-up can also be caused by common futex usage patterns in unrelated code that happened to have previously used the futex word's memory location (e.g., typical futex-based implementations of Pthreads mutexes can cause this under some conditions). Therefore, call‐ ers should always conservatively assume that a return value of 0 can mean a spurious wake-up, and use the futex word's value (i.e., the user-space synchronization scheme) to decide whether to continue to block or not. Solution ======== We therefore need to validate whether the value differs from -1 in user-space after the call to FUTEX_WAIT returns 0. Known drawbacks =============== None. Signed-off-by: Mathieu Desnoyers Change-Id: I87f7cd3b02820cefe850c3bdb8da27fb2f9be9b2 --- src/urcu-qsbr.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/urcu-qsbr.c b/src/urcu-qsbr.c index d69d93b..a338285 100644 --- a/src/urcu-qsbr.c +++ b/src/urcu-qsbr.c @@ -123,17 +123,25 @@ static void wait_gp(void) { /* Read reader_gp before read futex */ cmm_smp_rmb(); - if (uatomic_read(&urcu_qsbr_gp.futex) != -1) - return; - while (futex_noasync(&urcu_qsbr_gp.futex, FUTEX_WAIT, -1, - NULL, NULL, 0)) { + while (uatomic_read(&urcu_qsbr_gp.futex) == -1) { + if (!futex_noasync(&urcu_qsbr_gp.futex, FUTEX_WAIT, -1, NULL, NULL, 0)) { + /* + * Prior queued wakeups queued by unrelated code + * using the same address can cause futex wait to + * return 0 even through the futex value is still + * -1 (spurious wakeups). Check the value again + * in user-space to validate whether it really + * differs from -1. + */ + continue; + } switch (errno) { - case EWOULDBLOCK: + case EAGAIN: /* Value already changed. */ return; case EINTR: /* Retry if interrupted by signal. */ - break; /* Get out of switch. */ + break; /* Get out of switch. Check again. */ default: /* Unexpected error. */ urcu_die(errno); -- 2.34.1