From 7dae3ceaa08ead56104bae6471b236de536c3f51 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Tue, 12 Mar 2024 14:09:55 +0100 Subject: [PATCH] BUG/MAJOR: server: do not delete srv referenced by session A server can only be deleted if there is no elements which reference it. This is taken care via srv_check_for_deletion(), most notably for active and idle connections. A special case occurs for connections directly managed by a session. This is for so-called private connections, when using http-reuse never or H2 + http-reuse safe for example. In this case. server does not account these connections into its idle lists. This caused a bug as the server is deleted despite the session still being able to access it. To properly fix this, add a new referencing element into the server for these session connections. A mt_list has been chosen for this. On default http-reuse, private connections are typically not used so it won't make any difference. If using H2 servers, or more generally when dealing with private connections, insert/delete should typically occur only once per session lifetime so impact on performance should be minimal. This should be backported up to 2.4. Note that srv_check_for_deletion() was introduced in 3.0 dev tree. On backport, the extra condition in it should be placed in cli_parse_delete_server() instead. --- include/haproxy/server-t.h | 1 + include/haproxy/session-t.h | 7 ++++++- include/haproxy/session.h | 6 ++++++ src/server.c | 3 +++ src/session.c | 1 + 5 files changed, 17 insertions(+), 1 deletion(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index f077ff2f8..c2c3238ef 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -338,6 +338,7 @@ struct server { unsigned int est_need_conns; /* Estimate on the number of needed connections (max of curr and previous max_used) */ struct queue queue; /* pending connections */ + struct mt_list sess_conns; /* list of private conns managed by a session on this server */ /* Element below are usd by LB algorithms and must be doable in * parallel to other threads reusing connections above. diff --git a/include/haproxy/session-t.h b/include/haproxy/session-t.h index bf0079129..aa2bf1e82 100644 --- a/include/haproxy/session-t.h +++ b/include/haproxy/session-t.h @@ -62,11 +62,16 @@ struct session { struct sockaddr_storage *dst; /* destination address (pool), when known, otherwise NULL */ }; -/* List of private conns managed by a session, indexed by server */ +/* + * List of private conns managed by a session, indexed by server + * Stored both into the session and server instances + */ struct sess_priv_conns { void *target; /* Server or dispatch used for indexing */ struct list conn_list; /* Head of the connections list */ + struct list sess_el; /* Element of session.priv_conns */ + struct mt_list srv_el; /* Element of server.sess_conns */ }; #endif /* _HAPROXY_SESSION_T_H */ diff --git a/include/haproxy/session.h b/include/haproxy/session.h index 90970e8e2..0a73e74c3 100644 --- a/include/haproxy/session.h +++ b/include/haproxy/session.h @@ -161,6 +161,7 @@ static inline void session_unown_conn(struct session *sess, struct connection *c if (pconns->target == conn->target) { if (LIST_ISEMPTY(&pconns->conn_list)) { LIST_DELETE(&pconns->sess_el); + MT_LIST_DELETE(&pconns->srv_el); pool_free(pool_head_sess_priv_conns, pconns); } break; @@ -176,6 +177,7 @@ static inline void session_unown_conn(struct session *sess, struct connection *c static inline int session_add_conn(struct session *sess, struct connection *conn, void *target) { struct sess_priv_conns *pconns = NULL; + struct server *srv = objt_server(conn->target); int found = 0; BUG_ON(objt_listener(conn->target)); @@ -198,6 +200,10 @@ static inline int session_add_conn(struct session *sess, struct connection *conn pconns->target = target; LIST_INIT(&pconns->conn_list); LIST_APPEND(&sess->priv_conns, &pconns->sess_el); + + MT_LIST_INIT(&pconns->srv_el); + if (srv) + MT_LIST_APPEND(&srv->sess_conns, &pconns->srv_el); } LIST_APPEND(&pconns->conn_list, &conn->sess_el); return 1; diff --git a/src/server.c b/src/server.c index 1450b0477..e22b33806 100644 --- a/src/server.c +++ b/src/server.c @@ -2819,6 +2819,8 @@ struct server *new_server(struct proxy *proxy) srv->agent.proxy = proxy; srv->xprt = srv->check.xprt = srv->agent.xprt = xprt_get(XPRT_RAW); + MT_LIST_INIT(&srv->sess_conns); + srv->extra_counters = NULL; #ifdef USE_OPENSSL HA_RWLOCK_INIT(&srv->ssl_ctx.lock); @@ -5832,6 +5834,7 @@ int srv_check_for_deletion(const char *bename, const char *svname, struct proxy * cleanup function should be implemented to be used here. */ if (srv->curr_used_conns || srv->curr_idle_conns || + !MT_LIST_ISEMPTY(&srv->sess_conns) || !eb_is_empty(&srv->queue.head) || srv_has_streams(srv)) { msg = "Server still has connections attached to it, cannot remove it."; goto leave; diff --git a/src/session.c b/src/session.c index f54490e79..55e084b5d 100644 --- a/src/session.c +++ b/src/session.c @@ -102,6 +102,7 @@ void session_free(struct session *sess) conn_free(conn); } } + MT_LIST_DELETE(&pconns->srv_el); pool_free(pool_head_sess_priv_conns, pconns); } sockaddr_free(&sess->src);