From d79295d89bc086caa5ddf7ef255c711429a97174 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Wed, 12 Nov 2025 17:38:08 +0100 Subject: [PATCH] Revert "BUG/MEDIUM: connections: permit to permanently remove an idle conn" The target patch fixes a rare race condition which happen when a MUX IO handler is working on a connection already moved into the purge list. In this case, the handler will incorrectly moved back the connection into the idle list. To fix this, conn_delete_from_tree() was extended to remove flags along with the connection from the idle list. This was performed when the connection is moved into the purge list. However, it introduces another issue related to the idle server connection accounting. Thus it is necessary to revert it prior to the incoming newer fix. This patch must be backported to every version where the original commit is. --- include/haproxy/connection.h | 2 +- src/backend.c | 14 +++++++------- src/connection.c | 10 +++------- src/mux_fcgi.c | 4 ++-- src/mux_h1.c | 4 ++-- src/mux_h2.c | 10 +++++----- src/mux_quic.c | 6 +++--- src/mux_spop.c | 6 +++--- src/server.c | 9 +++++---- src/ssl_sock.c | 2 +- 10 files changed, 32 insertions(+), 35 deletions(-) diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h index 0c25e5184..7943ee39f 100644 --- a/include/haproxy/connection.h +++ b/include/haproxy/connection.h @@ -83,7 +83,7 @@ int conn_install_mux_be(struct connection *conn, void *ctx, struct session *sess const struct mux_ops *force_mux_ops); int conn_install_mux_chk(struct connection *conn, void *ctx, struct session *sess); -void conn_delete_from_tree(struct connection *conn, int thr, int permanent); +void conn_delete_from_tree(struct connection *conn, int thr); void conn_init(struct connection *conn, void *target); struct connection *conn_new(void *target); diff --git a/src/backend.c b/src/backend.c index ae6eac255..cac056efb 100644 --- a/src/backend.c +++ b/src/backend.c @@ -1319,7 +1319,7 @@ struct connection *conn_backend_get(int reuse_mode, HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); conn = srv_lookup_conn(is_safe ? &srv->per_thr[tid].safe_conns : &srv->per_thr[tid].idle_conns, hash); if (conn) - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); /* If we failed to pick a connection from the idle list, let's try again with * the safe list. @@ -1327,7 +1327,7 @@ struct connection *conn_backend_get(int reuse_mode, if (!conn && !is_safe && srv->curr_safe_nb > 0) { conn = srv_lookup_conn(&srv->per_thr[tid].safe_conns, hash); if (conn) { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); is_safe = 1; } } @@ -1388,7 +1388,7 @@ check_tgid: conn = srv_lookup_conn(tree, hash); while (conn) { if (conn->mux->takeover && conn->mux->takeover(conn, i, 0) == 0) { - conn_delete_from_tree(conn, i, 0); + conn_delete_from_tree(conn, i); _HA_ATOMIC_INC(&activity[tid].fd_takeover); found = 1; break; @@ -1490,7 +1490,7 @@ takeover_random_idle_conn(struct ceb_root **root, int curtid) conn = ceb64_item_first(root, hash_node.node, hash_node.key, struct connection); while (conn) { if (conn->mux->takeover && conn->mux->takeover(conn, curtid, 1) == 0) { - conn_delete_from_tree(conn, curtid, 0); + conn_delete_from_tree(conn, curtid); return conn; } conn = ceb64_item_next(root, hash_node.node, hash_node.key, conn); @@ -1751,7 +1751,7 @@ int be_reuse_connection(int64_t hash, struct session *sess, if (avail <= 1) { /* no more streams available, remove it from the list */ HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(srv_conn, tid, 0); + conn_delete_from_tree(srv_conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } @@ -1863,7 +1863,7 @@ int connect_server(struct stream *s) HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (!LIST_ISEMPTY(&srv->per_thr[tid].idle_conn_list)) { tokill_conn = LIST_ELEM(srv->per_thr[tid].idle_conn_list.n, struct connection *, idle_list); - conn_delete_from_tree(tokill_conn, tid, 1); + conn_delete_from_tree(tokill_conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); /* Release the idle lock before calling mux->destroy. @@ -1892,7 +1892,7 @@ int connect_server(struct stream *s) if (!LIST_ISEMPTY(&srv->per_thr[i].idle_conn_list)) { tokill_conn = LIST_ELEM(srv->per_thr[i].idle_conn_list.n, struct connection *, idle_list); - conn_delete_from_tree(tokill_conn, i, 1); + conn_delete_from_tree(tokill_conn, i); } HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[i].idle_conns_lock); diff --git a/src/connection.c b/src/connection.c index 201cd3497..d2ba279e6 100644 --- a/src/connection.c +++ b/src/connection.c @@ -74,13 +74,11 @@ struct conn_tlv_list *conn_get_tlv(struct connection *conn, int type) /* Remove idle connection from its attached tree (idle, safe or avail) * for the server in the connection's target and thread . If also present - * in the secondary server idle list, conn is removed from it. Finally, if - * is non-nul, the idle connection flags are cleared as well so - * that the connection is not re-inserted later. + * in the secondary server idle list, conn is removed from it. * * Must be called with idle_conns_lock held. */ -void conn_delete_from_tree(struct connection *conn, int thr, int permanent) +void conn_delete_from_tree(struct connection *conn, int thr) { struct ceb_root **conn_tree; struct server *srv = __objt_server(conn->target); @@ -102,8 +100,6 @@ void conn_delete_from_tree(struct connection *conn, int thr, int permanent) } ceb64_item_delete(conn_tree, hash_node.node, hash_node.key, conn); - if (permanent) - conn->flags &= ~CO_FL_LIST_MASK; } int conn_create_mux(struct connection *conn, int *closed_connection) @@ -231,7 +227,7 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index ff9521bc5..ba736f371 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -3068,7 +3068,7 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } } @@ -3327,7 +3327,7 @@ struct task *fcgi_timeout_task(struct task *t, void *context, unsigned int state * to steal it from us. */ if (fconn->conn->flags & CO_FL_LIST_MASK) - conn_delete_from_tree(fconn->conn, tid, 0); + conn_delete_from_tree(fconn->conn, tid); else if (fconn->conn->flags & CO_FL_SESS_IDLE) session_detach_idle_conn(fconn->conn->owner, fconn->conn); diff --git a/src/mux_h1.c b/src/mux_h1.c index 722143464..60862fc57 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -4349,7 +4349,7 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } } @@ -4493,7 +4493,7 @@ struct task *h1_timeout_task(struct task *t, void *context, unsigned int state) * to steal it from us. */ if (h1c->conn->flags & CO_FL_LIST_MASK) - conn_delete_from_tree(h1c->conn, tid, 1); + conn_delete_from_tree(h1c->conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); diff --git a/src/mux_h2.c b/src/mux_h2.c index 54d3c212b..963aa1a8a 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -4985,7 +4985,7 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } } @@ -5160,7 +5160,7 @@ static int h2_process(struct h2c *h2c) /* connections in error must be removed from the idle lists */ if (conn->flags & CO_FL_LIST_MASK) { HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(conn, tid, 1); + conn_delete_from_tree(conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } } @@ -5168,7 +5168,7 @@ static int h2_process(struct h2c *h2c) /* connections in error must be removed from the idle lists */ if (conn->flags & CO_FL_LIST_MASK) { HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(conn, tid, 1); + conn_delete_from_tree(conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } } @@ -5276,7 +5276,7 @@ struct task *h2_timeout_task(struct task *t, void *context, unsigned int state) * to steal it from us. */ if (h2c->conn->flags & CO_FL_LIST_MASK) - conn_delete_from_tree(h2c->conn, tid, 1); + conn_delete_from_tree(h2c->conn, tid); else if (h2c->conn->flags & CO_FL_SESS_IDLE) session_detach_idle_conn(h2c->conn->owner, h2c->conn); @@ -5358,7 +5358,7 @@ do_leave: /* in any case this connection must not be considered idle anymore */ if (h2c->conn->flags & CO_FL_LIST_MASK) { HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(h2c->conn, tid, 1); + conn_delete_from_tree(h2c->conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } diff --git a/src/mux_quic.c b/src/mux_quic.c index 135c7d8ec..da471cc57 100644 --- a/src/mux_quic.c +++ b/src/mux_quic.c @@ -3201,7 +3201,7 @@ static void qcc_shutdown(struct qcc *qcc) /* A connection is not reusable if app layer is closed. */ if (qcc->flags & QC_CF_IS_BACK) - conn_delete_from_tree(qcc->conn, tid, 1); + conn_delete_from_tree(qcc->conn, tid); out: qcc->app_st = QCC_APP_ST_SHUT; @@ -3404,7 +3404,7 @@ struct task *qcc_io_cb(struct task *t, void *ctx, unsigned int state) if (conn->flags & CO_FL_SESS_IDLE) session_detach_idle_conn(conn->owner, conn); else - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); @@ -3526,7 +3526,7 @@ static struct task *qcc_timeout_task(struct task *t, void *ctx, unsigned int sta * attempts to steal it from us. */ if (qcc->conn->flags & CO_FL_LIST_MASK) - conn_delete_from_tree(qcc->conn, tid, 1); + conn_delete_from_tree(qcc->conn, tid); else if (qcc->conn->flags & CO_FL_SESS_IDLE) session_unown_conn(qcc->conn->owner, qcc->conn); diff --git a/src/mux_spop.c b/src/mux_spop.c index 00878b669..1307ff6e0 100644 --- a/src/mux_spop.c +++ b/src/mux_spop.c @@ -2564,7 +2564,7 @@ static struct task *spop_io_cb(struct task *t, void *ctx, unsigned int state) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } } @@ -2674,7 +2674,7 @@ static int spop_process(struct spop_conn *spop_conn) /* connections in error must be removed from the idle lists */ if (conn->flags & CO_FL_LIST_MASK) { HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(conn, tid, 1); + conn_delete_from_tree(conn, tid); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } } @@ -2797,7 +2797,7 @@ static struct task *spop_timeout_task(struct task *t, void *context, unsigned in * to steal it from us. */ if (spop_conn->conn->flags & CO_FL_LIST_MASK) - conn_delete_from_tree(spop_conn->conn, tid, 1); + conn_delete_from_tree(spop_conn->conn, tid); else if (spop_conn->conn->flags & CO_FL_SESS_IDLE) session_detach_idle_conn(spop_conn->conn->owner, spop_conn->conn); diff --git a/src/server.c b/src/server.c index 60d138c95..b61489042 100644 --- a/src/server.c +++ b/src/server.c @@ -7214,7 +7214,7 @@ static int srv_migrate_conns_to_remove(struct server *srv, int thr, int toremove break; conn = LIST_ELEM(srv->per_thr[thr].idle_conn_list.n, struct connection *, idle_list); - conn_delete_from_tree(conn, thr, 1); + conn_delete_from_tree(conn, thr); MT_LIST_APPEND(&idle_conns[thr].toremove_conns, &conn->toremove_list); i++; } @@ -7289,7 +7289,8 @@ void srv_release_conn(struct server *srv, struct connection *conn) /* Remove the connection from any tree (safe, idle or available) */ if (ceb_intree(&conn->hash_node.node)) { HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(conn, tid, 1); + conn_delete_from_tree(conn, tid); + conn->flags &= ~CO_FL_LIST_MASK; HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } } @@ -7382,7 +7383,7 @@ int srv_add_to_idle_list(struct server *srv, struct connection *conn, int is_saf _HA_ATOMIC_DEC(&srv->curr_used_conns); HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); if (is_safe) { conn->flags = (conn->flags & ~CO_FL_LIST_MASK) | CO_FL_SAFE_LIST; @@ -7542,7 +7543,7 @@ static void srv_close_idle_conns(struct server *srv) hash_node.key, struct connection))) { if (conn->ctrl->ctrl_close) conn->ctrl->ctrl_close(conn); - conn_delete_from_tree(conn, i, 1); + conn_delete_from_tree(conn, i); } } } diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 673e7ceb8..fea5951ee 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -6817,7 +6817,7 @@ struct task *ssl_sock_io_cb(struct task *t, void *context, unsigned int state) conn_in_list = 0; } else { - conn_delete_from_tree(conn, tid, 0); + conn_delete_from_tree(conn, tid); } }