Relax unnecessary atomic reads in FIPS provider

Replace calls to ossl_get_self_test_state() with direct access to
st_all_tests[].state in the FIPS self-test code.

Atomic reads are unnecessary in functions like FIPS_kat_deferred()
and SELF_TEST_kats_execute() because they are executed with the
relevant lock already held.

For ossl_deferred_self_test(), removing the atomic read avoids
contention. The common case is that tests are already passed. If a
race occurs, the function safely falls back to the locked path in
FIPS_kat_deferred() which re-verifies the state.

Signed-off-by: Simo Sorce <simo@redhat.com>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/30009)

diff --git a/providers/fips/fipsprov.c b/providers/fips/fipsprov.c
index 3c519771837e0bd4afe639da735fb81004b71efa..1ba68d6180968ea4e30555e9865bf02830e9ddfe 100644 (file)
--- a/providers/fips/fipsprov.c
+++ b/providers/fips/fipsprov.c
@@ -1377,7 +1377,6 @@ static int FIPS_kat_deferred(OSSL_LIB_CTX *libctx, self_test_id_t id)
         bool unset_key = false;
         OSSL_CALLBACK *cb = NULL;
         void *cb_arg = NULL;
-        enum st_test_state state;
 
         /*
          * check again as another thread may have just performed this
@@ -1385,10 +1384,11 @@ static int FIPS_kat_deferred(OSSL_LIB_CTX *libctx, self_test_id_t id)
          * NOTE: SELF_TEST_STATE_INIT is not a vald state here,
          * deferred testing is only valid when SELF_TEST_post
          * marks tests with SELF_TEST_STATE_DEFER, under lock.
+         *
+         * NOTE: we do not need an atomic read, because writes are
+         * guaranteed to happen only with the deferred_lock held
          */
