From: Christopher Faulet <cfaulet@haproxy.com>
Date: Fri, 16 Mar 2018 09:04:47 +0000 (+0100)
Subject: BUG/MEDIUM: threads/unix: Fix a deadlock when a listener is temporarily disabled
X-Git-Tag: v1.9-dev1~375
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=510c0d67ef8c44172b63be1a3d69be5f03ef14c3;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: threads/unix: Fix a deadlock when a listener is temporarily disabled

When a listener is temporarily disabled, we start by locking it and then we call
.pause callback of the underlying protocol (tcp/unix). For TCP listeners, this
is not a problem. But listeners bound on an unix socket are in fact closed
instead. So .pause callback relies on unbind_listener function to do its job.

Unfortunatly, unbind_listener hold the listener's lock and then call an internal
function to unbind it. So, there is a deadlock here. This happens during a
reload. To fix the problemn, the function do_unbind_listener, which is lockless,
is now exported and is called when a listener bound on an unix socket is
temporarily disabled.

This patch must be backported in 1.8.
---

diff --git a/include/proto/listener.h b/include/proto/listener.h
index a33aeed4a8..fdace2b5f9 100644
--- a/include/proto/listener.h
+++ b/include/proto/listener.h
@@ -60,6 +60,11 @@ int disable_all_listeners(struct protocol *proto);
 /* Dequeues all of the listeners waiting for a resource in wait queue <queue>. */
 void dequeue_all_listeners(struct list *list);
 
+/* Must be called with the lock held. Depending on <do_close> value, it does
+ * what unbind_listener or unbind_listener_no_close should do.
+ */
+void do_unbind_listener(struct listener *listener, int do_close);
+
 /* This function closes the listening socket for the specified listener,
  * provided that it's already in a listening state. The listener enters the
  * LI_ASSIGNED state. This function is intended to be used as a generic
diff --git a/src/listener.c b/src/listener.c
index f60a14624f..283058ace4 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -49,8 +49,6 @@ static struct bind_kw_list bind_keywords = {
 
 struct xfer_sock_list *xfer_sock_list = NULL;
 
-static void __do_unbind_listener(struct listener *listener, int do_close);
-
 /* This function adds the specified listener's file descriptor to the polling
  * lists if it is in the LI_LISTEN state. The listener enters LI_READY or
  * LI_FULL state depending on its number of connections. In deamon mode, we
@@ -68,9 +66,9 @@ static void enable_listener(struct listener *listener)
 			 * want any fd event to reach it.
 			 */
 			if (!(global.tune.options & GTUNE_SOCKET_TRANSFER))
-				__do_unbind_listener(listener, 1);
+				do_unbind_listener(listener, 1);
 			else {
-				__do_unbind_listener(listener, 0);
+				do_unbind_listener(listener, 0);
 				listener->state = LI_LISTEN;
 			}
 		}
@@ -308,8 +306,10 @@ void dequeue_all_listeners(struct list *list)
 	HA_SPIN_UNLOCK(LISTENER_QUEUE_LOCK, &lq_lock);
 }
 
-/* must be called with the lock held */
-static void __do_unbind_listener(struct listener *listener, int do_close)
+/* Must be called with the lock held. Depending on <do_close> value, it does
+ * what unbind_listener or unbind_listener_no_close should do.
+ */
+void do_unbind_listener(struct listener *listener, int do_close)
 {
 	if (listener->state == LI_READY)
 		fd_stop_recv(listener->fd);
@@ -331,13 +331,6 @@ static void __do_unbind_listener(struct listener *listener, int do_close)
 	}
 }
 
-static void do_unbind_listener(struct listener *listener, int do_close)
-{
-	HA_SPIN_LOCK(LISTENER_LOCK, &listener->lock);
-	__do_unbind_listener(listener, do_close);
-	HA_SPIN_UNLOCK(LISTENER_LOCK, &listener->lock);
-}
-
 /* This function closes the listening socket for the specified listener,
  * provided that it's already in a listening state. The listener enters the
  * LI_ASSIGNED state. This function is intended to be used as a generic
@@ -345,7 +338,9 @@ static void do_unbind_listener(struct listener *listener, int do_close)
  */
 void unbind_listener(struct listener *listener)
 {
+	HA_SPIN_LOCK(LISTENER_LOCK, &listener->lock);
 	do_unbind_listener(listener, 1);
+	HA_SPIN_UNLOCK(LISTENER_LOCK, &listener->lock);
 }
 
 /* This function pretends the listener is dead, but keeps the FD opened, so
@@ -353,7 +348,9 @@ void unbind_listener(struct listener *listener)
  */
 void unbind_listener_no_close(struct listener *listener)
 {
+	HA_SPIN_LOCK(LISTENER_LOCK, &listener->lock);
 	do_unbind_listener(listener, 0);
+	HA_SPIN_UNLOCK(LISTENER_LOCK, &listener->lock);
 }
 
 /* This function closes all listening sockets bound to the protocol <proto>,
diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index 0f717385ec..9fc50dff46 100644
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -393,7 +393,9 @@ static int uxst_pause_listener(struct listener *l)
 	if (((struct sockaddr_un *)&l->addr)->sun_path[0])
 		return 1;
 
-	unbind_listener(l);
+	/* Listener's lock already held. Call lockless version of
+	 * unbind_listener. */
+	do_unbind_listener(l, 1);
 	return 0;
 }