mirror/openssl
2
0
Fork 0
mirror of https://github.com/openssl/openssl.git synced 2025-02-23 14:42:15 +08:00
Code Issues Packages Projects Releases Wiki Activity

Avoid taking a write lock in ossl_provider_doall_activated()

Browse Source 
We refactor ossl_provider_doall_activated() so that we only need to take
a read lock instead of a write lock for the flag_lock. This should improve
performance by avoiding the lock contention. We achieve this by protecting
the activatecnt via atomics rather than via a lock and by avoiding the full
provider activation/deactivation procedure where it is not needed.

Partial fix for #20286

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20927)
This commit is contained in:
Matt Caswell 2023-05-10 12:26:56 +01:00 committed by Pauli
parent 61f11cad7a
commit fc570b2605
1 changed files with 35 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
crypto/provider_core.c
View File

@ -73,11 +73,10 @@
* The locks available are: * The locks available are:
* *
* The provider flag_lock: Used to control updates to the various provider * The provider flag_lock: Used to control updates to the various provider
* "flags" (flag_initialized and flag_activated) and associated * "flags" (flag_initialized and flag_activated).
* "counts" (activatecnt).
* *
* The provider refcnt_lock: Only ever used to control updates to the provider * The provider refcnt_lock: Used to control updates to the provider refcnt and
* refcnt value. * activatecnt values.
* *
* The provider optbits_lock: Used to control access to the provider's * The provider optbits_lock: Used to control access to the provider's
* operation_bits and operation_bits_sz fields. * operation_bits and operation_bits_sz fields.
@ -149,7 +148,7 @@ struct ossl_provider_st {
/* OpenSSL library side data */ /* OpenSSL library side data */
CRYPTO_REF_COUNT refcnt; CRYPTO_REF_COUNT refcnt;
CRYPTO_RWLOCK *refcnt_lock; /* For the ref counter */ CRYPTO_RWLOCK *refcnt_lock; /* For the refcnt and activatecnt counters */
int activatecnt; int activatecnt;
char *name; char *name;
char *path; char *path;
@ -1067,8 +1066,9 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls,
return -1; return -1;
} }
CRYPTO_atomic_add(&prov->activatecnt, -1, &count, prov->refcnt_lock);
#ifndef FIPS_MODULE #ifndef FIPS_MODULE
if (prov->activatecnt >= 2 && prov->ischild && upcalls) { if (count >= 1 && prov->ischild && upcalls) {
/* /*
* We have had a direct activation in this child libctx so we need to * We have had a direct activation in this child libctx so we need to
* now down the ref count in the parent provider. We do the actual down * now down the ref count in the parent provider. We do the actual down
@ -1079,7 +1079,7 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls,
} }
#endif #endif
if ((count = --prov->activatecnt) < 1) if (count < 1)
prov->flag_activated = 0; prov->flag_activated = 0;
#ifndef FIPS_MODULE #ifndef FIPS_MODULE
else else
@ -1152,13 +1152,13 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
#endif #endif
return -1; return -1;
} }
if (CRYPTO_atomic_add(&prov->activatecnt, 1, &count, prov->refcnt_lock)) {
count = ++prov->activatecnt;
prov->flag_activated = 1; prov->flag_activated = 1;
if (prov->activatecnt == 1 && store != NULL) { if (count == 1 && store != NULL) {
ret = create_provider_children(prov); ret = create_provider_children(prov);
} }
}
if (lock) { if (lock) {
CRYPTO_THREAD_unlock(prov->flag_lock); CRYPTO_THREAD_unlock(prov->flag_lock);
CRYPTO_THREAD_unlock(store->lock); CRYPTO_THREAD_unlock(store->lock);
@ -1391,7 +1391,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
for (curr = max - 1; curr >= 0; curr--) { for (curr = max - 1; curr >= 0; curr--) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) if (!CRYPTO_THREAD_read_lock(prov->flag_lock))
goto err_unlock; goto err_unlock;
if (prov->flag_activated) { if (prov->flag_activated) {
/* /*
@ -1406,12 +1406,10 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
/* /*
* It's already activated, but we up the activated count to ensure * It's already activated, but we up the activated count to ensure
* it remains activated until after we've called the user callback. * it remains activated until after we've called the user callback.
* We do this with no locking (because we already hold the locks) * In theory this could mean the parent provider goes inactive,
* and no upcalls (which must not be called when locks are held). In * whilst still activated in the child for a short period. That's ok.
* 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, 0) < 0) { if (!CRYPTO_atomic_add(&prov->activatecnt, 1, &ref, prov->refcnt_lock)) {
CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock); CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
CRYPTO_THREAD_unlock(prov->flag_lock); CRYPTO_THREAD_unlock(prov->flag_lock);
goto err_unlock; goto err_unlock;
@ -1451,13 +1449,30 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
for (curr++; curr < max; curr++) { for (curr++; curr < max; curr++) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
if (!CRYPTO_atomic_add(&prov->activatecnt, -1, &ref, prov->refcnt_lock)) {
ret = 0;
continue;
}
if (ref < 1) {
/*
* Looks like we need to deactivate properly. We could just have
* done this originally, but it involves taking a write lock so
* we avoid it. We up the count again and do a full deactivation
*/
if (CRYPTO_atomic_add(&prov->activatecnt, 1, &ref, prov->refcnt_lock))
provider_deactivate(prov, 0, 1); provider_deactivate(prov, 0, 1);
else
ret = 0;
}
/* /*
* As above where we did the up-ref, we don't call ossl_provider_free * 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 avoid making upcalls. There should always be at least one ref
* to the provider in the store, so this should never drop to 0. * to the provider in the store, so this should never drop to 0.
*/ */
CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock); if (!CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock)) {
ret = 0;
continue;
}
/* /*
* Not much we can do if this assert ever fails. So we don't use * Not much we can do if this assert ever fails. So we don't use
* ossl_assert here. * ossl_assert here.