RCU: Ensure that qp's are actually retired in order

The current retirement code for rcu qp's has a race condition,
which can cause use-after-free errors, but only if more than
3 QPs are allocated, which is not the default configuration.

This fixes an oversight in commit 5949918f9afa ("Rework and
simplify RCU code")

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/26952)

(cherry picked from commit 6e7be995fd7fd24d38b95982cf90f801ac045743)

diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c
index 69169693d0bc9368384ba92518e409914f3a7df1..a06756926e7f531fe300ab0fedf1a02522973437 100644 (file)
--- a/crypto/threads_pthread.c
+++ b/crypto/threads_pthread.c
@@ -437,9 +437,6 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
     pthread_mutex_lock(&lock->prior_lock);
     while (lock->next_to_retire != curr_id)
         pthread_cond_wait(&lock->prior_signal, &lock->prior_lock);
-    lock->next_to_retire++;
-    pthread_cond_broadcast(&lock->prior_signal);
-    pthread_mutex_unlock(&lock->prior_lock);
 
     /*
      * wait for the reader count to reach zero
@@ -452,6 +449,10 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
         count = ATOMIC_LOAD_N(uint64_t, &qp->users, __ATOMIC_ACQUIRE);
     } while (count != (uint64_t)0);
 
+    lock->next_to_retire++;
+    pthread_cond_broadcast(&lock->prior_signal);
+    pthread_mutex_unlock(&lock->prior_lock);
+
     retire_qp(lock, qp);
 
     /* handle any callbacks that we have */

diff --git a/crypto/threads_win.c b/crypto/threads_win.c
index 22b50dac492e41b449bfcb9f960faadf4a186d31..993411a55e4fa0d621700491716a82ff3a5b73d6 100644 (file)
--- a/crypto/threads_win.c
+++ b/crypto/threads_win.c
@@ -324,15 +324,15 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
     while (lock->next_to_retire != curr_id)
         ossl_crypto_condvar_wait(lock->prior_signal, lock->prior_lock);
 
-    lock->next_to_retire++;
-    ossl_crypto_condvar_broadcast(lock->prior_signal);
-    ossl_crypto_mutex_unlock(lock->prior_lock);
-
     /* wait for the reader count to reach zero */
     do {
         count = InterlockedOr64(&qp->users, 0);
     } while (count != (uint64_t)0);
 
+    lock->next_to_retire++;
+    ossl_crypto_condvar_broadcast(lock->prior_signal);
+    ossl_crypto_mutex_unlock(lock->prior_lock);
+
     retire_qp(lock, qp);
 
     /* handle any callbacks that we have */

diff --git a/test/threadstest.c b/test/threadstest.c
index 1e44a5a5e34652ef95e8c6811847035a98ff763e..a22cfe70200ea1a9d1eeb4f8b653101424abe228 100644 (file)
--- a/test/threadstest.c
+++ b/test/threadstest.c
@@ -434,7 +434,7 @@ static int _torture_rcu(void)
     writer2_done = 0;
     rcu_torture_result = 1;
 
-    rcu_lock = ossl_rcu_lock_new(1, NULL);
+    rcu_lock = ossl_rcu_lock_new(contention == 2 ? 4 : 1, NULL);
     if (rcu_lock == NULL)
         goto out;
 
@@ -491,6 +491,12 @@ static int torture_rcu_high(void)
     contention = 1;
     return _torture_rcu();
 }
+
+static int torture_rcu_high2(void)
+{
+    contention = 2;
+    return _torture_rcu();
+}
 #endif
 
 static CRYPTO_ONCE once_run = CRYPTO_ONCE_STATIC_INIT;
@@ -1296,6 +1302,7 @@ int setup_tests(void)
     ADD_TEST(torture_rw_high);
     ADD_TEST(torture_rcu_low);
     ADD_TEST(torture_rcu_high);
+    ADD_TEST(torture_rcu_high2);
 #endif
     ADD_TEST(test_once);
     ADD_TEST(test_thread_local);