3661. [bug] Address lock order reversal deadlock with inline zones.

[RT #34856]

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]
 

config.h.in

@@ -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
 

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.
 	#

configure.in

@@ -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.
 	#

lib/dns/zone.c

@@ -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,
+static isc_result_t zone_send_securedb(dns_zone_t *zone, dns_db_t *db);
-				       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,
+zone_send_secureserial(dns_zone_t *zone, isc_uint32_t serial) {
-		       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)
+	INSIST(LOCKED_ZONE(zone->secure));
 	zone_iattach(zone->secure, &dummy);
-	else
-		dns_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)
+	INSIST(LOCKED_ZONE(zone->secure));
 	zone_iattach(zone->secure, &secure);
-	else
-		dns_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;
 }

lib/isc/nothreads/include/isc/thread.h

@@ -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
 

lib/isc/pthreads/include/isc/thread.h

@@ -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) \

lib/isc/pthreads/thread.c

@@ -21,6 +21,10 @@
 
 #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
+}

lib/isc/win32/include/isc/thread.h

@@ -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 */