-        if (!ossl_get_self_test_state(id, &state))
-            goto done;
-        switch (state) {
+        switch (st_all_tests[id].state) {
         case SELF_TEST_STATE_DEFER:
             break;
         case SELF_TEST_STATE_PASSED:
@@ -1471,21 +1471,20 @@ int ossl_deferred_self_test(OSSL_LIB_CTX *libctx, self_test_id_t id)
         return 0;
     }
 
-    /* return immediately if the test is marked as passed */
-    if (!ossl_get_self_test_state(id, &state)) {
-        ossl_set_error_state(NULL);
-        return 0;
-    }
-    if (state == SELF_TEST_STATE_PASSED)
-        return 1;
-
     /*
-     * NOTE: that the order in which we check the 'state' here is not important,
-     * if multiple threads are racing to check it the worst case scenario is
-     * that they will all try to run the tests. Proper locking for preventing
-     * concurrent tests runs and saving state from multiple threads is handled
-     * in FIPS_kat_deferred() so this race is of no real consequence.
+     * Return immediately if the test is marked as passed.
+     *
+     * NOTE: This would normally call for an atomic read, however we want
+     * to avoid contention in the general case where the test is always in
+     * PASSED state. This is true 100% of the time when tests are not deferred,
+     * and true 99% of the time when tests are deferred. For the remaining 1% of
+     * the time, if we race and do not read a PASSED value, the worst case is
+     * that this function continues until it obtains a lock in FIPS_deferred()
+     * and then it will recheck this value and immediately exit.
      */
+    if (st_all_tests[id].state == SELF_TEST_STATE_PASSED)
+        return 1;
+
     ret = FIPS_kat_deferred(libctx, id);
     if (!ossl_get_self_test_state(id, &state)) {
         ossl_set_error_state(NULL);

diff --git a/providers/fips/self_test.c b/providers/fips/self_test.c
index 734db2941efd58df79a2353192b3a51a4167f7f5..46f127da005493000f35129445ffe5e31770d8d4 100644 (file)
--- a/providers/fips/self_test.c
+++ b/providers/fips/self_test.c
@@ -362,12 +362,7 @@ int SELF_TEST_post(SELF_TEST_POST_PARAMS *st, void *fips_global,
             && strcmp(st->defer_tests, "1") == 0) {
             /* Mark all non executed tests as deferred */
             for (int i = 0; i < ST_ID_MAX; i++) {
-                enum st_test_state state;
-                if (!ossl_get_self_test_state(i, &state)) {
-                    errored = 1;
-                    goto locked_end;
-                }
-                if (state == SELF_TEST_STATE_INIT) {
+                if (st_all_tests[i].state == SELF_TEST_STATE_INIT) {
                     if (!ossl_set_self_test_state(i, SELF_TEST_STATE_DEFER)) {
                         errored = 1;
                         goto locked_end;

diff --git a/providers/fips/self_test_kats.c b/providers/fips/self_test_kats.c
index 61f2c9fd60b26bc6cf6c7320ad1277dc053e8d51..6b9515b24eedbf4cb7c7ee317c2c175f775013ef 100644 (file)
--- a/providers/fips/self_test_kats.c
+++ b/providers/fips/self_test_kats.c
@@ -1163,7 +1163,6 @@ int SELF_TEST_kats_execute(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx,
     self_test_id_t id, int switch_rand)
 {
     EVP_RAND_CTX *saved_rand = NULL;
-    enum st_test_state state;
     int ret;
 
     if (id >= ST_ID_MAX || st_all_tests[id].id != id) {
@@ -1174,10 +1173,12 @@ int SELF_TEST_kats_execute(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx,
     /*
      * Dependency chains may cause a test to be referenced multiple times,
      * immediately return if not in initial state.
+     * NOTE: In this function state can be read w/o atomics because this
+     * function is always executed under lock. However we need to use
+     * atomics to set the state so that other threads reading state always
+     * read a correct value.
      */
-    if (!ossl_get_self_test_state(id, &state))
-        return 0;
-    switch (state) {
+    switch (st_all_tests[id].state) {
     case SELF_TEST_STATE_INIT:
     case SELF_TEST_STATE_DEFER:
         break;
@@ -1217,9 +1218,7 @@ int SELF_TEST_kats_execute(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx,
     }
 
     /* may have already been run through dependency chains */
-    if (!ossl_get_self_test_state(id, &state))
-        return 0;
-    switch (state) {
+    switch (st_all_tests[id].state) {
     case SELF_TEST_STATE_IN_PROGRESS:
         ret = SELF_TEST_kats_single(st, libctx, id);
         break;
@@ -1237,14 +1236,9 @@ int SELF_TEST_kats_execute(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx,
      * ensure they are all executed as well otherwise we could not
      * mark it as passed.
      */
-    if (!ossl_get_self_test_state(id, &state))
-        return 0;
-    if (state == SELF_TEST_STATE_PASSED)
+    if (st_all_tests[id].state == SELF_TEST_STATE_PASSED)
         for (int i = 0; i < ST_ID_MAX; i++) {
-            enum st_test_state istate;
-            if (!ossl_get_self_test_state(i, &istate))
-                return 0;
-            if (istate == SELF_TEST_STATE_IMPLICIT
+            if (st_all_tests[i].state == SELF_TEST_STATE_IMPLICIT
                 && st_all_tests[i].depends_on != NULL)
                 if (!(ret = SELF_TEST_kat_deps(st, libctx, &st_all_tests[i])))
                     break;
@@ -1255,14 +1249,9 @@ done:
      * now mark (pass or fail) all the algorithm tests that have been marked
      * by this test implicitly tested.
      */
-    if (!ossl_get_self_test_state(id, &state))
-        return 0;
     for (int i = 0; i < ST_ID_MAX; i++) {
-        enum st_test_state istate;
-        if (!ossl_get_self_test_state(i, &istate))
-            return 0;
-        if (istate == SELF_TEST_STATE_IMPLICIT)
-            ossl_set_self_test_state(i, state);
+        if (st_all_tests[i].state == SELF_TEST_STATE_IMPLICIT)
+            ossl_set_self_test_state(i, st_all_tests[id].state);
     }
 
     if (switch_rand) {
@@ -1297,10 +1286,7 @@ int SELF_TEST_kats(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx)
     }
 
     for (i = 0; i < ST_ID_MAX; i++) {
-        enum st_test_state state;
-        if (!ossl_get_self_test_state(i, &state))
-            return 0;
-        if (state == SELF_TEST_STATE_INIT)
+        if (st_all_tests[i].state == SELF_TEST_STATE_INIT)
             if (!SELF_TEST_kats_execute(st, libctx, i, 0))
                 ret = 0;
     }