From 8de0807b74746e140b49c579cecea9e4ee413e30 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Mon, 18 Aug 2025 10:41:38 +0200 Subject: [PATCH] MEDIUM: conn/muxes/ssl: reinsert BE priv conn into sess on IO completion When dealing with input/output on a connection related handler, special care must be taken prior to access the connection if it is considered as idle, as it could be manipulated by another thread. Thus, connection is first removed from its idle tree before processing. The connection is reinserted on processing completion unless it has been freed during it. Idle private connections are not concerned by this, because takeover is not applied on them. However, a future patch will implement purging of these connections along with regular idle ones. As such, it is necessary to also protect private connections usage now. This is the subject of this patch and the next one. With this patch, input/output handlers epilogue of muxes/SSL/conn_notify_mux() are adjusted. A new code path is able to deal with a connection attached to a session instead of a server. In this case, session_reinsert_idle_conn() is used. Contrary to session_add_conn(), this new function is reserved for idle connections usage after a temporary removal. Contrary to _srv_add_idle() used by regular idle connections, session_reinsert_idle_conn() may fail as an allocation can be required. If this happens, the connection is immediately destroyed. This patch has no effect for now. It must be coupled with the next one which will temporarily remove private idle connections on input/output handler prologue. --- include/haproxy/session.h | 1 + src/connection.c | 17 +++++++++++++---- src/mux_fcgi.c | 18 +++++++++++++----- src/mux_h1.c | 18 +++++++++++++----- src/mux_h2.c | 18 +++++++++++++----- src/mux_spop.c | 18 +++++++++++++----- src/session.c | 32 ++++++++++++++++++++++++++++++++ src/ssl_sock.c | 28 +++++++++++++++++++--------- 8 files changed, 117 insertions(+), 33 deletions(-) diff --git a/include/haproxy/session.h b/include/haproxy/session.h index 7a8a73361..ffa750965 100644 --- a/include/haproxy/session.h +++ b/include/haproxy/session.h @@ -44,6 +44,7 @@ void __session_add_glitch_ctr(struct session *sess, uint inc); void session_embryonic_build_legacy_err(struct session *sess, struct buffer *out); int session_add_conn(struct session *sess, struct connection *conn); +int session_reinsert_idle_conn(struct session *sess, struct connection *conn); int session_check_idle_conn(struct session *sess, struct connection *conn); struct connection *session_get_conn(struct session *sess, void *target, int64_t hash); void session_unown_conn(struct session *sess, struct connection *conn); diff --git a/src/connection.c b/src/connection.c index 6dbca2c12..0299532b3 100644 --- a/src/connection.c +++ b/src/connection.c @@ -213,16 +213,25 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake) goto done; if (conn_in_list) { - if (srv->cur_admin & SRV_ADMF_MAINT) { + if (srv && (srv->cur_admin & SRV_ADMF_MAINT)) { /* Do not store an idle conn if server in maintenance. */ conn->mux->destroy(conn->ctx); ret = -1; goto done; } - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (conn->flags & CO_FL_SESS_IDLE) { + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + conn->mux->destroy(conn->ctx); + ret = -1; + } + } + else { + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } } done: diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index c0778f44c..7afbaf3e7 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -3089,12 +3089,20 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state) t = NULL; if (!ret && conn_in_list) { - struct server *srv = __objt_server(conn->target); + struct server *srv = objt_server(conn->target); - if (!(srv->cur_admin & SRV_ADMF_MAINT)) { - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (!srv || !(srv->cur_admin & SRV_ADMF_MAINT)) { + if (conn->flags & CO_FL_SESS_IDLE) { + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + goto release; + } + } + else { + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } else { /* Do not store an idle conn if server in maintenance. */ diff --git a/src/mux_h1.c b/src/mux_h1.c index 86ca866b4..bf17dfcb6 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -4334,12 +4334,20 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state) t = NULL; if (!ret && conn_in_list) { - struct server *srv = __objt_server(conn->target); + struct server *srv = objt_server(conn->target); - if (!(srv->cur_admin & SRV_ADMF_MAINT)) { - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (!srv || !(srv->cur_admin & SRV_ADMF_MAINT)) { + if (conn->flags & CO_FL_SESS_IDLE) { + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + goto release; + } + } + else { + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } else { /* Do not store an idle conn if server in maintenance. */ diff --git a/src/mux_h2.c b/src/mux_h2.c index 57fea0b5a..84f30469d 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -4993,12 +4993,20 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state) h2c->next_tasklet = NULL; if (!ret && conn_in_list) { - struct server *srv = __objt_server(conn->target); + struct server *srv = objt_server(conn->target); - if (!(srv->cur_admin & SRV_ADMF_MAINT)) { - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (!srv || !(srv->cur_admin & SRV_ADMF_MAINT)) { + if (conn->flags & CO_FL_SESS_IDLE) { + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + goto release; + } + } + else { + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } else { /* Do not store an idle conn if server in maintenance. */ diff --git a/src/mux_spop.c b/src/mux_spop.c index 4ba8dabca..d167e522d 100644 --- a/src/mux_spop.c +++ b/src/mux_spop.c @@ -2585,12 +2585,20 @@ static struct task *spop_io_cb(struct task *t, void *ctx, unsigned int state) t = NULL; if (!ret && conn_in_list) { - struct server *srv = __objt_server(conn->target); + struct server *srv = objt_server(conn->target); - if (!(srv->cur_admin & SRV_ADMF_MAINT)) { - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (!srv || !(srv->cur_admin & SRV_ADMF_MAINT)) { + if (conn->flags & CO_FL_SESS_IDLE) { + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + goto release; + } + } + else { + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } else { /* Do not store an idle conn if server in maintenance. */ diff --git a/src/session.c b/src/session.c index 89f81fde8..803a60d2d 100644 --- a/src/session.c +++ b/src/session.c @@ -697,6 +697,38 @@ int session_add_conn(struct session *sess, struct connection *conn) return ret; } +/* Reinsert a backend connection into session. This function is + * reserved for idle conns which were previously temporarily removed from + * session to protect it against other threads. + * + * Returns true on success else false. + */ +int session_reinsert_idle_conn(struct session *sess, struct connection *conn) +{ + struct sess_priv_conns *pconns; + int ret = 0; + + /* This function is reserved for idle private connections. */ + BUG_ON(!(conn->flags & CO_FL_SESS_IDLE)); + + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + + pconns = sess_get_sess_conns(sess, conn->target); + if (!pconns) { + pconns = sess_alloc_sess_conns(sess, conn->target); + if (!pconns) + goto out; + } + + LIST_APPEND(&pconns->conn_list, &conn->sess_el); + ++sess->idle_conns; + ret = 1; + + out: + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + return ret; +} + /* Check that session is able to keep idle connection . This must * be called each time a connection stored in a session becomes idle. * diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 9342bd6d1..fffa1802c 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -6478,19 +6478,29 @@ struct task *ssl_sock_io_cb(struct task *t, void *context, unsigned int state) #endif leave: if (!ret && conn_in_list) { - struct server *srv = __objt_server(conn->target); + struct server *srv = objt_server(conn->target); - if (!(srv->cur_admin & SRV_ADMF_MAINT)) { - TRACE_DEVEL("adding conn back to idle list", SSL_EV_CONN_IO_CB, conn); - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + /* Connection is idle which means MUX layer is already initialized. */ + BUG_ON(!conn->mux); + + if (!srv || !(srv->cur_admin & SRV_ADMF_MAINT)) { + if (conn->flags & CO_FL_SESS_IDLE) { + TRACE_DEVEL("adding conn back to session list", SSL_EV_CONN_IO_CB, conn); + if (!session_reinsert_idle_conn(conn->owner, conn)) { + /* session add conn failure */ + conn->mux->destroy(conn->ctx); + t = NULL; + } + } + else { + TRACE_DEVEL("adding conn back to idle list", SSL_EV_CONN_IO_CB, conn); + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + } } else { /* Do not store an idle conn if server in maintenance. */ - - /* Connection is idle which means MUX layer is already initialized. */ - BUG_ON(!conn->mux); conn->mux->destroy(conn->ctx); t = NULL; }