From 7e45ac6891ade57cb0141402745d144c4ce342cb Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 8 Mar 2024 11:58:07 -0500 Subject: [PATCH] Add CRYPTO_atomic_store api Generally we can get away with just using CRYPTO_atomic_load to do stores by reversing the source and target variables, but doing so creates a problem for the thread sanitizer as CRYPTO_atomic_load hard codes an __ATOMIC_ACQUIRE constraint, which confuses tsan into thinking that loads and stores aren't properly ordered, leading to RAW/WAR hazzards getting reported. Instead create a CRYPTO_atomic_store api that is identical to the load variant, save for the fact that the value is a unit64_t rather than a pointer that gets stored using an __ATOMIC_RELEASE constraint, satisfying tsan. Reviewed-by: Tomas Mraz Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/23671) --- crypto/threads_none.c | 7 +++++++ crypto/threads_pthread.c | 23 +++++++++++++++++++++++ crypto/threads_win.c | 16 ++++++++++++++++ doc/man3/CRYPTO_THREAD_run_once.pod | 9 ++++++++- include/openssl/crypto.h.in | 1 + util/libcrypto.num | 1 + 6 files changed, 56 insertions(+), 1 deletion(-) diff --git a/crypto/threads_none.c b/crypto/threads_none.c index 47a7c01f26..e0387650fe 100644 --- a/crypto/threads_none.c +++ b/crypto/threads_none.c @@ -226,6 +226,13 @@ int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) return 1; } +int CRYPTO_atomic_store(uint64_t *dst, uint64_t val, CRYPTO_RWLOCK *lock) +{ + *dst = val; + + return 1; +} + int CRYPTO_atomic_load_int(int *val, int *ret, CRYPTO_RWLOCK *lock) { *ret = *val; diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c index 69b68e5226..8e411671d9 100644 --- a/crypto/threads_pthread.c +++ b/crypto/threads_pthread.c @@ -919,6 +919,29 @@ int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) return 1; } +int CRYPTO_atomic_store(uint64_t *dst, uint64_t val, CRYPTO_RWLOCK *lock) +{ +# if defined(__GNUC__) && defined(__ATOMIC_ACQUIRE) && !defined(BROKEN_CLANG_ATOMICS) + if (__atomic_is_lock_free(sizeof(*dst), dst)) { + __atomic_store(dst, &val, __ATOMIC_RELEASE); + return 1; + } +# elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11)) + /* This will work for all future Solaris versions. */ + if (ret != NULL) { + atomic_swap_64(dst, val); + return 1; + } +# endif + if (lock == NULL || !CRYPTO_THREAD_read_lock(lock)) + return 0; + *dst = val; + if (!CRYPTO_THREAD_unlock(lock)) + return 0; + + return 1; +} + int CRYPTO_atomic_load_int(int *val, int *ret, CRYPTO_RWLOCK *lock) { # if defined(__GNUC__) && defined(__ATOMIC_ACQUIRE) && !defined(BROKEN_CLANG_ATOMICS) diff --git a/crypto/threads_win.c b/crypto/threads_win.c index 6bcbaea10f..ea72670f22 100644 --- a/crypto/threads_win.c +++ b/crypto/threads_win.c @@ -593,6 +593,22 @@ int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) #endif } +int CRYPTO_atomic_store(uint64_t *dst, uint64_t val, CRYPTO_RWLOCK *lock) +{ +#if (defined(NO_INTERLOCKEDOR64)) + if (lock == NULL || !CRYPTO_THREAD_read_lock(lock)) + return 0; + *dst = val; + if (!CRYPTO_THREAD_unlock(lock)) + return 0; + + return 1; +#else + InterlockedExchange64(dst, val); + return 1; +#endif +} + int CRYPTO_atomic_load_int(int *val, int *ret, CRYPTO_RWLOCK *lock) { #if (defined(NO_INTERLOCKEDOR64)) diff --git a/doc/man3/CRYPTO_THREAD_run_once.pod b/doc/man3/CRYPTO_THREAD_run_once.pod index 470b741c10..3658a39278 100644 --- a/doc/man3/CRYPTO_THREAD_run_once.pod +++ b/doc/man3/CRYPTO_THREAD_run_once.pod @@ -5,7 +5,7 @@ CRYPTO_THREAD_run_once, CRYPTO_THREAD_lock_new, CRYPTO_THREAD_read_lock, CRYPTO_THREAD_write_lock, CRYPTO_THREAD_unlock, CRYPTO_THREAD_lock_free, -CRYPTO_atomic_add, CRYPTO_atomic_or, CRYPTO_atomic_load, +CRYPTO_atomic_add, CRYPTO_atomic_or, CRYPTO_atomic_load, CRYPTO_atomic_store, CRYPTO_atomic_load_int, OSSL_set_max_threads, OSSL_get_max_threads, OSSL_get_thread_support_flags, OSSL_THREAD_SUPPORT_FLAG_THREAD_POOL, @@ -28,6 +28,7 @@ OSSL_THREAD_SUPPORT_FLAG_DEFAULT_SPAWN - OpenSSL thread support int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret, CRYPTO_RWLOCK *lock); int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock); + int CRYPTO_atomic_store(uint64_t *dst, uint64_t val, CRYPTO_RWLOCK *lock); int CRYPTO_atomic_load_int(int *val, int *ret, CRYPTO_RWLOCK *lock); int OSSL_set_max_threads(OSSL_LIB_CTX *ctx, uint64_t max_threads); @@ -112,6 +113,12 @@ NULL, then the function will fail. =item * +CRYPTO_atomic_store() atomically stores the contents of I into I<*dst>. +I will be locked, unless atomic operations are supported on the specific +plaform. + +=item * + CRYPTO_atomic_load_int() works identically to CRYPTO_atomic_load() but operates on an I value instead of a I value. diff --git a/include/openssl/crypto.h.in b/include/openssl/crypto.h.in index b2d691b90f..220a2df354 100644 --- a/include/openssl/crypto.h.in +++ b/include/openssl/crypto.h.in @@ -90,6 +90,7 @@ int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret, CRYPTO_RWLOCK *lock); int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock); int CRYPTO_atomic_load_int(int *val, int *ret, CRYPTO_RWLOCK *lock); +int CRYPTO_atomic_store(uint64_t *dst, uint64_t val, CRYPTO_RWLOCK *lock); /* No longer needed, so this is a no-op */ #define OPENSSL_malloc_init() while(0) continue diff --git a/util/libcrypto.num b/util/libcrypto.num index 75813690dc..0c83e9598e 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -5548,3 +5548,4 @@ X509_STORE_get1_objects 5675 3_3_0 EXIST::FUNCTION: OPENSSL_LH_set_thunks 5676 3_3_0 EXIST::FUNCTION: OPENSSL_LH_doall_arg_thunk 5677 3_3_0 EXIST::FUNCTION: OSSL_HTTP_REQ_CTX_set_max_response_hdr_lines 5678 3_3_0 EXIST::FUNCTION:HTTP +CRYPTO_atomic_store ? 3_4_0 EXIST::FUNCTION: