Improve EVP_sha, EVP_aes calls when using OpenSSL3

[baentsch]

I'm aware of the following antipattern - in 3.0. using old methods like EVP_sha3_256() instead of fetching a digest object via EVP_MD_fetch and using a fetched object gives a performance penalty. See https://github.com/openssl/openssl/pull/20354/files#diff-86fbd44f96c9f78b9c4a27d8b5fb9d83aa787b9a7d92c6774d9172c5952b791f for example. If it is the case (and looks like it is), it's worth fixing

Originally posted by @beldmit in https://github.com/open-quantum-safe/liboqs/issues/1426#issuecomment-1494591453

[baentsch]

Reference to AES added as per https://github.com/open-quantum-safe/liboqs/issues/1428#issuecomment-1496135182

[baentsch]

@beldmit Are you aware of how much performance this costs? If I get it right, it would mean adding "state" to (re)using Digests and AES (to stop OpenSSL re-searching and re-creating the corresponding EVP objects), right? Somewhat along the lines of what we already do for XKCP.

[beldmit]

Right,it just eliminates search/recreate. Didn't measure it myself, but it's a relatively expensive operation

[baentsch]

it's a relatively expensive operation

I can't help it, but the scientist in me wants numbers. So here we go:

If this is only due to the difference in the way algorithms are fetched, this indeed is a bummer (aka an issue worth while tackling).

[beldmit]

Speaking frankly, I'd run a test under valgrind --callgrind in some more or less realistic scenario.

[baentsch]

run a test under valgrind --callgrind in some more or less realistic scenario.

ACK. Just wanted to quickly get beyond

Didn't measure it

Contributions very welcome. Or are you indeed not at liberty to contribute?

[beldmit]

I don't have any problems with contribution beyond the capacity ones :).

[beldmit]

Is there a initializing and deinitializing function to place fetching in?

[baentsch]

AFAIK only for the ..._inc_... variants. I was just about to do a KISS version (not thread safe) using static initializers to see whether this really brings the >20% it looks like...

[beldmit]

I can draft a KISS version, I think

[baentsch]

I can draft a KISS version, I think

That'd be very welcome! When I did just this quick hack, no significant runtime change materialized in speed_common (run linked against OpenSSL3.2.0-dev), but maybe I misunderstood what'd be required (?):

static EVP_MD *oqs_sha3_256 = NULL;
void OQS_SHA3_sha3_256(uint8_t *output, const uint8_t *input, size_t inplen) {
        if (!oqs_sha3_256) oqs_sha3_256 = EVP_sha3_256();
        do_hash(output, input, inplen, oqs_sha3_256);
        //do_hash(output, input, inplen, EVP_sha3_256());

[beldmit]

original: 194 passed, 484 skipped in 742.97s (0:12:22) patched: 1 failed, 193 passed, 484 skipped in 430.60s (0:07:10)

The failed test is code_style:

. > tests/run_astyle.sh
Formatted  src/common/sha2/sha2_ossl.c
Formatted  src/common/sha3/ossl_sha3.c
Formatted  src/common/sha3/ossl_sha3x4.c
Formatted  src/common/aes/aes_ossl.c
Error: Some files need reformatting. Check output above.

strawman_fetch.txt

[beldmit]

It's a quite ad-hoc patch without any attempt to optimize and free fetched objects.

[beldmit]

The test is done against OpenSSL 3.0.8 on F37 laptop

[baentsch]

Impressive improvements visible also on my machine: Thanks! Do you want (have time) to make this into a PR?

[beldmit]

Sure. But again, I don't understand how to properly format my changes and how to properly initialize and free the variables.

[baentsch]

Yikes -- now I understand it: Calling EVP_sha3_256 really creates an EVP_MD object that royally sucks (and keeps doing so as it's used again and again). Just changing this to EVP_md_fetch is all that's needed. Amazing (that this is hidden underneath the "old" API).

Out of curiousity: What spoke then against implementing (in OpenSSL3)

EVP_MD *EVP_sha3_256() { return EVP_MD_fetch(NULL, "SHA3-256", NULL); }

??

Sure. But again, I don't understand how to properly format my changes and how to properly initialize and free the variables.

Formatting: Just run astyle --options=.astylerc src/common/sha3/ossl_sha3.c on the new files.

Initialize/free: Well, that'll require the addition of new functions to the API I'd gather. But maybe someone else reading along has a better idea.

[beldmit]

Out of curiousity: What spoke then against implementing (in OpenSSL3)

EVP_MD *EVP_sha3_256() { return EVP_MD_fetch(NULL, "SHA3-256", NULL); }

??

No idea. Probably it's worth asking upstream this question. The only version I have that currently old method are detected compile-time, and the change you suggest makes necessary run-time checks if the object is fetched.

[dstebila]

Is this resolved by #1431?

[beldmit]

I think yes.