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.

diff --git a/lib/dns/message.c b/lib/dns/message.c
index 08a230de00685868f56e21115cc3eaa2aa646171..9c632adae3742c1251c7ffe9f7fe1151532e3f95 100644 (file)
--- a/lib/dns/message.c
+++ b/lib/dns/message.c
@@ -940,7 +940,8 @@ getrdata(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t *dctx,
        do {                                 \
                if (best_effort)             \
                        seen_problem = true; \
-               else {                       \
+               else                         \
+               {                            \
                        result = r;          \
                        goto cleanup;        \
                }                            \

diff --git a/lib/dns/name.c b/lib/dns/name.c
index 5632442f32bf671bfa5265de2f2fe1a1fcf124de..73280d06b49ec540fc4e48caa897176a2a3a9985 100644 (file)
--- a/lib/dns/name.c
+++ b/lib/dns/name.c
@@ -101,7 +101,8 @@ static unsigned char maptolower[] = {
 #define SETUP_OFFSETS(name, var, default_offsets) \
        if ((name)->offsets != NULL)              \
                var = (name)->offsets;            \
-       else {                                    \
+       else                                      \
+       {                                         \
                var = (default_offsets);          \
                set_offsets(name, var, NULL);     \
        }

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 08402f60325db226841afec14d7310eb25fed86a..b4c851ee7802b633212ca7a8a427c6be2ae7c16b 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -536,11 +536,11 @@ struct dns_resolver {
        isc_refcount_t references;
        atomic_uint_fast32_t zspill; /* fetches-per-zone */
        atomic_bool exiting;
+       atomic_bool priming;
 
        /* Locked by lock. */
        isc_eventlist_t whenshutdown;
        unsigned int activebuckets;
-       bool priming;
        unsigned int spillat; /* clients-per-query */
 
        dns_badcache_t *badcache; /* Bad cache. */
@@ -9975,7 +9975,7 @@ destroy(dns_resolver_t *res) {
        alternate_t *a;
 
        REQUIRE(atomic_load(&res->references) == 0);
-       REQUIRE(!res->priming);
+       REQUIRE(!atomic_load_acquire(&res->priming));
        REQUIRE(res->primefetch == NULL);
 
        RTRACE("destroy");
@@ -10224,7 +10224,7 @@ dns_resolver_create(dns_view_t *view, isc_taskmgr_t *taskmgr,
        atomic_init(&res->exiting, false);
        res->frozen = false;
        ISC_LIST_INIT(res->whenshutdown);
-       res->priming = false;
+       atomic_init(&res->priming, false);
        res->primefetch = NULL;
 
        atomic_init(&res->nfctx, 0);
@@ -10337,16 +10337,13 @@ prime_done(isc_task_t *task, isc_event_t *event) {
 
        UNUSED(task);
 
-       LOCK(&res->lock);
-
-       INSIST(res->priming);
-       res->priming = false;
        LOCK(&res->primelock);
        fetch = res->primefetch;
        res->primefetch = NULL;
        UNLOCK(&res->primelock);
 
-       UNLOCK(&res->lock);
+       INSIST(atomic_compare_exchange_strong_acq_rel(&res->priming,
+                                                     &(bool){ true }, false));
 
        if (fevent->result == ISC_R_SUCCESS && res->view->cache != NULL &&
            res->view->hints != NULL)
@@ -10384,17 +10381,11 @@ dns_resolver_prime(dns_resolver_t *res) {
 
        RTRACE("dns_resolver_prime");
 
-       LOCK(&res->lock);
-
-       /* XXXOND: cas needs to be used here */
-       if (!atomic_load_acquire(&res->exiting) && !res->priming) {
-               INSIST(res->primefetch == NULL);
-               res->priming = true;
-               want_priming = true;
+       if (!atomic_load_acquire(&res->exiting)) {
+               want_priming = atomic_compare_exchange_strong_acq_rel(
+                       &res->priming, &(bool){ false }, true);
        }
 
-       UNLOCK(&res->lock);
-
        if (want_priming) {
                /*
                 * To avoid any possible recursive locking problems, we
@@ -10408,19 +10399,20 @@ dns_resolver_prime(dns_resolver_t *res) {
                RTRACE("priming");
                rdataset = isc_mem_get(res->mctx, sizeof(*rdataset));
                dns_rdataset_init(rdataset);
+
                LOCK(&res->primelock);
+               INSIST(res->primefetch == NULL);
                result = dns_resolver_createfetch(
                        res, dns_rootname, dns_rdatatype_ns, NULL, NULL, NULL,
                        NULL, 0, DNS_FETCHOPT_NOFORWARD, 0, NULL,
                        res->buckets[0].task, prime_done, res, rdataset, NULL,
                        &res->primefetch);
                UNLOCK(&res->primelock);
+
                if (result != ISC_R_SUCCESS) {
                        isc_mem_put(res->mctx, rdataset, sizeof(*rdataset));
-                       LOCK(&res->lock);
-                       INSIST(res->priming);
-                       res->priming = false;
-                       UNLOCK(&res->lock);
+                       INSIST(atomic_compare_exchange_strong_acq_rel(
+                               &res->priming, &(bool){ true }, false));
                }
                inc_stats(res, dns_resstatscounter_priming);
        }

diff --git a/lib/isc/include/isc/atomic.h b/lib/isc/include/isc/atomic.h
index 7a1bc8c38f39c58a6a5678d53bdd70fb38e0c560..39a78e2b76c9c72c8dccf840037bcba93b6d2886 100644 (file)
--- a/lib/isc/include/isc/atomic.h
+++ b/lib/isc/include/isc/atomic.h
@@ -46,7 +46,7 @@
 #define atomic_compare_exchange_strong_relaxed(o, e, d) \
        atomic_compare_exchange_strong_explicit(        \
                (o), (e), (d), memory_order_relaxed, memory_order_relaxed)
-#define atomic_compare_exchange_strong_acq_rel(o, e, d)        \
+#define atomic_compare_exchange_strong_acq_rel(o, e, d) \
        atomic_compare_exchange_strong_explicit(        \
                (o), (e), (d), memory_order_acq_rel, memory_order_acquire)
 

diff --git a/lib/isc/include/isc/list.h b/lib/isc/include/isc/list.h
index bab6f1f619f5875ddb1fa15019823774a96a2378..cc38680b44175073da99da860aca930685bf80ad 100644 (file)
--- a/lib/isc/include/isc/list.h
+++ b/lib/isc/include/isc/list.h
@@ -90,13 +90,15 @@
        do {                                                            \
                if ((elt)->link.next != NULL)                           \
                        (elt)->link.next->link.prev = (elt)->link.prev; \
-               else {                                                  \
+               else                                                    \
+               {                                                       \
                        ISC_INSIST((list).tail == (elt));               \
                        (list).tail = (elt)->link.prev;                 \
                }                                                       \
                if ((elt)->link.prev != NULL)                           \
                        (elt)->link.prev->link.next = (elt)->link.next; \
-               else {                                                  \
+               else                                                    \
+               {                                                       \
                        ISC_INSIST((list).head == (elt));               \
                        (list).head = (elt)->link.next;                 \
                }                                                       \
@@ -124,7 +126,8 @@
        do {                                                    \
                if ((before)->link.prev == NULL)                \
                        ISC_LIST_PREPEND(list, elt, link);      \
-               else {                                          \
+               else                                            \
+               {                                               \
                        (elt)->link.prev = (before)->link.prev; \
                        (before)->link.prev = (elt);            \
                        (elt)->link.prev->link.next = (elt);    \
@@ -143,7 +146,8 @@
        do {                                                   \
                if ((after)->link.next == NULL)                \
                        ISC_LIST_APPEND(list, elt, link);      \
-               else {                                         \
+               else                                           \
+               {                                              \
                        (elt)->link.next = (after)->link.next; \
                        (after)->link.next = (elt);            \
                        (elt)->link.next->link.prev = (elt);   \
@@ -162,7 +166,8 @@
        do {                                                    \
                if (ISC_LIST_EMPTY(list1))                      \
                        (list1) = (list2);                      \
-               else if (!ISC_LIST_EMPTY(list2)) {              \
+               else if (!ISC_LIST_EMPTY(list2))                \
+               {                                               \
                        (list1).tail->link.next = (list2).head; \
                        (list2).head->link.prev = (list1).tail; \
                        (list1).tail = (list2).tail;            \
@@ -175,7 +180,8 @@
        do {                                                    \
                if (ISC_LIST_EMPTY(list1))                      \
                        (list1) = (list2);                      \
-               else if (!ISC_LIST_EMPTY(list2)) {              \
+               else if (!ISC_LIST_EMPTY(list2))                \
+               {                                               \
                        (list2).tail->link.next = (list1).head; \
                        (list1).head->link.prev = (list2).tail; \
                        (list1).head = (list2).head;            \

diff --git a/lib/isccc/include/isccc/util.h b/lib/isccc/include/isccc/util.h
index 9a10fd5895aa532d2f26de38d18b301c48ba8f9e..f59e9b20b1df8ef2e70a5c99515ce2f4cdbb0da0 100644 (file)
--- a/lib/isccc/include/isccc/util.h
+++ b/lib/isccc/include/isccc/util.h
@@ -85,7 +85,8 @@
                GET8(v, w);                  \
                if (v == 0)                  \
                        d = ISCCC_TRUE;      \
-               else {                       \
+               else                         \
+               {                            \
                        d = ISCCC_FALSE;     \
                        if (v == 255)        \
                                GET16(v, w); \
@@ -165,7 +166,8 @@
        do {                          \
                if (v > 0 && v < 255) \
                        PUT8(v, w);   \
-               else {                \
+               else                  \
+               {                     \
                        PUT8(255, w); \
                        PUT16(v, w);  \
                }                     \
@@ -175,7 +177,8 @@
        do {                                 \
                if (v < 0xffffffU)           \
                        PUT24(v, w);         \
-               else {                       \
+               else                         \
+               {                            \
                        PUT24(0xffffffU, w); \
                        PUT32(v, w);         \
                }                            \