The branch master has been updated via ea08f8b294d129371536649463c76a81dc4d4e55 (commit) via 49fff26d674adb65f3532eec4f0f37369b41a594 (commit) via db6bcc81ab86fca74730566f0b471a7c3757c95c (commit) via d5e742de653954bfae88f0e5f6c8f0a7a5f6c437 (commit) from 30af356df487b2dad571be15574b454daf70743c (commit)
- Log ----------------------------------------------------------------- commit ea08f8b294d129371536649463c76a81dc4d4e55 Author: Matt Caswell <m...@openssl.org> Date: Wed Dec 23 11:35:54 2020 +0000 Add a test for the new CRYPTO_atomic_* functions Also tests the older CRYPTO_atomic_add() which was without a test Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/13733) commit 49fff26d674adb65f3532eec4f0f37369b41a594 Author: Matt Caswell <m...@openssl.org> Date: Wed Dec 23 11:15:03 2020 +0000 Add documentation for CRYPTO_atomic_or and CRYPTO_atomic_load Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/13733) commit db6bcc81ab86fca74730566f0b471a7c3757c95c Author: Matt Caswell <m...@openssl.org> Date: Tue Dec 22 17:44:07 2020 +0000 Optimise OPENSSL_init_crypto If everything has already been initialised we can check this with a single test at the beginning of OPENSSL_init_crypto() and therefore reduce the amount of time spent in this function. Since this is called via very many codepaths this should have significant performance benefits. Partially fixes #13725 and #13578 Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/13733) commit d5e742de653954bfae88f0e5f6c8f0a7a5f6c437 Author: Matt Caswell <m...@openssl.org> Date: Tue Dec 22 17:43:07 2020 +0000 Add some more CRYPTO_atomic functions We add an implementation for CRYPTO_atomic_or() and CRYPTO_atomic_load() Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/13733) ----------------------------------------------------------------------- Summary of changes: crypto/init.c | 38 +++++++++++++++++++ crypto/threads_none.c | 16 ++++++++ crypto/threads_pthread.c | 50 +++++++++++++++++++++++- crypto/threads_win.c | 19 ++++++++-- doc/man3/CRYPTO_THREAD_run_once.pod | 43 ++++++++++++++++----- include/openssl/crypto.h.in | 3 ++ test/threadstest.c | 76 +++++++++++++++++++++++++++++++++++++ util/libcrypto.num | 2 + 8 files changed, 233 insertions(+), 14 deletions(-) diff --git a/crypto/init.c b/crypto/init.c index f1100df169..50aec32c3d 100644 --- a/crypto/init.c +++ b/crypto/init.c @@ -34,6 +34,7 @@ #include <openssl/trace.h> static int stopped = 0; +static uint64_t optsdone = 0; typedef struct ossl_init_stop_st OPENSSL_INIT_STOP; struct ossl_init_stop_st { @@ -464,6 +465,28 @@ void OPENSSL_cleanup(void) */ int OPENSSL_init_crypto(uint64_t opts, const OPENSSL_INIT_SETTINGS *settings) { + uint64_t tmp; + int aloaddone = 0; + + /* + * We ignore failures from this function. It is probably because we are + * on a platform that doesn't support lockless atomic loads (we may not + * have created init_lock yet so we can't use it). This is just an + * optimisation to skip the full checks in this function if we don't need + * to, so we carry on regardless in the event of failure. + * + * There could be a race here with other threads, so that optsdone has not + * been updated yet, even though the options have in fact been initialised. + * This doesn't matter - it just means we will run the full function + * unnecessarily - but all the critical code is contained in RUN_ONCE + * functions anyway so we are safe. + */ + if (CRYPTO_atomic_load(&optsdone, &tmp, NULL)) { + if ((tmp & opts) == opts) + return 1; + aloaddone = 1; + } + /* * TODO(3.0): This function needs looking at with a view to moving most/all * of this into OSSL_LIB_CTX. @@ -492,6 +515,18 @@ int OPENSSL_init_crypto(uint64_t opts, const OPENSSL_INIT_SETTINGS *settings) if (opts & OPENSSL_INIT_BASE_ONLY) return 1; + /* + * init_lock should definitely be set up now, so we can now repeat the + * same check from above but be sure that it will work even on platforms + * without lockless CRYPTO_atomic_load + */ + if (!aloaddone) { + if (!CRYPTO_atomic_load(&optsdone, &tmp, init_lock)) + return 0; + if ((tmp & opts) == opts) + return 1; + } + /* * Now we don't always set up exit handlers, the INIT_BASE_ONLY calls * should not have the side-effect of setting up exit handlers, and @@ -614,6 +649,9 @@ int OPENSSL_init_crypto(uint64_t opts, const OPENSSL_INIT_SETTINGS *settings) return 0; #endif + if (!CRYPTO_atomic_or(&optsdone, opts, &tmp, init_lock)) + return 0; + return 1; } diff --git a/crypto/threads_none.c b/crypto/threads_none.c index c12d5610aa..55d4b5f0f8 100644 --- a/crypto/threads_none.c +++ b/crypto/threads_none.c @@ -133,6 +133,22 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock) return 1; } +int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret, + CRYPTO_RWLOCK *lock) +{ + *val |= op; + *ret = *val; + + return 1; +} + +int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) +{ + *ret = *val; + + return 1; +} + int openssl_init_fork_handlers(void) { return 0; diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c index afc29b7961..22ba793161 100644 --- a/crypto/threads_pthread.c +++ b/crypto/threads_pthread.c @@ -185,7 +185,7 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock) return 1; } # endif - if (!CRYPTO_THREAD_write_lock(lock)) + if (lock == NULL || !CRYPTO_THREAD_write_lock(lock)) return 0; *val += amount; @@ -197,6 +197,54 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock) return 1; } +int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret, + CRYPTO_RWLOCK *lock) +{ +# if defined(__GNUC__) && defined(__ATOMIC_ACQ_REL) + if (__atomic_is_lock_free(sizeof(*val), val)) { + *ret = __atomic_or_fetch(val, op, __ATOMIC_ACQ_REL); + return 1; + } +# elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11)) + /* This will work for all future Solaris versions. */ + if (ret != NULL) { + *ret = atomic_or_64_nv(val, op); + return 1; + } +# endif + if (lock == NULL || !CRYPTO_THREAD_write_lock(lock)) + return 0; + *val |= op; + *ret = *val; + + if (!CRYPTO_THREAD_unlock(lock)) + return 0; + + return 1; +} + +int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) +{ +# if defined(__GNUC__) && defined(__ATOMIC_ACQUIRE) + if (__atomic_is_lock_free(sizeof(*val), val)) { + __atomic_load(val, ret, __ATOMIC_ACQUIRE); + return 1; + } +# elif defined(__sun) && (defined(__SunOS_5_10) || defined(__SunOS_5_11)) + /* This will work for all future Solaris versions. */ + if (ret != NULL) { + *ret = atomic_or_64_nv(val, 0); + return 1; + } +# endif + if (lock == NULL || !CRYPTO_THREAD_read_lock(lock)) + return 0; + *ret = *val; + if (!CRYPTO_THREAD_unlock(lock)) + return 0; + + return 1; +} # ifndef FIPS_MODULE # ifdef OPENSSL_SYS_UNIX diff --git a/crypto/threads_win.c b/crypto/threads_win.c index a008831a3e..ef68fe2d24 100644 --- a/crypto/threads_win.c +++ b/crypto/threads_win.c @@ -66,9 +66,9 @@ void CRYPTO_THREAD_lock_free(CRYPTO_RWLOCK *lock) return; } -# define ONCE_UNINITED 0 -# define ONCE_ININIT 1 -# define ONCE_DONE 2 +# define ONCE_UNINITED 0 +# define ONCE_ININIT 1 +# define ONCE_DONE 2 /* * We don't use InitOnceExecuteOnce because that isn't available in WinXP which @@ -159,6 +159,19 @@ int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock) return 1; } +int CRYPTO_atomic_or(uint64_t *val, uint64_t op, uint64_t *ret, + CRYPTO_RWLOCK *lock) +{ + *ret = (uint64_t)InterlockedOr64((LONG64 volatile *)val, (LONG64)op) | op; + return 1; +} + +int CRYPTO_atomic_load(uint64_t *val, uint64_t *ret, CRYPTO_RWLOCK *lock) +{ + *ret = (uint64_t)InterlockedOr64((LONG64 volatile *)val, 0); + return 1; +} + int openssl_init_fork_handlers(void) { return 0; diff --git a/doc/man3/CRYPTO_THREAD_run_once.pod b/doc/man3/CRYPTO_THREAD_run_once.pod index 5cffc42026..a182359f47 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 - OpenSSL thread support +CRYPTO_atomic_add, CRYPTO_atomic_or, CRYPTO_atomic_load - OpenSSL thread support =head1 SYNOPSIS @@ -21,6 +21,9 @@ CRYPTO_atomic_add - OpenSSL thread support void CRYPTO_THREAD_lock_free(CRYPTO_RWLOCK *lock); int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock); + 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); =head1 DESCRIPTION @@ -38,10 +41,10 @@ The following multi-threading function are provided: =item * CRYPTO_THREAD_run_once() can be used to perform one-time initialization. -The B<once> argument must be a pointer to a static object of type +The I<once> argument must be a pointer to a static object of type B<CRYPTO_ONCE> that was statically initialized to the value B<CRYPTO_ONCE_STATIC_INIT>. -The B<init> argument is a pointer to a function that performs the desired +The I<init> argument is a pointer to a function that performs the desired exactly once initialization. In particular, this can be used to allocate locks in a thread-safe manner, which can then be used with the locking functions below. @@ -53,27 +56,47 @@ lock. =item * -CRYPTO_THREAD_read_lock() locks the provided B<lock> for reading. +CRYPTO_THREAD_read_lock() locks the provided I<lock> for reading. =item * -CRYPTO_THREAD_write_lock() locks the provided B<lock> for writing. +CRYPTO_THREAD_write_lock() locks the provided I<lock> for writing. =item * -CRYPTO_THREAD_unlock() unlocks the previously locked B<lock>. +CRYPTO_THREAD_unlock() unlocks the previously locked I<lock>. =item * -CRYPTO_THREAD_lock_free() frees the provided B<lock>. +CRYPTO_THREAD_lock_free() frees the provided I<lock>. =item * -CRYPTO_atomic_add() atomically adds B<amount> to B<val> and returns the -result of the operation in B<ret>. B<lock> will be locked, unless atomic +CRYPTO_atomic_add() atomically adds I<amount> to I<*val> and returns the +result of the operation in I<*ret>. I<lock> will be locked, unless atomic operations are supported on the specific platform. Because of this, if a variable is modified by CRYPTO_atomic_add() then CRYPTO_atomic_add() must -be the only way that the variable is modified. +be the only way that the variable is modified. If atomic operations are not +supported and I<lock> is NULL, then the function will fail. + +=item * + +CRYPTO_atomic_or() performs an atomic bitwise or of I<op> and I<*val> and stores +the result back in I<*val>. It also returns the result of the operation in +I<*ret>. I<lock> will be locked, unless atomic operations are supported on the +specific platform. Because of this, if a variable is modified by +CRYPTO_atomic_or() or read by CRYPTO_atomic_load() then CRYPTO_atomic_or() must +be the only way that the variable is modified. If atomic operations are not +supported and I<lock> is NULL, then the function will fail. + +=item * + +CRYPTO_atomic_load() atomically loads the contents of I<*val> into I<*ret>. +I<lock> will be locked, unless atomic operations are supported on the specific +platform. Because of this, if a variable is modified by CRYPTO_atomic_or() or +read by CRYPTO_atomic_load() then CRYPTO_atomic_load() must be the only way that +the variable is read. If atomic operations are not supported and I<lock> is +NULL, then the function will fail. =back diff --git a/include/openssl/crypto.h.in b/include/openssl/crypto.h.in index 0641db3a44..0b9aeefe04 100644 --- a/include/openssl/crypto.h.in +++ b/include/openssl/crypto.h.in @@ -86,6 +86,9 @@ int CRYPTO_THREAD_unlock(CRYPTO_RWLOCK *lock); void CRYPTO_THREAD_lock_free(CRYPTO_RWLOCK *lock); int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock); +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); /* No longer needed, so this is a no-op */ #define OPENSSL_malloc_init() while(0) continue diff --git a/test/threadstest.c b/test/threadstest.c index 5f373fe75f..d7ed59781d 100644 --- a/test/threadstest.c +++ b/test/threadstest.c @@ -184,10 +184,86 @@ static int test_thread_local(void) return 1; } +static int test_atomic(void) +{ + int val = 0, ret = 0, testresult = 0; + uint64_t val64 = 1, ret64 = 0; + CRYPTO_RWLOCK *lock = CRYPTO_THREAD_lock_new(); + + if (!TEST_ptr(lock)) + return 0; + + if (CRYPTO_atomic_add(&val, 1, &ret, NULL)) { + /* This succeeds therefore we're on a platform with lockless atomics */ + if (!TEST_int_eq(val, 1) || !TEST_int_eq(val, ret)) + goto err; + } else { + /* This failed therefore we're on a platform without lockless atomics */ + if (!TEST_int_eq(val, 0) || !TEST_int_eq(val, ret)) + goto err; + } + val = 0; + ret = 0; + + if (!TEST_true(CRYPTO_atomic_add(&val, 1, &ret, lock))) + goto err; + if (!TEST_int_eq(val, 1) || !TEST_int_eq(val, ret)) + goto err; + + if (CRYPTO_atomic_or(&val64, 2, &ret64, NULL)) { + /* This succeeds therefore we're on a platform with lockless atomics */ + if (!TEST_uint_eq((unsigned int)val64, 3) + || !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64)) + goto err; + } else { + /* This failed therefore we're on a platform without lockless atomics */ + if (!TEST_uint_eq((unsigned int)val64, 1) + || !TEST_int_eq((unsigned int)ret64, 0)) + goto err; + } + val64 = 1; + ret64 = 0; + + if (!TEST_true(CRYPTO_atomic_or(&val64, 2, &ret64, lock))) + goto err; + + if (!TEST_uint_eq((unsigned int)val64, 3) + || !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64)) + goto err; + + ret64 = 0; + if (CRYPTO_atomic_load(&val64, &ret64, NULL)) { + /* This succeeds therefore we're on a platform with lockless atomics */ + if (!TEST_uint_eq((unsigned int)val64, 3) + || !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64)) + goto err; + } else { + /* This failed therefore we're on a platform without lockless atomics */ + if (!TEST_uint_eq((unsigned int)val64, 3) + || !TEST_int_eq((unsigned int)ret64, 0)) + goto err; + } + + ret64 = 0; + if (!TEST_true(CRYPTO_atomic_load(&val64, &ret64, lock))) + goto err; + + if (!TEST_uint_eq((unsigned int)val64, 3) + || !TEST_uint_eq((unsigned int)val64, (unsigned int)ret64)) + goto err; + + testresult = 1; + err: + + CRYPTO_THREAD_lock_free(lock); + return testresult; +} + int setup_tests(void) { ADD_TEST(test_lock); ADD_TEST(test_once); ADD_TEST(test_thread_local); + ADD_TEST(test_atomic); return 1; } diff --git a/util/libcrypto.num b/util/libcrypto.num index 93ca779831..289a6672f9 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -5284,3 +5284,5 @@ PEM_write_bio_PrivateKey_ex ? 3_0_0 EXIST::FUNCTION: PEM_write_PUBKEY_ex ? 3_0_0 EXIST::FUNCTION:STDIO PEM_write_bio_PUBKEY_ex ? 3_0_0 EXIST::FUNCTION: EVP_PKEY_get_group_name ? 3_0_0 EXIST::FUNCTION: +CRYPTO_atomic_or ? 3_0_0 EXIST::FUNCTION: +CRYPTO_atomic_load ? 3_0_0 EXIST::FUNCTION: