[openssl] master update
Dr. Paul Dale
pauli at openssl.org
Tue Aug 31 10:45:09 UTC 2021
The branch master has been updated
via 9b6d17e423da138ea7fd190ae366580c539dceca (commit)
via 4f8e0272c1bde43d97bc1c4471dbaecfc89f7aae (commit)
via 2b4a611ef18b0696bff57da889622e0e42ed4521 (commit)
via 03c137de971354b7c2e00f0198e85446ead6cfc3 (commit)
from c7468c17d7090492c266492ffa4ccf5baf93ffc4 (commit)
- Log -----------------------------------------------------------------
commit 9b6d17e423da138ea7fd190ae366580c539dceca
Author: Matt Caswell <matt at openssl.org>
Date: Mon Aug 30 15:54:22 2021 +0100
Add a warning about locking in the child provider callback docs
The child provider callbacks can hold the store lock. In order to avoid
deadlocks we require that the callback implementations don't themselves
call functions that may aquire those locks.
Reviewed-by: Tomas Mraz <tomas at openssl.org>
Reviewed-by: Paul Dale <pauli at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16469)
commit 4f8e0272c1bde43d97bc1c4471dbaecfc89f7aae
Author: Pauli <pauli at openssl.org>
Date: Mon Aug 16 12:20:56 2021 +1000
Add additional test to thread sanitizer build
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16469)
commit 2b4a611ef18b0696bff57da889622e0e42ed4521
Author: Matt Caswell <matt at openssl.org>
Date: Mon Aug 30 13:04:31 2021 +0100
Refactor provider_core.c to adhere to the locking rules
The previous commit provided some guidelines and some rules for using
locking in order to avoid deadlocks. This commit refactors the code in
order to adhere to those guidelines and rules.
Fixes #16312
Reviewed-by: Tomas Mraz <tomas at openssl.org>
Reviewed-by: Paul Dale <pauli at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16469)
commit 03c137de971354b7c2e00f0198e85446ead6cfc3
Author: Matt Caswell <matt at openssl.org>
Date: Mon Aug 30 15:33:07 2021 +0100
Add commentary about lock usage in provider_core.c
Provide some guidelines, as well as some rules for using the locks in
provider_core.c, in order to avoid the introduction of deadlocks.
Reviewed-by: Tomas Mraz <tomas at openssl.org>
Reviewed-by: Paul Dale <pauli at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16469)
-----------------------------------------------------------------------
Summary of changes:
.github/workflows/ci.yml | 2 +-
crypto/provider_core.c | 239 +++++++++++++++++++++++++++++++++++----------
doc/man7/provider-base.pod | 6 +-
3 files changed, 190 insertions(+), 57 deletions(-)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 2f2a9b9fb2..601ba5f6b1 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -157,7 +157,7 @@ jobs:
- name: make
run: make -s -j4
- name: make test
- run: make TESTS=test_threads test HARNESS_JOBS=${HARNESS_JOBS:-4}
+ run: make V=1 TESTS="test_threads test_internal_provider test_provfetch test_provider test_pbe test_evp_kdf test_pkcs12 test_store test_evp" test HARNESS_JOBS=${HARNESS_JOBS:-4}
enable_non-default_options:
runs-on: ubuntu-latest
diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index 1f688557c1..e4069eb4f7 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -28,6 +28,89 @@
# include <openssl/self_test.h>
#endif
+/*
+ * This file defines and uses a number of different structures:
+ *
+ * OSSL_PROVIDER (provider_st): Used to represent all information related to a
+ * single instance of a provider.
+ *
+ * provider_store_st: Holds information about the collection of providers that
+ * are available within the current library context (OSSL_LIB_CTX). It also
+ * holds configuration information about providers that could be loaded at some
+ * future point.
+ *
+ * OSSL_PROVIDER_CHILD_CB: An instance of this structure holds the callbacks
+ * that have been registered for a child library context and the associated
+ * provider that registered those callbacks.
+ *
+ * Where a child library context exists then it has its own instance of the
+ * provider store. Each provider that exists in the parent provider store, has
+ * an associated child provider in the child library context's provider store.
+ * As providers get activated or deactivated this needs to be mirrored in the
+ * associated child providers.
+ *
+ * LOCKING
+ * =======
+ *
+ * There are a number of different locks used in this file and it is important
+ * to understand how they should be used in order to avoid deadlocks.
+ *
+ * Fields within a structure can often be "write once" on creation, and then
+ * "read many". Creation of a structure is done by a single thread, and
+ * therefore no lock is required for the "write once/read many" fields. It is
+ * safe for multiple threads to read these fields without a lock, because they
+ * will never be changed.
+ *
+ * However some fields may be changed after a structure has been created and
+ * shared between multiple threads. Where this is the case a lock is required.
+ *
+ * The locks available are:
+ *
+ * The provider flag_lock: Used to control updates to the various provider
+ * "flags" (flag_initialized, flag_activated, flag_fallback) and associated
+ * "counts" (activatecnt).
+ *
+ * The provider refcnt_lock: Only ever used to control updates to the provider
+ * refcnt value.
+ *
+ * The provider optbits_lock: Used to control access to the provider's
+ * operation_bits and operation_bits_sz fields.
+ *
+ * The store default_path_lock: Used to control access to the provider store's
+ * default search path value (default_path)
+ *
+ * The store lock: Used to control the stack of provider's held within the
+ * provider store, as well as the stack of registered child provider callbacks.
+ *
+ * As a general rule-of-thumb it is best to:
+ * - keep the scope of the code that is protected by a lock to the absolute
+ * minimum possible;
+ * - try to keep the scope of the lock to within a single function (i.e. avoid
+ * making calls to other functions while holding a lock);
+ * - try to only ever hold one lock at a time.
+ *
+ * Unfortunately, it is not always possible to stick to the above guidelines.
+ * Where they are not adhered to there is always a danger of inadvertently
+ * introducing the possibility of deadlock. The following rules MUST be adhered
+ * to in order to avoid that:
+ * - Holding multiple locks at the same time is only allowed for the
+ * provider store lock, the provider flag_lock and the provider refcnt_lock.
+ * - When holding multiple locks they must be acquired in the following order of
+ * precedence:
+ * 1) provider store lock
+ * 2) provider flag_lock
+ * 3) provider refcnt_lock
+ * - When releasing locks they must be released in the reverse order to which
+ * they were acquired
+ * - No locks may be held when making an upcall. NOTE: Some common functions
+ * can make upcalls as part of their normal operation. If you need to call
+ * some other function while holding a lock make sure you know whether it
+ * will make any upcalls or not. For example ossl_provider_up_ref() can call
+ * ossl_provider_up_ref_parent() which can call the c_prov_up_ref() upcall.
+ * - It is permissible to hold the store lock when calling child provider
+ * callbacks. No other locks may be held during such callbacks.
+ */
+
static OSSL_PROVIDER *provider_new(const char *name,
OSSL_provider_init_fn *init_function,
STACK_OF(INFOPAIR) *parameters);
@@ -343,11 +426,11 @@ OSSL_PROVIDER *ossl_provider_find(OSSL_LIB_CTX *libctx, const char *name,
tmpl.name = (char *)name;
if (!CRYPTO_THREAD_read_lock(store->lock))
return NULL;
- if ((i = sk_OSSL_PROVIDER_find(store->providers, &tmpl)) == -1
- || (prov = sk_OSSL_PROVIDER_value(store->providers, i)) == NULL
- || !ossl_provider_up_ref(prov))
- prov = NULL;
+ if ((i = sk_OSSL_PROVIDER_find(store->providers, &tmpl)) != -1)
+ prov = sk_OSSL_PROVIDER_value(store->providers, i);
CRYPTO_THREAD_unlock(store->lock);
+ if (prov != NULL && !ossl_provider_up_ref(prov))
+ prov = NULL;
}
return prov;
@@ -532,15 +615,6 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
else
actualtmp = sk_OSSL_PROVIDER_value(store->providers, idx);
- if (actualprov != NULL) {
- if (!ossl_provider_up_ref(actualtmp)) {
- ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
- actualtmp = NULL;
- goto err;
- }
- *actualprov = actualtmp;
- }
-
if (idx == -1) {
if (sk_OSSL_PROVIDER_push(store->providers, prov) == 0)
goto err;
@@ -555,7 +629,16 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
CRYPTO_THREAD_unlock(store->lock);
- if (actualtmp != prov) {
+ if (actualprov != NULL) {
+ if (!ossl_provider_up_ref(actualtmp)) {
+ ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
+ actualtmp = NULL;
+ goto err;
+ }
+ *actualprov = actualtmp;
+ }
+
+ if (idx >= 0) {
/*
* The provider is already in the store. Probably two threads
* independently initialised their own provider objects with the same
@@ -923,10 +1006,13 @@ static int provider_init(OSSL_PROVIDER *prov)
* Deactivate a provider.
* Return -1 on failure and the activation count on success
*/
-static int provider_deactivate(OSSL_PROVIDER *prov)
+static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls)
{
int count;
struct provider_store_st *store;
+#ifndef FIPS_MODULE
+ int freeparent = 0, removechildren = 0;
+#endif
if (!ossl_assert(prov != NULL))
return -1;
@@ -943,32 +1029,42 @@ static int provider_deactivate(OSSL_PROVIDER *prov)
}
#ifndef FIPS_MODULE
- if (prov->activatecnt == 2 && prov->ischild) {
+ if (prov->activatecnt >= 2 && prov->ischild && upcalls) {
/*
* We have had a direct activation in this child libctx so we need to
- * now down the ref count in the parent provider.
+ * now down the ref count in the parent provider. We do the actual down
+ * ref outside of the flag_lock, since it could involve getting other
+ * locks.
*/
- ossl_provider_free_parent(prov, 1);
+ freeparent = 1;
}
#endif
if ((count = --prov->activatecnt) < 1) {
prov->flag_activated = 0;
#ifndef FIPS_MODULE
- {
- int i, max = sk_OSSL_PROVIDER_CHILD_CB_num(store->child_cbs);
- OSSL_PROVIDER_CHILD_CB *child_cb;
-
- for (i = 0; i < max; i++) {
- child_cb = sk_OSSL_PROVIDER_CHILD_CB_value(store->child_cbs, i);
- child_cb->remove_cb((OSSL_CORE_HANDLE *)prov, child_cb->cbdata);
- }
- }
+ removechildren = 1;
#endif
}
CRYPTO_THREAD_unlock(prov->flag_lock);
+
+#ifndef FIPS_MODULE
+ if (removechildren) {
+ int i, max = sk_OSSL_PROVIDER_CHILD_CB_num(store->child_cbs);
+ OSSL_PROVIDER_CHILD_CB *child_cb;
+
+ for (i = 0; i < max; i++) {
+ child_cb = sk_OSSL_PROVIDER_CHILD_CB_value(store->child_cbs, i);
+ child_cb->remove_cb((OSSL_CORE_HANDLE *)prov, child_cb->cbdata);
+ }
+ }
+#endif
CRYPTO_THREAD_unlock(store->lock);
+#ifndef FIPS_MODULE
+ if (freeparent)
+ ossl_provider_free_parent(prov, 1);
+#endif
/* We don't deinit here, that's done in ossl_provider_free() */
return count;
@@ -982,7 +1078,7 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
{
int count = -1;
struct provider_store_st *store;
- int ret = 1;
+ int ret = 1, createchildren = 0;
store = prov->store;
/*
@@ -995,31 +1091,41 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
return -1;
}
- if (lock && !CRYPTO_THREAD_read_lock(store->lock))
+#ifndef FIPS_MODULE
+ if (prov->ischild && upcalls && !ossl_provider_up_ref_parent(prov, 1))
return -1;
+#endif
- if (lock && !CRYPTO_THREAD_write_lock(prov->flag_lock)) {
- CRYPTO_THREAD_unlock(store->lock);
+ if (lock && !CRYPTO_THREAD_read_lock(store->lock)) {
+#ifndef FIPS_MODULE
+ if (prov->ischild && upcalls)
+ ossl_provider_free_parent(prov, 1);
+#endif
return -1;
}
+ if (lock && !CRYPTO_THREAD_write_lock(prov->flag_lock)) {
+ CRYPTO_THREAD_unlock(store->lock);
#ifndef FIPS_MODULE
- if (prov->ischild && upcalls)
- ret = ossl_provider_up_ref_parent(prov, 1);
+ if (prov->ischild && upcalls)
+ ossl_provider_free_parent(prov, 1);
#endif
+ return -1;
+ }
- if (ret) {
- count = ++prov->activatecnt;
- prov->flag_activated = 1;
+ count = ++prov->activatecnt;
+ prov->flag_activated = 1;
- if (prov->activatecnt == 1 && store != NULL)
- ret = create_provider_children(prov);
- }
+ if (prov->activatecnt == 1 && store != NULL)
+ createchildren = 1;
- if (lock) {
+ if (lock)
CRYPTO_THREAD_unlock(prov->flag_lock);
+ if (createchildren)
+ ret = create_provider_children(prov);
+ if (lock)
CRYPTO_THREAD_unlock(store->lock);
- }
+
if (!ret)
return -1;
@@ -1068,7 +1174,7 @@ int ossl_provider_deactivate(OSSL_PROVIDER *prov)
{
int count;
- if (prov == NULL || (count = provider_deactivate(prov)) < 0)
+ if (prov == NULL || (count = provider_deactivate(prov, 1)) < 0)
return 0;
return count == 0 ? provider_flush_store_cache(prov) : 1;
}
@@ -1155,7 +1261,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
void *cbdata),
void *cbdata)
{
- int ret = 0, curr, max;
+ int ret = 0, curr, max, ref = 0;
struct provider_store_st *store = get_provider_store(ctx);
STACK_OF(OSSL_PROVIDER) *provs = NULL;
@@ -1195,16 +1301,25 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
if (!CRYPTO_THREAD_write_lock(prov->flag_lock))
goto err_unlock;
if (prov->flag_activated) {
- if (!ossl_provider_up_ref(prov)){
+ /*
+ * We call CRYPTO_UP_REF directly rather than ossl_provider_up_ref
+ * to avoid upping the ref count on the parent provider, which we
+ * must not do while holding locks.
+ */
+ if (CRYPTO_UP_REF(&prov->refcnt, &ref, prov->refcnt_lock) <= 0) {
CRYPTO_THREAD_unlock(prov->flag_lock);
goto err_unlock;
}
/*
* It's already activated, but we up the activated count to ensure
* it remains activated until after we've called the user callback.
+ * We do this with no locking (because we already hold the locks)
+ * and no upcalls (which must not be called when locks are held). In
+ * theory this could mean the parent provider goes inactive, whilst
+ * still activated in the child for a short period. That's ok.
*/
- if (provider_activate(prov, 0, 1) < 0) {
- ossl_provider_free(prov);
+ if (provider_activate(prov, 0, 0) < 0) {
+ CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
CRYPTO_THREAD_unlock(prov->flag_lock);
goto err_unlock;
}
@@ -1241,8 +1356,18 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
for (curr++; curr < max; curr++) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
- provider_deactivate(prov);
- ossl_provider_free(prov);
+ provider_deactivate(prov, 0);
+ /*
+ * As above where we did the up-ref, we don't call ossl_provider_free
+ * to avoid making upcalls. There should always be at least one ref
+ * to the provider in the store, so this should never drop to 0.
+ */
+ CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
+ /*
+ * Not much we can do if this assert ever fails. So we don't use
+ * ossl_assert here.
+ */
+ assert(ref > 0);
}
sk_OSSL_PROVIDER_free(provs);
return ret;
@@ -1562,19 +1687,25 @@ static int ossl_provider_register_child_cb(const OSSL_CORE_HANDLE *handle,
}
max = sk_OSSL_PROVIDER_num(store->providers);
for (i = 0; i < max; i++) {
+ int activated;
+
prov = sk_OSSL_PROVIDER_value(store->providers, i);
if (!CRYPTO_THREAD_read_lock(prov->flag_lock))
break;
+ activated = prov->flag_activated;
+ CRYPTO_THREAD_unlock(prov->flag_lock);
/*
- * We hold the lock while calling the user callback. This means that the
- * user callback must be short and simple and not do anything likely to
- * cause a deadlock.
+ * We hold the store lock while calling the user callback. This means
+ * that the user callback must be short and simple and not do anything
+ * likely to cause a deadlock. We don't hold the flag_lock during this
+ * call. In theory this means that another thread could deactivate it
+ * while we are calling create. This is ok because the other thread
+ * will also call remove_cb, but won't be able to do so until we release
+ * the store lock.
*/
- if (prov->flag_activated
- && !create_cb((OSSL_CORE_HANDLE *)prov, cbdata))
+ if (activated && !create_cb((OSSL_CORE_HANDLE *)prov, cbdata))
break;
- CRYPTO_THREAD_unlock(prov->flag_lock);
}
if (i == max) {
/* Success */
diff --git a/doc/man7/provider-base.pod b/doc/man7/provider-base.pod
index 92c167638b..ac197accca 100644
--- a/doc/man7/provider-base.pod
+++ b/doc/man7/provider-base.pod
@@ -123,7 +123,7 @@ provider-base
All "functions" mentioned here are passed as function pointers between
F<libcrypto> and the provider in B<OSSL_DISPATCH> arrays, in the call
of the provider initialization function. See L<provider(7)/Provider>
-for a description of the initialization function.
+for a description of the initialization function. They are known as "upcalls".
All these "functions" have a corresponding function type definition
named B<OSSL_FUNC_{name}_fn>, and a helper function to retrieve the
@@ -328,7 +328,9 @@ provider_register_child_cb() registers callbacks for being informed about the
loading and unloading of providers in the application's library context.
I<handle> is this provider's handle and I<cbdata> is this provider's data
that will be passed back to the callbacks. It returns 1 on success or 0
-otherwise.
+otherwise. These callbacks may be called while holding locks in libcrypto. In
+order to avoid deadlocks the callback implementation must not be long running
+and must not call other OpenSSL API functions or upcalls.
I<create_cb> is a callback that will be called when a new provider is loaded
into the application's library context. It is also called for any providers that
More information about the openssl-commits
mailing list