BUG/MEDIUM: server: fix race on servers_list during server deletion

Each server is inserted in a global list named servers_list on
new_server(). This list is then only used to finalize servers
initialization after parsing.

On dynamic server creation, there is no issue as new_server() is under
thread isolation. However, when a server is deleted after its refcount
reached zero, srv_drop() removes it from servers_list without lock
protection. In the longterm, this can cause list corruption and crashes,
especially if multiple adjacent servers are removed in parallel.

To fix this, convert servers_list to a mt_list. This should not impact
performance as servers_list is not used during runtime outside of server
creation/deletion.

This should fix github issue #2733. Thanks to Chris Staite who first
found the issue here.

This must be backported up to 2.6.

diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h
index 84b3b87c0c845b70b114959caf446df3793e91ad..e909a4d3b1ba8d1a7b6549e881dd212dcb5aaf8d 100644 (file)
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -301,7 +301,7 @@ struct server {
        signed char use_ssl;                    /* ssl enabled (1: on, 0: disabled, -1 forced off)  */
        unsigned int flags;                     /* server flags (SRV_F_*) */
        unsigned int pp_opts;                   /* proxy protocol options (SRV_PP_*) */
-       struct list global_list;                /* attach point in the global servers_list */
+       struct mt_list global_list;             /* attach point in the global servers_list */
        struct server *next;
        struct mt_list prev_deleted;            /* deleted servers with 'next' ptr pointing to us */
        int cklen;                              /* the len of the cookie, to speed up checks */

diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index b8f8c71316f8c638ce972847a0a71b2f871fa667..ade27c9408ca07a160e368708faaabd2339ae0be 100644 (file)
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -41,7 +41,7 @@
 __decl_thread(extern HA_SPINLOCK_T idle_conn_srv_lock);
 extern struct idle_conns idle_conns[MAX_THREADS];
 extern struct task *idle_conn_task;
-extern struct list servers_list;
+extern struct mt_list servers_list;
 extern struct dict server_key_dict;
 
 int srv_downtime(const struct server *s);

diff --git a/src/cfgparse.c b/src/cfgparse.c
index af414972027c55bed01eadbd438dbf5aeb1db002..c29b6f6dbae125693651fd2c832e611cb2255f5d 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2810,6 +2810,7 @@ int check_config_validity()
        struct proxy *init_proxies_list = NULL;
        struct stktable *t;
        struct server *newsrv = NULL;
+       struct mt_list back;
        int err_code = 0;
        unsigned int next_pxid = 1;
        struct bind_conf *bind_conf;
@@ -4250,7 +4251,7 @@ out_uri_auth_compat:
 
        /* we must finish to initialize certain things on the servers */
 
-       list_for_each_entry(newsrv, &servers_list, global_list) {
+       MT_LIST_FOR_EACH_ENTRY_LOCKED(newsrv, &servers_list, global_list, back) {
                /* initialize idle conns lists */
                if (srv_init_per_thr(newsrv) == -1) {
                        ha_alert("parsing [%s:%d] : failed to allocate per-thread lists for server '%s'.\n",

diff --git a/src/server.c b/src/server.c
index fdc750212858cfb5eefdd3fe7ef1cad0e1d0bfc3..7d2ba4a12b44a6773f1742f894ba0166ea561cbb 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -72,7 +72,7 @@ struct srv_kw_list srv_keywords = {
 __decl_thread(HA_SPINLOCK_T idle_conn_srv_lock);
 struct eb_root idle_conn_srv = EB_ROOT;
 struct task *idle_conn_task __read_mostly = NULL;
-struct list servers_list = LIST_HEAD_INIT(servers_list);
+struct mt_list servers_list = MT_LIST_HEAD_INIT(servers_list);
 static struct task *server_atomic_sync_task = NULL;
 static event_hdl_async_equeue server_atomic_sync_queue;
 
@@ -2962,7 +2962,7 @@ struct server *new_server(struct proxy *proxy)
        srv->obj_type = OBJ_TYPE_SERVER;
        srv->proxy = proxy;
        queue_init(&srv->queue, proxy, srv);
-       LIST_APPEND(&servers_list, &srv->global_list);
+       MT_LIST_APPEND(&servers_list, &srv->global_list);
        LIST_INIT(&srv->srv_rec_item);
        LIST_INIT(&srv->ip_rec_item);
        LIST_INIT(&srv->pp_tlvs);
@@ -3088,7 +3088,7 @@ struct server *srv_drop(struct server *srv)
 
        HA_SPIN_DESTROY(&srv->lock);
 
-       LIST_DELETE(&srv->global_list);
+       MT_LIST_DELETE(&srv->global_list);
        event_hdl_sub_list_destroy(&srv->e_subs);
 
        EXTRA_COUNTERS_FREE(srv->extra_counters);
@@ -3267,7 +3267,7 @@ static int _srv_parse_tmpl_init(struct server *srv, struct proxy *px)
                release_sample_expr(newsrv->ssl_ctx.sni);
                free_check(&newsrv->agent);
                free_check(&newsrv->check);
-               LIST_DELETE(&newsrv->global_list);
+               MT_LIST_DELETE(&newsrv->global_list);
        }
        free(newsrv);
        return i - srv->tmpl_info.nb_low;