From 9d326aaba32d28674b20c426537c0014dd044318 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Sep 2023 16:33:19 +0200 Subject: [PATCH 1/5] Use incremental hashing in the isc_sockaddr_hash() function Instead of copying address back and forth when hashing addr+port, we can use incremental hashing. Additionally, switch from 64-bit isc_hash_function to 32-bit isc_hash32() as the resulting value is 32-bit. --- lib/isc/include/isc/sockaddr.h | 2 +- lib/isc/sockaddr.c | 46 ++++++++++++++++------------------ 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/lib/isc/include/isc/sockaddr.h b/lib/isc/include/isc/sockaddr.h index 9523865c5a..b9dda2b0c2 100644 --- a/lib/isc/include/isc/sockaddr.h +++ b/lib/isc/include/isc/sockaddr.h @@ -85,7 +85,7 @@ isc_sockaddr_eqaddrprefix(const isc_sockaddr_t *a, const isc_sockaddr_t *b, * If 'b''s scope is zero then 'a''s scope will be ignored. */ -unsigned int +uint32_t isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only); /*%< * Return a hash value for the socket address 'sockaddr'. If 'address_only' diff --git a/lib/isc/sockaddr.c b/lib/isc/sockaddr.c index 765e73e566..020cc58ae9 100644 --- a/lib/isc/sockaddr.c +++ b/lib/isc/sockaddr.c @@ -187,51 +187,49 @@ isc_sockaddr_format(const isc_sockaddr_t *sa, char *array, unsigned int size) { } } -unsigned int +uint32_t isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only) { - unsigned int length = 0; - const unsigned char *s = NULL; - unsigned int h = 0; + REQUIRE(sockaddr != NULL); + + size_t len = 0; + const uint8_t *s = NULL; unsigned int p = 0; const struct in6_addr *in6; + isc_hash32_t hash; - REQUIRE(sockaddr != NULL); + isc_hash32_init(&hash); switch (sockaddr->type.sa.sa_family) { case AF_INET: - s = (const unsigned char *)&sockaddr->type.sin.sin_addr; - p = ntohs(sockaddr->type.sin.sin_port); - length = sizeof(sockaddr->type.sin.sin_addr.s_addr); + s = (const uint8_t *)&sockaddr->type.sin.sin_addr; + len = sizeof(sockaddr->type.sin.sin_addr.s_addr); + if (!address_only) { + p = ntohs(sockaddr->type.sin.sin_port); + } break; case AF_INET6: in6 = &sockaddr->type.sin6.sin6_addr; - s = (const unsigned char *)in6; + s = (const uint8_t *)in6; if (IN6_IS_ADDR_V4MAPPED(in6)) { s += 12; - length = sizeof(sockaddr->type.sin.sin_addr.s_addr); + len = sizeof(sockaddr->type.sin.sin_addr.s_addr); } else { - length = sizeof(sockaddr->type.sin6.sin6_addr); + len = sizeof(sockaddr->type.sin6.sin6_addr); + } + if (!address_only) { + p = ntohs(sockaddr->type.sin6.sin6_port); } - p = ntohs(sockaddr->type.sin6.sin6_port); break; default: - UNEXPECTED_ERROR("unknown address family: %d", - (int)sockaddr->type.sa.sa_family); - s = (const unsigned char *)&sockaddr->type; - length = sockaddr->length; - p = 0; + UNREACHABLE(); } - uint8_t buf[sizeof(struct sockaddr_storage) + sizeof(p)]; - memmove(buf, s, length); + isc_hash32_hash(&hash, s, len, true); if (!address_only) { - memmove(buf + length, &p, sizeof(p)); - h = isc_hash_function(buf, length + sizeof(p), true); - } else { - h = isc_hash_function(buf, length, true); + isc_hash32_hash(&hash, &p, sizeof(p), true); } - return (h); + return (isc_hash32_finalize(&hash)); } void From 1653fa61c7c268a0aa4afb22b78d10ec5ddb478d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Sep 2023 16:35:29 +0200 Subject: [PATCH 2/5] Use 32-bit hashing in isc memory debugging Switch from 64-bit isc_hash_function() to 32-bit isc_hash32() as we were using the 32-bit value only anyway. --- lib/isc/mem.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index bffe2a9e6d..fffdaade8a 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -216,11 +216,11 @@ add_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size FLARG) { #ifdef __COVERITY__ /* * Use simple conversion from pointer to hash to avoid - * tainting 'ptr' due to byte swap in isc_hash_function. + * tainting 'ptr' due to byte swap in isc_hash32. */ hash = (uintptr_t)ptr >> 3; #else - hash = isc_hash_function(&ptr, sizeof(ptr), true); + hash = isc_hash32(&ptr, sizeof(ptr), true); #endif idx = hash % DEBUG_TABLE_COUNT; @@ -260,11 +260,11 @@ delete_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size, #ifdef __COVERITY__ /* * Use simple conversion from pointer to hash to avoid - * tainting 'ptr' due to byte swap in isc_hash_function. + * tainting 'ptr' due to byte swap in isc_hash32. */ hash = (uintptr_t)ptr >> 3; #else - hash = isc_hash_function(&ptr, sizeof(ptr), true); + hash = isc_hash32(&ptr, sizeof(ptr), true); #endif idx = hash % DEBUG_TABLE_COUNT; From 9f40eee0a86f445944ef121d7fdb9ff2fa42c09e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Sep 2023 16:37:50 +0200 Subject: [PATCH 3/5] Remove isc_hash_function macro The last two users of 64-bit isc_hash_function() macro were removed in the previous commits, remove the macro as well. --- lib/isc/include/isc/hash.h | 2 -- tests/isc/hash_test.c | 69 ++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/lib/isc/include/isc/hash.h b/lib/isc/include/isc/hash.h index bac9d9f7b8..231ecfb321 100644 --- a/lib/isc/include/isc/hash.h +++ b/lib/isc/include/isc/hash.h @@ -44,8 +44,6 @@ isc_hash_get_initializer(void); void isc_hash_set_initializer(const void *initializer); -#define isc_hash_function isc_hash64 - void isc_hash32_init(isc_hash32_t *restrict state); void diff --git a/tests/isc/hash_test.c b/tests/isc/hash_test.c index a0a7093209..6d04943866 100644 --- a/tests/isc/hash_test.c +++ b/tests/isc/hash_test.c @@ -34,60 +34,87 @@ #include /* Hash function test */ -ISC_RUN_TEST_IMPL(isc_hash_function) { - unsigned int h1; - unsigned int h2; - - UNUSED(state); +ISC_RUN_TEST_IMPL(isc_hash32) { + uint32_t h1; + uint32_t h2; /* Immutability of hash function */ - h1 = isc_hash_function(NULL, 0, true); - h2 = isc_hash_function(NULL, 0, true); + h1 = isc_hash32(NULL, 0, true); + h2 = isc_hash32(NULL, 0, true); assert_int_equal(h1, h2); /* Hash function characteristics */ - h1 = isc_hash_function("Hello world", 12, true); - h2 = isc_hash_function("Hello world", 12, true); + h1 = isc_hash32("Hello world", 12, true); + h2 = isc_hash32("Hello world", 12, true); assert_int_equal(h1, h2); /* Case */ - h1 = isc_hash_function("Hello world", 12, false); - h2 = isc_hash_function("heLLo WorLd", 12, false); + h1 = isc_hash32("Hello world", 12, false); + h2 = isc_hash32("heLLo WorLd", 12, false); assert_int_equal(h1, h2); /* Unequal */ - h1 = isc_hash_function("Hello world", 12, true); - h2 = isc_hash_function("heLLo WorLd", 12, true); + h1 = isc_hash32("Hello world", 12, true); + h2 = isc_hash32("heLLo WorLd", 12, true); + + assert_int_not_equal(h1, h2); +} + +/* Hash function test */ +ISC_RUN_TEST_IMPL(isc_hash64) { + uint64_t h1; + uint64_t h2; + + /* Immutability of hash function */ + h1 = isc_hash64(NULL, 0, true); + h2 = isc_hash64(NULL, 0, true); + + assert_int_equal(h1, h2); + + /* Hash function characteristics */ + h1 = isc_hash64("Hello world", 12, true); + h2 = isc_hash64("Hello world", 12, true); + + assert_int_equal(h1, h2); + + /* Case */ + h1 = isc_hash64("Hello world", 12, false); + h2 = isc_hash64("heLLo WorLd", 12, false); + + assert_int_equal(h1, h2); + + /* Unequal */ + h1 = isc_hash64("Hello world", 12, true); + h2 = isc_hash64("heLLo WorLd", 12, true); assert_int_not_equal(h1, h2); } /* Hash function initializer test */ ISC_RUN_TEST_IMPL(isc_hash_initializer) { - unsigned int h1; - unsigned int h2; + uint64_t h1; + uint64_t h2; - UNUSED(state); - - h1 = isc_hash_function("Hello world", 12, true); - h2 = isc_hash_function("Hello world", 12, true); + h1 = isc_hash64("Hello world", 12, true); + h2 = isc_hash64("Hello world", 12, true); assert_int_equal(h1, h2); isc_hash_set_initializer(isc_hash_get_initializer()); /* Hash value must not change */ - h2 = isc_hash_function("Hello world", 12, true); + h2 = isc_hash64("Hello world", 12, true); assert_int_equal(h1, h2); } ISC_TEST_LIST_START -ISC_TEST_ENTRY(isc_hash_function) +ISC_TEST_ENTRY(isc_hash32) +ISC_TEST_ENTRY(isc_hash64) ISC_TEST_ENTRY(isc_hash_initializer) ISC_TEST_LIST_END From 3230c8e369313020ea48f995790624acec94dc8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 18 Sep 2023 09:59:10 +0200 Subject: [PATCH 4/5] Add isc_sockaddr_hash_ex that can be used in incremental hashing Add a sockaddr hashing function that can be used as part of incremental hashing. --- lib/isc/include/isc/sockaddr.h | 9 +++++++++ lib/isc/sockaddr.c | 21 ++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/lib/isc/include/isc/sockaddr.h b/lib/isc/include/isc/sockaddr.h index b9dda2b0c2..e962a7a11c 100644 --- a/lib/isc/include/isc/sockaddr.h +++ b/lib/isc/include/isc/sockaddr.h @@ -85,6 +85,15 @@ isc_sockaddr_eqaddrprefix(const isc_sockaddr_t *a, const isc_sockaddr_t *b, * If 'b''s scope is zero then 'a''s scope will be ignored. */ +void +isc_sockaddr_hash_ex(isc_hash32_t *hash, const isc_sockaddr_t *sockaddr, + bool address_only); +/*%< + * Add the hash of the sockaddr into the hash for incremental hashing + * + * See isc_sockaddr_hash() for details. + */ + uint32_t isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only); /*%< diff --git a/lib/isc/sockaddr.c b/lib/isc/sockaddr.c index 020cc58ae9..263d652b38 100644 --- a/lib/isc/sockaddr.c +++ b/lib/isc/sockaddr.c @@ -187,17 +187,15 @@ isc_sockaddr_format(const isc_sockaddr_t *sa, char *array, unsigned int size) { } } -uint32_t -isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only) { +void +isc_sockaddr_hash_ex(isc_hash32_t *hash, const isc_sockaddr_t *sockaddr, + bool address_only) { REQUIRE(sockaddr != NULL); size_t len = 0; const uint8_t *s = NULL; unsigned int p = 0; const struct in6_addr *in6; - isc_hash32_t hash; - - isc_hash32_init(&hash); switch (sockaddr->type.sa.sa_family) { case AF_INET: @@ -224,10 +222,19 @@ isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only) { UNREACHABLE(); } - isc_hash32_hash(&hash, s, len, true); + isc_hash32_hash(hash, s, len, true); if (!address_only) { - isc_hash32_hash(&hash, &p, sizeof(p), true); + isc_hash32_hash(hash, &p, sizeof(p), true); } +} + +uint32_t +isc_sockaddr_hash(const isc_sockaddr_t *sockaddr, bool address_only) { + isc_hash32_t hash; + + isc_hash32_init(&hash); + + isc_sockaddr_hash_ex(&hash, sockaddr, address_only); return (isc_hash32_finalize(&hash)); } From 4cf4cc484d5eb53d21d1b176f59233ad4be3dead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 18 Sep 2023 10:02:37 +0200 Subject: [PATCH 5/5] Use the new isc_sockaddr_hash_ex() to fix QID table hashing The QID table hashing used a custom merging of the sockaddr, port and id into a single hashvalue. Normalize the QID table hashing function to use isc_hash32 API for all the values. --- lib/dns/dispatch.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 3ba6d91b32..0b82990a1a 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -373,12 +373,15 @@ setup_socket(dns_dispatch_t *disp, dns_dispentry_t *resp, static uint32_t qid_hash(const dns_dispentry_t *dispentry) { - /* - * TODO(OS): Add incremental isc_sockaddr_hash() function and then use - * isc_hash32 API - */ - uint32_t hashval = isc_sockaddr_hash(&dispentry->peer, true); - return (hashval ^ (((uint32_t)dispentry->id << 16) | dispentry->port)); + isc_hash32_t hash; + + isc_hash32_init(&hash); + + isc_sockaddr_hash_ex(&hash, &dispentry->peer, true); + isc_hash32_hash(&hash, &dispentry->id, sizeof(dispentry->id), true); + isc_hash32_hash(&hash, &dispentry->port, sizeof(dispentry->port), true); + + return (isc_hash32_finalize(&hash)); } static int