From 03152360db6fcb0fcc95fa63c20c5c829c95f1f6 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 8 Oct 2013 11:43:08 +1100 Subject: [PATCH] 3661. [bug] Address lock order reversal deadlock with inline zones. [RT #34856] --- CHANGES | 3 + config.h.in | 15 +++ configure | 76 +++++++++++++ configure.in | 10 ++ lib/dns/zone.c | 146 +++++++++++++++++++++---- lib/isc/nothreads/include/isc/thread.h | 1 + lib/isc/pthreads/include/isc/thread.h | 3 + lib/isc/pthreads/thread.c | 15 +++ lib/isc/win32/include/isc/thread.h | 2 + 9 files changed, 247 insertions(+), 24 deletions(-) diff --git a/CHANGES b/CHANGES index 6fda168e6a..b27f7e5266 100644 --- 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] diff --git a/config.h.in b/config.h.in index 89aa492ffe..0607f0842f 100644 --- a/config.h.in +++ b/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 header file. */ #undef HAVE_REGEX_H +/* Define to 1 if you have the 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 diff --git a/configure b/configure index a77b2f84d6..e50e85f22e 100755 --- 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. # diff --git a/configure.in b/configure.in index f577dc88c2..a90de0fda1 100644 --- a/configure.in +++ b/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. # diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 246540f1e2..d4c22d3fa8 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -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; } diff --git a/lib/isc/nothreads/include/isc/thread.h b/lib/isc/nothreads/include/isc/thread.h index 313bc5f959..9dfc94a313 100644 --- a/lib/isc/nothreads/include/isc/thread.h +++ b/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 diff --git a/lib/isc/pthreads/include/isc/thread.h b/lib/isc/pthreads/include/isc/thread.h index 7dcc9527cf..e8a9ca2ba7 100644 --- a/lib/isc/pthreads/include/isc/thread.h +++ b/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) \ diff --git a/lib/isc/pthreads/thread.c b/lib/isc/pthreads/thread.c index 1b250fac28..97821a2cad 100644 --- a/lib/isc/pthreads/thread.c +++ b/lib/isc/pthreads/thread.c @@ -21,6 +21,10 @@ #include +#if defined(HAVE_SCHED_H) +#include +#endif + #include #include @@ -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 +} diff --git a/lib/isc/win32/include/isc/thread.h b/lib/isc/win32/include/isc/thread.h index e748d87277..bfe860b659 100644 --- a/lib/isc/win32/include/isc/thread.h +++ b/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 */