Fix possible deadlock in unix/socket.c

In process_fd we lock sock->lock and then internal_accept locks mgr->lock,
in isc_sockmgr_render* functions we lock mgr->lock and then lock sock->lock,
that can cause a deadlock when accessing stats. Unlock sock->lock early in
all the internal_{send,recv,connect,accept} functions instead of late
in process_fd.

diff --git a/CHANGES b/CHANGES
index 1e86ad87eb83301250e9a8ff32fd28168a7b9eb2..25924a89fe3e144a2e2be252cd4e2cd05768691f 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,5 @@
+5416.  [bug]           Fix a lock order inversion in unix/socket.c. [GL #1859]
+
 5415.  [test]          Address race in dnssec system test that led to
                        test failures. [GL #1852]
 

diff --git a/lib/isc/unix/socket.c b/lib/isc/unix/socket.c
index f1cb70ff715bb418e0ef806ef8ef2347000c7318..ee0f2f2596cf0fcb3a827c7b8b31cbf428d48f5d 100644 (file)
--- a/lib/isc/unix/socket.c
+++ b/lib/isc/unix/socket.c
@@ -2791,6 +2791,7 @@ 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;
        }
 
@@ -2919,6 +2920,12 @@ internal_accept(isc__socket_t *sock) {
                }
        }
 
+       /*
+        * We need to unlock sock->lock now to be able to lock manager->lock
+        * without risking a deadlock with xmlstats.
+        */
+       UNLOCK(&sock->lock);
+
        /*
         * -1 means the new socket didn't happen.
         */
@@ -3008,6 +3015,7 @@ 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;
@@ -3064,6 +3072,7 @@ finish:
                unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd,
                           SELECT_POKE_READ);
        }
+       UNLOCK(&sock->lock);
 }
 
 static void
@@ -3103,6 +3112,7 @@ finish:
                unwatch_fd(&sock->manager->threads[sock->threadid], sock->fd,
                           SELECT_POKE_WRITE);
        }
+       UNLOCK(&sock->lock);
 }
 
 /*
@@ -3133,6 +3143,7 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
        }
 
        LOCK(&sock->lock);
+
        if (sock->fd < 0) {
                /*
                 * Sock is being closed - the final external reference
@@ -3141,9 +3152,12 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
                 * thread->fdlock[lockid] or sock->lock that we're holding.
                 * Just release the locks and bail.
                 */
-               goto unlock;
+               UNLOCK(&sock->lock);
+               UNLOCK(&thread->fdlock[lockid]);
+               return;
        }
 
+       REQUIRE(readable || writeable);
        if (readable) {
                if (sock->listener) {
                        internal_accept(sock);
@@ -3160,9 +3174,9 @@ process_fd(isc__socketthread_t *thread, int fd, bool readable, bool writeable) {
                }
        }
 
-unlock:
-       UNLOCK(&sock->lock);
+       /* sock->lock is unlocked in internal_* function */
        UNLOCK(&thread->fdlock[lockid]);
+
        /*
         * Socket destruction might be pending, it will resume
         * after releasing fdlock and sock->lock.
@@ -4936,6 +4950,7 @@ 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