On 5/18/22 20:00, Ilya Maximets wrote:
> 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.
>
Ack.
>>
>> 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?
>
Sounds legitimate indeed, thanks for the details.
>>
>>> + 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.
>
Sounds good to me, no need for v3 from my perspective.
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
