On 5/7/22 00:55, Ilya Maximets wrote: > Implementation of SHA1 in OpenSSL library is much faster and optimized > for all available CPU architectures and instruction sets. OVS should > use it instead of internal implementation if possible. > > Depending on compiler options OpenSSL's version finishes our sha1 > unit tests from 3 to 12 times faster. Performance of OpenSSL's > version is constant, but OVS's implementation highly depends on > compiler. Interestingly, default build with '-g -O2' works faster > than optimized '-march=native -Ofast'. > > Tests with ovsdb-server on big databases shows ~5-10% improvement of > the time needed for database compaction (sha1 is only a part of this > operation), depending on compiler options. > > We still need internal implementation, because OpenSSL can be not > available on some platforms. Tests enhanced to check both versions > of API. > > Signed-off-by: Ilya Maximets <[email protected]> > ---
Hi Ilya, I'm no openssl expert so this is a rather shallow review. In any case, the change looks good to me overall. I left a question below and a minor nit comment. Regardless: Reviewed-by: Dumitru Ceara <[email protected]> Thanks, Dumitru > > Version 2: > - Deprecated low level SHA1_* API replaced with EVP_Digest* API. > - Added more accurate logging of OpenSSL errors. > - Stack-based allocation of the digest context is no longer possible > in modern versions of OpenSSL, so converted to supported dynamic > allocation with EVP_MD_CTX_create(). Technically, this function > was also renamed to EVP_MD_CTX_new() in OpenSSL 3.0, but the old > name still works. > > lib/sha1.c | 166 +++++++++++++++++++++++++++++++++++----------- > lib/sha1.h | 27 ++++++-- > ovsdb/log.c | 1 + > tests/library.at | 2 +- > tests/test-sha1.c | 60 +++++++++++------ > 5 files changed, 193 insertions(+), 63 deletions(-) > > diff --git a/lib/sha1.c b/lib/sha1.c > index 87360d9cd..3ecc8fb9f 100644 > --- a/lib/sha1.c > +++ b/lib/sha1.c > @@ -31,12 +31,131 @@ > > #include <config.h> > #include "sha1.h" > + > +#ifdef HAVE_OPENSSL > +#include <openssl/err.h> > +#include <openssl/evp.h> > +#endif > + > #include <ctype.h> > #include <string.h> > #include "compiler.h" > +#include "openvswitch/vlog.h" > #include "util.h" > > -/* a bit faster & bigger, if defined */ > +VLOG_DEFINE_THIS_MODULE(sha1); > + > +#ifdef HAVE_OPENSSL > +static void > +log_openssl_err(const char *func) > +{ > + char buf[1024]; > + > + ERR_error_string_n(ERR_get_error(), buf, 1024); > + VLOG_FATAL("%s failed: %s", func, buf); > +} > +#endif > + > +/* > + * Initialize the SHA digest. > + * context: The SHA context to initialize > + */ > +void > +sha1_init(struct sha1_ctx *sha_info) > +{ > +#ifdef HAVE_OPENSSL > + sha_info->ctx = EVP_MD_CTX_create(); > + if (!EVP_DigestInit_ex(sha_info->ctx, EVP_sha1(), NULL)) { > + log_openssl_err("EVP_DigestInit_ex"); > + } > +#else > + ovs_sha1_init(sha_info); > +#endif > +} > + > +/* > + * Update the SHA digest. > + * context: The SHA1 context to update. > + * input: The buffer to add to the SHA digest. > + * inputLen: The length of the input buffer. > + */ > +void > +sha1_update(struct sha1_ctx *ctx, const void *buffer_, uint32_t count) > +{ > +#ifdef HAVE_OPENSSL > + if (!EVP_DigestUpdate(ctx->ctx, buffer_, count)) { > + log_openssl_err("EVP_DigestUpdate"); > + } > +#else > + ovs_sha1_update(ctx, buffer_, count); > +#endif > +} > + > +/* > + * Finish computing the SHA digest. > + * digest: the output buffer in which to store the digest. > + * context: The context to finalize. > + */ > +void > +sha1_final(struct sha1_ctx *ctx, uint8_t digest[SHA1_DIGEST_SIZE]) > +{ > +#ifdef HAVE_OPENSSL > + unsigned int len; > + > + if (!EVP_DigestFinal_ex(ctx->ctx, digest, &len)) { According to the openssl man page, "at most EVP_MAX_MD_SIZE bytes will be written". Will there be ever be a case when less than SHA1_DIGEST_SIZE bytes are written causing the assert below to fail? Also, will there ever be a case when more than SHA1_DIGEST_SIZE are written? Should we ensure that all callers of sha1_final pass a digest that's at least EVP_MAX_MD_SIZE bytes long? > + log_openssl_err("EVP_DigestFinal_ex"); > + } > + ovs_assert(len == SHA1_DIGEST_SIZE); > + EVP_MD_CTX_destroy(ctx->ctx); > +#else > + ovs_sha1_final(ctx, digest); > +#endif > +} > + > +/* Computes the hash of 'n' bytes in 'data' into 'digest'. */ > +void > +sha1_bytes(const void *data, uint32_t n, uint8_t digest[SHA1_DIGEST_SIZE]) > +{ > + struct sha1_ctx ctx; > + > + sha1_init(&ctx); > + sha1_update(&ctx, data, n); > + sha1_final(&ctx, digest); > +} > + > +void > +sha1_to_hex(const uint8_t digest[SHA1_DIGEST_SIZE], > + char hex[SHA1_HEX_DIGEST_LEN + 1]) > +{ > + int i; > + > + for (i = 0; i < SHA1_DIGEST_SIZE; i++) { > + *hex++ = "0123456789abcdef"[digest[i] >> 4]; > + *hex++ = "0123456789abcdef"[digest[i] & 15]; > + } > + *hex = '\0'; > +} > + > +bool > +sha1_from_hex(uint8_t digest[SHA1_DIGEST_SIZE], const char *hex) > +{ > + int i; > + > + for (i = 0; i < SHA1_DIGEST_SIZE; i++) { > + bool ok; > + > + digest[i] = hexits_value(hex, 2, &ok); > + if (!ok) { > + return false; > + } > + hex += 2; > + } > + return true; > +} > + > +/* Generic implementation for a case where OpenSSL is not available. */ Nit: s/for a case/for the case/ > + > +/* A bit faster & bigger, if defined */ > #define UNROLL_LOOPS > > /* SHA f()-functions */ > @@ -178,7 +297,7 @@ maybe_byte_reverse(uint32_t *buffer OVS_UNUSED, int count > OVS_UNUSED) > * context: The SHA context to initialize > */ > void > -sha1_init(struct sha1_ctx *sha_info) > +ovs_sha1_init(struct sha1_ctx *sha_info) > { > sha_info->digest[0] = 0x67452301L; > sha_info->digest[1] = 0xefcdab89L; > @@ -197,7 +316,7 @@ sha1_init(struct sha1_ctx *sha_info) > * inputLen: The length of the input buffer. > */ > void > -sha1_update(struct sha1_ctx *ctx, const void *buffer_, uint32_t count) > +ovs_sha1_update(struct sha1_ctx *ctx, const void *buffer_, uint32_t count) > { > const uint8_t *buffer = buffer_; > unsigned int i; > @@ -240,7 +359,7 @@ sha1_update(struct sha1_ctx *ctx, const void *buffer_, > uint32_t count) > * context: The context to finalize. > */ > void > -sha1_final(struct sha1_ctx *ctx, uint8_t digest[SHA1_DIGEST_SIZE]) > +ovs_sha1_final(struct sha1_ctx *ctx, uint8_t digest[SHA1_DIGEST_SIZE]) > { > int count, i, j; > uint32_t lo_bit_count, hi_bit_count, k; > @@ -274,42 +393,11 @@ sha1_final(struct sha1_ctx *ctx, uint8_t > digest[SHA1_DIGEST_SIZE]) > > /* Computes the hash of 'n' bytes in 'data' into 'digest'. */ > void > -sha1_bytes(const void *data, uint32_t n, uint8_t digest[SHA1_DIGEST_SIZE]) > +ovs_sha1_bytes(const void *data, uint32_t n, uint8_t > digest[SHA1_DIGEST_SIZE]) > { > struct sha1_ctx ctx; > > - sha1_init(&ctx); > - sha1_update(&ctx, data, n); > - sha1_final(&ctx, digest); > -} > - > -void > -sha1_to_hex(const uint8_t digest[SHA1_DIGEST_SIZE], > - char hex[SHA1_HEX_DIGEST_LEN + 1]) > -{ > - int i; > - > - for (i = 0; i < SHA1_DIGEST_SIZE; i++) { > - *hex++ = "0123456789abcdef"[digest[i] >> 4]; > - *hex++ = "0123456789abcdef"[digest[i] & 15]; > - } > - *hex = '\0'; > + ovs_sha1_init(&ctx); > + ovs_sha1_update(&ctx, data, n); > + ovs_sha1_final(&ctx, digest); > } > - > -bool > -sha1_from_hex(uint8_t digest[SHA1_DIGEST_SIZE], const char *hex) > -{ > - int i; > - > - for (i = 0; i < SHA1_DIGEST_SIZE; i++) { > - bool ok; > - > - digest[i] = hexits_value(hex, 2, &ok); > - if (!ok) { > - return false; > - } > - hex += 2; > - } > - return true; > -} > - > diff --git a/lib/sha1.h b/lib/sha1.h > index a635ff768..710e5751c 100644 > --- a/lib/sha1.h > +++ b/lib/sha1.h > @@ -33,15 +33,26 @@ > #include <stddef.h> > #include <stdint.h> > > +#ifdef HAVE_OPENSSL > +#include <openssl/evp.h> > +#endif > + > #define SHA1_DIGEST_SIZE 20 /* Size of the SHA1 digest. */ > #define SHA1_HEX_DIGEST_LEN 40 /* Length of SHA1 digest as hex in ASCII. */ > > /* SHA1 context structure. */ > struct sha1_ctx { > - uint32_t digest[5]; /* Message digest. */ > - uint32_t count_lo, count_hi; /* 64-bit bit counts. */ > - uint32_t data[16]; /* SHA data buffer */ > - int local; /* Unprocessed amount in data. */ > + union { > +#ifdef HAVE_OPENSSL > + EVP_MD_CTX *ctx; /* OpenSSL context. */ > +#endif > + struct { > + uint32_t digest[5]; /* Message digest. */ > + uint32_t count_lo, count_hi; /* 64-bit bit counts. */ > + uint32_t data[16]; /* SHA data buffer */ > + int local; /* Unprocessed amount in data. */ > + }; > + }; > }; > > void sha1_init(struct sha1_ctx *); > @@ -63,4 +74,12 @@ void sha1_to_hex(const uint8_t digest[SHA1_DIGEST_SIZE], > char hex[SHA1_HEX_DIGEST_LEN + 1]); > bool sha1_from_hex(uint8_t digest[SHA1_DIGEST_SIZE], const char *hex); > > +/* Generic implementation for the case where OpenSSL is not available. > + * This API should not be used directly. Exposed for unit testing. */ > +void ovs_sha1_init(struct sha1_ctx *); > +void ovs_sha1_update(struct sha1_ctx *, const void *, uint32_t size); > +void ovs_sha1_final(struct sha1_ctx *, uint8_t digest[SHA1_DIGEST_SIZE]); > +void ovs_sha1_bytes(const void *, uint32_t size, > + uint8_t digest[SHA1_DIGEST_SIZE]); > + > #endif /* sha1.h */ > diff --git a/ovsdb/log.c b/ovsdb/log.c > index 4a28fa3db..e42f00246 100644 > --- a/ovsdb/log.c > +++ b/ovsdb/log.c > @@ -400,6 +400,7 @@ parse_body(struct ovsdb_log *file, off_t offset, unsigned > long int length, > > chunk = MIN(length, sizeof input); > if (fread(input, 1, chunk, file->stream) != chunk) { > + sha1_final(&ctx, sha1); > json_parser_abort(parser); > return ovsdb_io_error(ferror(file->stream) ? errno : EOF, > "%s: error reading %lu bytes " > diff --git a/tests/library.at b/tests/library.at > index db4997d8f..ab56f3672 100644 > --- a/tests/library.at > +++ b/tests/library.at > @@ -54,7 +54,7 @@ AT_CLEANUP > > AT_SETUP([SHA-1]) > AT_KEYWORDS([sha1]) > -AT_CHECK([ovstest test-sha1], [0], [.......... > +AT_CHECK([ovstest test-sha1], [0], [.................... > ]) > AT_CLEANUP > > diff --git a/tests/test-sha1.c b/tests/test-sha1.c > index cc80888a7..f5a310bc9 100644 > --- a/tests/test-sha1.c > +++ b/tests/test-sha1.c > @@ -32,6 +32,14 @@ struct test_vector { > const uint8_t output[20]; > }; > > +struct test_api { > + void (*sha1_init)(struct sha1_ctx *); > + void (*sha1_update)(struct sha1_ctx *, const void *, uint32_t size); > + void (*sha1_final)(struct sha1_ctx *, uint8_t digest[SHA1_DIGEST_SIZE]); > + void (*sha1_bytes)(const void *, uint32_t size, > + uint8_t digest[SHA1_DIGEST_SIZE]); > +}; > + > static const struct test_vector vectors[] = { > /* FIPS 180-1. */ > { > @@ -92,13 +100,13 @@ static const struct test_vector vectors[] = { > }; > > static void > -test_one(const struct test_vector *vec) > +test_one(const struct test_api *api, const struct test_vector *vec) > { > uint8_t md[SHA1_DIGEST_SIZE]; > int i; > > /* All at once. */ > - sha1_bytes(vec->data, vec->size, md); > + api->sha1_bytes(vec->data, vec->size, md); > assert(!memcmp(md, vec->output, SHA1_DIGEST_SIZE)); > > /* In two pieces. */ > @@ -107,10 +115,10 @@ test_one(const struct test_vector *vec) > int n1 = vec->size - n0; > struct sha1_ctx sha1; > > - sha1_init(&sha1); > - sha1_update(&sha1, vec->data, n0); > - sha1_update(&sha1, vec->data + n0, n1); > - sha1_final(&sha1, md); > + api->sha1_init(&sha1); > + api->sha1_update(&sha1, vec->data, n0); > + api->sha1_update(&sha1, vec->data + n0, n1); > + api->sha1_final(&sha1, md); > assert(!memcmp(md, vec->output, SHA1_DIGEST_SIZE)); > } > > @@ -119,7 +127,7 @@ test_one(const struct test_vector *vec) > } > > static void > -test_big_vector(void) > +test_big_vector(const struct test_api *api) > { > enum { SIZE = 1000000 }; > struct test_vector vec = { > @@ -133,12 +141,12 @@ test_big_vector(void) > for (i = 0; i < SIZE; i++) { > vec.data[i] = 'a'; > } > - test_one(&vec); > + test_one(api, &vec); > free(vec.data); > } > > static void > -test_huge_vector(void) > +test_huge_vector(const struct test_api *api) > { > enum { SIZE = 1000000000 }; > struct test_vector vec = { > @@ -159,13 +167,13 @@ test_huge_vector(void) > vec.data[i] = 'a'; > } > > - sha1_init(&sha1); > + api->sha1_init(&sha1); > for (sz = 0; sz < SIZE; sz += chunk) { > int n = sz + chunk < SIZE ? chunk : SIZE - sz; > > - sha1_update(&sha1, vec.data, n); > + api->sha1_update(&sha1, vec.data, n); > } > - sha1_final(&sha1, md); > + api->sha1_final(&sha1, md); > ovs_assert(!memcmp(md, vec.output, SHA1_DIGEST_SIZE)); > > free(vec.data); > @@ -176,15 +184,29 @@ test_huge_vector(void) > static void > test_shar1_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) > { > - int i; > - > - for (i = 0; i < ARRAY_SIZE(vectors); i++) { > - test_one(&vectors[i]); > - } > + struct test_api api[] = { > + { > + .sha1_init = sha1_init, > + .sha1_update = sha1_update, > + .sha1_final = sha1_final, > + .sha1_bytes = sha1_bytes, > + }, > + { > + .sha1_init = ovs_sha1_init, > + .sha1_update = ovs_sha1_update, > + .sha1_final = ovs_sha1_final, > + .sha1_bytes = ovs_sha1_bytes, > + }, > + }; > > - test_big_vector(); > - test_huge_vector(); > + for (int i = 0; i < ARRAY_SIZE(api); i++) { > + for (int j = 0; j < ARRAY_SIZE(vectors); j++) { > + test_one(&api[i], &vectors[j]); > + } > > + test_big_vector(&api[i]); > + test_huge_vector(&api[i]); > + } > putchar('\n'); > } > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
