From 43f3b3211f202e31b714c360b3f304f274e91d79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 1 Jul 2019 15:19:53 +0200 Subject: [PATCH 1/3] Convert couple isc__socket_t members to atomic to prevent data race (from TSAN) --- lib/isc/unix/socket.c | 56 ++++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c index efdb16b955c..ec935bb00ca 100644 --- a/lib/isc/unix/socket.c +++ b/lib/isc/unix/socket.c @@ -345,8 +345,8 @@ struct isc__socket { int fd; int pf; int threadid; - char name[16]; - void * tag; + char name[16]; + void * tag; ISC_LIST(isc_socketevent_t) send_list; ISC_LIST(isc_socketevent_t) recv_list; @@ -355,13 +355,13 @@ struct isc__socket { isc_sockaddr_t peer_address; /* remote address */ - unsigned int listener : 1, /* listener socket */ - connected : 1, - connecting : 1, /* connect pending */ - bound : 1, /* bound to local addr */ - dupped : 1, - active : 1, /* currently active */ - pktdscp : 1; /* per packet dscp */ + unsigned int listener : 1, /* listener socket */ + connected : 1, + connecting : 1, /* connect pending */ + bound : 1, /* bound to local addr */ + dupped : 1, + active : 1, /* currently active */ + pktdscp : 1; /* per packet dscp */ #ifdef ISC_PLATFORM_RECVOVERFLOW unsigned char overflow; /* used for MSG_TRUNC fake */ @@ -573,7 +573,7 @@ gen_threadid(isc__socket_t *sock); static int gen_threadid(isc__socket_t *sock) { - return sock->fd % sock->manager->nthreads; + return (sock->fd % sock->manager->nthreads); } static void @@ -878,9 +878,10 @@ wakeup_socket(isc__socketthread_t *thread, int fd, int msg) { INSIST(fd >= 0 && fd < (int)thread->manager->maxsocks); if (msg == SELECT_POKE_CLOSE) { - /* No one should be updating fdstate, so no need to lock it */ + LOCK(&thread->fdlock[lockid]); INSIST(thread->fdstate[fd] == CLOSE_PENDING); thread->fdstate[fd] = CLOSED; + UNLOCK(&thread->fdlock[lockid]); (void)unwatch_fd(thread, fd, SELECT_POKE_READ); (void)unwatch_fd(thread, fd, SELECT_POKE_WRITE); (void)close(fd); @@ -1310,8 +1311,9 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, " failed: %s", sock->fd, dscp >> 2, strbuf); - } else + } else { sock->dscp = dscp; + } } #endif #if defined(IPPROTO_IPV6) && defined(IPV6_TCLASS) @@ -1336,8 +1338,9 @@ build_msghdr_send(isc__socket_t *sock, char* cmsgbuf, isc_socketevent_t *dev, "%.02x) failed: %s", sock->fd, dscp >> 2, strbuf); - } else + } else { sock->dscp = dscp; + } } #endif if (msg->msg_controllen != 0 && @@ -1518,7 +1521,7 @@ doio_recv(isc__socket_t *sock, isc_socketevent_t *dev) { if (isc_log_wouldlog(isc_lctx, IOEVENT_LEVEL)) { strerror_r(recv_errno, strbuf, sizeof(strbuf)); socket_log(sock, NULL, IOEVENT, - "doio_recv: recvmsg(%d) %d bytes, err %d/%s", + "doio_recv: recvmsg(%d) %d bytes, err %d/%s", sock->fd, cc, recv_errno, strbuf); } @@ -1784,11 +1787,14 @@ socketclose(isc__socketthread_t *thread, isc__socket_t *sock, int fd) { select_poke(thread->manager, thread->threadid, fd, SELECT_POKE_CLOSE); inc_stats(thread->manager->stats, sock->statsindex[STATID_CLOSE]); + + LOCK(&sock->lock); if (sock->active == 1) { dec_stats(thread->manager->stats, sock->statsindex[STATID_ACTIVE]); sock->active = 0; } + UNLOCK(&sock->lock); /* * update manager->maxfd here (XXX: this should be implemented more @@ -1822,10 +1828,10 @@ socketclose(isc__socketthread_t *thread, isc__socket_t *sock, int fd) { static void destroy(isc__socket_t **sockp) { - int fd; + int fd = 0; isc__socket_t *sock = *sockp; isc__socketmgr_t *manager = sock->manager; - isc__socketthread_t *thread; + isc__socketthread_t *thread = NULL; socket_log(sock, NULL, CREATION, "destroying"); @@ -1835,11 +1841,16 @@ destroy(isc__socket_t **sockp) { INSIST(ISC_LIST_EMPTY(sock->send_list)); INSIST(sock->fd >= -1 && sock->fd < (int)manager->maxsocks); + LOCK(&sock->lock); if (sock->fd >= 0) { fd = sock->fd; thread = &manager->threads[sock->threadid]; sock->fd = -1; sock->threadid = -1; + } + UNLOCK(&sock->lock); + + if (fd > 0) { socketclose(thread, sock, fd); } @@ -1847,8 +1858,9 @@ destroy(isc__socket_t **sockp) { ISC_LIST_UNLINK(manager->socklist, sock, link); - if (ISC_LIST_EMPTY(manager->socklist)) + if (ISC_LIST_EMPTY(manager->socklist)) { SIGNAL(&manager->shutdown_ok); + } /* can't unlock manager as its memory context is still used */ free_socket(sockp); @@ -1872,7 +1884,7 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, sock->type = type; sock->fd = -1; sock->threadid = -1; - sock->dscp = 0; /* TOS/TCLASS is zero until set. */ + sock->dscp = 0; /* TOS/TCLASS is zero until set. */ sock->dupped = 0; sock->statsindex = NULL; sock->active = 0; @@ -1889,6 +1901,7 @@ allocate_socket(isc__socketmgr_t *manager, isc_sockettype_t type, ISC_LIST_INIT(sock->send_list); ISC_LIST_INIT(sock->accept_list); ISC_LIST_INIT(sock->connect_list); + sock->listener = 0; sock->connected = 0; sock->connecting = 0; @@ -2156,8 +2169,9 @@ opensocket(isc__socketmgr_t *manager, isc__socket_t *sock, sock->dupped = 1; sock->bound = dup_socket->bound; } - if (sock->fd == -1 && errno == EINTR && tries++ < 42) + if (sock->fd == -1 && errno == EINTR && tries++ < 42) { goto again; + } #ifdef F_DUPFD /* @@ -4124,6 +4138,10 @@ socket_send(isc__socket_t *sock, isc_socketevent_t *dev, isc_task_t *task, case DOIO_HARD: case DOIO_SUCCESS: + if (!have_lock) { + LOCK(&sock->lock); + have_lock = true; + } if ((flags & ISC_SOCKFLAG_IMMEDIATE) == 0) { send_senddone_event(sock, &dev); } -- GitLab From 9808d7360ea9fcedcadcffe8c424f247f8306fe5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 4 Jul 2019 16:10:19 +0200 Subject: [PATCH 2/3] Move the lock from internal_{accept,connect,recv,send} to global level to protect more socket variables --- lib/isc/unix/socket.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c index ec935bb00ca..7c53ec2325a 100644 --- a/lib/isc/unix/socket.c +++ b/lib/isc/unix/socket.c @@ -2778,10 +2778,8 @@ internal_accept(isc__socket_t *sock) { INSIST(VALID_SOCKET(sock)); - LOCK(&sock->lock); if (sock->fd < 0) { /* Socket is gone */ - UNLOCK(&sock->lock); return; } socket_log(sock, NULL, TRACE, @@ -2800,7 +2798,6 @@ internal_accept(isc__socket_t *sock) { dev = ISC_LIST_HEAD(sock->accept_list); if (dev == NULL) { unwatch_fd(thread, sock->fd, SELECT_POKE_ACCEPT); - UNLOCK(&sock->lock); return; } @@ -2916,11 +2913,10 @@ internal_accept(isc__socket_t *sock) { /* * Poke watcher if there are more pending accepts. */ - if (ISC_LIST_EMPTY(sock->accept_list)) + if (ISC_LIST_EMPTY(sock->accept_list)) { unwatch_fd(thread, sock->fd, SELECT_POKE_ACCEPT); - - UNLOCK(&sock->lock); + } if (fd != -1) { result = make_nonblock(fd); @@ -3017,7 +3013,6 @@ internal_accept(isc__socket_t *sock) { soft_error: watch_fd(thread, sock->fd, SELECT_POKE_ACCEPT); - UNLOCK(&sock->lock); inc_stats(manager->stats, sock->statsindex[STATID_ACCEPTFAIL]); return; @@ -3029,10 +3024,8 @@ internal_recv(isc__socket_t *sock) { INSIST(VALID_SOCKET(sock)); - LOCK(&sock->lock); if (sock->fd < 0) { /* Socket is gone */ - UNLOCK(&sock->lock); return; } dev = ISC_LIST_HEAD(sock->recv_list); @@ -3079,7 +3072,6 @@ internal_recv(isc__socket_t *sock) { unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd, SELECT_POKE_READ); } - UNLOCK(&sock->lock); } static void @@ -3088,10 +3080,8 @@ internal_send(isc__socket_t *sock) { INSIST(VALID_SOCKET(sock)); - LOCK(&sock->lock); if (sock->fd < 0) { /* Socket is gone */ - UNLOCK(&sock->lock); return; } dev = ISC_LIST_HEAD(sock->send_list); @@ -3125,7 +3115,6 @@ internal_send(isc__socket_t *sock) { unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd, SELECT_POKE_WRITE); } - UNLOCK(&sock->lock); } /* @@ -3166,6 +3155,7 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, return; } + LOCK(&sock->lock); if (readable) { if (sock->listener) { internal_accept(sock); @@ -3181,6 +3171,7 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, internal_send(sock); } } + UNLOCK(&sock->lock); UNLOCK(&thread->fdlock[lockid]); if (isc_refcount_decrement(&sock->references) == 1) { @@ -4875,8 +4866,6 @@ internal_connect(isc__socket_t *sock) { INSIST(VALID_SOCKET(sock)); - LOCK(&sock->lock); - /* * Get the first item off the connect list. * If it is empty, unlock the socket and return. @@ -4907,7 +4896,6 @@ internal_connect(isc__socket_t *sock) { */ if (SOFT_ERROR(errno) || errno == EINPROGRESS) { sock->connecting = 1; - UNLOCK(&sock->lock); return; } @@ -4960,8 +4948,6 @@ internal_connect(isc__socket_t *sock) { finish: unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd, SELECT_POKE_CONNECT); - - UNLOCK(&sock->lock); } isc_result_t -- GitLab From 718a317dc7dbf836b604073e477ce4029272f321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Sat, 20 Jul 2019 16:10:49 -0400 Subject: [PATCH 3/3] Fix unprotected access to thread->epoll_events[fd] in unwatch_fd() --- lib/isc/unix/socket.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c index 7c53ec2325a..50636198800 100644 --- a/lib/isc/unix/socket.c +++ b/lib/isc/unix/socket.c @@ -881,17 +881,15 @@ wakeup_socket(isc__socketthread_t *thread, int fd, int msg) { LOCK(&thread->fdlock[lockid]); INSIST(thread->fdstate[fd] == CLOSE_PENDING); thread->fdstate[fd] = CLOSED; - UNLOCK(&thread->fdlock[lockid]); (void)unwatch_fd(thread, fd, SELECT_POKE_READ); (void)unwatch_fd(thread, fd, SELECT_POKE_WRITE); (void)close(fd); + UNLOCK(&thread->fdlock[lockid]); return; } LOCK(&thread->fdlock[lockid]); if (thread->fdstate[fd] == CLOSE_PENDING) { - UNLOCK(&thread->fdlock[lockid]); - /* * We accept (and ignore) any error from unwatch_fd() as we are * closing the socket, hoping it doesn't leave dangling state in @@ -902,6 +900,7 @@ wakeup_socket(isc__socketthread_t *thread, int fd, int msg) { */ (void)unwatch_fd(thread, fd, SELECT_POKE_READ); (void)unwatch_fd(thread, fd, SELECT_POKE_WRITE); + UNLOCK(&thread->fdlock[lockid]); return; } if (thread->fdstate[fd] != MANAGED) { -- GitLab