From: Simo Sorce Date: Mon, 16 Feb 2026 17:37:36 +0000 (-0500) Subject: Relax unnecessary atomic reads in FIPS provider X-Git-Tag: openssl-4.0.0-alpha1~279 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=91bdbe19837446663962852aed39e41284b71197;p=thirdparty%2Fopenssl.git 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 Reviewed-by: Neil Horman Reviewed-by: Shane Lontis Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/30009) --- diff --git a/providers/fips/fipsprov.c b/providers/fips/fipsprov.c index 3c519771837..1ba68d61809 100644 --- 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 734db2941ef..46f127da005 100644 --- 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 61f2c9fd60b..6b9515b24ee 100644 --- 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; }