mir/bind
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-09-02 07:35:26 +00:00
Code Issues Releases Wiki Activity

Clone the csock in accept_connection(), not in callback

Browse Source 
If we clone the csock (children socket) in TCP accept_connection()
instead of passing the ssock (server socket) to the call back and
cloning it there we unbreak the assumption that every socket is handled
inside it's own worker thread and therefore we can get rid of (at least)
callback locking.
This commit is contained in:
Ondřej Surý
2020-09-18 12:27:40 +02:00
committed by Ondřej Surý
parent d86a74d8a4
commit e8b56acb49
4 changed files with 42 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
lib/isc/netmgr/netmgr.c
View File

@@ -1040,6 +1040,7 @@ isc__nmsocket_init(isc_nmsocket_t *sock, isc_nm_t *mgr, isc_nmsocket_type type,
void void
isc__nmsocket_clearcb(isc_nmsocket_t *sock) { isc__nmsocket_clearcb(isc_nmsocket_t *sock) {
REQUIRE(VALID_NMSOCK(sock)); REQUIRE(VALID_NMSOCK(sock));
REQUIRE(!isc__nm_in_netthread() || sock->tid == isc_nm_tid());
sock->recv_cb = NULL; sock->recv_cb = NULL;
sock->recv_cbarg = NULL; sock->recv_cbarg = NULL;

82
lib/isc/netmgr/tcp.c
View File

@@ -431,8 +431,7 @@ void
isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) { isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) {
isc__netievent_tcpchildaccept_t *ievent = isc__netievent_tcpchildaccept_t *ievent =
(isc__netievent_tcpchildaccept_t *)ev0; (isc__netievent_tcpchildaccept_t *)ev0;
isc_nmsocket_t *ssock = ievent->sock; isc_nmsocket_t *sock = ievent->sock;
isc_nmsocket_t *csock = NULL;
isc_nmhandle_t *handle; isc_nmhandle_t *handle;
isc_result_t result; isc_result_t result;
struct sockaddr_storage ss; struct sockaddr_storage ss;
@@ -442,20 +441,15 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) {
void *accept_cbarg; void *accept_cbarg;
REQUIRE(isc__nm_in_netthread()); REQUIRE(isc__nm_in_netthread());
REQUIRE(ssock->type == isc_nm_tcplistener); REQUIRE(sock->tid == isc_nm_tid());
csock = isc_mem_get(ssock->mgr->mctx, sizeof(isc_nmsocket_t)); sock->quota = ievent->quota;
isc__nmsocket_init(csock, ssock->mgr, isc_nm_tcpsocket, ssock->iface);
csock->tid = isc_nm_tid();
csock->extrahandlesize = ssock->extrahandlesize;
csock->quota = ievent->quota;
ievent->quota = NULL; ievent->quota = NULL;
worker = &ssock->mgr->workers[isc_nm_tid()]; worker = &sock->mgr->workers[isc_nm_tid()];
uv_tcp_init(&worker->loop, &csock->uv_handle.tcp); uv_tcp_init(&worker->loop, &sock->uv_handle.tcp);
r = isc_uv_import(&csock->uv_handle.stream, &ievent->streaminfo); r = isc_uv_import(&sock->uv_handle.stream, &ievent->streaminfo);
if (r != 0) { if (r != 0) {
isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL,
ISC_LOGMODULE_NETMGR, ISC_LOG_ERROR, ISC_LOGMODULE_NETMGR, ISC_LOG_ERROR,
@@ -465,20 +459,19 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) {
goto error; goto error;
} }
r = uv_tcp_getpeername(&csock->uv_handle.tcp, (struct sockaddr *)&ss, r = uv_tcp_getpeername(&sock->uv_handle.tcp, (struct sockaddr *)&ss,
&(int){ sizeof(ss) }); &(int){ sizeof(ss) });
if (r != 0) { if (r != 0) {
result = isc__nm_uverr2result(r); result = isc__nm_uverr2result(r);
goto error; goto error;
} }
result = isc_sockaddr_fromsockaddr(&csock->peer, result = isc_sockaddr_fromsockaddr(&sock->peer, (struct sockaddr *)&ss);
(struct sockaddr *)&ss);
if (result != ISC_R_SUCCESS) { if (result != ISC_R_SUCCESS) {
goto error; goto error;
} }
r = uv_tcp_getsockname(&csock->uv_handle.tcp, (struct sockaddr *)&ss, r = uv_tcp_getsockname(&sock->uv_handle.tcp, (struct sockaddr *)&ss,
&(int){ sizeof(ss) }); &(int){ sizeof(ss) });
if (r != 0) { if (r != 0) {
result = isc__nm_uverr2result(r); result = isc__nm_uverr2result(r);
@@ -490,23 +483,19 @@ isc__nm_async_tcpchildaccept(isc__networker_t *worker, isc__netievent_t *ev0) {
goto error; goto error;
} }
isc__nmsocket_attach(ssock, &csock->server); handle = isc__nmhandle_get(sock, NULL, &local);
handle = isc__nmhandle_get(csock, NULL, &local); INSIST(sock->accept_cb != NULL);
accept_cb = sock->accept_cb;
accept_cbarg = sock->accept_cbarg;
LOCK(&ssock->lock); sock->read_timeout = sock->mgr->init;
INSIST(ssock->accept_cb != NULL);
accept_cb = ssock->accept_cb;
accept_cbarg = ssock->accept_cbarg;
UNLOCK(&ssock->lock);
csock->read_timeout = ssock->mgr->init;
accept_cb(handle, ISC_R_SUCCESS, accept_cbarg); accept_cb(handle, ISC_R_SUCCESS, accept_cbarg);
/* /*
* csock is now attached to the handle. * sock is now attached to the handle.
*/ */
isc__nmsocket_detach(&csock); isc__nmsocket_detach(&sock);
/* /*
* The accept callback should have attached to the handle. * The accept callback should have attached to the handle.
@@ -521,8 +510,8 @@ error:
* otherwise it'd be detached later asynchronously, and clog * otherwise it'd be detached later asynchronously, and clog
* the quota unnecessarily. * the quota unnecessarily.
*/ */
if (csock->quota != NULL) { if (sock->quota != NULL) {
isc_quota_detach(&csock->quota); isc_quota_detach(&sock->quota);
} }
isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR,
ISC_LOG_ERROR, "Accepting TCP connection failed: %s", ISC_LOG_ERROR, "Accepting TCP connection failed: %s",
@@ -531,7 +520,7 @@ error:
/* /*
* Detach the socket properly to make sure uv_close() is called. * Detach the socket properly to make sure uv_close() is called.
*/ */
isc__nmsocket_detach(&csock); isc__nmsocket_detach(&sock);
} }
void void
@@ -616,11 +605,9 @@ readtimeout_cb(uv_timer_t *handle) {
isc_quota_detach(&sock->quota); isc_quota_detach(&sock->quota);
} }
LOCK(&sock->lock);
cb = sock->recv_cb; cb = sock->recv_cb;
cbarg = sock->recv_cbarg; cbarg = sock->recv_cbarg;
isc__nmsocket_clearcb(sock); isc__nmsocket_clearcb(sock);
UNLOCK(&sock->lock);
if (cb != NULL) { if (cb != NULL) {
cb(sock->statichandle, ISC_R_TIMEDOUT, NULL, cbarg); cb(sock->statichandle, ISC_R_TIMEDOUT, NULL, cbarg);
@@ -637,10 +624,9 @@ isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
sock = handle->sock; sock = handle->sock;
LOCK(&sock->lock); REQUIRE(sock->tid == isc_nm_tid());
sock->recv_cb = cb; sock->recv_cb = cb;
sock->recv_cbarg = cbarg; sock->recv_cbarg = cbarg;
UNLOCK(&sock->lock);
ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpstartread); ievent = isc__nm_get_ievent(sock->mgr, netievent_tcpstartread);
ievent->sock = sock; ievent->sock = sock;
@@ -749,12 +735,11 @@ isc__nm_tcp_resumeread(isc_nmsocket_t *sock) {
isc__netievent_startread_t *ievent = NULL; isc__netievent_startread_t *ievent = NULL;
REQUIRE(VALID_NMSOCK(sock)); REQUIRE(VALID_NMSOCK(sock));
LOCK(&sock->lock); REQUIRE(sock->tid == isc_nm_tid());
if (sock->recv_cb == NULL) { if (sock->recv_cb == NULL) {
UNLOCK(&sock->lock);
return (ISC_R_CANCELED); return (ISC_R_CANCELED);
} }
UNLOCK(&sock->lock);
if (!atomic_load(&sock->readpaused)) { if (!atomic_load(&sock->readpaused)) {
return (ISC_R_SUCCESS); return (ISC_R_SUCCESS);
@@ -784,12 +769,11 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
void *cbarg; void *cbarg;
REQUIRE(VALID_NMSOCK(sock)); REQUIRE(VALID_NMSOCK(sock));
REQUIRE(sock->tid == isc_nm_tid());
REQUIRE(buf != NULL); REQUIRE(buf != NULL);
LOCK(&sock->lock);
cb = sock->recv_cb; cb = sock->recv_cb;
cbarg = sock->recv_cbarg; cbarg = sock->recv_cbarg;
UNLOCK(&sock->lock);
if (nread >= 0) { if (nread >= 0) {
isc_region_t region = { .base = (unsigned char *)buf->base, isc_region_t region = { .base = (unsigned char *)buf->base,
@@ -895,7 +879,6 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
int r, w; int r, w;
REQUIRE(VALID_NMSOCK(ssock)); REQUIRE(VALID_NMSOCK(ssock));
REQUIRE(ssock->tid == isc_nm_tid());
if (!atomic_load_relaxed(&ssock->active) || if (!atomic_load_relaxed(&ssock->active) ||
atomic_load_relaxed(&ssock->mgr->closing)) atomic_load_relaxed(&ssock->mgr->closing))
@@ -955,7 +938,18 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
/* We have an accepted TCP socket, pass it to a random worker */ /* We have an accepted TCP socket, pass it to a random worker */
w = isc_random_uniform(ssock->mgr->nworkers); w = isc_random_uniform(ssock->mgr->nworkers);
event = isc__nm_get_ievent(ssock->mgr, netievent_tcpchildaccept); event = isc__nm_get_ievent(ssock->mgr, netievent_tcpchildaccept);
event->sock = ssock;
/* Duplicate the server socket */
isc_nmsocket_t *csock = isc_mem_get(ssock->mgr->mctx,
sizeof(isc_nmsocket_t));
isc__nmsocket_init(csock, ssock->mgr, isc_nm_tcpsocket, ssock->iface);
csock->tid = w;
csock->extrahandlesize = ssock->extrahandlesize;
isc__nmsocket_attach(ssock, &csock->server);
csock->accept_cb = ssock->accept_cb;
csock->accept_cbarg = ssock->accept_cbarg;
event->sock = csock;
event->quota = quota; event->quota = quota;
r = isc_uv_export((uv_stream_t *)uvstream, &event->streaminfo); r = isc_uv_export((uv_stream_t *)uvstream, &event->streaminfo);
@@ -1149,16 +1143,15 @@ isc__nm_async_tcpclose(isc__networker_t *worker, isc__netievent_t *ev0) {
void void
isc__nm_tcp_shutdown(isc_nmsocket_t *sock) { isc__nm_tcp_shutdown(isc_nmsocket_t *sock) {
REQUIRE(VALID_NMSOCK(sock)); REQUIRE(VALID_NMSOCK(sock));
REQUIRE(sock->tid == isc_nm_tid());
if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) { if (sock->type == isc_nm_tcpsocket && sock->statichandle != NULL) {
isc_nm_recv_cb_t cb; isc_nm_recv_cb_t cb;
void *cbarg; void *cbarg;
LOCK(&sock->lock);
cb = sock->recv_cb; cb = sock->recv_cb;
cbarg = sock->recv_cbarg; cbarg = sock->recv_cbarg;
isc__nmsocket_clearcb(sock); isc__nmsocket_clearcb(sock);
UNLOCK(&sock->lock);
if (cb != NULL) { if (cb != NULL) {
cb(sock->statichandle, ISC_R_CANCELED, NULL, cbarg); cb(sock->statichandle, ISC_R_CANCELED, NULL, cbarg);
@@ -1176,16 +1169,15 @@ isc__nm_tcp_cancelread(isc_nmhandle_t *handle) {
REQUIRE(VALID_NMSOCK(sock)); REQUIRE(VALID_NMSOCK(sock));
REQUIRE(sock->type == isc_nm_tcpsocket); REQUIRE(sock->type == isc_nm_tcpsocket);
REQUIRE(sock->tid == isc_nm_tid());
if (atomic_load(&sock->client)) { if (atomic_load(&sock->client)) {
isc_nm_recv_cb_t cb; isc_nm_recv_cb_t cb;
void *cbarg; void *cbarg;
LOCK(&sock->lock);
cb = sock->recv_cb; cb = sock->recv_cb;
cbarg = sock->recv_cbarg; cbarg = sock->recv_cbarg;
isc__nmsocket_clearcb(sock); isc__nmsocket_clearcb(sock);
UNLOCK(&sock->lock);
cb(handle, ISC_R_EOF, NULL, cbarg); cb(handle, ISC_R_EOF, NULL, cbarg);
} }

14
lib/isc/netmgr/tcpdns.c
View File

@@ -119,10 +119,8 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
return (result); return (result);
} }
LOCK(&dnslistensock->lock);
accept_cb = dnslistensock->accept_cb; accept_cb = dnslistensock->accept_cb;
accept_cbarg = dnslistensock->accept_cbarg; accept_cbarg = dnslistensock->accept_cbarg;
UNLOCK(&dnslistensock->lock);
if (accept_cb != NULL) { if (accept_cb != NULL) {
result = accept_cb(handle, ISC_R_SUCCESS, accept_cbarg); result = accept_cb(handle, ISC_R_SUCCESS, accept_cbarg);
@@ -184,6 +182,7 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
size_t len; size_t len;
REQUIRE(VALID_NMSOCK(dnssock)); REQUIRE(VALID_NMSOCK(dnssock));
REQUIRE(dnssock->tid == isc_nm_tid());
REQUIRE(handlep != NULL && *handlep == NULL); REQUIRE(handlep != NULL && *handlep == NULL);
/* /*
@@ -214,12 +213,9 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
listener = dnssock->listener; listener = dnssock->listener;
if (listener != NULL) { if (listener != NULL) {
LOCK(&listener->lock);
cb = listener->recv_cb; cb = listener->recv_cb;
cbarg = listener->recv_cbarg; cbarg = listener->recv_cbarg;
UNLOCK(&listener->lock);
} else if (dnssock->recv_cb != NULL) { } else if (dnssock->recv_cb != NULL) {
LOCK(&dnssock->lock);
cb = dnssock->recv_cb; cb = dnssock->recv_cb;
cbarg = dnssock->recv_cbarg; cbarg = dnssock->recv_cbarg;
/* /*
@@ -228,7 +224,6 @@ processbuffer(isc_nmsocket_t *dnssock, isc_nmhandle_t **handlep) {
* call to isc_nm_read() and set up a new callback. * call to isc_nm_read() and set up a new callback.
*/ */
isc__nmsocket_clearcb(dnssock); isc__nmsocket_clearcb(dnssock);
UNLOCK(&dnssock->lock);
} }
if (cb != NULL) { if (cb != NULL) {
@@ -265,6 +260,7 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult,
size_t len; size_t len;
REQUIRE(VALID_NMSOCK(dnssock)); REQUIRE(VALID_NMSOCK(dnssock));
REQUIRE(dnssock->tid == isc_nm_tid());
REQUIRE(VALID_NMHANDLE(handle)); REQUIRE(VALID_NMHANDLE(handle));
if (region == NULL || eresult != ISC_R_SUCCESS) { if (region == NULL || eresult != ISC_R_SUCCESS) {
@@ -314,10 +310,8 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult,
uv_timer_stop(&dnssock->timer); uv_timer_stop(&dnssock->timer);
} }
LOCK(&dnssock->lock);
if (atomic_load(&dnssock->sequential) || if (atomic_load(&dnssock->sequential) ||
dnssock->recv_cb == NULL) { dnssock->recv_cb == NULL) {
UNLOCK(&dnssock->lock);
/* /*
* There are two reasons we might want to pause here: * There are two reasons we might want to pause here:
* - We're in sequential mode and we've received * - We're in sequential mode and we've received
@@ -328,7 +322,6 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult,
isc_nm_pauseread(dnssock->outerhandle); isc_nm_pauseread(dnssock->outerhandle);
done = true; done = true;
} else { } else {
UNLOCK(&dnssock->lock);
/* /*
* We're pipelining, so we now resume processing * We're pipelining, so we now resume processing
* packets until the clients-per-connection limit * packets until the clients-per-connection limit
@@ -364,12 +357,10 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_recv_cb_t cb,
REQUIRE(VALID_NM(mgr)); REQUIRE(VALID_NM(mgr));
isc__nmsocket_init(dnslistensock, mgr, isc_nm_tcpdnslistener, iface); isc__nmsocket_init(dnslistensock, mgr, isc_nm_tcpdnslistener, iface);
LOCK(&dnslistensock->lock);
dnslistensock->recv_cb = cb; dnslistensock->recv_cb = cb;
dnslistensock->recv_cbarg = cbarg; dnslistensock->recv_cbarg = cbarg;
dnslistensock->accept_cb = accept_cb; dnslistensock->accept_cb = accept_cb;
dnslistensock->accept_cbarg = accept_cbarg; dnslistensock->accept_cbarg = accept_cbarg;
UNLOCK(&dnslistensock->lock);
dnslistensock->extrahandlesize = extrahandlesize; dnslistensock->extrahandlesize = extrahandlesize;
/* /*
@@ -558,6 +549,7 @@ isc__nm_async_tcpdnssend(isc__networker_t *worker, isc__netievent_t *ev0) {
isc_nmsocket_t *sock = ievent->sock; isc_nmsocket_t *sock = ievent->sock;
REQUIRE(worker->id == sock->tid); REQUIRE(worker->id == sock->tid);
REQUIRE(sock->tid == isc_nm_tid());
result = ISC_R_NOTCONNECTED; result = ISC_R_NOTCONNECTED;
if (atomic_load(&sock->active) && sock->outerhandle != NULL) { if (atomic_load(&sock->active) && sock->outerhandle != NULL) {

7
lib/isc/netmgr/udp.c
View File

@@ -379,12 +379,7 @@ udp_recv_cb(uv_udp_t *handle, ssize_t nrecv, const uv_buf_t *buf,
region.base = (unsigned char *)buf->base; region.base = (unsigned char *)buf->base;
region.length = nrecv; region.length = nrecv;
/* INSIST(sock->tid == isc_nm_tid());
* In tcp.c and tcpdns.c, this would need to be locked
* by sock->lock because callbacks may be set to NULL
* unexpectedly when the connection drops, but that isn't
* a factor in the UDP case.
*/
INSIST(sock->recv_cb != NULL); INSIST(sock->recv_cb != NULL);
cb = sock->recv_cb; cb = sock->recv_cb;
cbarg = sock->recv_cbarg; cbarg = sock->recv_cbarg;