provider: update to structure based atomics

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21260)

1 files changed, 26 insertions, 20 deletions

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index ee5ce9b3a6..5a7f603037 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -75,8 +75,8 @@
  * The provider flag_lock: Used to control updates to the various provider
  * "flags" (flag_initialized and flag_activated).
  *
- * The provider refcnt_lock: Used to control updates to the provider refcnt and
- * activatecnt values.
+ * The provider activatecnt_lock: Used to control updates to the provider
+ * activatecnt value.
  *
  * The provider optbits_lock: Used to control access to the provider's
  * operation_bits and operation_bits_sz fields.
@@ -99,12 +99,12 @@
  * 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.
+ *    provider store lock, the provider activatecnt_lock and the provider flag_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
+ *        3) provider activatecnt_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
@@ -148,7 +148,7 @@ struct ossl_provider_st {
 
     /* OpenSSL library side data */
     CRYPTO_REF_COUNT refcnt;
-    CRYPTO_RWLOCK *refcnt_lock;  /* For the refcnt and activatecnt counters */
+    CRYPTO_RWLOCK *activatecnt_lock; /* For the activatecnt counter */
     int activatecnt;
     char *name;
     char *path;
@@ -442,16 +442,18 @@ static OSSL_PROVIDER *provider_new(const char *name,
 
     if ((prov = OPENSSL_zalloc(sizeof(*prov))) == NULL)
         return NULL;
+    if (!CRYPTO_NEW_REF(&prov->refcnt, 1)) {
+        ossl_provider_free(prov);
+        return NULL;
+    }
 #ifndef HAVE_ATOMICS
-    if ((prov->refcnt_lock = CRYPTO_THREAD_lock_new()) == NULL) {
-        OPENSSL_free(prov);
+    if ((prov->activatecnt_lock = CRYPTO_THREAD_lock_new()) == NULL) {
+        ossl_provider_free(prov);
         ERR_raise(ERR_LIB_CRYPTO, ERR_R_CRYPTO_LIB);
         return NULL;
     }
 #endif
 
-    prov->refcnt = 1; /* 1 One reference to be returned */
-
     if ((prov->opbits_lock = CRYPTO_THREAD_lock_new()) == NULL
         || (prov->flag_lock = CRYPTO_THREAD_lock_new()) == NULL
         || (prov->parameters = sk_INFOPAIR_deep_copy(parameters,
@@ -475,7 +477,7 @@ int ossl_provider_up_ref(OSSL_PROVIDER *prov)
 {
     int ref = 0;
 
-    if (CRYPTO_UP_REF(&prov->refcnt, &ref, prov->refcnt_lock) <= 0)
+    if (CRYPTO_UP_REF(&prov->refcnt, &ref) <= 0)
         return 0;
 
 #ifndef FIPS_MODULE
@@ -668,7 +670,7 @@ void ossl_provider_free(OSSL_PROVIDER *prov)
     if (prov != NULL) {
         int ref = 0;
 
-        CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
+        CRYPTO_DOWN_REF(&prov->refcnt, &ref);
 
         /*
          * When the refcount drops to zero, we clean up the provider.
@@ -711,8 +713,9 @@ void ossl_provider_free(OSSL_PROVIDER *prov)
             CRYPTO_THREAD_lock_free(prov->opbits_lock);
             CRYPTO_THREAD_lock_free(prov->flag_lock);
 #ifndef HAVE_ATOMICS
-            CRYPTO_THREAD_lock_free(prov->refcnt_lock);
+            CRYPTO_THREAD_lock_free(prov->activatecnt_lock);
 #endif
+            CRYPTO_FREE_REF(&prov->refcnt);
             OPENSSL_free(prov);
         }
 #ifndef FIPS_MODULE
@@ -1066,7 +1069,7 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls,
         return -1;
     }
 
-    CRYPTO_atomic_add(&prov->activatecnt, -1, &count, prov->refcnt_lock);
+    CRYPTO_atomic_add(&prov->activatecnt, -1, &count, prov->activatecnt_lock);
 #ifndef FIPS_MODULE
     if (count >= 1 && prov->ischild && upcalls) {
         /*
@@ -1152,7 +1155,7 @@ 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)) {
+    if (CRYPTO_atomic_add(&prov->activatecnt, 1, &count, prov->activatecnt_lock)) {
         prov->flag_activated = 1;
 
         if (count == 1 && store != NULL) {
@@ -1399,7 +1402,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
              * 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) {
+            if (CRYPTO_UP_REF(&prov->refcnt, &ref) <= 0) {
                 CRYPTO_THREAD_unlock(prov->flag_lock);
                 goto err_unlock;
             }
@@ -1409,8 +1412,9 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
              * In theory this could mean the parent provider goes inactive,
              * whilst still activated in the child for a short period. That's ok.
              */
-            if (!CRYPTO_atomic_add(&prov->activatecnt, 1, &ref, prov->refcnt_lock)) {
-                CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock);
+            if (!CRYPTO_atomic_add(&prov->activatecnt, 1, &ref,
+                                   prov->activatecnt_lock)) {
+                CRYPTO_DOWN_REF(&prov->refcnt, &ref);
                 CRYPTO_THREAD_unlock(prov->flag_lock);
                 goto err_unlock;
             }
@@ -1449,7 +1453,8 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
     for (curr++; curr < max; curr++) {
         OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
 
-        if (!CRYPTO_atomic_add(&prov->activatecnt, -1, &ref, prov->refcnt_lock)) {
+        if (!CRYPTO_atomic_add(&prov->activatecnt, -1, &ref,
+                               prov->activatecnt_lock)) {
             ret = 0;
             continue;
         }
@@ -1459,7 +1464,8 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
              * 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))
+            if (CRYPTO_atomic_add(&prov->activatecnt, 1, &ref,
+                                  prov->activatecnt_lock))
                 provider_deactivate(prov, 0, 1);
             else
                 ret = 0;
@@ -1469,7 +1475,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
          * 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.
          */
-        if (!CRYPTO_DOWN_REF(&prov->refcnt, &ref, prov->refcnt_lock)) {
+        if (!CRYPTO_DOWN_REF(&prov->refcnt, &ref)) {
             ret = 0;
             continue;
         }