From 25f8e2c15b701514b7b2fe652634289b6fb8581f Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 10 Jan 2025 14:37:28 -0500 Subject: Fix premature reuse of qp's in rcu locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An intermittent failure was noted on our new ppc64le CI runner, in which what appeared to be a corrupted or invalid value getting returned from a shared pointer under rcu protection Investigation showed that the problem was with our small number of qp's in a lock, and slightly incorrect accounting of the number of qp's available we were prematurely recycling qp's, which led in turn to premature completion of synchronization states, resulting in readers reading memory that may have already been freed. Fix it by: a) Ensuring that we account for the fact that the first qp in an rcu lock is allocated at the time the lock is created and b) Ensuring that we have a minimum number of 3 qp's: 1 that is free for write side allocation 1 that is in use by the write side currently 1 "next" qp that the read side can update while the prior qp is being retired With this change, the rcu threadstest runs indefinately in my testing Fixes #26356 Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz Reviewed-by: Saša Nedvědický Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/26384) --- crypto/threads_pthread.c | 13 ++++++++++--- crypto/threads_win.c | 12 +++++++++--- 2 files changed, 19 insertions(+), 6 deletions(-) (limited to 'crypto') diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c index bfd04fa4f1..4e8df06207 100644 --- a/crypto/threads_pthread.c +++ b/crypto/threads_pthread.c @@ -667,8 +667,11 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx) { struct rcu_lock_st *new; - if (num_writers < 1) - num_writers = 1; + /* + * We need a minimum of 3 qp's + */ + if (num_writers < 3) + num_writers = 3; ctx = ossl_lib_ctx_get_concrete(ctx); if (ctx == NULL) @@ -684,11 +687,15 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx) pthread_mutex_init(&new->alloc_lock, NULL); pthread_cond_init(&new->prior_signal, NULL); pthread_cond_init(&new->alloc_signal, NULL); - new->qp_group = allocate_new_qp_group(new, num_writers + 1); + /* By default our first writer is already alloced */ + new->writers_alloced = 1; + + new->qp_group = allocate_new_qp_group(new, num_writers); if (new->qp_group == NULL) { OPENSSL_free(new); new = NULL; } + return new; } diff --git a/crypto/threads_win.c b/crypto/threads_win.c index bdbd4d381f..03a22fd2c9 100644 --- a/crypto/threads_win.c +++ b/crypto/threads_win.c @@ -159,8 +159,11 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx) { struct rcu_lock_st *new; - if (num_writers < 1) - num_writers = 1; + /* + * We need a minimum of 3 qps + */ + if (num_writers < 3) + num_writers = 3; ctx = ossl_lib_ctx_get_concrete(ctx); if (ctx == NULL) @@ -178,7 +181,9 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx) new->prior_signal = ossl_crypto_condvar_new(); new->alloc_lock = ossl_crypto_mutex_new(); new->prior_lock = ossl_crypto_mutex_new(); - new->qp_group = allocate_new_qp_group(new, num_writers + 1); + new->qp_group = allocate_new_qp_group(new, num_writers); + /* By default the first qp is already alloced */ + new->writers_alloced = 1; if (new->qp_group == NULL || new->alloc_signal == NULL || new->prior_signal == NULL @@ -196,6 +201,7 @@ CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx) OPENSSL_free(new); new = NULL; } + return new; } -- cgit v1.2.3