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)

1 files changed, 35 insertions, 20 deletions

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index 60a1efedfb..ee5ce9b3a6 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -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.