DSCP has not been fully working since the network manager was
introduced in 9.16, and has been completely broken since 9.18.
This seems to have caused very few difficulties for anyone,
so we have now marked it as obsolete and removed the
implementation.
To ensure that old config files don't fail, the code to parse
dscp key-value pairs is still present, but a warning is logged
that the feature is obsolete and should not be used. Nothing is
done with configured values, and there is no longer any
range checking.
Previously, dns_dispatch_gettcp() could pick a TCP connection created by
different thread - this breaks our contractual promise to DNS dispatch
by using the TCP connection on a different thread than it was created.
Add .tid member to the dns_dispatch_t struct and skip the dispatches
from other threads when looking up a TCP dispatch that we can reuse in
dns_request.
NOTE: This is going to be properly refactored, but this change could be
also backported to 9.18 for better stability and thread-affinity.
The dns_request code is very sensitive about calling the connected and
deadlocks when the timing is "right" in several places. Move the call
to the connected callback to the (udp|tcp)_connected() functions, so
they are called asynchronously instead of directly from
the (udp|tcp)_dispentry_cancel() functions.
The TCP dispatches are removed from the dispatchmgr->list in the
dispatch_destroy() and there's a brief period of time where
dns_dispatch_gettcp() can find a dispatch in connected state that's
being destroyed.
Set the dispatch state to DNS_DISPATCHSTATE_NONE in the TCP connection
callback if there are no responses waiting, and ignore TCP dispatches
with zero references in dns_dispatch_gettcp().
In tcp_connected() a typo has turned a DbC check into an assignment
breaking the state machine and making the dns_dispatch_gettcp() try to
attach to dispatch in process of destruction.
The TCP dispatches in DNS_DISPATCHSTATE_NONE could be either very
fresh or those could be dispatches that failed connecting to the
destination. Ignore them when trying to connect to an existing
TCP dispatch via dns_dispatch_gettcp().
The dispatches are not thread-bound, and used freely between various
threads (see the dns_resolver and dns_request units for details).
This refactoring make sure that all non-const dns_dispatch_t and
dns_dispentry_t members are accessed under a lock, and both object now
track their internal state (NONE, CONNECTING, CONNECTED, CANCELED)
instead of guessing the state from the state of various struct members.
During the refactoring, the artificial limit DNS_DISPATCH_SOCKSQUOTA on
UDP sockets per dispatch was removed as the limiting needs to happen and
happens on in dns_resolver and limiting the number of UDP sockets
artificially in dispatch could lead to unpredictable behaviour in case
one dispatch has the limit exhausted by others are idle.
The TCP artificial limit of DNS_DISPATCH_MAXREQUESTS makes even less
sense as the TCP connections are only reused in the dns_request API
that's not a heavy user of the outgoing connections.
As a side note, the fact that UDP and TCP dispatch pretends to be same
thing, but in fact the connected UDP is handled from dns_dispentry_t and
dns_dispatch_t acts as a broker, but connected TCP is handled from
dns_dispatch_t and dns_dispatchmgr_t acts as a broker doesn't really
help the clarity of this unit.
This refactoring kept to API almost same - only dns_dispatch_cancel()
and dns_dispatch_done() were merged into dns_dispatch_done() as we need
to cancel active netmgr handles in any case to not leave dangling
connections around. The functions handling UDP and TCP have been mostly
split to their matching counterparts and the dns_dispatch_<function>
functions are now thing wrappers that call <udp|tcp>_dispatch_<function>
based on the socket type.
More debugging-level logging was added to the unit to accomodate for
this fact.
This change prepares ground for sending DNS requests using DoT,
which, in particular, will be used for forwarding dynamic updates
to TLS-enabled primaries.
When a thread calls dns_dispatch_connect() on an unconnected TCP socket
it sets `tcpstate` from `DNS_DISPATCHSTATE_NONE` to `_CONNECTING`.
Previously, it then INSISTed that there were no pending connections
before calling isc_nm_tcpdnsconnect().
If a second thread called dns_dispatch_connect() during that window
of time, it could add a pending connection to the list, and trigger
an assertion failure.
This commit removes the INSIST since the condition is actually
harmless.
it's a style violation to have REQUIRE or INSIST contain code that
must run for the server to work. this was being done with some
atomic_compare_exchange calls. these have been cleaned up. uses
of atomic_compare_exchange in assertions have been replaced with
a new macro atomic_compare_exchange_enforced, which uses RUNTIME_CHECK
to ensure that the exchange was successful.
There is a possibility for `udp_recv()` to be called with `eresult`
being `ISC_R_SUCCESS`, but nevertheless with already deactivated `resp`,
which can happen when the request has been canceled in the meantime.
Previously, it was possible to assign a bit of memory space in the
nmhandle to store the client data. This was complicated and prevents
further refactoring of isc_nmhandle_t caching (future work).
Instead of caching the data in the nmhandle, allocate the hot-path
ns_client_t objects from per-thread clientmgr memory context and just
assign it to the isc_nmhandle_t via isc_nmhandle_set().
Historically, the inline keyword was a strong suggestion to the compiler
that it should inline the function marked inline. As compilers became
better at optimising, this functionality has receded, and using inline
as a suggestion to inline a function is obsolete. The compiler will
happily ignore it and inline something else entirely if it finds that's
a better optimisation.
Therefore, remove all the occurences of the inline keyword with static
functions inside single compilation unit and leave the decision whether
to inline a function or not entirely on the compiler
NOTE: We keep the usage the inline keyword when the purpose is to change
the linkage behaviour.
Previously, the unreachable code paths would have to be tagged with:
INSIST(0);
ISC_UNREACHABLE();
There was also older parts of the code that used comment annotation:
/* NOTREACHED */
Unify the handling of unreachable code paths to just use:
UNREACHABLE();
The UNREACHABLE() macro now asserts when reached and also uses
__builtin_unreachable(); when such builtin is available in the compiler.
Gcc 7+ and Clang 10+ have implemented __attribute__((fallthrough)) which
is explicit version of the /* FALLTHROUGH */ comment we are currently
using.
Add and apply FALLTHROUGH macro that uses the attribute if available,
but does nothing on older compilers.
In one case (lib/dns/zone.c), using the macro revealed that we were
using the /* FALLTHROUGH */ comment in wrong place, remove that comment.
- certain TCP result codes, including ISC_R_EOF and
ISC_R_CONNECTIONRESET, were being mapped to ISC_R_SHUTTINGDOWN
before calling the response handler in tcp_recv_cancelall().
the result codes should be passed through to the response handler
without being changed.
- the response handlers, resquery_response() and req_response(), had
code to return immediately if encountering ISC_R_EOF, but this is
not the correct behavior; that should only happen in the case of
ISC_R_CANCELED when it was the caller that canceled the operation
- ISC_R_CONNECTIONRESET was not being caught in rctx_dispfail().
- removed code in rctx_dispfail() to retry queries without EDNS
when receiving ISC_R_EOF; this is now treated the same as any
other connection failure.
This commit converts the license handling to adhere to the REUSE
specification. It specifically:
1. Adds used licnses to LICENSES/ directory
2. Add "isc" template for adding the copyright boilerplate
3. Changes all source files to include copyright and SPDX license
header, this includes all the C sources, documentation, zone files,
configuration files. There are notes in the doc/dev/copyrights file
on how to add correct headers to the new files.
4. Handle the rest that can't be modified via .reuse/dep5 file. The
binary (or otherwise unmodifiable) files could have license places
next to them in <foo>.license file, but this would lead to cluttered
repository and most of the files handled in the .reuse/dep5 file are
system test files.
the 'dipsatchmgr->state' was never set, so the MGR_IS_SHUTTINGDOWN
macro was always false. both of these have been removed.
renamed the 'dispatch->state' field to 'tcpstate' to make its purpose
less ambiguous.
changed an FCTXTRACE log message from "response did not match question"
to the more correctly descriptive "invalid question section".
When a non-matching DNS response is received by the resolver,
it calls dns_dispatch_getnext() to resume reading. This is necessary
for UDP but not for TCP, because TCP connections automatically
resume reading after any valid DNS response.
This commit adds a 'tcpreading' flag to TCP dispatches, so that
`dispatch_getnext()` can be called multiple times without subsequent
calls having any effect.
A TCP connection may be held open past its proper timeout if it's
receiving a stream of DNS responses that don't match any queries.
In this case, we now check whether the oldest query should have timed
out.
When the outgoing TCP dispatch times-out active response, we might still
receive the answer during the lifetime of the connection. Previously,
we would just ignore any non-matching DNS answers, which would allow the
server to feed us with otherwise valid DNS answer and keep the
connection open.
Add a counter for timed-out DNS queries over TCP and tear down the whole
TCP connection if we receive unexpected number of DNS answers.
Previously, when invalid DNS message is received over TCP we throw the
garbage DNS message away and continued looking for valid DNS message
that would match our outgoing queries. This logic makes sense for UDP,
because anyone can send DNS message over UDP.
Change the logic that the TCP connection is closed when we receive
garbage, because the other side is acting malicious.
When outgoing TCP connection was prematurely terminated (f.e. with
connection reset), the dispatch code would not cleanup the resources
used by such connection leading to dangling dns_dispentry_t entries.
When a UDP dispatch receives a mismatched response, it checks whether
there is still enough time to wait for the correct one to arrive before
the timeout fires. If there is not, the result code is set to
ISC_R_TIMEDOUT, but it is not subsequently used anywhere as 'response'
is set to NULL a few lines earlier. This results in the higher-level
read callback (resquery_response() in case of resolver code) not being
called. However, shortly afterwards, a few levels up the call chain,
isc__nm_udp_read_cb() calls isc__nmsocket_timer_stop() on the dispatch
socket, effectively disabling read timeout handling for that socket.
Combined with the fact that reading is not restarted in such a case
(e.g. by calling dispatch_getnext() from udp_recv()), this leads to the
higher-level query structure remaining referenced indefinitely because
the dispatch socket it uses will neither be read from nor closed due to
a timeout. This in turn causes fetch contexts to linger around
indefinitely, which in turn i.a. prevents certain cache nodes (those
containing rdatasets used by fetch contexts, like fctx->nameservers)
from being cleaned.
Fix by making sure the higher-level callback does get invoked with the
ISC_R_TIMEDOUT result code when udp_recv() determines there is no more
time left to receive the correct UDP response before the timeout fires.
This allows the higher-level callback to clean things up, preventing the
reference leak described above.
there was a race possible in which a dispatch was put into
the 'connected' state before it had a TCP handle attached,
which could cause an assertion failure in dns_dispatch_gettcp().
Renamed some functions for clarity and readability:
- dns_dispatch_addresponse() -> dns_dispatch_add()
- dns_dispatch_removeresponse() -> dns_dispatch_done()
The dns_dispatch_cancel() function now calls dns_dispatch_done()
directly, so it is no longer ever necessary to call both functions.
dns_dispatch_cancel() is used to terminate dispatch connections
that are still pending, while dns_dispatch_done() is used when they
are complete.
udp_recv() will call dispatch_getnext() if the message received is
invalid or doesn't match; we need to reduce the timeout each time this
happens so we can't be starved forever by someone sending garbage
packets.
- disp_connected() has been split into two functions,
udp_connected() (which takes 'resp' as an argument) and
tcp_connected() (which takes 'disp', and calls the connect callbacks
for all pending resps).
- In dns_dispatch_connect(), if a connection is already open, we need to
detach the dispentry immediately because we won't be running
tcp_connected().
- dns_disptach_cancel() also now calls the connect callbacks for pending
TCP responses, and the response callbacks for open TCP connections
waiting on read.
- If udp_connected() runs after dns_dispatch_cancel() has been called,
ensure that the caller's connect callback is run.
- If a UDP connection fails with EADDRINUSE, we try again up to five
times with a different local port number before giving up.
- If a TCP connection is canceled while still pending connection, the
connect timeout may still fire. we attach the dispatch before
connecting to ensure that it won't be detached too soon in this case.
- The dispentry is no longer removed from the pending list when
deactivating, so that the connect callback can still be run if
dns_dispatch_removeresponse() was run while the connecting was
pending.
- Rewrote dns_dispatch_gettcp() to avoid a data race.
- startrecv() and dispatch_getnext() can be called with a NULL resp when
using TCP.
- Refactored udp_recv() and tcp_recv() and added result logging.
- EOF is now treated the same as CANCELED in response callbacks.
- ISC_R_SHUTTINGDOWN is sent to the reponse callbacks for all resps if
tcp_recv() is triggered by a netmgr shutdown. (response callbacks
are *not* sent by udp_recv() in this case.)
- startrecv() and getnext() have been rewritten.
- Don't set TCP flag when connecting a UDP dispatch.
- Prevent TCP connections from trying to connect twice.
- dns_dispatch_gettcp() can now find a matching TCP dispatch that has
not yet fully connected, and attach to it. when the connection is
completed, the connect callbacks are run for all of the pending
entries.
- An atomic 'state' variable is now used for connection state instead of
attributes.
- When dns_dispatch_cancel() is called on a TCP dispatch entry, only
that one entry is canceled. the dispatch itself should not be shut
down until there are no dispatch entries left associated with it.
- Other incidental cleanup, including removing DNS_DISPATCHATTR_IPV4 and
_IPV6 (they were being set in the dispatch attributes but never used),
cleaning up dns_requestmgr_create(), and renaming dns_dispatch_read()
to the more descriptive dns_dispatch_resume().
- It is no longer necessary to pass a 'timeout' callback to
dns_dispatch_addresponse(); timeouts are handled directly by the
'response' callback instead.
- The netmgr handle is no longer passed to dispatch callbacks, since
they don't (and can't) use it. instead, dispatch_cb_t now takes a
result code, region, and argument.
- Cleaned up timeout-related tests in dispatch_test.c
- Responses received by the dispatch are no longer sent to the caller
via a task event, but via a netmgr-style recv callback. the 'action'
parameter to dns_dispatch_addresponse() is now called 'response' and
is called directly from udp_recv() or tcp_recv() when a valid response
has been received.
- All references to isc_task and isc_taskmgr have been removed from
dispatch functions.
- All references to dns_dispatchevent_t have been removed and the type
has been deleted.
- Added a task to the resolver response context, to be used for fctx
events.
- When the caller cancels an operation, the response handler will be
called with ISC_R_CANCELED; it can abort immediately since the caller
will presumably have taken care of cleanup already.
- Cleaned up attach/detach in resquery and request.
Remove the debugging printfs. (leaving this as a separate commit rather
than squashing it so we can restore it in the future if we ever need it
again.)
Since every dispsock was associated with a dispentry anyway (though not
always vice versa), the members of dispsock have been combined into
dispentry, which is now reference-counted. dispentry objects are now
attached before connecting and detached afterward to prevent races
between the connect callback and dns_dispatch_removeresponse().
Dispatch and dispatchmgr objects are now reference counted as well, and
the shutdown process has been simplified. reference counting of
resquery and request objects has also been cleaned up significantly.
dns_dispatch_cancel() now flags a dispentry as having been canceled, so
that if the connect callback runs after cancellation, it will not
initiate a read.
The isblackholed() function has been simplified.
- The `timeout_action` parameter to dns_dispatch_addresponse() been
replaced with a netmgr callback that is called when a dispatch read
times out. this callback may optionally reset the read timer and
resume reading.
- Added a function to convert isc_interval to milliseconds; this is used
to translate fctx->interval into a value that can be passed to
dns_dispatch_addresponse() as the timeout.
- Note that netmgr timeouts are accurate to the millisecond, so code to
check whether a timeout has been reached cannot rely on microsecond
accuracy.
- If serve-stale is configured, then a timeout received by the resolver
may trigger it to return stale data, and then resume waiting for the
read timeout. this is no longer based on a separate stale timer.
- The code for canceling requests in request.c has been altered so that
it can run asynchronously.
- TCP timeout events apply to the dispatch, which may be shared by
multiple queries. since in the event of a timeout we have no query ID
to use to identify the resp we wanted, we now just send the timeout to
the oldest query that was pending.
- There was some additional refactoring in the resolver: combining
fctx_join() and fctx_try_events() into one function to reduce code
duplication, and using fixednames in fetchctx and fetchevent.
- Incidental fix: new_adbaddrinfo() can't return NULL anymore, so the
code can be simplified.
The flow of operations in dispatch is changing and will now be similar
for both UDP and TCP queries:
1) Call dns_dispatch_addresponse() to assign a query ID and register
that we'll be listening for a response with that ID soon. the
parameters for this function include callback functions to inform the
caller when the socket is connected and when the message has been
sent, as well as a task action that will be sent when the response
arrives. (later this could become a netmgr callback, but at this
stage to minimize disruption to the calling code, we continue to use
isc_task for the response event.) on successful completion of this
function, a dispatch entry object will be instantiated.
2) Call dns_dispatch_connect() on the dispatch entry. this runs
isc_nm_udpconnect() or isc_nm_tcpdnsconnect(), as needed, and begins
listening for responses. the caller is informed via a callback
function when the connection is established.
3) Call dns_dispatch_send() on the dispatch entry. this runs
isc_nm_send() to send a request.
4) Call dns_dispatch_removeresponse() to terminate listening and close
the connection.
Implementation comments below:
- As we will be using netmgr buffers now. code to send the length in
TCP queries has also been removed as that is handled by the netmgr.
- TCP dispatches can be used by multiple simultaneous queries, so
dns_dispatch_connect() now checks whether the dispatch is already
connected before calling isc_nm_tcpdnsconnect() again.
- Running dns_dispatch_getnext() from a non-network thread caused a
crash due to assertions in the netmgr read functions that appear to be
unnecessary now. the assertions have been removed.
- fctx->nqueries was formerly incremented when the connection was
successful, but is now incremented when the query is started and
decremented if the connection fails.
- It's no longer necessary for each dispatch to have a pool of tasks, so
there's now a single task per dispatch.
- Dispatch code to avoid UDP ports already in use has been removed.
- dns_resolver and dns_request have been modified to use netmgr callback
functions instead of task events. some additional changes were needed
to handle shutdown processing correctly.
- Timeout processing is not yet fully converted to use netmgr timeouts.
- Fixed a lock order cycle reported by TSAN (view -> zone-> adb -> view)
by by calling dns_zt functions without holding the view lock.
We now use dns_dispatch_cancel() for this purpose. NOTE: The caller
still has to track whether there are pending send or connect events in
the dispatch or dispatch entry; later this should be moved into the
dispatch module as well.
Also removed some public dns_dispatch_*() API calls that are no longer
used outside dispatch itself.
dns_dispatch_connect() connects a dispatch socket (for TCP) or a
dispatch entry socket (for UDP). This is the next step in moving all
uses of the isc_socket code into the dispatch module.
This API is temporary; it needs to be cleaned up further so that it can
be called the same way for both TCP and UDP.
Continuing the effort to move all uses of the isc_socket API into
dispatch.c, this commit removes the dns_tcpmsg module entirely, as
dispatch was its only caller, and moves the parts of its functionality
that were being used into the dispatch module.
This code will be removed when we switch to using netmgr TCPDNS.
Previously, creation of TCP dispatches differed from UDP in that a TCP
dispatch was created to attach to an existing socket, whereas a UDP
dispatch would be created in a vacuum and sockets would be opened on
demand when a transaction was initiated.
We are moving as much socket code as possible into the dispatch module,
so that it can be replaced with a netmgr version as easily as
possible. (This will also have the side effect of making TCP and UDP
dispatches more similar.)
As a step in that direction, this commit changes
dns_dispatch_createtcp() so that it creates the TCP socket.
