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)
* The locks available are:
*
* The provider flag_lock: Used to control updates to the various provider
* 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.
*
* The provider optbits_lock: Used to control access to the provider's
* operation_bits and operation_bits_sz fields.
/* OpenSSL library side data */
CRYPTO_REF_COUNT refcnt;
/* 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;
int activatecnt;
char *name;
char *path;
+ CRYPTO_atomic_add(&prov->activatecnt, -1, &count, prov->refcnt_lock);
- 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
/*
* 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
- if ((count = --prov->activatecnt) < 1)
prov->flag_activated = 0;
#ifndef FIPS_MODULE
else
prov->flag_activated = 0;
#ifndef FIPS_MODULE
else
+ 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);
}
if (lock) {
CRYPTO_THREAD_unlock(prov->flag_lock);
for (curr = max - 1; curr >= 0; curr--) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
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) {
/*
goto err_unlock;
if (prov->flag_activated) {
/*
/*
* It's already activated, but we up the activated count to ensure
* it remains activated until after we've called the user callback.
/*
* 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;
CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
CRYPTO_THREAD_unlock(prov->flag_lock);
goto err_unlock;
for (curr++; curr < max; curr++) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
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.
*/
/*
* 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.
/*
* Not much we can do if this assert ever fails. So we don't use
* ossl_assert here.