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 31cb64ba2224a460d01fbad695971bd483b9018b..6b8fc258dc7d1e079ef365acb68c244e25ec1585 100644 (file)
--- a/crypto/threads_pthread.c
+++ b/crypto/threads_pthread.c
@@ -464,9 +464,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
@@ -479,6 +476,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 f687ad82f0314058582266a82240b3bce0ab27b8..e8e73c8830ec95a3360a9cf4f833020f1c6f737e 100644 (file)
--- a/crypto/threads_win.c
+++ b/crypto/threads_win.c
@@ -383,15 +383,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 {
         CRYPTO_atomic_load(&qp->users, &count, lock->rw_lock);
     } 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 71cdb37296e41ff222976b9ae14c8cc03e735f64..aba95b403264695b6fbda73ea0cbcad7ed4ecee2 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;
@@ -1329,6 +1335,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);