Ondřej Surý [Wed, 12 Feb 2020 08:17:55 +0000 (09:17 +0100)]
Replace non-loop usage of atomic_compare_exchange_weak with strong variant
While testing BIND 9 on arm64 8+ core machine, it was discovered that
the weak variants in fact does spuriously fail, we haven't observed that
on other architectures.
This commit replaces all non-loop usage of atomic_compare_exchange_weak
with atomic_compare_exchange_strong.
Diego Fronza [Mon, 10 Feb 2020 21:01:10 +0000 (18:01 -0300)]
Fixed potential-lock-inversion
This commit simplifies a bit the lock management within dns_resolver_prime()
and prime_done() functions by means of turning resolver's attribute
"priming" into an atomic_bool and by creating only one dependent object on the
lock "primelock", namely the "primefetch" attribute.
By having the attribute "priming" as an atomic type, it save us from having to
use a lock just to test if priming is on or off for the given resolver context
object, within "dns_resolver_prime" function.
The "primelock" lock is still necessary, since dns_resolver_prime() function
internally calls dns_resolver_createfetch(), and whenever this function
succeeds it registers an event in the task manager which could be called by
another thread, namely the "prime_done" function, and this function is
responsible for disposing the "primefetch" attribute in the resolver object,
also for resetting "priming" attribute to false.
It is important that the invariant "priming == false AND primefetch == NULL"
remains constant, so that any thread calling "dns_resolver_prime" knows for sure
that if the "priming" attribute is false, "primefetch" attribute should also be
NULL, so a new fetch context could be created to fulfill this purpose, and
assigned to "primefetch" attribute under the lock protection.
To honor the explanation above, dns_resolver_prime is implemented as follow:
1. Atomically checks the attribute "priming" for the given resolver context.
2. If "priming" is false, assumes that "primefetch" is NULL (this is
ensured by the "prime_done" implementation), acquire "primelock"
lock and create a new fetch context, update "primefetch" pointer to
point to the newly allocated fetch context.
3. If "priming" is true, assumes that the job is already in progress,
no locks are acquired, nothing else to do.
To keep the previous invariant consistent, "prime_done" is implemented as follow:
1. Acquire "primefetch" lock.
2. Keep a reference to the current "primefetch" object;
3. Reset "primefetch" attribute to NULL.
4. Release "primefetch" lock.
5. Atomically update "priming" attribute to false.
6. Destroy the "primefetch" object by using the temporary reference.
This ensures that if "priming" is false, "primefetch" was already reset to NULL.
It doesn't make any difference in having the "priming" attribute not protected
by a lock, since the visible state of this variable would depend on the calling
order of the functions "dns_resolver_prime" and "prime_done".
As an example, suppose that instead of using an atomic for the "priming" attribute
we employed a lock to protect it.
Now suppose that "prime_done" function is called by Thread A, it is then preempted
before acquiring the lock, thus not reseting "priming" to false.
In parallel to that suppose that a Thread B is scheduled and that it calls
"dns_resolver_prime()", it then acquires the lock and check that "priming" is true,
thus it will consider that this resolver object is already priming and it won't do
any more job.
Conversely if the lock order was acquired in the other direction, Thread B would check
that "priming" is false (since prime_done acquired the lock first and set "priming" to false)
and it would initiate a priming fetch for this resolver.
An atomic variable wouldn't change this behavior, since it would behave exactly the
same, depending on the function call order, with the exception that it would avoid
having to use a lock.
There should be no side effects resulting from this change, since the previous
implementation employed use of the more general resolver's "lock" mutex, which
is used in far more contexts, but in the specifics of the "dns_resolver_prime"
and "prime_done" it was only used to protect "primefetch" and "priming" attributes,
which are not used in any of the other critical sections protected by the same lock,
thus having zero dependency on those variables.
Evan Hunt [Thu, 13 Feb 2020 18:16:25 +0000 (10:16 -0800)]
revise .clang-format and add a C formatting script in util
- add util/cformat.sh, which runs clang-format on all C files with
the default .clang-format, and on all header files with a slightly
modified version.
- use correct bracing after multi-line control statements
- stop aligning variable declarations to avoid problems with pointer
alignment, but retain aligned declarations in header files so that
struct definitions look cleaner.
- static function prototypes in C files can skip the line break after
the return type, but function prototypes in header files still have
the line break.
- don't break-before-brace in function definitions. ISC style calls
for braces on the same line when function parameters fit on a single
line, and a line break if they don't, but clang-format doesn't yet
support that distinction. one-line function definitions are about
four times more common than multi-line, so let's use the option that
deviates less.
Michal Nowak [Wed, 12 Feb 2020 14:44:55 +0000 (14:44 +0000)]
Run Coverity Scan only when specific variables are present
Submissions to Coverity Scan should be limited to those originated from
release branches and only from a specific schedule which holds
COVERITY_SCAN_PROJECT_NAME and COVERITY_SCAN_TOKEN variables.
Witold Kręcicki [Mon, 10 Feb 2020 13:00:36 +0000 (14:00 +0100)]
Don't limit the size of uvreq/nmhandle pool artificially.
There was a hard limit set on number of uvreq and nmhandles
that can be allocated by a pool, but we don't handle a situation
where we can't get an uvreq. Don't limit the number at all,
let the OS deal with it.
Ondřej Surý [Sat, 1 Feb 2020 09:48:20 +0000 (10:48 +0100)]
Convert all atomic operations in isc_rwlock to release-acquire memory ordering
The memory ordering in the rwlock was all wrong, I am copying excerpts
from the https://en.cppreference.com/w/c/atomic/memory_order#Relaxed_ordering
for the convenience of the reader:
Relaxed ordering
Atomic operations tagged memory_order_relaxed are not synchronization
operations; they do not impose an order among concurrent memory
accesses. They only guarantee atomicity and modification order
consistency.
Release-Acquire ordering
If an atomic store in thread A is tagged memory_order_release and an
atomic load in thread B from the same variable is tagged
memory_order_acquire, all memory writes (non-atomic and relaxed atomic)
that happened-before the atomic store from the point of view of thread
A, become visible side-effects in thread B. That is, once the atomic
load is completed, thread B is guaranteed to see everything thread A
wrote to memory.
The synchronization is established only between the threads releasing
and acquiring the same atomic variable. Other threads can see different
order of memory accesses than either or both of the synchronized
threads.
Which basically means that we had no or weak synchronization between
threads using the same variables in the rwlock structure. There should
not be a significant performance drop because the critical sections were
already protected by:
while(1) {
if (relaxed_atomic_operation) {
break;
}
LOCK(lock);
if (!relaxed_atomic_operation) {
WAIT(sem, lock);
}
UNLOCK(lock)l
}
I would add one more thing to "Don't do your own crypto, folks.":
Ondřej Surý [Wed, 22 Jan 2020 09:16:22 +0000 (10:16 +0100)]
Cleanup support for specifying PKCS#11 engine as part of the label
The code for specifying OpenSSL PKCS#11 engine as part of the label
(e.g. -l "pkcs11:token=..." instead of -E pkcs11 -l "token=...")
was non-functional. This commit just cleans the related code.
Ondřej Surý [Sat, 8 Feb 2020 12:37:54 +0000 (04:37 -0800)]
Clear the pointer to destroyed object early using the semantic patch
Also disable the semantic patch as the code needs tweaks here and there because
some destroy functions might not destroy the object and return early if the
object is still in use.
Ondřej Surý [Sat, 8 Feb 2020 12:31:51 +0000 (04:31 -0800)]
Add semantic patch to NULL the destroyed pointer early
Our destroy functions usually look like this:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
...destroy the contents of foo...
*foop = NULL;
}
nulling the pointer should be done as soon as possible which is
not always the case. This commit adds simple semantic patch that
changes the example function to:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
*foop = NULL;
...destroy the contents of foo...
}
Michał Kępień [Thu, 6 Feb 2020 12:36:32 +0000 (13:36 +0100)]
Fix the "pipelined" system test on OpenBSD
On OpenBSD, the bin/tests/system/pipelined/ans5/ans.py script does not
shut down when it is sent the SIGTERM signal. What seems to be
happening is that starting the UDP listening thread somehow makes the
accept() calls in the script's main thread uninterruptible and thus the
SIGTERM signal sent to the main thread does not get processed until a
TCP connection is established with the script's TCP socket. Work around
the issue by setting a timeout for operations performed on the script's
TCP socket, so that each accept() call in the main thread's infinite
loop returns after at most 1 second, allowing termination signals sent
to the script to be processed.
The key-directory keyword actually does nothing right now but may
be useful in the future if we want to differentiate between key
directories or HSM keys, or if we want to speficy different
directories for different keys or policies. Make it optional for
the time being.
Witold Kręcicki [Tue, 28 Jan 2020 07:46:52 +0000 (08:46 +0100)]
Disable OpenSSL siphash.
Creation of EVP_MD_CTX and EVP_PKEY is quite expensive, until
we fix the code to reuse the context and key we'll use our own
implementation of siphash.
CID 1452691 (#1 of 1): Unchecked return value (CHECKED_RETURN)
5. check_return: Calling dns_db_find without checking return
value (as is done elsewhere 39 out of 45 times).
Add checks to the kasp system test to verify CDNSKEY publication.
This test is not entirely complete, because when there is a CDNSKEY
available but there should not be one for KEY N, it is hard to tell
whether the existing CDNSKEY actually belongs to KEY N or another
key.
The check works if we expect a CDNSKEY although we cannot guarantee
that the CDNSKEY is correct: The test verifies existence, not
correctness of the record.
When you do a restart or reconfig of named, or rndc loadkeys, this
triggers the key manager to run. The key manager will check if new
keys need to be created. If there is an active key, and key rollover
is scheduled far enough away, no new key needs to be created.
However, there was a bug that when you just start to sign your zone,
it takes a while before the KSK becomes an active key. An active KSK
has its DS submitted or published, but before the key manager allows
that, the DNSKEY needs to be omnipresent. If you restart named
or rndc loadkeys in quick succession when you just started to sign
your zone, new keys will be created because the KSK is not yet
considered active.
Fix is to check for introducing as well as active keys. These keys
all have in common that their goal is to become omnipresent.
projects / thirdparty / bind9.git / log
