From cefeb4cd7e75698c524f423d74b5a8e92d99bd8c Mon Sep 17 00:00:00 2001 From: Daniel Pouzzner Date: Thu, 14 Aug 2025 09:33:14 -0500 Subject: [PATCH] atomics/cpuid_flags fixes from peer review: wolfcrypt/src/cpuid.c: cpuid_set_flag() and cpuid_clear_flag() thread safety; wolfcrypt/src/wc_port.c: comments re __ATOMIC_SEQ_CST and __ATOMIC_ACQUIRE; wolfssl/wolfcrypt/wc_port.h: single overrideable definitions for WOLFSSL_ATOMIC_COERCE_[U]INT(), and comment cleanup. also added WOLFSSL_USER_DEFINED_ATOMICS. --- .wolfssl_known_macro_extras | 1 + wolfcrypt/src/cpuid.c | 12 +++++++---- wolfcrypt/src/wc_port.c | 24 +++++++++++++++++++++ wolfssl/wolfcrypt/cpuid.h | 2 +- wolfssl/wolfcrypt/wc_port.h | 42 ++++++++++++++++++++++++------------- 5 files changed, 62 insertions(+), 19 deletions(-) diff --git a/.wolfssl_known_macro_extras b/.wolfssl_known_macro_extras index 0b2d773a3..8eff27a88 100644 --- a/.wolfssl_known_macro_extras +++ b/.wolfssl_known_macro_extras @@ -877,6 +877,7 @@ WOLFSSL_TLSX_PQC_MLKEM_STORE_PRIV_KEY WOLFSSL_TRACK_MEMORY_FULL WOLFSSL_TRAP_MALLOC_SZ WOLFSSL_UNALIGNED_64BIT_ACCESS +WOLFSSL_USER_DEFINED_ATOMICS WOLFSSL_USER_FILESYSTEM WOLFSSL_USER_LOG WOLFSSL_USER_MUTEX diff --git a/wolfcrypt/src/cpuid.c b/wolfcrypt/src/cpuid.c index 4b1882dc3..0250911db 100644 --- a/wolfcrypt/src/cpuid.c +++ b/wolfcrypt/src/cpuid.c @@ -355,14 +355,18 @@ void cpuid_set_flag(word32 flag) { - WOLFSSL_ATOMIC_STORE - (cpuid_flags, WOLFSSL_ATOMIC_LOAD(cpuid_flags) | flag); + word32 current_flags = WOLFSSL_ATOMIC_LOAD(cpuid_flags); + while (! wolfSSL_Atomic_Uint_CompareExchange + (&cpuid_flags, ¤t_flags, current_flags | flag)) + WC_RELAX_LONG_LOOP(); } void cpuid_clear_flag(word32 flag) { - WOLFSSL_ATOMIC_STORE - (cpuid_flags, WOLFSSL_ATOMIC_LOAD(cpuid_flags) & ~flag); + word32 current_flags = WOLFSSL_ATOMIC_LOAD(cpuid_flags); + while (! wolfSSL_Atomic_Uint_CompareExchange + (&cpuid_flags, ¤t_flags, current_flags & ~flag)) + WC_RELAX_LONG_LOOP(); } #endif /* HAVE_CPUID */ diff --git a/wolfcrypt/src/wc_port.c b/wolfcrypt/src/wc_port.c index 76ea14651..fc42f6fdb 100644 --- a/wolfcrypt/src/wc_port.c +++ b/wolfcrypt/src/wc_port.c @@ -1310,6 +1310,12 @@ int wolfSSL_Atomic_Int_SubFetch(wolfSSL_Atomic_Int* c, int i) int wolfSSL_Atomic_Int_CompareExchange(wolfSSL_Atomic_Int* c, int *expected_i, int new_i) { + /* For the success path, use full synchronization with barriers -- + * "Sequentially-consistent ordering" -- so that all threads see the same + * "single total modification order of all atomic operations" -- but on + * failure we just need to be sure we acquire the value that changed out + * from under us. + */ return __atomic_compare_exchange_n(c, expected_i, new_i, 0 /* weak */, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE); } @@ -1341,6 +1347,12 @@ unsigned int wolfSSL_Atomic_Uint_SubFetch(wolfSSL_Atomic_Uint* c, int wolfSSL_Atomic_Uint_CompareExchange( wolfSSL_Atomic_Uint* c, unsigned int *expected_i, unsigned int new_i) { + /* For the success path, use full synchronization with barriers -- + * "Sequentially-consistent ordering" -- so that all threads see the same + * "single total modification order of all atomic operations" -- but on + * failure we just need to be sure we acquire the value that changed out + * from under us. + */ return __atomic_compare_exchange_n( c, expected_i, new_i, 0 /* weak */, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE); } @@ -1383,6 +1395,12 @@ int wolfSSL_Atomic_Int_SubFetch(wolfSSL_Atomic_Int* c, int i) int wolfSSL_Atomic_Int_CompareExchange( wolfSSL_Atomic_Int* c, int *expected_i, int new_i) { + /* For the success path, use full synchronization with barriers -- + * "Sequentially-consistent ordering" -- so that all threads see the same + * "single total modification order of all atomic operations" -- but on + * failure we just need to be sure we acquire the value that changed out + * from under us. + */ return atomic_compare_exchange_strong_explicit( c, expected_i, new_i, memory_order_seq_cst, memory_order_acquire); } @@ -1416,6 +1434,12 @@ unsigned int wolfSSL_Atomic_Uint_SubFetch(wolfSSL_Atomic_Uint* c, int wolfSSL_Atomic_Uint_CompareExchange( wolfSSL_Atomic_Uint* c, unsigned int *expected_i, unsigned int new_i) { + /* For the success path, use full synchronization with barriers -- + * "Sequentially-consistent ordering" -- so that all threads see the same + * "single total modification order of all atomic operations" -- but on + * failure we just need to be sure we acquire the value that changed out + * from under us. + */ return atomic_compare_exchange_strong_explicit( c, expected_i, new_i, memory_order_seq_cst, memory_order_acquire); } diff --git a/wolfssl/wolfcrypt/cpuid.h b/wolfssl/wolfcrypt/cpuid.h index 56bc61401..3ba3405b1 100644 --- a/wolfssl/wolfcrypt/cpuid.h +++ b/wolfssl/wolfcrypt/cpuid.h @@ -115,7 +115,7 @@ return 0; } - /* Public APIs to modify flags -- note that these are not threadsafe. */ + /* Public APIs to modify flags. */ WOLFSSL_API void cpuid_select_flags(word32 flags); WOLFSSL_API void cpuid_set_flag(word32 flag); WOLFSSL_API void cpuid_clear_flag(word32 flag); diff --git a/wolfssl/wolfcrypt/wc_port.h b/wolfssl/wolfcrypt/wc_port.h index 634f2e17a..b30a499f5 100644 --- a/wolfssl/wolfcrypt/wc_port.h +++ b/wolfssl/wolfcrypt/wc_port.h @@ -466,13 +466,20 @@ #endif #ifndef WOLFSSL_NO_ATOMICS - #ifdef SINGLE_THREADED + #if defined(WOLFSSL_USER_DEFINED_ATOMICS) + /* user-supplied bindings for wolfSSL_Atomic_Int etc. */ + #if !defined(WOLFSSL_ATOMIC_INITIALIZER) || \ + !defined(WOLFSSL_ATOMIC_LOAD) || \ + !defined(WOLFSSL_ATOMIC_STORE) + #error WOLFSSL_USER_DEFINED_ATOMICS is set but macro(s) are missing. + #else + #define WOLFSSL_ATOMIC_OPS + #endif + #elif defined(SINGLE_THREADED) typedef int wolfSSL_Atomic_Int; typedef unsigned int wolfSSL_Atomic_Uint; #define WOLFSSL_ATOMIC_INITIALIZER(x) (x) #define WOLFSSL_ATOMIC_LOAD(x) (x) - #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) - #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) #define WOLFSSL_ATOMIC_STORE(x, val) (x) = (val) #define WOLFSSL_ATOMIC_OPS #elif defined(HAVE_C___ATOMIC) @@ -484,8 +491,6 @@ #define WOLFSSL_ATOMIC_INITIALIZER(x) (x) #define WOLFSSL_ATOMIC_LOAD(x) __atomic_load_n(&(x), \ __ATOMIC_CONSUME) - #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) - #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) #define WOLFSSL_ATOMIC_STORE(x, val) __atomic_store_n(&(x), \ val, __ATOMIC_RELEASE) #define WOLFSSL_ATOMIC_OPS @@ -498,8 +503,6 @@ typedef atomic_uint wolfSSL_Atomic_Uint; #define WOLFSSL_ATOMIC_INITIALIZER(x) (x) #define WOLFSSL_ATOMIC_LOAD(x) atomic_load(&(x)) - #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) - #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) #define WOLFSSL_ATOMIC_STORE(x, val) atomic_store(&(x), val) #define WOLFSSL_ATOMIC_OPS #endif /* WOLFSSL_HAVE_ATOMIC_H */ @@ -515,8 +518,6 @@ typedef volatile unsigned long wolfSSL_Atomic_Uint; #define WOLFSSL_ATOMIC_INITIALIZER(x) (x) #define WOLFSSL_ATOMIC_LOAD(x) (x) - #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) - #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) #define WOLFSSL_ATOMIC_STORE(x, val) (x) = (val) #define WOLFSSL_ATOMIC_OPS #endif @@ -530,18 +531,31 @@ #ifdef WOLFSSL_NO_ATOMICS #define WOLFSSL_ATOMIC_INITIALIZER(x) (x) #define WOLFSSL_ATOMIC_LOAD(x) (x) - #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) - #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) #define WOLFSSL_ATOMIC_STORE(x, val) (x) = (val) #endif /* WOLFSSL_NO_ATOMICS */ -#if defined(WOLFSSL_ATOMIC_OPS) && !defined(SINGLE_THREADED) +/* WOLFSSL_ATOMIC_COERCE_INT() needs to accept either a regular int or an + * wolfSSL_Atomic_Int as its argument, and evaluate to a regular int. + * Allows a user-supplied override definition with type introspection. + */ +#ifndef WOLFSSL_ATOMIC_COERCE_INT + #define WOLFSSL_ATOMIC_COERCE_INT(x) ((int)(x)) +#endif +#ifndef WOLFSSL_ATOMIC_COERCE_UINT + #define WOLFSSL_ATOMIC_COERCE_UINT(x) ((unsigned int)(x)) +#endif + +#ifdef WOLFSSL_USER_DEFINED_ATOMICS + /* user-supplied bindings for wolfSSL_Atomic_Int_Init(), + * wolfSSL_Atomic_Int_FetchAdd(), etc. + */ +#elif defined(WOLFSSL_ATOMIC_OPS) && !defined(SINGLE_THREADED) WOLFSSL_API void wolfSSL_Atomic_Int_Init(wolfSSL_Atomic_Int* c, int i); WOLFSSL_API void wolfSSL_Atomic_Uint_Init( wolfSSL_Atomic_Uint* c, unsigned int i); - /* Fetch* functions return the value of the counter immediately preceding + /* FetchOp functions return the value of the counter immediately preceding * the effects of the operation. - * *Fetch functions return the value of the counter immediately after + * OpFetch functions return the value of the counter immediately after * the effects of the operation. */ WOLFSSL_API int wolfSSL_Atomic_Int_FetchAdd(wolfSSL_Atomic_Int* c, int i);