]> git.ipfire.org Git - thirdparty/bind9.git/commitdiff
3661. [bug] Address lock order reversal deadlock with inline zones.
authorMark Andrews <marka@isc.org>
Tue, 8 Oct 2013 00:43:08 +0000 (11:43 +1100)
committerMark Andrews <marka@isc.org>
Tue, 8 Oct 2013 00:43:08 +0000 (11:43 +1100)
                        [RT #34856]

CHANGES
config.h.in
configure
configure.in
lib/dns/zone.c
lib/isc/nothreads/include/isc/thread.h
lib/isc/pthreads/include/isc/thread.h
lib/isc/pthreads/thread.c
lib/isc/win32/include/isc/thread.h

diff --git a/CHANGES b/CHANGES
index 6fda168e6aae526ccf79c69dc711c1a12a31eafc..b27f7e52665c7bcefb0f82eade73f711c916fd7d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+3661.  [bug]           Address lock order reversal deadlock with inline zones.
+                       [RT #34856]
+
 3660.  [cleanup]       Changed the name of "isc-config.sh" to "bind9-config".
                        [RT #23825]
 
index 89aa492ffed47993642b144c610f035b13deac5d..0607f0842fe98d847713bee6a83b9db36ee2637c 100644 (file)
@@ -269,6 +269,9 @@ int sigwait(const unsigned int *set, int *sig);
 /* Define to 1 if you have the `pthread' library (-lpthread). */
 #undef HAVE_LIBPTHREAD
 
+/* Define to 1 if you have the `rt' library (-lrt). */
+#undef HAVE_LIBRT
+
 /* Define to 1 if you have the `scf' library (-lscf). */
 #undef HAVE_LIBSCF
 
@@ -308,12 +311,24 @@ int sigwait(const unsigned int *set, int *sig);
 /* Define if your OpenSSL version supports GOST. */
 #undef HAVE_OPENSSL_GOST
 
+/* Define to 1 if you have the `pthread_yield' function. */
+#undef HAVE_PTHREAD_YIELD
+
+/* Define to 1 if you have the `pthread_yield_np' function. */
+#undef HAVE_PTHREAD_YIELD_NP
+
 /* Define to 1 if you have the `readline' function. */
 #undef HAVE_READLINE
 
 /* Define to 1 if you have the <regex.h> header file. */
 #undef HAVE_REGEX_H
 
+/* Define to 1 if you have the <sched.h> header file. */
+#undef HAVE_SCHED_H
+
+/* Define to 1 if you have the `sched_yield' function. */
+#undef HAVE_SCHED_YIELD
+
 /* Define to 1 if you have the `setegid' function. */
 #undef HAVE_SETEGID
 
index a77b2f84d6cf31c9ee670f143ab2db0f565de040..e50e85f22ec2827750049fb679e30ac34db1f89e 100755 (executable)
--- a/configure
+++ b/configure
@@ -15852,6 +15852,82 @@ if test "x$ac_cv_func_pthread_attr_setstacksize" = xyes; then :
 fi
 
 
+       for ac_header in sched.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "sched.h" "ac_cv_header_sched_h" "$ac_includes_default"
+if test "x$ac_cv_header_sched_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_SCHED_H 1
+_ACEOF
+
+fi
+
+done
+
+
+       case "$host" in
+       *solaris-*)
+               { $as_echo "$as_me:${as_lineno-$LINENO}: checking for sched_yield in -lrt" >&5
+$as_echo_n "checking for sched_yield in -lrt... " >&6; }
+if ${ac_cv_lib_rt_sched_yield+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lrt  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char sched_yield ();
+int
+main ()
+{
+return sched_yield ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_rt_sched_yield=yes
+else
+  ac_cv_lib_rt_sched_yield=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_rt_sched_yield" >&5
+$as_echo "$ac_cv_lib_rt_sched_yield" >&6; }
+if test "x$ac_cv_lib_rt_sched_yield" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBRT 1
+_ACEOF
+
+  LIBS="-lrt $LIBS"
+
+fi
+
+               ;;
+       esac
+
+       for ac_func in sched_yield pthread_yield pthread_yield_np
+do :
+  as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
+ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
+if eval test \"x\$"$as_ac_var"\" = x"yes"; then :
+  cat >>confdefs.h <<_ACEOF
+#define `$as_echo "HAVE_$ac_func" | $as_tr_cpp` 1
+_ACEOF
+
+fi
+done
+
+
        #
        # Additional OS-specific issues related to pthreads and sigwait.
        #
index f577dc88c237d7184dfd84f24d40aeeb4e4e55fb..a90de0fda169aef3a67a4acacfea5b5dcb2b9119 100644 (file)
@@ -1398,6 +1398,16 @@ then
        AC_CHECK_FUNC(pthread_attr_setstacksize,
                      AC_DEFINE(HAVE_PTHREAD_ATTR_SETSTACKSIZE),)
 
+       AC_CHECK_HEADERS(sched.h)
+
+       case "$host" in
+       *solaris-*)
+               AC_CHECK_LIB(rt, sched_yield)
+               ;;
+       esac
+
+       AC_CHECK_FUNCS(sched_yield pthread_yield pthread_yield_np)
+
        #
        # Additional OS-specific issues related to pthreads and sigwait.
        #
index 246540f1e293b8c20386f997cd49869d607fa75f..d4c22d3fa85a02d812979e87b9f4d806cb32e226 100644 (file)
@@ -37,6 +37,7 @@
 #include <isc/strerror.h>
 #include <isc/string.h>
 #include <isc/taskpool.h>
+#include <isc/thread.h>
 #include <isc/timer.h>
 #include <isc/util.h>
 
@@ -164,10 +165,20 @@ typedef struct dns_include dns_include_t;
 #define UNLOCK_ZONE(z) \
        do { (z)->locked = ISC_FALSE; UNLOCK(&(z)->lock); } while (0)
 #define LOCKED_ZONE(z) ((z)->locked)
+#define TRYLOCK_ZONE(result, z) \
+       do { \
+             result = pthread_mutex_trylock(&(z)->lock); \
+             if (result == ISC_R_SUCCESS) {  \
+                    INSIST((z)->locked == ISC_FALSE); \
+                    (z)->locked = ISC_TRUE; \
+             } \
+       } while (0)
 #else
 #define LOCK_ZONE(z) LOCK(&(z)->lock)
 #define UNLOCK_ZONE(z) UNLOCK(&(z)->lock)
 #define LOCKED_ZONE(z) ISC_TRUE
+#define TRYLOCK_ZONE(result, z) \
+       do { result = pthread_mutex_trylock(&(z)->lock); } while (0)
 #endif
 
 #ifdef ISC_RWLOCK_USEATOMIC
@@ -698,7 +709,6 @@ static void zone_name_tostr(dns_zone_t *zone, char *buf, size_t length);
 static void zone_rdclass_tostr(dns_zone_t *zone, char *buf, size_t length);
 static void zone_viewname_tostr(dns_zone_t *zone, char *buf, size_t length);
 static isc_result_t zone_send_secureserial(dns_zone_t *zone,
-                                          isc_boolean_t secure_locked,
                                           isc_uint32_t serial);
 
 #if 0
@@ -754,8 +764,7 @@ static isc_result_t delete_nsec(dns_db_t *db, dns_dbversion_t *ver,
                                dns_dbnode_t *node, dns_name_t *name,
                                dns_diff_t *diff);
 static void zone_rekey(dns_zone_t *zone);
-static isc_result_t zone_send_securedb(dns_zone_t *zone, isc_boolean_t locked,
-                                      dns_db_t *db);
+static isc_result_t zone_send_securedb(dns_zone_t *zone, dns_db_t *db);
 
 #define ENTER zone_debuglog(zone, me, 1, "enter")
 
@@ -3982,9 +3991,9 @@ maybe_send_secure(dns_zone_t *zone) {
                                                  NULL, &soacount, &serial, NULL,
                                                  NULL, NULL, NULL, NULL);
                        if (result == ISC_R_SUCCESS && soacount > 0U)
-                               zone_send_secureserial(zone->raw, ISC_TRUE, serial);
+                               zone_send_secureserial(zone->raw, serial);
                } else
-                       zone_send_securedb(zone->raw, ISC_TRUE, zone->raw->db);
+                       zone_send_securedb(zone->raw, zone->raw->db);
 
        } else
                DNS_ZONE_SETFLAG(zone->raw, DNS_ZONEFLG_SENDSECURE);
