From 98d59bdf62db8be585683c800250dd11860d8b24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 4 Mar 2024 13:21:35 +0100 Subject: [PATCH 1/2] Pin the xfr to a specific loop Instead of getting the loop from the zone every time, attach the xfrin directly to the loop. This also allows to remove the extra safety tid checks from the dns_xfrin unit. --- lib/dns/xfrin.c | 40 +++++++++++++++++++++------------------- lib/dns/zone.c | 4 +--- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c index 9ac923f5e7..8b5d41233f 100644 --- a/lib/dns/xfrin.c +++ b/lib/dns/xfrin.c @@ -186,6 +186,8 @@ struct dns_xfrin { isc_tlsctx_cache_t *tlsctx_cache; + isc_loop_t *loop; + isc_timer_t *max_time_timer; isc_timer_t *max_idle_timer; @@ -206,7 +208,7 @@ typedef struct xfrin_work { */ static void -xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, +xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_loop_t *loop, dns_name_t *zonename, dns_rdataclass_t rdclass, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, @@ -404,8 +406,7 @@ axfr_commit(dns_xfrin_t *xfr) { .result = ISC_R_UNSET, }; xfr->diff_running = true; - isc_work_enqueue(dns_zone_getloop(xfr->zone), axfr_apply, - axfr_apply_done, work); + isc_work_enqueue(xfr->loop, axfr_apply, axfr_apply_done, work); } static isc_result_t @@ -587,8 +588,7 @@ ixfr_apply_done(void *arg) { /* Reschedule */ if (!cds_wfcq_empty(&xfr->diff_head, &xfr->diff_tail)) { - isc_work_enqueue(dns_zone_getloop(xfr->zone), ixfr_apply, - ixfr_apply_done, work); + isc_work_enqueue(xfr->loop, ixfr_apply, ixfr_apply_done, work); return; } @@ -642,8 +642,7 @@ ixfr_commit(dns_xfrin_t *xfr) { .result = ISC_R_UNSET, }; xfr->diff_running = true; - isc_work_enqueue(dns_zone_getloop(xfr->zone), ixfr_apply, - ixfr_apply_done, work); + isc_work_enqueue(xfr->loop, ixfr_apply, ixfr_apply_done, work); } failure: @@ -886,13 +885,15 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, dns_xfrin_t *xfr = NULL; isc_result_t result; dns_db_t *db = NULL; + isc_loop_t *loop = NULL; REQUIRE(xfrp != NULL && *xfrp == NULL); REQUIRE(done != NULL); REQUIRE(isc_sockaddr_getport(primaryaddr) != 0); REQUIRE(zone != NULL); REQUIRE(dns_zone_getview(zone) != NULL); - REQUIRE(dns_zone_gettid(zone) == isc_tid()); + + loop = dns_zone_getloop(zone); (void)dns_zone_getdb(zone, &db); @@ -900,9 +901,9 @@ dns_xfrin_create(dns_zone_t *zone, dns_rdatatype_t xfrtype, REQUIRE(db != NULL); } - xfrin_create(mctx, zone, db, zonename, dns_zone_getclass(zone), xfrtype, - primaryaddr, sourceaddr, tsigkey, soa_transport_type, - transport, tlsctx_cache, &xfr); + xfrin_create(mctx, zone, db, loop, zonename, dns_zone_getclass(zone), + xfrtype, primaryaddr, sourceaddr, tsigkey, + soa_transport_type, transport, tlsctx_cache, &xfr); if (db != NULL) { xfr->zone_had_db = true; @@ -1064,7 +1065,6 @@ dns_xfrin_gettsigkeyname(const dns_xfrin_t *xfr) { void dns_xfrin_shutdown(dns_xfrin_t *xfr) { REQUIRE(VALID_XFRIN(xfr)); - REQUIRE(dns_zone_gettid(xfr->zone) == isc_tid()); xfrin_fail(xfr, ISC_R_CANCELED, "shut down"); } @@ -1112,15 +1112,14 @@ xfrin_reset(dns_xfrin_t *xfr) { static void xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { + REQUIRE(VALID_XFRIN(xfr)); + dns_xfrin_ref(xfr); /* Make sure only the first xfrin_fail() trumps */ if (atomic_compare_exchange_strong(&xfr->shuttingdown, &(bool){ false }, true)) { - isc_timer_stop(xfr->max_time_timer); - isc_timer_stop(xfr->max_idle_timer); - if (result != DNS_R_UPTODATE && result != DNS_R_TOOMANYRECORDS) { xfrin_log(xfr, ISC_LOG_ERROR, "%s: %s", msg, @@ -1142,7 +1141,7 @@ xfrin_fail(dns_xfrin_t *xfr, isc_result_t result, const char *msg) { } static void -xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, +xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, isc_loop_t *loop, dns_name_t *zonename, dns_rdataclass_t rdclass, dns_rdatatype_t reqtype, const isc_sockaddr_t *primaryaddr, const isc_sockaddr_t *sourceaddr, dns_tsigkey_t *tsigkey, @@ -1165,6 +1164,7 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, .magic = XFRIN_MAGIC, }; + isc_loop_attach(loop, &xfr->loop); isc_mem_attach(mctx, &xfr->mctx); dns_zone_iattach(zone, &xfr->zone); dns_view_weakattach(dns_zone_getview(zone), &xfr->view); @@ -1172,7 +1172,6 @@ xfrin_create(isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, __cds_wfcq_init(&xfr->diff_head, &xfr->diff_tail); - atomic_init(&xfr->shuttingdown, false); atomic_init(&xfr->is_ixfr, false); if (db != NULL) { @@ -1276,7 +1275,7 @@ xfrin_start(dns_xfrin_t *xfr) { * XXX: timeouts are hard-coded to 30 seconds; this needs to be * configurable. */ - CHECK(dns_dispatch_add(xfr->disp, dns_zone_getloop(xfr->zone), 0, 30000, + CHECK(dns_dispatch_add(xfr->disp, xfr->loop, 0, 30000, &xfr->primaryaddr, xfr->transport, xfr->tlsctx_cache, xfrin_connect_done, xfrin_send_done, xfrin_recv_done, xfr, &xfr->id, @@ -1667,6 +1666,8 @@ xfrin_end(dns_xfrin_t *xfr, isc_result_t result) { atomic_store(&xfr->shuttingdown, true); isc_timer_stop(xfr->max_time_timer); + isc_timer_stop(xfr->max_idle_timer); + if (xfr->shutdown_result == ISC_R_UNSET) { xfr->shutdown_result = result; } @@ -1984,7 +1985,6 @@ xfrin_destroy(dns_xfrin_t *xfr) { isc_time_t now = isc_time_now(); REQUIRE(VALID_XFRIN(xfr)); - REQUIRE(dns_zone_gettid(xfr->zone) == isc_tid()); /* Safe-guards */ REQUIRE(atomic_load(&xfr->shuttingdown)); @@ -2101,6 +2101,8 @@ xfrin_destroy(dns_xfrin_t *xfr) { isc_timer_destroy(&xfr->max_idle_timer); isc_timer_destroy(&xfr->max_time_timer); + isc_loop_detach(&xfr->loop); + isc_mem_putanddetach(&xfr->mctx, xfr, sizeof(*xfr)); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 3dbf5ff06d..7d98ae4741 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -18152,7 +18152,6 @@ got_transfer_quota(void *arg) { const char *soa_before = ""; bool loaded; isc_tlsctx_cache_t *zmgr_tlsctx_cache = NULL; - dns_xfrin_t *xfr = NULL; if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING)) { zone_xfrdone(zone, NULL, ISC_R_CANCELED); @@ -18303,7 +18302,7 @@ got_transfer_quota(void *arg) { result = dns_xfrin_create(zone, xfrtype, &primaryaddr, &sourceaddr, zone->tsigkey, soa_transport_type, zone->transport, zmgr_tlsctx_cache, - zone->mctx, zone_xfrdone, &xfr); + zone->mctx, zone_xfrdone, &zone->xfr); isc_tlsctx_cache_detach(&zmgr_tlsctx_cache); @@ -18318,7 +18317,6 @@ got_transfer_quota(void *arg) { } LOCK_ZONE(zone); - zone->xfr = xfr; if (xfrtype == dns_rdatatype_axfr) { if (isc_sockaddr_pf(&primaryaddr) == PF_INET) { inc_stats(zone, dns_zonestatscounter_axfrreqv4); From e74c7dcf51cf623c6500146aa7c140c01ed637eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 4 Mar 2024 12:58:56 +0100 Subject: [PATCH 2/2] Always call the TCP dispatch connected callbacks asynchronously The TCP dispatch connected callbacks could be called synchronously which in turn could destroy xfrin before we return from dns_xfrin_create(). Delay the calling the callback called from tcp_dispatch_connect() by calling it always asynchronously. --- lib/dns/dispatch.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 603add2ba3..e5412abe9e 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -1776,6 +1777,16 @@ tcp_startrecv(dns_dispatch_t *disp, dns_dispentry_t *resp) { disp->reading = true; } +static void +resp_connected(void *arg) { + dns_dispentry_t *resp = arg; + dispentry_log(resp, ISC_LOG_DEBUG(90), "connect callback: %s", + isc_result_totext(resp->result)); + + resp->connected(resp->result, NULL, resp->arg); + dns_dispentry_detach(&resp); /* DISPENTRY005 */ +} + static void tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { dns_dispatch_t *disp = (dns_dispatch_t *)arg; @@ -1846,10 +1857,7 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { next = ISC_LIST_NEXT(resp, rlink); ISC_LIST_UNLINK(resps, resp, rlink); - dispentry_log(resp, ISC_LOG_DEBUG(90), "connect callback: %s", - isc_result_totext(resp->result)); - resp->connected(resp->result, NULL, resp->arg); - dns_dispentry_detach(&resp); /* DISPENTRY005 */ + resp_connected(resp); } dns_dispatch_detach(&disp); /* DISPATCH003 */ @@ -1999,10 +2007,9 @@ tcp_dispatch_connect(dns_dispatch_t *disp, dns_dispentry_t *resp) { tcp_startrecv(disp, resp); } - /* We are already connected; call the connected cb */ - dispentry_log(resp, ISC_LOG_DEBUG(90), "connect callback: %s", - isc_result_totext(ISC_R_SUCCESS)); - resp->connected(ISC_R_SUCCESS, NULL, resp->arg); + /* Already connected; call the connected cb asynchronously */ + dns_dispentry_ref(resp); /* DISPENTRY005 */ + isc_async_run(resp->loop, resp_connected, resp); break; default: