On 5/16/22 09:40, Dumitru Ceara wrote:
> 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?
I don't think so. The openssl documentation is notoriously bad,
but IIUC, digest sizes are constant for a specific algorithm, as
they should be. SHA1 hash is always 160 bits long.
If openssl doesn't fail but writes less than the hash size, it's
a bug in openssl and we can't actually proceed here. As if we
can't calculate the hash, we can't perform any operations that
requires hashing.
So, this should never happen, but if that happens, that's a fatal
problem.
>
> 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?
If we will want to literally follow the documentation, then yes,
we need to use EVP_MAX_MD_SIZE long buffer. But callers of the
sha1_*() API in OVS doesn't know that. And, probably, shouldn't.
But again, if openssl will return a hash for SHA1 which is not
160 bits long, that's a fatal error. So, assertion should be
an OK solution?
>
>> + 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/
OK. Can fix that on commit, if v3 will not be needed.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