@@ -4006,6 +4015,7 @@ zone_unchanged(dns_db_t *db1, dns_db_t *db2, isc_mem_t *mctx) {
 
 /*
  * The zone is presumed to be locked.
+ * If this is a inline_raw zone the secure version is also locked.
  */
 static isc_result_t
 zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
@@ -4021,6 +4031,10 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
        isc_boolean_t nomaster = ISC_FALSE;
        unsigned int options;
 
+       INSIST(LOCKED_ZONE(zone));
+       if (inline_raw(zone))
+               INSIST(LOCKED_ZONE(zone->secure));
+
        TIME_NOW(&now);
 
        /*
@@ -4373,9 +4387,9 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
                    inline_raw(zone))
                {
                        if (zone->secure->db == NULL)
-                               zone_send_securedb(zone, ISC_FALSE, db);
+                               zone_send_securedb(zone, db);
                        else
-                               zone_send_secureserial(zone, ISC_FALSE, serial);
+                               zone_send_secureserial(zone, serial);
                }
        }
 
@@ -9252,11 +9266,28 @@ void
 dns_zone_markdirty(dns_zone_t *zone) {
        isc_uint32_t serial;
        isc_result_t result = ISC_R_SUCCESS;
+       dns_zone_t *secure = NULL;
 
+       /*
+        * Obtaining a lock on the zone->secure (see zone_send_secureserial)
+        * could result in a deadlock due to a LOR so we will spin if we
+        * can't obtain the both locks.
+        */
+ again:
        LOCK_ZONE(zone);
        if (zone->type == dns_zone_master) {
                if (inline_raw(zone)) {
                        unsigned int soacount;
+                       secure = zone->secure;
+                       TRYLOCK_ZONE(result, secure);
+                       if (result != ISC_R_SUCCESS) {
+                               UNLOCK_ZONE(zone);
+                               secure = NULL;
+#if ISC_PLATFORM_USETHREADS
+                               isc_thread_yield();
+#endif
+                               goto again;
+                       }
 
                        ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_read);
                        if (zone->db != NULL) {
@@ -9268,13 +9299,15 @@ dns_zone_markdirty(dns_zone_t *zone) {
                                result = DNS_R_NOTLOADED;
                        ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_read);
                        if (result == ISC_R_SUCCESS && soacount > 0U)
-                               zone_send_secureserial(zone, ISC_FALSE, serial);
+                               zone_send_secureserial(zone, serial);
                }
 
                /* XXXMPA make separate call back */
                if (result == ISC_R_SUCCESS)
                        set_resigntime(zone);
        }
+       if (secure != NULL)
+               UNLOCK_ZONE(secure);
        zone_needdump(zone, DNS_DUMP_DELAY);
        UNLOCK_ZONE(zone);
 }
