This is second in the series of fixing the usage of hashtables in the
dns_adb and the dns_resolver units.
Currently, the fetch buckets (used to hold the fetch context) and zone
buckets (used to hold per-domain counters) would never get cleaned from
the memory. Combined with the fact that the hashtable now grows as
needed (instead of using hashtable as buckets), the memory usage in the
resolver can just grow and it never drops down.
In this commit, the usage of hashtables (hashmaps) has been completely
rewritten, so there are no "buckets" and all the matching conditions are
directly mapped into the hashtable key:
1. For per-domain counter hashtable, this is simple as the lowercase
domain name is used directly as a counter.
2. For fetch context hashtable, this requires copying some extra flags
back and forth in the key.
As we don't hold the "buckets" forever, the cleaning mechanism has been
rewritten as well:
1. For per-domain counter hashtable, this is again much simpler, as we
only need to check whether the usage counter is still zero under the
lock and bail-out on cleaning if the counter is in use.
2. For fetch context hashtable, this is more complicated as the fetch
context cannot be reused after it has been finished. The algorithm
is different, the fetch context is always removed from the
hashtable, but if we find the fetch context that has been marked
as finished in the lookup function, we help with the cleaning from
the hashtable and try again.
Couple of additional changes have been implemented in this refactoring
as those were needed for correct functionality and could not be split
into individual commits (or would not make sense as seperate commits):
1. The dns_resolver_createfetch() has an option to create "unshared"
fetch. The "unshared" fetch will never get matched, so there's
little point in storing the "unshared" fetch in the hashtable.
Therefore the "unshared" fetches are now detached from the
hashtable and live just on their own.
2. Replace the custom reference counting with ISC_REFCOUNT_DECL/IMPL
macros for better tracing.
3. fctx_done_detach() is idempotent, it makes the "final" detach (the
one matching the create function) only once. But that also means
that it has to be called before the detach that kept the fetch
context alive in the callback. A new macro fctx_done_unref() has
been added to allow this code flow:
fctx_done_unref(fctx, result);
fctx_detach(&fctx);
Doing this the other way around could cause fctx to get destroyed in
the fctx_unref() first and fctx_done_detach() would cause UAF.
4. The resume_qmin() and resume_dslookup() callbacks have been
refactored for more readability and simpler code paths. The
validated() callback has also received some of the simplifications,
but it should be refactored in the future as it is bit of spaghetti
now.
The mechanism for associating a worker task to a database now
uses loops rather than tasks.
For this reason, the parameters to dns_cache_create() have been
updated to take a loop manager rather than a task manager.
After change 5995, zone transfers were using a small
compression context that only had space for the first
few dozen names in each message. They now use a large
compression context with enough space for every name.
The "final reference detached" message was meant to be DEBUG(1), but was
instead kept at INFO level. Move it to the DEBUG(1) logging level, so
it's not printed under normal operations.
Currently, the ADB uses TTL of 0 for ADB names that the server is
authoritative for and TTL of 10 seconds for HINT and GLUE ADB names.
This requires the unlinked ADB entries to be kept around, because they
would disappear too quickly. This especially affect the root zone as
the trust level is "ultimate" for the root zone nameservers.
This commit restores the ability to keep the unlinked ADB entries in the
database for later reuse, restores printing the unlinked entries and
adds some extra cleaning of the unlinked ADB entries on the tail of the
LRU list (similar to what we are doing for the ADB names).
The dns_adb unit has been refactored to be much simpler. Following
changes have been made:
1. Simplify the ADB to always allow GLUE and hints
There were only two places where dns_adb_createfind() was used - in
the dns_resolver unit where hints and GLUE addresses were ok, and in
the dns_zone where dns_adb_createfind() would be called without
DNS_ADBFIND_HINTOK and DNS_ADBFIND_GLUEOK set.
Simplify the logic by allowing hint and GLUE addresses when looking
up the nameserver addresses to notify. The difference is negligible
and would cause a difference in the notified addresses only when
there's mismatch between the parent and child addresses and we
haven't cached the child addresses yet.
2. Drop the namebuckets and entrybuckets
Formerly, the namebuckets and entrybuckets were used to reduced the
lock contention when accessing the double-linked lists stored in each
bucket. In the previous refactoring, the custom hashtable for the
buckets has been replaced with isc_ht/isc_hashmap, so only a single
item (mostly, see below) would end up in each bucket.
Removing the entrybuckets has been straightforward, the only matching
was done on the isc_sockaddr_t member of the dns_adbentry.
Removing the zonebuckets required GLUEOK and HINTOK bits to be
removed because the find could match entries with-or-without the bits
set, and creating a custom key that stores the
DNS_ADBFIND_STARTATZONE in the first byte of the key, so we can do a
straightforward lookup into the hashtable without traversing a list
that contains items with different flags.
3. Remove unassociated entries from ADB database
Previously, the adbentries could live in the ADB database even after
unlinking them from dns_adbnames. Such entries would show up as
"Unassociated entries" in the ADB dump. The benefit of keeping such
entries is little - the chance that we link such entry to a adbname
is small, and it's simpler to evict unlinked entries from the ADB
cache (and the hashtable) than create second LRU cleaning mechanism.
Unlinked ADB entries are now directly deleted from the hash
table (hashmap) upon destruction.
4. Cleanup expired entries from the hash table
When buckets were still in place, the code would keep the buckets
always allocated and never shrink the hash table (hashmap). With
proper reference counting in place, we can delete the adbnames from
the hash table and the LRU list.
5. Stop purging the names early when we hit the time limit
Because the LRU list is now time ordered, we can stop purging the
names when we find a first entry that doesn't fullfil our time-based
eviction criteria because no further entry on the LRU list will meet
the criteria.
Future work:
1. Lock contention
In this commit, the focus was on correctness of the data structure,
but in the future, the lock contention in the ADB database needs to
be addressed. Currently, we use simple mutex to lock the hash
tables, because we almost always need to use a write lock for
properly purging the hashtables. The ADB database needs to be
sharded (similar to the effect that buckets had in the past). Each
shard would contain own hashmap and own LRU list.
2. Time-based purging
The ADB names and entries stay intact when there are no lookups.
When we add separate shards, a timer needs to be added for time-based
cleaning in case there's no traffic hashing to the inactive shard.
3. Revisit the 30 minutes limit
The ADB cache is capped at 30 minutes. This needs to be revisited,
and at least the limit should be configurable (in both directions).
The dns_adb would serialize all fetches on a single task. Create a
per-thread task, so the fetches will stay local to the thread that
initiated the fetch.
Before the refactoring, there was only few buckets with many names in
them, so cleaning up stale ADB names per-bucket made sense. After the
refactoring, each bucket directly maps to ADB name, so purging has been
effectively disabled.
Create a global LRU list for ADB names (and ADB entries) and purge the
stale ADB names globally.
Previously, the name and entry buckets were much larger, so the dead
names and entries were moved to a secondary list to be cleaned
later (f.e. after the already running fetch has been canceled). After
the last refactoring, the bucket now contains only the name (entry)
itself and thus the extra list has a little use. Remove the .deadnames
and .deadentries from dns_adbnamebucket_t and dns_adbentrybucket_t
structures.
The dns_rpz_zones structure was using .refs and .irefs for strong and
weak reference counting. Rewrite the unit to use just a single
reference counting + shutdown sequence (dns_rpz_destroy_rpzs) that must
be called by the creator of the dns_rpz_zones_t object. Remove the
reference counting from the dns_rpz_zone structure as it is not needed
because the zone objects are fully embedded into the dns_rpz_zones
structure and dns_rpz_zones_t object must never be destroyed before all
dns_rpz_zone_t objects.
The dns_rps_zones_t reference counting uses the new ISC_REFCOUNT_TRACE
capability - enable by defining DNS_RPZ_TRACE in the dns/rpz.h header.
Additionally, add magic numbers to the dns_rpz_zone and dns_rpz_zones
structures.
The dns_cache API contained a cache cleaning mechanism that would be
disabled for 'rbt' based cache. As named doesn't have any other cache
implementations, remove the cache cleaning mechanism from dns_cache API.
The RBTDB has own cache cleaning mechanism and therefor the iterator
.cleaning member would never be set to true. Remove the code that
checks for iterator->cleaning from the RBTDB.
The `gen` program was causing a lengthy single-threaded pause in
the BIND build. When generating RDATATYPE_FROMTEXT_SW(), `gen` hit
the inner loop of `find_typename()` over 1.2 billion times. This
change avoids long deeply-nested loops, so `gen` now runs in less
than 10ms, about 300x faster.
No changes to the output.
The various factors like NS_PER_MS are now defined in a single place
and the names are no longer inconsistent. I chose the _PER_SEC names
rather than _PER_S because it is slightly more clear in isolation;
but the smaller units are always NS, US, and MS.
Extract the tlss values if present from the ipkeylist entry and add
the resulting tls setting to the constructed configuration for the
primary.
When comparing catalog zone entries for reuse also check the
masters.tlss values for equality.
The aim is to do less work per byte:
* Check the bounds for each label, instead of checking the
bounds for each character.
* Instead of copying one character at a time from the wire to
the name, copy entire runs of sequential labels using memmove()
to make the most of its fast loop.
* To remember where the name ends, we only need to set the end
marker when we see a compression pointer or when we reach the
root label. There is no need to check if we jumped back and
conditionally update the counter for every character.
* To parse a compression pointer, we no longer take a diversion
around the outer loop in between reading the upper byte of the
pointer and the lower byte.
* The parser state machine is now implicit in the instruction
pointer, instead of being an explicit variable. Similarly,
when we reach the root label we break directly out of the loop
instead of setting a second state machine variable.
* DNS_NAME_DOWNCASE is never used with dns_name_fromwire() so
that option is no longer supported.
I have removed this comment which dated from January 1999 when
dns_name_fromwire() was first introduced:
/*
* Note: The following code is not optimized for speed, but
* rather for correctness. Speed will be addressed in the future.
*/
No functional change, apart from removing support for the unused
DNS_NAME_DOWNCASE option. The new code is about 2x faster than the
old code: best case 11x faster, worst case 1.4x faster.
When using dual-stack-servers the covering namespace to check whether
answers are in scope or not should be fctx->domain. To do this we need
to be able to distingish forwarding due to forwarders clauses and
dual-stack-servers. A new flag FCTX_ADDRINFO_DUALSTACK has been added
to signal this.
There were a number of places where the zone table should have been
locked, but wasn't, when dns_zt_apply was called.
Added a isc_rwlocktype_t type parameter to dns_zt_apply and adjusted
all calls to using it. Removed locks in callers.
Despite the RFC says that the NSEC3PARAM is not something that is
intended for the resolver to be cached, and thus the TTL of 0 is most
logical, a zero TTL RRset can be abused by bad actors.
Change the default to SOA MINIMUM.
The call to dns_view_flushcache() is done under exclusive mode, but we
still need to check if view->adb is still attached before calling
dns_adb_flush() because the shutdown might have been already
initialized. This most likely only a theoretical problem on shutdown
because there's either no way how to initiate cache flush when shutting
down or very slim window where the `rndc flush` would have to hit the
slim time during named shutdown.
When starting priming from dns_view_find(), the dns_view shutdown could
be initiated by different thread, detaching from the resolver. Use
dns_view_getresolver() to attach to the resolver under view->lock, so we
don't try to call dns_resolver_prime() with NULL pointer.
There are more accesses to view->resolver, (and also view->adb and
view->requestmgr that suffer from the same problem) in the dns_view
module, but they are all done in exclusive mode or under a view->lock.
Replace the use of isc_ht API with isc_hashmap API in the dns_resolver
implementation. This requires extending the fctxbucket_t structure to
include keysize and copy of the key because the isc_hashmap API needs
the raw key in case of resizing the hashmap table.
Replace the use of isc_ht API with isc_hashmap API in the dns_adb
database implementation. This requires extending the
dns_adbnamebucket_t and dns_adbentrybucket_t structures to include
keysize and copy of the key because the isc_hashmap API needs the raw
key in case of resizing the hashmap table.
Previously, the tree read lock could be upgraded to a write lock in
decrement_reference() and then downgraded back to read lock in
dereference_iter_node(). When the use of isc_rwlock_downgrade() was
removed, the downgrade was changed to a simple unlock+lock. This allows
some delete operations to sneak in and delete nodes that the iterator
expects to be in place.
Expand decrement_reference() so the caller can indicate whether the
tree read lock should be upgraded, and disallow the upgrade when
calling from dereference_iter_node(), so there will be no need to
release the lock afterward.
The zone_refreshkeys() could run before the zone_shutdown(), but after
the last .erefs has been "detached" causing assertion failure when doing
dns_zone_attach(). Remove the use of .erefs (dns_zone_attach/detach)
and replace it with using the .irefs and additional checks whether the
zone is exiting in the callbacks.
There was an exception for dnssec-policy that allowed DNSSEC in the
unsigned version of the zone. This however causes a crash if the
zone switches from dynamic to inline-signing in the case of NSEC3,
because we are now trying to add an NSEC3 record to a non-NSEC3 node.
This is because BIND expects none of the records in the unsigned
version of the zone to be NSEC3.
Remove the exception for dnssec-policy when copying non DNSSEC
records, but do allow for DNSKEY as this may be a published DNSKEY
from a different provider.
The dead nodes might get reactivated during the db iterator walks the
version of the tree, so we can't cleanup the dead nodes while the db
version is open. Restore the previous behaviour that cleaned up the
dead nodes when we are closing the version.
The dns_rbtdb unit already tracks the state of the node and tree rwlocks
during the top level function and passes the states of the locks to the
called functions.
Add the tree locking family of macros modeled after node locking macros,
and expand both to track the state of the lock in an external variable.
Additionally, in developer mode, add precondition to the macros, so the
lock is in required state - this should cause an assertion failure on
double locking instead of the thread getting stuck.
The only place where isc_rwlock_downgrade was being used was the
decrement_reference() where the code tries either relocks the node
rwlock to write and then tries to upgrade the tree lock. When returning
from the function it tries to restore the locks into a previous state
which is nice, but kind of moot, because at every use of
decrement_reference() the node locks is immediately or almost
immeditately unlocked, and same holds for the tree lock.
Instead of trying to restore the node and tree lock into the initial
state, the decrement_reference now returns the state of the locks, so
the caller can then use the right unlock operation (read or write).
Only when the tree lock was originally unlocked, the decrement_reference
unlocks the tree lock before returning to the caller.
When named starts it creates an empty KEYDATA record in the managed-keys
zone as a placeholder, then schedules a key refresh. If key refresh
fails for some reason (e.g. connectivity problems), named will load the
placeholder key into secroots as a trusted key during the next startup,
which will break the chain of trust, and named will never recover from
that state until managed-keys.bind and managed-keys.bind.jnl files are
manually deleted before (re)starting named again.
Before calling load_secroots(), check that we are not dealing with a
placeholder.
Because dns_resolver_createfetch() locks the view, it was necessary
to unlock the zone in zone_refreshkeys() before calling it in order
to maintain the lock order, and relock afterward. this permitted a race
with dns_zone_synckeyzone().
This commit moves the call to dns_resolver_createfetch() into a separate
function which is called asynchronously after the zone has been
unlocked.
The keyfetch object now attaches to the zone to ensure that
it won't be shut down before the asynchronous call completes.
This necessitated refactoring dns_zone_detach() so it always runs
unlocked. For managed zones it now schedules zone_shutdown() to
run asynchronously, and for unmanaged zones, it requires the last
dns_zone_detach() to be run without loopmgr running.
Instead of doing incremental zone loading with fixed quantum - 100
loaded lines per event, move the zone loading process to the offloaded
libuv threads using isc_work_enqueue() API.
This has the advantage that the thread scheduling is given back to the
operating system that understands blocking operations, and the zone
loading operation doesn't block the networking threads directly.
Incremental file loads now use loopmgr events instead of task events.
The dns_master_loadstreaminc(), _loadbufferinc(), _loadlexer() and
_loadlexerinc() functions were not used in BIND, and have been removed.
dns_rdata_checksvcb performs data entry checks on SVCB records.
In particular that _dns SVBC record have an 'alpn' and if that 'alpn'
parameter indicates HTTP is in use that 'dophath' is present.
The wrong tls configuration was picked here. It should be of the
primary that is selected by forward->which, not zone->curprimary.
This bug may cause BIND to select the wrong primary when retrieving
the TLS settings, or cause a crash in case the wrongly selected primary
has no TLS settings.
ARM states that the "eligibility" TTL is the smallest original TTL
value that is accepted for a record to be eligible for prefetching,
but the code, which implements the condition doesn't behave in that
manner for the edge case when the TTL is equal to the configured
eligibility value.
Fix the code to check that the TTL is greater than, or equal to the
configured eligibility value, instead of just greater than it.
For UDP queries, after calling dns_adb_beginudpfetch() in fctx_query(),
make sure that dns_adb_endudpfetch() is also called on error path, in
order to adjust the quota back.
Ondřej Surý
Evan Hunt
Tony FinchandTony Finch
Michal Nowak
Mark Andrews
Matthijs Mekking
Aram Sargsyan
Petr Špaček