> Put a space before opening parentheses only after control statement
> keywords (for/if/while...) except this option doesn’t apply to ForEach
> and If macros. This is useful in projects where ForEach/If macros are
> treated as function calls instead of control statements.
All databases in the codebase follow the same structure: a database is
an associative container from DNS names to nodes, and each node is an
associative container from RR types to RR data.
Each database implementation (qpzone, qpcache, sdlz, builtin, dyndb) has
its own corresponding node type (qpznode, qpcnode, etc). However, some
code needs to work with nodes generically regardless of their specific
type - for example, to acquire locks, manage references, or
register/unregister slabs from the heap.
Currently, these generic node operations are implemented as methods in
the database vtable, which creates problematic coupling between database
and node lifetimes. If a node outlives its parent database, the node
destructor will destroy all RR data, and each RR data destructor will
try to unregister from heaps by calling a virtual function from the
database vtable. Since the database was already freed, this causes a
crash.
This commit breaks the coupling by standardizing the layout of all
database nodes, adding a dedicated vtable for node operations, and
moving node-specific methods from the database vtable to the node
vtable.
All the applications built on top of the loop manager were required to
create just a single instance of the loop manager. Refactor the loop
manager to not expose this instance to the callers and keep the loop
manager object internal to the isc_loop compilation unit.
This significantly simplifies a number of data structures and calls to
the isc_loop API.
Replace the read-write locked isc_hashmap with lock-free cds_lfht
hashtable and replace the singular LRU tables for ADB names and entries
with a per-thread LRU tables. These changes allowed to remove all the
read-write locking on the names and entries tables.
The dns_adb cleaning is little bit muddled as it mixes the "TTL"
based cleaning (.expire_v4 and .expire_v6 for adbname, .expires for
adbentry) with overmem cleaning.
Rewrite the LRU based cleaning to use SIEVE algorithm and to be overmem
cleaning only with a requirement to always cleanup at least 2-times the
size of the newly added entry.
The purge_stale_names()/purge_stale_entries() is opportunistic even when
we are under memory pressure (overmem). Split the opportunistic LRU
cleaning and overmem cleaning. This makes the stale purging much
simpler as we don't have to try that hard and makes the overmem cleaning
always cleanup double the amount of the newly allocated ADB name/entry.
There are three adbname flags that are used to identify different
types of adbname lookups when hashing rather than using multiple
hash tables. Separate these to their own structure element as these
need to be able to be read without locking the adbname structure.
Instead of giving the memory context names with an explicit call to
isc_mem_setname(), add the name to isc_mem_create() call to have all the
memory contexts an unconditional name.
replace the pattern `for (result = dns_rdataset_first(x); result ==
ISC_R_SUCCES; result = dns_rdataset_next(x)` with a new
`DNS_RDATASET_FOREACH` macro throughout BIND.
the import_rdataset() function can't return any value other
than ISC_R_SUCCESS, so it's been changed to void and its callers
don't rely on its return value any longer.
previously, ISC_LIST_FOREACH and ISC_LIST_FOREACH_SAFE were
two separate macros, with the _SAFE version allowing entries
to be unlinked during the loop. ISC_LIST_FOREACH is now also
safe, and the separate _SAFE macro has been removed.
similarly, the ISC_LIST_FOREACH_REV macro is now safe, and
ISC_LIST_FOREACH_REV_SAFE has also been removed.
use the ISC_LIST_FOREACH pattern in places where lists had
been iterated using a different pattern from the typical
`for` loop: for example, `while (!ISC_LIST_EMPTY(...))` or
`while ((e = ISC_LIST_HEAD(...)) != NULL)`.
the pattern `for (x = ISC_LIST_HEAD(...); x != NULL; ISC_LIST_NEXT(...)`
has been changed to `ISC_LIST_FOREACH` throughout BIND, except in a few
cases where the change would be excessively complex.
in most cases this was a straightforward change. in some places,
however, the list element variable was referenced after the loop
ended, and the code was refactored to avoid this necessity.
also, because `ISC_LIST_FOREACH` uses typeof(list.head) to declare
the list elements, compilation failures can occur if the list object
has a `const` qualifier. some `const` qualifiers have been removed
from function parameters to avoid this problem, and where that was not
possible, `UNCONST` was used.
the target name parameter to dns_adb_createfind() was always passed as
NULL, so we can safely remove it.
relatedly, the 'target' field in the dns_adbname structure was never
referenced after being set. the 'expire_target' field was used, but
only as a way to check whether an ADB name represents a CNAME or DNAME,
and that information can be stored as a single flag.
the target buffer passed to dns_name_concatenate() was never
used (except for one place in dig, where it wasn't actually
needed, and has already been removed in a prior commit).
we can safely remove the parameter.
When there are parent and child zones on the same server, the DNSKEY
lookup was failing as the pending record we are validating is needed
to fetch the DNSKEY records. This change allows that to happen.
The caller is already setting STARTATZONE when the name being looked
up is a subdomain of the current domain.
The address lookups from ADB were not being validated, allowing
spoofed responses to be accepted and used for other lookups.
Validate the answers except when CD=1 is set in the triggering
request. Separate ADB names looked up with CD=1 from those without
CD=1, to prevent the use of unvalidated answers in the normal lookup
case (CD=0). Set the TTL on unvalidated (pending) responses to
ADB_CACHE_MINIMUM when adding them to the ADB.
Instead of mixing the dns_resolver and dns_validator units directly with
the EDE code, split-out the dns_ede functionality into own separate
compilation unit and hide the implementation details behind abstraction.
Additionally, the EDE codes are directly copied into the ns_client
buffers by passing the EDE context to dns_resolver_createfetch().
This makes the dns_ede implementation simpler to use, although sligtly
more complicated on the inside.
Co-authored-by: Colin Vidal <colin@isc.org>
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Address Database (ADB) shares the memory for the short lived ADB
objects (finds, fetches, addrinfo) and the long lived ADB
objects (names, entries, namehooks). This could lead to a situation
where the resolver-heavy load would force evict ADB objects from the
database to point where ADB is completely empty, leading to even more
resolver-heavy load.
Make the short lived ADB objects use the other memory context that we
already created for the hashmaps. This makes the ADB overmem condition
to not be triggered by the ongoing resolver fetches.
Add support for Extended DNS Errors (EDE) error 22: No reachable
authority. This occurs when after a timeout delay when the resolver is
trying to query an authority server.
When canceling the ADB find, the lock on the find gets released for
a brief period of time to be locked again inside adbname lock. During
the brief period that the ADB find is unlocked, it can get canceled by
other means removing it from the adbname list which in turn causes
assertion failure due to a double removal from the adbname list.
Recheck if the find->adbname is still valid after acquiring the lock
again and if not just skip the double removal. Additionally, attach to
the adbname as in the worst case, the adbname might also cease to exist
if the scheduler would block this particular thread for a longer period
of time invalidating the lock we are going to acquire and release.
Static-stub address and addresses from other sources where being
mixed together resulting in static-stub queries going to addresses
not specified in the configuration or alternatively static-stub
addresses being used instead of the real addresses.
Give prefetches a free pass through the quota so that the cache
entries for popular zones could be updated successfully even if the
quota for is already reached.
Remove the complicated mechanism that could be (in theory) used by
external libraries to register new categories and modules with
statically defined lists in <isc/log.h>. This is similar to what we
have done for <isc/result.h> result codes. All the libraries are now
internal to BIND 9, so we don't need to provide a mechanism to register
extra categories and modules.
The previous value of 30 minutes used to cache the ADB names and entries
was quite long. Change the value to 60 seconds for faster recovery
after cached intermittent failure of the remote nameservers.
The algorithm from the previous commit[1] is now used to calculate all
the expiration values through the code (ncache results, cname/dname
targets).
1. ISC_MIN(cur, ISC_MAX(now + ADB_ENTRY_WINDOW, now + rdataset->ttl))
Correct the logic to set the expiration period of expire_{v4,v6} as
follows:
1. If the trust is ultimate (local entry), immediately set the entry as
expired, so the changes to the local zones have immediate effect.
3. If the expiration is already set and smaller than the new value, then
leave the expiration value as it is.
2. Otherwise pick larger of `now + ADB_ENTRY_WINDOW` and `now + TTL` as
the new expiration value.
When ADB entry was created it was set to never expire. If we never
called any of the functions that adjust the expiration, it could linger
in the ADB forever.
Set the expiration (.expires) to now + ADB_ENTRY_WINDOW when creating
the new ADB entry to ensure the ADB entry will always expire.
if we had a method to get the running loop, similar to how
isc_tid() gets the current thread ID, we can simplify loop
and loopmgr initialization.
remove most uses of isc_loop_current() in favor of isc_loop().
in some places where that was the only reason to pass loopmgr,
remove loopmgr from the function parameters.
Previously, there were two methods of working with the overmem
condition:
1. hi/lo water callback - when the overmem condition was reached
for the first time, the water callback was called with HIWATER
mark and .is_overmem boolean was set internally. Similarly,
when the used memory went below the lo water mark, the water
callback would be called with LOWATER mark and .is_overmem
was reset. This check would be called **every** time memory
was allocated or freed.
2. isc_mem_isovermem() - a simple getter for the internal
.is_overmem flag
This commit refactors removes the first method and move the hi/lo water
checks to the isc_mem_isovermem() function, thus we now have only a
single method of checking overmem condition and the check for hi/lo
water is removed from the hot path for memory contexts that doesn't use
overmem checks.
If a child zone is served by the same servers as a parent zone and
a NS query is made for the zone name then the addresses of the
nameservers are returned in the additional section are tagged as
trust additional.
There was a microoptimization for smoothing srtt with bitshifts. Revert
the code to use * 98 / 100, it doesn't really make that difference on
modern CPUs, for comparison here:
muldiv:
imul eax, edi, 98
imul rax, rax, 1374389535
shr rax, 37
ret
shift:
mov eax, edi
sal eax, 9
sub eax, edi
shr eax, 9
ret
If factor == DNS_ADB_RTTADJAGE and addr->entry->lastage == now we would
load value into new_srtt and then immediatelly store it back which
triggers the synchronization between threads using .srtt values.
Use atomics on couple of ADB entry members (.srtt, .flags, .expires, and
.lastage) to remove ADB entry locking from couple of hot spots. The
most prominent place is copy_namehook_lists() that gets called under ADB
name lock and if the namehook list is long it acquires-releases quite a
few ADB entry locks. Changing those ADB entry members to atomics
allowed us to new_adbaddrinfo() not require locked ADB entry and since
adbentry_overquota() already used atomics and handling lame information
was dropped in the previous commit, we could not make the
copy_namehook_lists() lockless.
The other hotspot is dns_adb_adjustsrtt() and dns_adb_agesrtt() that can
now use atomics because .srtt is already atomic_uint.
And the last place that could now use atomics is dns_adb_changeflags().
Keeping the information about lame server in the ADB was done in !322 to
fix following security issue:
[CVE-2021-25219] Disable "lame-ttl" cache
The handling of the lame servers needs to be redesigned and it is not
going to be enabled any time soon, and the current code is just dead
code that takes up space, code and stands in the way of making ADB work
faster.
Remove all the internals needed for handling the lame servers in the ADB
for now. It might get reintroduced later if and when we redesign ADB.
