BUG/MEDIUM: protocols: add a global lock for the init/deinit stuff

Dragan Dosen found that the listeners lock is not sufficient to protect
the listeners list when proxies are stopping because the listeners are
also unlinked from the protocol list, and under certain situations like
bombing with soft-stop signals or shutting down many frontends in parallel
from multiple CLI connections, it could be possible to provoke multiple
instances of delete_listener() to be called in parallel for different
listeners, thus corrupting the protocol lists.

Such operations are pretty rare, they are performed once per proxy upon
startup and once per proxy on shut down. Thus there is no point trying
to optimize anything and we can use a global lock to protect the protocol
lists during these manipulations.

This fix (or a variant) will have to be backported as far as 1.8.

diff --git a/include/proto/protocol.h b/include/proto/protocol.h
index 7bbebb8e8c243fc0ee8483efd8076f44948cdc00..f25f77f0a06455c8582aa3f6a50eb1c1643cd8fd 100644 (file)
--- a/include/proto/protocol.h
+++ b/include/proto/protocol.h
@@ -23,9 +23,11 @@
 #define _PROTO_PROTOCOL_H
 
 #include <sys/socket.h>
+#include <common/hathreads.h>
 #include <types/protocol.h>
 
 extern struct protocol *__protocol_by_family[AF_CUST_MAX];
+__decl_hathreads(extern HA_SPINLOCK_T proto_lock);
 
 /* Registers the protocol <proto> */
 void protocol_register(struct protocol *proto);

diff --git a/include/types/protocol.h b/include/types/protocol.h
index 1d3404b91dc903aec9570234871a8e21f611a507..f38baeb976c37ed5295c90abddff0898d4a91443 100644 (file)
--- a/include/types/protocol.h
+++ b/include/types/protocol.h
@@ -80,9 +80,9 @@ struct protocol {
        int (*pause)(struct listener *l);               /* temporarily pause this listener for a soft restart */
        void (*add)(struct listener *l, int port);      /* add a listener for this protocol and port */
 
-       struct list listeners;                          /* list of listeners using this protocol */
-       int nb_listeners;                               /* number of listeners */
-       struct list list;                               /* list of registered protocols */
+       struct list listeners;                          /* list of listeners using this protocol (under proto_lock) */
+       int nb_listeners;                               /* number of listeners (under proto_lock) */
+       struct list list;                               /* list of registered protocols (under proto_lock) */
 };
 
 #define CONNECT_HAS_DATA                        0x00000001 /* There's data available to be sent */

