From 666c7bf9ac4eb9366956109d8f9be50b99b116f2 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Thu, 30 Oct 2025 08:36:31 -0400 Subject: [PATCH 1/4] Use C11 atomic to manage session stats --- ssl/internal.h | 33 +++++++++++++++++++++------------ ssl/ssl_lib.cc | 15 ++++++++++++++- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/ssl/internal.h b/ssl/internal.h index 2b7c0281a38..ed43832c942 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -179,6 +179,15 @@ OPENSSL_MSVC_PRAGMA(warning(pop)) #endif +#include + +typedef int SSL_stats_t; + +// Using ATOMIC_INT_LOCK_FREE as SSL_stats_t is an int +#if !defined(OPENSSL_STATS_C11_ATOMIC) && defined(OPENSSL_THREADS) && ATOMIC_INT_LOCK_FREE == 2 +#define OPENSSL_STATS_C11_ATOMIC +#endif + BSSL_NAMESPACE_BEGIN struct SSL_CONFIG; @@ -1468,7 +1477,7 @@ class OPENSSL_EXPORT SSLBuffer { int cap_ = 0; // inline_buf_ is a static buffer for short reads. uint8_t inline_buf_[SSL3_RT_HEADER_LENGTH]; - + // The V1 version has some intricacies were solved in later serialization versions. // This is mainly to capture if a V1 version was restored and whether it needs to be // re-serialized as that version. @@ -3756,7 +3765,7 @@ void ssl_set_read_error(SSL *ssl); // ssl_update_counter updates the stat counters in |SSL_CTX|. lock should be // set to false when the mutex in |SSL_CTX| has already been locked. -void ssl_update_counter(SSL_CTX *ctx, int &counter, bool lock); +void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock); BSSL_NAMESPACE_END @@ -3873,16 +3882,16 @@ struct ssl_ctx_st : public bssl::RefCounted { int *copy) = nullptr; struct { - int sess_connect = 0; // SSL new conn - started - int sess_connect_renegotiate = 0; // SSL reneg - requested - int sess_connect_good = 0; // SSL new conne/reneg - finished - int sess_accept = 0; // SSL new accept - started - int sess_accept_good = 0; // SSL accept/reneg - finished - int sess_miss = 0; // session lookup misses - int sess_timeout = 0; // reuse attempt on timeouted session - int sess_cache_full = 0; // session removed due to full cache - int sess_hit = 0; // session reuse actually done - int sess_cb_hit = 0; // session-id that was not + SSL_stats_t sess_connect = 0; // SSL new conn - started + SSL_stats_t sess_connect_renegotiate = 0; // SSL reneg - requested + SSL_stats_t sess_connect_good = 0; // SSL new conne/reneg - finished + SSL_stats_t sess_accept = 0; // SSL new accept - started + SSL_stats_t sess_accept_good = 0; // SSL accept/reneg - finished + SSL_stats_t sess_miss = 0; // session lookup misses + SSL_stats_t sess_timeout = 0; // reuse attempt on timeouted session + SSL_stats_t sess_cache_full = 0; // session removed due to full cache + SSL_stats_t sess_hit = 0; // session reuse actually done + SSL_stats_t sess_cb_hit = 0; // session-id that was not // in the cache was // passed back via the callback. This // indicates that the application is diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 7503c0f6db0..772265aabd1 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -166,6 +166,7 @@ #endif + BSSL_NAMESPACE_BEGIN #define GUARD_SUSPENDED_STATE(ptr,code) \ @@ -483,10 +484,22 @@ bool SSL_get_traffic_secrets(const SSL *ssl, return true; } -void ssl_update_counter(SSL_CTX *ctx, int &counter, bool lock) { +void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock) { if (lock) { +#if defined(OPENSSL_STATS_C11_ATOMIC) + std::atomic *count = reinterpret_cast*>(&counter); + int expected = count->load(); + + while (expected != INT_MAX) { + int new_value = expected + 1; + if (count->compare_exchange_weak(expected, new_value)) { + break; + } + } +#else MutexWriteLock ctx_lock(&ctx->lock); counter++; +#endif } else { counter++; } From 9c7b0c99f06a7addf28537df3ff1e0909e075091 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Thu, 30 Oct 2025 10:33:01 -0400 Subject: [PATCH 2/4] Also ssl_read_counter --- ssl/ssl_lib.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 772265aabd1..aa097cedaa8 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -505,9 +505,14 @@ void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock) { } } -static int ssl_read_counter(const SSL_CTX *ctx, int counter) { +static int ssl_read_counter(const SSL_CTX *ctx, const int &counter) { +#if defined(OPENSSL_STATS_C11_ATOMIC) + const std::atomic *count = reinterpret_cast*>(&counter); + return count->load(); +#else MutexReadLock lock(const_cast(&ctx->lock)); return counter; +#endif } void SSL_CTX_set_aes_hw_override_for_testing(SSL_CTX *ctx, From 103a5761c40e807d8b564d6f333bc8a1bc33782a Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Fri, 31 Oct 2025 17:36:03 -0400 Subject: [PATCH 3/4] More cleanup --- ssl/internal.h | 29 ++++++++++++++++++----------- ssl/ssl_lib.cc | 22 ++++++++++++++-------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/ssl/internal.h b/ssl/internal.h index ed43832c942..b5c7aaf78d8 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -188,6 +188,13 @@ typedef int SSL_stats_t; #define OPENSSL_STATS_C11_ATOMIC #endif +// Define the actual storage type for statistics counters +#if defined(OPENSSL_STATS_C11_ATOMIC) +#define SSL_STATS_COUNTER_TYPE std::atomic +#else +#define SSL_STATS_COUNTER_TYPE SSL_stats_t +#endif + BSSL_NAMESPACE_BEGIN struct SSL_CONFIG; @@ -3765,7 +3772,7 @@ void ssl_set_read_error(SSL *ssl); // ssl_update_counter updates the stat counters in |SSL_CTX|. lock should be // set to false when the mutex in |SSL_CTX| has already been locked. -void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock); +void ssl_update_counter(SSL_CTX *ctx, SSL_STATS_COUNTER_TYPE &counter, bool lock); BSSL_NAMESPACE_END @@ -3882,16 +3889,16 @@ struct ssl_ctx_st : public bssl::RefCounted { int *copy) = nullptr; struct { - SSL_stats_t sess_connect = 0; // SSL new conn - started - SSL_stats_t sess_connect_renegotiate = 0; // SSL reneg - requested - SSL_stats_t sess_connect_good = 0; // SSL new conne/reneg - finished - SSL_stats_t sess_accept = 0; // SSL new accept - started - SSL_stats_t sess_accept_good = 0; // SSL accept/reneg - finished - SSL_stats_t sess_miss = 0; // session lookup misses - SSL_stats_t sess_timeout = 0; // reuse attempt on timeouted session - SSL_stats_t sess_cache_full = 0; // session removed due to full cache - SSL_stats_t sess_hit = 0; // session reuse actually done - SSL_stats_t sess_cb_hit = 0; // session-id that was not + SSL_STATS_COUNTER_TYPE sess_connect{}; // SSL new conn - started + SSL_STATS_COUNTER_TYPE sess_connect_renegotiate{}; // SSL reneg - requested + SSL_STATS_COUNTER_TYPE sess_connect_good{}; // SSL new conne/reneg - finished + SSL_STATS_COUNTER_TYPE sess_accept{}; // SSL new accept - started + SSL_STATS_COUNTER_TYPE sess_accept_good{}; // SSL accept/reneg - finished + SSL_STATS_COUNTER_TYPE sess_miss{}; // session lookup misses + SSL_STATS_COUNTER_TYPE sess_timeout{}; // reuse attempt on timeouted session + SSL_STATS_COUNTER_TYPE sess_cache_full{}; // session removed due to full cache + SSL_STATS_COUNTER_TYPE sess_hit{}; // session reuse actually done + SSL_STATS_COUNTER_TYPE sess_cb_hit{}; // session-id that was not // in the cache was // passed back via the callback. This // indicates that the application is diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index aa097cedaa8..1d205f56004 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -484,15 +484,17 @@ bool SSL_get_traffic_secrets(const SSL *ssl, return true; } -void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock) { +void ssl_update_counter(SSL_CTX *ctx, SSL_STATS_COUNTER_TYPE &counter, bool lock) { if (lock) { #if defined(OPENSSL_STATS_C11_ATOMIC) - std::atomic *count = reinterpret_cast*>(&counter); - int expected = count->load(); + // counter is already std::atomic, work with it directly + int expected = counter.load(std::memory_order_relaxed); while (expected != INT_MAX) { int new_value = expected + 1; - if (count->compare_exchange_weak(expected, new_value)) { + if (counter.compare_exchange_weak(expected, new_value, + std::memory_order_relaxed, + std::memory_order_relaxed)) { break; } } @@ -500,15 +502,19 @@ void ssl_update_counter(SSL_CTX *ctx, SSL_stats_t &counter, bool lock) { MutexWriteLock ctx_lock(&ctx->lock); counter++; #endif - } else { + } else if (counter != INT_MAX) { + // Lock is already held by caller +#if defined(OPENSSL_STATS_C11_ATOMIC) + counter.fetch_add(1, std::memory_order_relaxed); +#else counter++; +#endif } } -static int ssl_read_counter(const SSL_CTX *ctx, const int &counter) { +static int ssl_read_counter(const SSL_CTX *ctx, const SSL_STATS_COUNTER_TYPE &counter) { #if defined(OPENSSL_STATS_C11_ATOMIC) - const std::atomic *count = reinterpret_cast*>(&counter); - return count->load(); + return counter.load(std::memory_order_relaxed); #else MutexReadLock lock(const_cast(&ctx->lock)); return counter; From 5bbf367f28a2d8a8e471345b7aa4a2c8c1115f40 Mon Sep 17 00:00:00 2001 From: Justin Smith Date: Sat, 1 Nov 2025 17:28:14 -0400 Subject: [PATCH 4/4] Simplify --- ssl/ssl_lib.cc | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 1d205f56004..83f8bf79c58 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -485,31 +485,17 @@ bool SSL_get_traffic_secrets(const SSL *ssl, } void ssl_update_counter(SSL_CTX *ctx, SSL_STATS_COUNTER_TYPE &counter, bool lock) { - if (lock) { #if defined(OPENSSL_STATS_C11_ATOMIC) - // counter is already std::atomic, work with it directly - int expected = counter.load(std::memory_order_relaxed); - - while (expected != INT_MAX) { - int new_value = expected + 1; - if (counter.compare_exchange_weak(expected, new_value, - std::memory_order_relaxed, - std::memory_order_relaxed)) { - break; - } - } + counter.fetch_add(1, std::memory_order_relaxed); #else + if (lock) { MutexWriteLock ctx_lock(&ctx->lock); counter++; -#endif - } else if (counter != INT_MAX) { + } else { // Lock is already held by caller -#if defined(OPENSSL_STATS_C11_ATOMIC) - counter.fetch_add(1, std::memory_order_relaxed); -#else counter++; -#endif } +#endif } static int ssl_read_counter(const SSL_CTX *ctx, const SSL_STATS_COUNTER_TYPE &counter) {