@@ -13299,9 +13332,7 @@ receive_secure_serial(isc_task_t *task, isc_event_t *event) {
 }
 
 static isc_result_t
-zone_send_secureserial(dns_zone_t *zone, isc_boolean_t locked,
-                      isc_uint32_t serial)
-{
+zone_send_secureserial(dns_zone_t *zone, isc_uint32_t serial) {
        isc_event_t *e;
        dns_zone_t *dummy = NULL;
 
@@ -13312,10 +13343,8 @@ zone_send_secureserial(dns_zone_t *zone, isc_boolean_t locked,
        if (e == NULL)
                return (ISC_R_NOMEMORY);
        ((struct secure_event *)e)->serial = serial;
-       if (locked)
-               zone_iattach(zone->secure, &dummy);
-       else
-               dns_zone_iattach(zone->secure, &dummy);
+       INSIST(LOCKED_ZONE(zone->secure));
+       zone_iattach(zone->secure, &dummy);
        isc_task_send(zone->secure->task, &e);
 
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_SENDSECURE);
@@ -13507,7 +13536,7 @@ receive_secure_db(isc_task_t *task, isc_event_t *event) {
 }
 
 static isc_result_t
-zone_send_securedb(dns_zone_t *zone, isc_boolean_t locked, dns_db_t *db) {
+zone_send_securedb(dns_zone_t *zone, dns_db_t *db) {
        isc_event_t *e;
        dns_db_t *dummy = NULL;
        dns_zone_t *secure = NULL;
@@ -13520,11 +13549,8 @@ zone_send_securedb(dns_zone_t *zone, isc_boolean_t locked, dns_db_t *db) {
                return (ISC_R_NOMEMORY);
        dns_db_attach(db, &dummy);
        ((struct secure_event *)e)->db = dummy;
-       if (locked)
-               zone_iattach(zone->secure, &secure);
-       else
-               dns_zone_iattach(zone->secure, &secure);
-
+       INSIST(LOCKED_ZONE(zone->secure));
+       zone_iattach(zone->secure, &secure);
        isc_task_send(zone->secure->task, &e);
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_SENDSECURE);
        return (ISC_R_SUCCESS);
@@ -13533,12 +13559,28 @@ zone_send_securedb(dns_zone_t *zone, isc_boolean_t locked, dns_db_t *db) {
 isc_result_t
 dns_zone_replacedb(dns_zone_t *zone, dns_db_t *db, isc_boolean_t dump) {
        isc_result_t result;
+       dns_zone_t *secure = NULL;
 
        REQUIRE(DNS_ZONE_VALID(zone));
+ again:
        LOCK_ZONE(zone);
+       if (inline_raw(zone)) {
+               secure = zone->secure;
+               TRYLOCK_ZONE(result, secure);
+               if (result != ISC_R_SUCCESS) {
+                       UNLOCK_ZONE(zone);
+                       secure = NULL;
+#if ISC_PLATFORM_USETHREADS
+                       isc_thread_yield();
+#endif
+                       goto again;
+               }
+       }
        ZONEDB_LOCK(&zone->dblock, isc_rwlocktype_write);
        result = zone_replacedb(zone, db, dump);
        ZONEDB_UNLOCK(&zone->dblock, isc_rwlocktype_write);
+       if (secure != NULL)
+               UNLOCK_ZONE(secure);
        UNLOCK_ZONE(zone);
        return (result);
 }
@@ -13555,6 +13597,8 @@ zone_replacedb(dns_zone_t *zone, dns_db_t *db, isc_boolean_t dump) {
         */
        REQUIRE(DNS_ZONE_VALID(zone));
        REQUIRE(LOCKED_ZONE(zone));
+       if (inline_raw(zone))
+               REQUIRE(LOCKED_ZONE(zone->secure));
 
        result = dns_db_rpz_ready(db);
        if (result != ISC_R_SUCCESS)
@@ -13658,7 +13702,7 @@ zone_replacedb(dns_zone_t *zone, dns_db_t *db, isc_boolean_t dump) {
                        }
                }
                if (zone->type == dns_zone_master && inline_raw(zone))
-                       zone_send_secureserial(zone, ISC_FALSE, serial);
+                       zone_send_secureserial(zone, serial);
        } else {
                if (dump && zone->masterfile != NULL) {
                        /*
@@ -13711,7 +13755,7 @@ zone_replacedb(dns_zone_t *zone, dns_db_t *db, isc_boolean_t dump) {
                }
 
                if (inline_raw(zone))
-                       zone_send_securedb(zone, ISC_FALSE, db);
+                       zone_send_securedb(zone, db);
        }
 
        dns_db_closeversion(db, &ver, ISC_FALSE);
@@ -13766,13 +13810,33 @@ zone_xfrdone(dns_zone_t *zone, isc_result_t result) {
        isc_uint32_t serial, refresh, retry, expire, minimum;
        isc_result_t xfrresult = result;
        isc_boolean_t free_needed;
+       dns_zone_t *secure = NULL;
 
        REQUIRE(DNS_ZONE_VALID(zone));
 
        dns_zone_log(zone, ISC_LOG_DEBUG(1),
                     "zone transfer finished: %s", dns_result_totext(result));
 
+       /*
+        * Obtaining a lock on the zone->secure (see zone_send_secureserial)
+        * could result in a deadlock due to a LOR so we will spin if we
+        * can't obtain the both locks.
+        */
+ again:
        LOCK_ZONE(zone);
+       if (inline_raw(zone)) {
+               secure = zone->secure;
+               TRYLOCK_ZONE(result, secure);
+               if (result != ISC_R_SUCCESS) {
+                       UNLOCK_ZONE(zone);
+                       secure = NULL;
+#if ISC_PLATFORM_USETHREADS
+                       isc_thread_yield();
+#endif
+                       goto again;
+               }
+       }
+
        INSIST((zone->flags & DNS_ZONEFLG_REFRESH) != 0);
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_REFRESH);
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_SOABEFOREAXFR);
@@ -13862,7 +13926,7 @@ zone_xfrdone(dns_zone_t *zone, isc_result_t result) {
                                     "transferred serial %u%s",
                                     serial, buf);
                        if (inline_raw(zone))
-                               zone_send_secureserial(zone, ISC_FALSE, serial);
+                               zone_send_secureserial(zone, serial);
                }
 
                /*
@@ -13974,6 +14038,8 @@ zone_xfrdone(dns_zone_t *zone, isc_result_t result) {
                DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_NEEDCOMPACT);
        }
 
+       if (secure != NULL)
+               UNLOCK_ZONE(secure);
        /*
         * This transfer finishing freed up a transfer quota slot.
         * Let any other zones waiting for quota have it.
@@ -14006,6 +14072,7 @@ zone_loaddone(void *arg, isc_result_t result) {
        dns_load_t *load = arg;
        dns_zone_t *zone;
        isc_result_t tresult;
+       dns_zone_t *secure = NULL;
 
        REQUIRE(DNS_LOAD_VALID(load));
        zone = load->zone;
@@ -14020,10 +14087,23 @@ zone_loaddone(void *arg, isc_result_t result) {
        /*
         * Lock hierarchy: zmgr, zone, raw.
         */
+ again:
        LOCK_ZONE(zone);
        INSIST(zone != zone->raw);
        if (inline_secure(zone))
                LOCK_ZONE(zone->raw);
+       else if (inline_raw(zone)) {
+               secure = zone->secure;
+               TRYLOCK_ZONE(result, secure);
+               if (result != ISC_R_SUCCESS) {
+                       UNLOCK_ZONE(zone);
+                       secure = NULL;
+#if ISC_PLATFORM_USETHREADS
+                       isc_thread_yield();
+#endif
+                       goto again;
+               }
+       }
        (void)zone_postload(zone, load->db, load->loadtime, result);
        zonemgr_putio(&zone->readio);
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_LOADING);
@@ -14037,6 +14117,8 @@ zone_loaddone(void *arg, isc_result_t result) {
        DNS_ZONE_CLRFLAG(zone, DNS_ZONEFLG_THAW);
        if (inline_secure(zone))
                UNLOCK_ZONE(zone->raw);
+       else if (secure != NULL)
+               UNLOCK_ZONE(secure);
        UNLOCK_ZONE(zone);
 
        load->magic = 0;
@@ -16772,19 +16854,35 @@ dns_zone_dlzpostload(dns_zone_t *zone, dns_db_t *db)
 {
        isc_time_t loadtime;
        isc_result_t result;
+       dns_zone_t *secure = NULL;
 
        TIME_NOW(&loadtime);
 
        /*
         * Lock hierarchy: zmgr, zone, raw.
         */
+ again:
        LOCK_ZONE(zone);
        INSIST(zone != zone->raw);
        if (inline_secure(zone))
                LOCK_ZONE(zone->raw);
+       else if (inline_raw(zone)) {
+               secure = zone->secure;
+               TRYLOCK_ZONE(result, secure);
+               if (result != ISC_R_SUCCESS) {
+                       UNLOCK_ZONE(zone);
+                       secure = NULL;
+#if ISC_PLATFORM_USETHREADS
+                       isc_thread_yield();
+#endif
+                       goto again;
+               }
+       }
        result = zone_postload(zone, db, loadtime, ISC_R_SUCCESS);
        if (inline_secure(zone))
                UNLOCK_ZONE(zone->raw);
+       else if (secure != NULL)
+               UNLOCK_ZONE(secure);
        UNLOCK_ZONE(zone);
        return result;
 }
index 313bc5f9590aa53d33c96ef7658f20882e7b94d1..9dfc94a3130b1c11aa079b088d7fcd0ecf7db350 100644 (file)
@@ -29,6 +29,7 @@ void
 isc_thread_setconcurrency(unsigned int level);
 
 #define isc_thread_self() ((unsigned long)0)
+#define isc_thread_yield() ((void)0)
 
 ISC_LANG_ENDDECLS
 
index 7dcc9527cf7b46452acd6889eb6179212463e563..e8a9ca2ba7d5ee5f8f1772f2596935f7f79ef40c 100644 (file)
@@ -41,6 +41,9 @@ isc_thread_create(isc_threadfunc_t, isc_threadarg_t, isc_thread_t *);
 void
 isc_thread_setconcurrency(unsigned int level);
 
+void
+isc_thread_yield(void);
+
 /* XXX We could do fancier error handling... */
 
 #define isc_thread_join(t, rp) \
index 1b250fac2801f9fdba9dcdc2ae5d1a5ae8654986..97821a2cadb0499b0dd4532e6d1e57f8ad55b1d7 100644 (file)
 
 #include <config.h>
 
+#if defined(HAVE_SCHED_H)
+#include <sched.h>
+#endif
+
 #include <isc/thread.h>
 #include <isc/util.h>
 
@@ -74,3 +78,14 @@ isc_thread_setconcurrency(unsigned int level) {
        UNUSED(level);
 #endif
 }
+
+void
+isc_thread_yield(void) {
+#if defined(HAVE_SCHED_YIELD)
+       sched_yield();
+#elif defined( HAVE_PTHREAD_YIELD)
+       pthread_yield();
+#elif defined( HAVE_PTHREAD_YIELD_NP)
+       pthread_yield_np();
+#endif
+}
index e748d8727718c1bfa1d5563a93596ab85ff93b9d..bfe860b659f7156ebd2dc2c7c598165dd62e7b63 100644 (file)
@@ -95,6 +95,8 @@ isc_thread_key_getspecific(isc_thread_key_t);
 int
 isc_thread_key_setspecific(isc_thread_key_t key, void *value);
 
+#define isc_thread_yield() Sleep(0)
+
 ISC_LANG_ENDDECLS
 
 #endif /* ISC_THREAD_H */