From 8c06d627b3f6ce4223410d4c7811a08dd0008934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Aug 2025 11:31:57 +0200 Subject: [PATCH 1/3] Unify the NONEXISTENT() macro in qpzone to EXISTS() In the dns_qpcache unit, we use EXISTS() macro, but in the dns_qpzone there's a NONEXISTENT() macro for the same slabheader attribute. Unify the macro to be also EXISTS() in dns_qpzone. --- lib/dns/qpzone.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 139e0a1954..65f6c211b6 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -70,9 +70,9 @@ goto failure; \ } while (0) -#define NONEXISTENT(header) \ +#define EXISTS(header) \ ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NONEXISTENT) != 0) + DNS_SLABHEADERATTR_NONEXISTENT) == 0) #define IGNORE(header) \ ((atomic_load_acquire(&(header)->attributes) & \ DNS_SLABHEADERATTR_IGNORE) != 0) @@ -1123,7 +1123,7 @@ setnsec3parameters(dns_db_t *db, qpz_version_t *version) { if (header->serial <= version->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -1655,7 +1655,7 @@ qpzone_findrdataset(dns_db_t *db, dns_dbnode_t *dbnode, header_next = header->next; do { if (header->serial <= serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -1787,7 +1787,7 @@ cname_and_other(qpznode_t *node, uint32_t serial) { do { if (header->serial <= serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -1805,7 +1805,7 @@ cname_and_other(qpznode_t *node, uint32_t serial) { do { if (header->serial <= serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -1862,7 +1862,7 @@ recordsize(dns_slabheader_t *header, unsigned int namelen) { static void maybe_update_recordsandsize(bool add, qpz_version_t *version, dns_slabheader_t *header, unsigned int namelen) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { return; } @@ -1934,7 +1934,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename, * we'll try to create a new rdataset that is the union * of 'newheader' and 'header'. */ - if (merge && !NONEXISTENT(header)) { + if (merge && EXISTS(header)) { unsigned int flags = 0; INSIST(version->serial >= header->serial); merged = NULL; @@ -2043,7 +2043,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename, * * If we're trying to delete the type, don't bother. */ - if (NONEXISTENT(newheader)) { + if (!EXISTS(newheader)) { dns_slabheader_destroy(&newheader); return DNS_R_UNCHANGED; } @@ -2751,7 +2751,7 @@ step(qpz_search_t *search, dns_qpiter_t *it, direction_t direction, if (header->serial <= search->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -2914,7 +2914,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, const dns_name_t *qname, for (header = node->data; header != NULL; header = header->next) { if (header->serial <= search->serial && - !IGNORE(header) && !NONEXISTENT(header)) + !IGNORE(header) && EXISTS(header)) { break; } @@ -2954,8 +2954,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, const dns_name_t *qname, header = header->next) { if (header->serial <= search->serial && - !IGNORE(header) && - !NONEXISTENT(header)) + !IGNORE(header) && EXISTS(header)) { break; } @@ -3141,7 +3140,7 @@ again: if (header->serial <= search->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -3286,7 +3285,7 @@ qpzone_check_zonecut(qpznode_t *node, void *arg DNS__DB_FLARG) { if (header->serial <= search->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -3613,7 +3612,7 @@ found: do { if (header->serial <= search.serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -4130,7 +4129,7 @@ rdatasetiter_first(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { if (header->serial <= version->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -4183,7 +4182,7 @@ rdatasetiter_next(dns_rdatasetiter_t *iterator DNS__DB_FLARG) { if (header->serial <= version->serial && !IGNORE(header)) { - if (NONEXISTENT(header)) { + if (!EXISTS(header)) { header = NULL; } break; @@ -4994,7 +4993,7 @@ qpzone_subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode, while (header != NULL && IGNORE(header)) { header = header->down; } - if (header != NULL && !NONEXISTENT(header)) { + if (header != NULL && EXISTS(header)) { unsigned int flags = 0; subresult = NULL; result = ISC_R_SUCCESS; From eba76df247eae9838ffcf817c0c218e4a98eaccf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Aug 2025 12:21:56 +0200 Subject: [PATCH 2/3] Move the slabheader attribute helpers to private header The slabheader.c, qpzone.c and qpcache.c had couple of shared macros that were copied and paste between the units. Move these common attributes access macros into private header, so these can be shared among the three compilation units. --- lib/dns/qpcache.c | 32 +--------------------- lib/dns/qpzone.c | 17 +----------- lib/dns/rdataslab.c | 19 +++---------- lib/dns/rdataslab_p.h | 59 +++++++++++++++++++++++++++++++++++++++++ tests/dns/qpzone_test.c | 6 ++--- 5 files changed, 67 insertions(+), 66 deletions(-) create mode 100644 lib/dns/rdataslab_p.h diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 9d02239a24..c5de17203a 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -59,6 +59,7 @@ #include "db_p.h" #include "qpcache_p.h" +#include "rdataslab_p.h" #ifndef DNS_QPCACHE_LOG_STATS_LEVEL #define DNS_QPCACHE_LOG_STATS_LEVEL 3 @@ -71,37 +72,6 @@ goto failure; \ } while (0) -#define EXISTS(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NONEXISTENT) == 0) -#define NXDOMAIN(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NXDOMAIN) != 0) -#define STALE(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_STALE) != 0) -#define STALE_WINDOW(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_STALE_WINDOW) != 0) -#define OPTOUT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_OPTOUT) != 0) -#define NEGATIVE(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NEGATIVE) != 0) -#define PREFETCH(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_PREFETCH) != 0) -#define ZEROTTL(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_ZEROTTL) != 0) -#define ANCIENT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_ANCIENT) != 0) -#define STATCOUNT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_STATCOUNT) != 0) - #define STALE_TTL(header, qpdb) \ (NXDOMAIN(header) ? 0 : qpdb->common.serve_stale_ttl) diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 65f6c211b6..76eb96c9d5 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -62,6 +62,7 @@ #include "db_p.h" #include "qpzone_p.h" +#include "rdataslab_p.h" #define CHECK(op) \ do { \ @@ -70,22 +71,6 @@ goto failure; \ } while (0) -#define EXISTS(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NONEXISTENT) == 0) -#define IGNORE(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_IGNORE) != 0) -#define RESIGN(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_RESIGN) != 0) -#define OPTOUT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_OPTOUT) != 0) -#define STATCOUNT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_STATCOUNT) != 0) - #define HEADERNODE(h) ((qpznode_t *)((h)->node)) #define QPDB_ATTR_LOADED 0x01 diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index f5d4cb2454..82e810c3ef 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -30,18 +30,7 @@ #include #include -#define CASESET(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_CASESET) != 0) -#define CASEFULLYLOWER(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_CASEFULLYLOWER) != 0) -#define NONEXISTENT(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NONEXISTENT) != 0) -#define NEGATIVE(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_NEGATIVE) != 0) +#include "rdataslab_p.h" /* * The rdataslab structure allows iteration to occur in both load order @@ -915,10 +904,10 @@ dns_slabheader_destroy(dns_slabheader_t **headerp) { isc_mem_t *mctx = header->node->mctx; dns_db_deletedata(header->node, header); - if (NONEXISTENT(header)) { - size = sizeof(*header); - } else { + if (EXISTS(header)) { size = dns_rdataslab_size(header); + } else { + size = sizeof(*header); } isc_mem_put(mctx, header, size); diff --git a/lib/dns/rdataslab_p.h b/lib/dns/rdataslab_p.h new file mode 100644 index 0000000000..e3b4fbb573 --- /dev/null +++ b/lib/dns/rdataslab_p.h @@ -0,0 +1,59 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#pragma once + +#include + +#define ANCIENT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_ANCIENT) != 0) +#define CASEFULLYLOWER(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_CASEFULLYLOWER) != 0) +#define CASESET(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_CASESET) != 0) +#define EXISTS(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_NONEXISTENT) == 0) +#define IGNORE(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_IGNORE) != 0) +#define NEGATIVE(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_NEGATIVE) != 0) +#define NXDOMAIN(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_NXDOMAIN) != 0) +#define OPTOUT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_OPTOUT) != 0) +#define PREFETCH(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_PREFETCH) != 0) +#define RESIGN(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_RESIGN) != 0) +#define STALE(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_STALE) != 0) +#define STALE_WINDOW(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_STALE_WINDOW) != 0) +#define STATCOUNT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_STATCOUNT) != 0) +#define ZEROTTL(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + DNS_SLABHEADERATTR_ZEROTTL) != 0) diff --git a/tests/dns/qpzone_test.c b/tests/dns/qpzone_test.c index 73cd7514a1..ad3d5b972a 100644 --- a/tests/dns/qpzone_test.c +++ b/tests/dns/qpzone_test.c @@ -34,6 +34,8 @@ #include #define KEEP_BEFORE +#include "rdataslab_p.h" + /* Include the main file */ #pragma GCC diagnostic push @@ -45,10 +47,6 @@ #undef CHECK #include -#define CASESET(header) \ - ((atomic_load_acquire(&(header)->attributes) & \ - DNS_SLABHEADERATTR_CASESET) != 0) - const char *ownercase_vectors[12][2] = { { "AaBbCcDdEeFfGgHhIiJjKkLlMmNnOoPpQqRrSsTtUuVvWwXxYyZz", From d0fef4d5e5dd2a2a57c947134ec6b2c3aa39d1cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 12 Aug 2025 12:10:24 +0200 Subject: [PATCH 3/3] Remove locking from rdataslab_getownercase() Under normal circumstances, the case bitfield in the slabheader should be set only once. By actually (soft-)enforcing this, the read locking can be completely removed from the rdataslab_getownercase() as we can check whether the case has been already set or not and making everything immutable once the case has been set. --- lib/dns/rdataslab.c | 56 +++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index 82e810c3ef..35645b66e3 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -833,36 +834,38 @@ dns_slabheader_raw(dns_slabheader_t *header) { void dns_slabheader_setownercase(dns_slabheader_t *header, const dns_name_t *name) { - unsigned int i; - bool fully_lower; + REQUIRE(!CASESET(header)); + + bool casefullylower = true; /* * We do not need to worry about label lengths as they are all * less than or equal to 63. */ memset(header->upper, 0, sizeof(header->upper)); - fully_lower = true; - for (i = 0; i < name->length; i++) { + for (size_t i = 0; i < name->length; i++) { if (isupper(name->ndata[i])) { header->upper[i / 8] |= 1 << (i % 8); - fully_lower = false; + casefullylower = false; } } - DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_CASESET); - if (fully_lower) { + if (casefullylower) { DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_CASEFULLYLOWER); } + DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_CASESET); } void dns_slabheader_copycase(dns_slabheader_t *dest, dns_slabheader_t *src) { + REQUIRE(!CASESET(dest)); if (CASESET(src)) { - uint_least16_t attr = DNS_SLABHEADER_GETATTR( - src, DNS_SLABHEADERATTR_CASESET | - DNS_SLABHEADERATTR_CASEFULLYLOWER); - DNS_SLABHEADER_SETATTR(dest, attr); memmove(dest->upper, src->upper, sizeof(src->upper)); + if (CASEFULLYLOWER(src)) { + DNS_SLABHEADER_SETATTR( + dest, DNS_SLABHEADERATTR_CASEFULLYLOWER); + } + DNS_SLABHEADER_SETATTR(dest, DNS_SLABHEADERATTR_CASESET); } } @@ -1198,7 +1201,10 @@ static void rdataset_setownercase(dns_rdataset_t *rdataset, const dns_name_t *name) { dns_slabheader_t *header = dns_rdataset_getheader(rdataset); - DNS_SLABHEADER_CLRATTR(header, DNS_SLABHEADERATTR_CASEFULLYLOWER); + /* The case could be set just once for the same header */ + if (CASESET(header)) { + return; + } dns_db_locknode(header->node, isc_rwlocktype_write); dns_slabheader_setownercase(header, name); @@ -1211,26 +1217,26 @@ rdataset_getownercase(const dns_rdataset_t *rdataset, dns_name_t *name) { uint8_t mask = (1 << 7); uint8_t bits = 0; + if (!CASESET(header)) { + return; + } + if (CASEFULLYLOWER(header)) { isc_ascii_lowercopy(name->ndata, name->ndata, name->length); return; } - dns_db_locknode(header->node, isc_rwlocktype_read); - if (CASESET(header)) { - uint8_t *nd = name->ndata; - for (size_t i = 0; i < name->length; i++) { - if (mask == (1 << 7)) { - bits = header->upper[i / 8]; - mask = 1; - } else { - mask <<= 1; - } - nd[i] = (bits & mask) ? isc_ascii_toupper(nd[i]) - : isc_ascii_tolower(nd[i]); + uint8_t *nd = name->ndata; + for (size_t i = 0; i < name->length; i++) { + if (mask == (1 << 7)) { + bits = header->upper[i / 8]; + mask = 1; + } else { + mask <<= 1; } + nd[i] = (bits & mask) ? isc_ascii_toupper(nd[i]) + : isc_ascii_tolower(nd[i]); } - dns_db_unlocknode(header->node, isc_rwlocktype_read); } static dns_slabheader_t *