From 7a02fcaf20dbc19db36052bbc7001bcea3912ab5 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Wed, 23 Oct 2024 18:18:48 +0200 Subject: [PATCH] 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. --- include/haproxy/server-t.h | 2 +- include/haproxy/server.h | 2 +- src/cfgparse.c | 3 ++- src/server.c | 8 ++++---- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index 84b3b87c0c..e909a4d3b1 100644 --- 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 b8f8c71316..ade27c9408 100644 --- 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 af41497202..c29b6f6dba 100644 --- 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 @@ int check_config_validity() /* 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 fdc7502128..7d2ba4a12b 100644 --- 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;