From ead7b3dc539f5edc867a4123a1a189364fcfe0df Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Fri, 22 Nov 2019 18:34:50 -0300 Subject: [PATCH 1/4] Fix tcp-highwater initial value During BIND startup it scans for network interfaces available, in this process it ensures that for every interface it will bind and listen to, at least one socket will be always available accepting connections on that interface, this way avoiding some DOS attacks that could exploit tcp quota on some interface and make others unavailable. In the previous network implementation this initial "reserved" tcp-quota used by BIND was already been added to the tcp-highwater stats, but with the new network code it was necesary to add this workaround to ensure tcp-highwater stats reflect the tcp-quota used by BIND after startup. --- lib/ns/interfacemgr.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/ns/interfacemgr.c b/lib/ns/interfacemgr.c index 3bbeee2a50..1e9011bf11 100644 --- a/lib/ns/interfacemgr.c +++ b/lib/ns/interfacemgr.c @@ -28,6 +28,7 @@ #include #include #include +#include #ifdef HAVE_NET_ROUTE_H #include @@ -458,6 +459,7 @@ ns_interface_listenudp(ns_interface_t *ifp) { static isc_result_t ns_interface_listentcp(ns_interface_t *ifp) { + unsigned int tcpquota; isc_result_t result; result = isc_nm_listentcpdns(ifp->mgr->nm, @@ -473,6 +475,16 @@ ns_interface_listentcp(ns_interface_t *ifp) { isc_result_totext(result)); } + /* + * We update tcp-highwater stats here, since named itself adds to + * the TCP quota when starting, as it ensures that at least one + * client will be created for every interface it is listening to. + */ + tcpquota = isc_quota_getused(&ifp->mgr->sctx->tcpquota); + ns_stats_update_if_greater(ifp->mgr->sctx->nsstats, + ns_statscounter_tcphighwater, + tcpquota); + #if 0 #ifndef ISC_ALLOW_MAPPED isc_socket_ipv6only(ifp->tcpsocket, true); From ed9853e73932e68ad6090efba80f717a9797bfb6 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Mon, 25 Nov 2019 18:36:14 -0300 Subject: [PATCH 2/4] Fix tcp-highwater stats updating After the network manager rewrite, tcp-higwater stats was only being updated when a valid DNS query was received over tcp. It turns out tcp-quota is updated right after a tcp connection is accepted, before any data is read, so in the event that some client connect but don't send a valid query, it wouldn't be taken into account to update tcp-highwater stats, that is wrong. This commit fix tcp-highwater to update its stats whenever a tcp connection is established, independent of what happens after (timeout/invalid request, etc). --- lib/isc/include/isc/netmgr.h | 11 +++++++---- lib/isc/netmgr/netmgr-int.h | 3 +++ lib/isc/netmgr/tcpdns.c | 16 +++++++++++----- lib/ns/client.c | 19 ++++++++++++++----- lib/ns/include/ns/client.h | 12 ++++++++++-- lib/ns/interfacemgr.c | 13 +++++-------- 6 files changed, 50 insertions(+), 24 deletions(-) diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index 157961609f..d3000036c4 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -264,10 +264,10 @@ isc_nm_tcp_stoplistening(isc_nmsocket_t *sock); isc_result_t isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, - isc_nm_recv_cb_t cb, void *arg, - size_t extrahandlesize, int backlog, - isc_quota_t *quota, - isc_nmsocket_t **sockp); + isc_nm_recv_cb_t cb, void *cbarg, + isc_nm_cb_t accept_cb, void *accept_cbarg, + size_t extrahandlesize, int backlog, isc_quota_t *quota, + isc_nmsocket_t **sockp); /*%< * Start listening for DNS messages over the TCP interface 'iface', using * net manager 'mgr'. @@ -281,6 +281,9 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, * When a complete DNS message is received on the socket, 'cb' will be * called with 'cbarg' as its argument. * + * When a new TCP connection is accepted, 'accept_cb' will be called + * with 'accept_cbarg' as its argument. + * * When handles are allocated for the socket, 'extrasize' additional bytes * will be allocated along with the handle for an associated object * (typically ns_client). diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h index daae5fd880..235fbd2288 100644 --- a/lib/isc/netmgr/netmgr-int.h +++ b/lib/isc/netmgr/netmgr-int.h @@ -477,6 +477,9 @@ struct isc_nmsocket { isc__nm_readcb_t rcb; void *rcbarg; + + isc__nm_cb_t accept_cb; + void *accept_cbarg; }; bool diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c index 5048200aca..a6cf921490 100644 --- a/lib/isc/netmgr/tcpdns.c +++ b/lib/isc/netmgr/tcpdns.c @@ -96,7 +96,7 @@ dnstcp_readtimeout(uv_timer_t *timer) { } /* - * Accept callback for TCP-DNS connection + * Accept callback for TCP-DNS connection. */ static void dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { @@ -111,10 +111,14 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { return; } + if (dnslistensock->accept_cb.accept != NULL) { + dnslistensock->accept_cb.accept(handle, ISC_R_SUCCESS, + dnslistensock->accept_cbarg); + } + /* We need to create a 'wrapper' dnssocket for this connection */ dnssock = isc_mem_get(handle->sock->mgr->mctx, sizeof(*dnssock)); - isc__nmsocket_init(dnssock, handle->sock->mgr, - isc_nm_tcpdnssocket); + isc__nmsocket_init(dnssock, handle->sock->mgr, isc_nm_tcpdnssocket); /* We need to copy read callbacks from outer socket */ dnssock->rcb.recv = dnslistensock->rcb.recv; @@ -278,8 +282,8 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { isc_result_t isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb, void *cbarg, - size_t extrahandlesize, int backlog, - isc_quota_t *quota, + isc_nm_cb_t accept_cb, void *accept_cbarg, + size_t extrahandlesize, int backlog, isc_quota_t *quota, isc_nmsocket_t **sockp) { /* A 'wrapper' socket object with outer set to true TCP socket */ @@ -293,6 +297,8 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, dnslistensock->iface = iface; dnslistensock->rcb.recv = cb; dnslistensock->rcbarg = cbarg; + dnslistensock->accept_cb.accept = accept_cb; + dnslistensock->accept_cbarg = accept_cbarg; dnslistensock->extrahandlesize = extrahandlesize; /* We set dnslistensock->outer to a true listening socket */ diff --git a/lib/ns/client.c b/lib/ns/client.c index cf8467316f..c178c043c6 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1657,11 +1657,6 @@ ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { } if (isc_nmhandle_is_stream(handle)) { client->attributes |= NS_CLIENTATTR_TCP; - unsigned int curr_tcpquota = - isc_quota_getused(&client->sctx->tcpquota); - ns_stats_update_if_greater(client->sctx->nsstats, - ns_statscounter_tcphighwater, - curr_tcpquota); } INSIST(client->recursionquota == NULL); @@ -2185,6 +2180,20 @@ ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg) { isc_task_unpause(client->task); } +void +ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) { + ns_server_t *sctx = (ns_server_t *) arg; + unsigned int tcpquota; + + UNUSED(handle); + UNUSED(result); + + tcpquota = isc_quota_getused(&sctx->tcpquota); + ns_stats_update_if_greater(sctx->nsstats, + ns_statscounter_tcphighwater, + tcpquota); +} + static void get_clientmctx(ns_clientmgr_t *manager, isc_mem_t **mctxp) { isc_mem_t *clientmctx; diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index c808064665..d1e4f28f33 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -487,7 +487,7 @@ isc_result_t ns_client_addopt(ns_client_t *client, dns_message_t *message, dns_rdataset_t **opt); -/* +/*%< * Get a client object from the inactive queue, or create one, as needed. * (Not intended for use outside this module and associated tests.) */ @@ -495,11 +495,19 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message, void ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg); -/* +/*%< * Handle client requests. * (Not intended for use outside this module and associated tests.) */ + void + ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg); + + /*%< + * Called every time a TCP connection is establish. This is used for + * updating TCP statistics. + */ + dns_rdataset_t * ns_client_newrdataset(ns_client_t *client); diff --git a/lib/ns/interfacemgr.c b/lib/ns/interfacemgr.c index 1e9011bf11..60f916de48 100644 --- a/lib/ns/interfacemgr.c +++ b/lib/ns/interfacemgr.c @@ -459,12 +459,12 @@ ns_interface_listenudp(ns_interface_t *ifp) { static isc_result_t ns_interface_listentcp(ns_interface_t *ifp) { - unsigned int tcpquota; isc_result_t result; result = isc_nm_listentcpdns(ifp->mgr->nm, (isc_nmiface_t *) &ifp->addr, ns__client_request, ifp, + ns__client_tcpconn, ifp->mgr->sctx, sizeof(ns_client_t), ifp->mgr->backlog, &ifp->mgr->sctx->tcpquota, @@ -476,14 +476,11 @@ ns_interface_listentcp(ns_interface_t *ifp) { } /* - * We update tcp-highwater stats here, since named itself adds to - * the TCP quota when starting, as it ensures that at least one - * client will be created for every interface it is listening to. + * We call this now to update the tcp-highwater statistic: + * this is necessary because we are adding to the TCP quota just + * by listening. */ - tcpquota = isc_quota_getused(&ifp->mgr->sctx->tcpquota); - ns_stats_update_if_greater(ifp->mgr->sctx->nsstats, - ns_statscounter_tcphighwater, - tcpquota); + ns__client_tcpconn(NULL, ISC_R_SUCCESS, ifp->mgr->sctx); #if 0 #ifndef ISC_ALLOW_MAPPED From 114520425caa59e54d15ee8cff123b6fecafca32 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Thu, 12 Dec 2019 16:09:19 -0300 Subject: [PATCH 3/4] Added tcp-highwater test on initial statistics verification The initial tcp statistics test was not testing tcp-highwater counter, but only initial number of current TCP clients, so this missing test was added to ensure initial tcp-highwater value is correct. --- bin/tests/system/tcp/tests.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bin/tests/system/tcp/tests.sh b/bin/tests/system/tcp/tests.sh index 9f57e30805..8606efd7e5 100644 --- a/bin/tests/system/tcp/tests.sh +++ b/bin/tests/system/tcp/tests.sh @@ -105,6 +105,10 @@ echo_i "TCP high-water: check initial statistics ($n)" ret=0 refresh_tcp_stats assert_int_equal "${TCP_CUR}" 0 "current TCP clients count" || ret=1 +# We compare initial tcp-highwater value with 1 because as part of the +# system test startup, the script start.pl executes dig to check if target +# named is running, and that increments tcp-quota by one. +assert_int_equal "${TCP_HIGH}" 1 "tcp-highwater count" || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) From 8c48c4f7383f7adfc2303e94c18b7f4785295b40 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 12 Dec 2019 11:24:26 -0800 Subject: [PATCH 4/4] CHANGES --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 52577f1f06..adbbeaf75c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5336. [bug] The TCP high-water statistic could report an + incorrect value on startup. [GL #1392] + 5335. [func] Make TCP listening code multithreaded. [GL !2659] 5334. [doc] Update documentation with dnssec-policy clarifications.