diff --git a/src/listener.c b/src/listener.c
index 40a774ed5768cda2c7933ca4b4ba488354062743..b5fe2ac21fbc978624c0c6c572f93dd4f51b2d6b 100644 (file)
--- a/src/listener.c
+++ b/src/listener.c
@@ -433,6 +433,9 @@ static void limit_listener(struct listener *l, struct list *list)
  * used as a protocol's generic enable_all() primitive, for use after the
  * fork(). It puts the listeners into LI_READY or LI_FULL states depending on
  * their number of connections. It always returns ERR_NONE.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 int enable_all_listeners(struct protocol *proto)
 {
@@ -447,6 +450,9 @@ int enable_all_listeners(struct protocol *proto)
  * the polling lists when they are in the LI_READY or LI_FULL states. It is
  * intended to be used as a protocol's generic disable_all() primitive. It puts
  * the listeners into LI_LISTEN, and always returns ERR_NONE.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 int disable_all_listeners(struct protocol *proto)
 {
@@ -516,6 +522,9 @@ void unbind_listener_no_close(struct listener *listener)
 /* This function closes all listening sockets bound to the protocol <proto>,
  * and the listeners end in LI_ASSIGNED state if they were higher. It does not
  * detach them from the protocol. It always returns ERR_NONE.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 int unbind_all_listeners(struct protocol *proto)
 {
@@ -580,14 +589,19 @@ int create_listeners(struct bind_conf *bc, const struct sockaddr_storage *ss,
  * number of listeners is updated, as well as the global number of listeners
  * and jobs. Note that the listener must have previously been unbound. This
  * is the generic function to use to remove a listener.
+ *
+ * Will grab the proto_lock.
+ *
  */
 void delete_listener(struct listener *listener)
 {
        HA_SPIN_LOCK(LISTENER_LOCK, &listener->lock);
        if (listener->state == LI_ASSIGNED) {
                listener->state = LI_INIT;
+               HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
                LIST_DEL(&listener->proto_list);
                listener->proto->nb_listeners--;
+               HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
                _HA_ATOMIC_SUB(&jobs, 1);
                _HA_ATOMIC_SUB(&listeners, 1);
        }

diff --git a/src/proto_sockpair.c b/src/proto_sockpair.c
index 163666799f2d2a50772370089a1519fa2ab14e0e..a6005ee5bf99a39283e0817bb7e3d0953e266d25 100644 (file)
--- a/src/proto_sockpair.c
+++ b/src/proto_sockpair.c
@@ -80,6 +80,9 @@ INITCALL1(STG_REGISTER, protocol_register, &proto_sockpair);
 /* Add <listener> to the list of sockpair listeners (port is ignored). The
  * listener's state is automatically updated from LI_INIT to LI_ASSIGNED.
  * The number of listeners for the protocol is updated.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static void sockpair_add_listener(struct listener *listener, int port)
 {
@@ -97,6 +100,8 @@ static void sockpair_add_listener(struct listener *listener, int port)
  * loose them across the fork(). A call to uxst_enable_listeners() is needed
  * to complete initialization.
  *
+ * Must be called with proto_lock held.
+ *
  * The return value is composed from ERR_NONE, ERR_RETRYABLE and ERR_FATAL.
  */
 static int sockpair_bind_listeners(struct protocol *proto, char *errmsg, int errlen)

diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index fd7847d1a9efb3cc55b4de128d49db595b24c025..ff252e8916a4745ac551ee1ec6027af4483fe632 100644 (file)
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -1107,6 +1107,9 @@ int tcp_bind_listener(struct listener *listener, char *errmsg, int errlen)
  * The sockets will be registered but not added to any fd_set, in order not to
  * loose them across the fork(). A call to enable_all_listeners() is needed
  * to complete initialization. The return value is composed from ERR_*.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static int tcp_bind_listeners(struct protocol *proto, char *errmsg, int errlen)
 {
@@ -1125,6 +1128,9 @@ static int tcp_bind_listeners(struct protocol *proto, char *errmsg, int errlen)
 /* Add <listener> to the list of tcpv4 listeners, on port <port>. The
  * listener's state is automatically updated from LI_INIT to LI_ASSIGNED.
  * The number of listeners for the protocol is updated.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static void tcpv4_add_listener(struct listener *listener, int port)
 {
@@ -1140,6 +1146,9 @@ static void tcpv4_add_listener(struct listener *listener, int port)
 /* Add <listener> to the list of tcpv6 listeners, on port <port>. The
  * listener's state is automatically updated from LI_INIT to LI_ASSIGNED.
  * The number of listeners for the protocol is updated.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static void tcpv6_add_listener(struct listener *listener, int port)
 {

diff --git a/src/proto_uxst.c b/src/proto_uxst.c
index 513f34dad73c55ca4648f3eed3a58fe75f64eb59..6074daa46c5a4c732c39129d9cdc411b3efd082d 100644 (file)
--- a/src/proto_uxst.c
+++ b/src/proto_uxst.c
@@ -379,6 +379,9 @@ static int uxst_unbind_listener(struct listener *listener)
 /* Add <listener> to the list of unix stream listeners (port is ignored). The
  * listener's state is automatically updated from LI_INIT to LI_ASSIGNED.
  * The number of listeners for the protocol is updated.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static void uxst_add_listener(struct listener *listener, int port)
 {
@@ -594,6 +597,8 @@ static int uxst_connect_server(struct connection *conn, int flags)
  * loose them across the fork(). A call to uxst_enable_listeners() is needed
  * to complete initialization.
  *
+ * Must be called with proto_lock held.
+ *
  * The return value is composed from ERR_NONE, ERR_RETRYABLE and ERR_FATAL.
  */
 static int uxst_bind_listeners(struct protocol *proto, char *errmsg, int errlen)
@@ -613,6 +618,9 @@ static int uxst_bind_listeners(struct protocol *proto, char *errmsg, int errlen)
 /* This function stops all listening UNIX sockets bound to the protocol
  * <proto>. It does not detaches them from the protocol.
  * It always returns ERR_NONE.
+ *
+ * Must be called with proto_lock held.
+ *
  */
 static int uxst_unbind_listeners(struct protocol *proto)
 {

diff --git a/src/protocol.c b/src/protocol.c
index 96e01c82787190823f9cd68c1a584713aa85afe1..ac45cf2e2180b9169f88ef4d48ccf7f7aacc6c65 100644 (file)
--- a/src/protocol.c
+++ b/src/protocol.c
@@ -18,18 +18,26 @@
 #include <common/mini-clist.h>
 #include <common/standard.h>
 
-#include <types/protocol.h>
+#include <proto/protocol.h>
 
 /* List head of all registered protocols */
 static struct list protocols = LIST_HEAD_INIT(protocols);
 struct protocol *__protocol_by_family[AF_CUST_MAX] = { };
 
+/* This is the global spinlock we may need to register/unregister listeners or
+ * protocols. Its main purpose is in fact to serialize the rare stop/deinit()
+ * phases.
+ */
+__decl_spinlock(proto_lock);
+
 /* Registers the protocol <proto> */
 void protocol_register(struct protocol *proto)
 {
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        LIST_ADDQ(&protocols, &proto->list);
        if (proto->sock_domain >= 0 && proto->sock_domain < AF_CUST_MAX)
                __protocol_by_family[proto->sock_domain] = proto;
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
 }
 
 /* Unregisters the protocol <proto>. Note that all listeners must have
@@ -37,8 +45,10 @@ void protocol_register(struct protocol *proto)
  */
 void protocol_unregister(struct protocol *proto)
 {
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        LIST_DEL(&proto->list);
        LIST_INIT(&proto->list);
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
 }
 
 /* binds all listeners of all registered protocols. Returns a composition
@@ -50,6 +60,7 @@ int protocol_bind_all(char *errmsg, int errlen)
        int err;
 
        err = 0;
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        list_for_each_entry(proto, &protocols, list) {
                if (proto->bind_all) {
                        err |= proto->bind_all(proto, errmsg, errlen);
@@ -57,6 +68,7 @@ int protocol_bind_all(char *errmsg, int errlen)
                                break;
                }
        }
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
        return err;
 }
 
@@ -71,11 +83,13 @@ int protocol_unbind_all(void)
        int err;
 
        err = 0;
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        list_for_each_entry(proto, &protocols, list) {
                if (proto->unbind_all) {
                        err |= proto->unbind_all(proto);
                }
        }
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
        return err;
 }
 
@@ -89,11 +103,13 @@ int protocol_enable_all(void)
        int err;
 
        err = 0;
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        list_for_each_entry(proto, &protocols, list) {
                if (proto->enable_all) {
                        err |= proto->enable_all(proto);
                }
        }
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
        return err;
 }
 
@@ -107,11 +123,13 @@ int protocol_disable_all(void)
        int err;
 
        err = 0;
+       HA_SPIN_LOCK(PROTO_LOCK, &proto_lock);
        list_for_each_entry(proto, &protocols, list) {
                if (proto->disable_all) {
                        err |= proto->disable_all(proto);
                }
        }
+       HA_SPIN_UNLOCK(PROTO_LOCK, &proto_lock);
        return err;
 }