openssl
/
openssl
mirror of https://github.com/openssl/openssl
1
0
Fork 0
Code Issues Releases Wiki Activity

Avoid taking a write lock in ossl_provider_doall_activated() 

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

55
crypto/provider_core.c
View File

@ -73,11 +73,10 @@
* The locks available are:
*
* The provider flag_lock: Used to control updates to the various provider
* "flags" (flag_initialized and flag_activated) and associated
* "counts" (activatecnt).
* "flags" (flag_initialized and flag_activated).
*
* The provider refcnt_lock: Only ever used to control updates to the provider
* refcnt value.
* The provider refcnt_lock: Used to control updates to the provider refcnt and
* activatecnt values.
*
* The provider optbits_lock: Used to control access to the provider's
* operation_bits and operation_bits_sz fields.
@ -149,7 +148,7 @@ struct ossl_provider_st {
/* OpenSSL library side data */
CRYPTO_REF_COUNT refcnt;
CRYPTO_RWLOCK *refcnt_lock; /* For the ref counter */
CRYPTO_RWLOCK *refcnt_lock; /* For the refcnt and activatecnt counters */
int activatecnt;
char *name;
char *path;
@ -1067,8 +1066,9 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls,
return -1;
}
CRYPTO_atomic_add(&prov->activatecnt, -1, &count, prov->refcnt_lock);
#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
* 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
if ((count = --prov->activatecnt) < 1)
if (count < 1)
prov->flag_activated = 0;
#ifndef FIPS_MODULE
else
@ -1152,12 +1152,12 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
#endif
return -1;
}
if (CRYPTO_atomic_add(&prov->activatecnt, 1, &count, prov->refcnt_lock)) {
prov->flag_activated = 1;
count = ++prov->activatecnt;
prov->flag_activated = 1;
if (prov->activatecnt == 1 && store != NULL) {
ret = create_provider_children(prov);
if (count == 1 && store != NULL) {
ret = create_provider_children(prov);
}
}
if (lock) {
CRYPTO_THREAD_unlock(prov->flag_lock);
@ -1391,7 +1391,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
for (curr = max - 1; curr >= 0; 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;
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 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.
* 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, 0) < 0) {
if (!CRYPTO_atomic_add(&prov->activatecnt, 1, &ref, prov->refcnt_lock)) {
CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
CRYPTO_THREAD_unlock(prov->flag_lock);
goto err_unlock;
@ -1451,13 +1449,30 @@ 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, 0, 1);
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);
else
ret = 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);
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
* ossl_assert here.