From 852234848241f61a976f8856123a34a3c19275ba Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 29 Sep 2022 20:32:43 +0200 Subject: [PATCH] BUG/MAJOR: conn-idle: fix hash indexing issues on idle conns Idle connections do not work on 32-bit machines due to an alignment issue causing the connection nodes to be indexed with their lower 32-bits set to zero and the higher 32 ones containing the 32 lower bitss of the hash. The cause is the use of ebmb_node with an aligned data, as on this platform ebmb_node is only 32-bit aligned, leaving a hole before the following hash which is a uint64_t: $ pahole -C conn_hash_node ./haproxy struct conn_hash_node { struct ebmb_node node; /* 0 20 */ /* XXX 4 bytes hole, try to pack */ int64_t hash; /* 24 8 */ struct connection * conn; /* 32 4 */ /* size: 40, cachelines: 1, members: 3 */ /* sum members: 32, holes: 1, sum holes: 4 */ /* padding: 4 */ /* last cacheline: 40 bytes */ }; Instead, eb64 nodes should be used when it comes to simply storing a 64-bit key, and that is what this patch does. For backports, a variant consisting in simply marking the "hash" member with a "packed" attribute on the struct also does the job (tested), and might be preferable if the fix is difficult to adapt. Only 2.6 and 2.5 are affected by this. --- include/haproxy/connection-t.h | 5 ++--- include/haproxy/connection.h | 2 +- include/haproxy/session.h | 2 +- src/backend.c | 9 ++++----- src/connection.c | 8 ++++---- src/mux_fcgi.c | 9 ++++----- src/mux_h1.c | 4 ++-- src/mux_h2.c | 9 ++++----- src/server.c | 14 +++++++------- src/ssl_sock.c | 4 ++-- 10 files changed, 31 insertions(+), 35 deletions(-) diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h index 1de43356da..eb02bbcf22 100644 --- a/include/haproxy/connection-t.h +++ b/include/haproxy/connection-t.h @@ -484,7 +484,7 @@ enum conn_hash_params_t { #define CONN_HASH_PARAMS_TYPE_COUNT 6 #define CONN_HASH_PAYLOAD_LEN \ - (((sizeof(((struct conn_hash_node *)0)->hash)) * 8) - CONN_HASH_PARAMS_TYPE_COUNT) + (((sizeof(((struct conn_hash_node *)0)->node.key)) * 8) - CONN_HASH_PARAMS_TYPE_COUNT) #define CONN_HASH_GET_PAYLOAD(hash) \ (((hash) << CONN_HASH_PARAMS_TYPE_COUNT) >> CONN_HASH_PARAMS_TYPE_COUNT) @@ -549,8 +549,7 @@ struct connection { * A connection is identified by a hash generated from its specific parameters */ struct conn_hash_node { - struct ebmb_node node; - int64_t hash; /* key for ebmb tree */ + struct eb64_node node; /* contains the hashing key */ struct connection *conn; /* connection owner of the node */ }; diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h index 90b47b54b3..4d289e7b34 100644 --- a/include/haproxy/connection.h +++ b/include/haproxy/connection.h @@ -81,7 +81,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 ebmb_node *node); +void conn_delete_from_tree(struct eb64_node *node); void conn_init(struct connection *conn, void *target); struct connection *conn_new(void *target); diff --git a/include/haproxy/session.h b/include/haproxy/session.h index ab4f28a01b..13152cf1ba 100644 --- a/include/haproxy/session.h +++ b/include/haproxy/session.h @@ -213,7 +213,7 @@ static inline struct connection *session_get_conn(struct session *sess, void *ta list_for_each_entry(srv_list, &sess->srv_list, srv_list) { if (srv_list->target == target) { list_for_each_entry(srv_conn, &srv_list->conn_list, session_list) { - if ((srv_conn->hash_node && srv_conn->hash_node->hash == hash) && + if ((srv_conn->hash_node && srv_conn->hash_node->node.key == hash) && srv_conn->mux && (srv_conn->mux->avail_streams(srv_conn) > 0) && !(srv_conn->flags & CO_FL_WAIT_XPRT)) { diff --git a/src/backend.c b/src/backend.c index dc30c76270..45e9eab412 100644 --- a/src/backend.c +++ b/src/backend.c @@ -1264,9 +1264,8 @@ static struct connection *conn_backend_get(struct stream *s, struct server *srv, session_add_conn(s->sess, conn, conn->target); } else { - ebmb_insert(&srv->per_thr[tid].avail_conns, - &conn->hash_node->node, - sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].avail_conns, + &conn->hash_node->node); } } return conn; @@ -1610,7 +1609,7 @@ skip_reuse: return SF_ERR_RESOURCE; } - srv_conn->hash_node->hash = hash; + srv_conn->hash_node->node.key = hash; } } @@ -1778,7 +1777,7 @@ skip_reuse: if (srv && reuse_mode == PR_O_REUSE_ALWS && !(srv_conn->flags & CO_FL_PRIVATE) && srv_conn->mux->avail_streams(srv_conn) > 0) { - ebmb_insert(&srv->per_thr[tid].avail_conns, &srv_conn->hash_node->node, sizeof(srv_conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].avail_conns, &srv_conn->hash_node->node); } else if (srv_conn->flags & CO_FL_PRIVATE || (reuse_mode == PR_O_REUSE_SAFE && diff --git a/src/connection.c b/src/connection.c index f68c14a173..4a73dbcc80 100644 --- a/src/connection.c +++ b/src/connection.c @@ -52,9 +52,9 @@ struct mux_stopping_data mux_stopping_data[MAX_THREADS]; /* disables sending of proxy-protocol-v2's LOCAL command */ static int pp2_never_send_local; -void conn_delete_from_tree(struct ebmb_node *node) +void conn_delete_from_tree(struct eb64_node *node) { - ebmb_delete(node); + eb64_delete(node); } int conn_create_mux(struct connection *conn) @@ -83,7 +83,7 @@ int conn_create_mux(struct connection *conn) */ if (srv && ((srv->proxy->options & PR_O_REUSE_MASK) == PR_O_REUSE_ALWS) && !(conn->flags & CO_FL_PRIVATE) && conn->mux->avail_streams(conn) > 0) - ebmb_insert(&srv->per_thr[tid].avail_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].avail_conns, &conn->hash_node->node); else if (conn->flags & CO_FL_PRIVATE) { /* If it fail now, the same will be done in mux->detach() callback */ session_add_conn(sess, conn, conn->target); @@ -165,7 +165,7 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake) &srv->per_thr[tid].idle_conns; HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - ebmb_insert(root, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(root, &conn->hash_node->node); 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 c93ddd4657..8535b6271e 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -3069,9 +3069,9 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state) HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (conn_in_list == CO_FL_SAFE_LIST) - ebmb_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node); else - ebmb_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } return t; @@ -3689,9 +3689,8 @@ static void fcgi_detach(struct sedesc *sd) else if (!fconn->conn->hash_node->node.node.leaf_p && fcgi_avail_streams(fconn->conn) > 0 && objt_server(fconn->conn->target) && !LIST_INLIST(&fconn->conn->session_list)) { - ebmb_insert(&__objt_server(fconn->conn->target)->per_thr[tid].avail_conns, - &fconn->conn->hash_node->node, - sizeof(fconn->conn->hash_node->hash)); + eb64_insert(&__objt_server(fconn->conn->target)->per_thr[tid].avail_conns, + &fconn->conn->hash_node->node); } } } diff --git a/src/mux_h1.c b/src/mux_h1.c index b1be71df52..5739d91d91 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -3126,9 +3126,9 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state) HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (conn_in_list == CO_FL_SAFE_LIST) - ebmb_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node); else - ebmb_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } return t; diff --git a/src/mux_h2.c b/src/mux_h2.c index b0fb036789..a10dfb078a 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -3903,9 +3903,9 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state) HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (conn_in_list == CO_FL_SAFE_LIST) - ebmb_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node); else - ebmb_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } @@ -4357,9 +4357,8 @@ static void h2_detach(struct sedesc *sd) else if (!h2c->conn->hash_node->node.node.leaf_p && h2_avail_streams(h2c->conn) > 0 && objt_server(h2c->conn->target) && !LIST_INLIST(&h2c->conn->session_list)) { - ebmb_insert(&__objt_server(h2c->conn->target)->per_thr[tid].avail_conns, - &h2c->conn->hash_node->node, - sizeof(h2c->conn->hash_node->hash)); + eb64_insert(&__objt_server(h2c->conn->target)->per_thr[tid].avail_conns, + &h2c->conn->hash_node->node); } } } diff --git a/src/server.c b/src/server.c index f35190d361..4b59e90606 100644 --- a/src/server.c +++ b/src/server.c @@ -5779,11 +5779,11 @@ void srv_release_conn(struct server *srv, struct connection *conn) */ struct connection *srv_lookup_conn(struct eb_root *tree, uint64_t hash) { - struct ebmb_node *node = NULL; + struct eb64_node *node = NULL; struct connection *conn = NULL; struct conn_hash_node *hash_node = NULL; - node = ebmb_lookup(tree, &hash, sizeof(hash_node->hash)); + node = eb64_lookup(tree, hash); if (node) { hash_node = ebmb_entry(node, struct conn_hash_node, node); conn = hash_node->conn; @@ -5797,13 +5797,13 @@ struct connection *srv_lookup_conn(struct eb_root *tree, uint64_t hash) */ struct connection *srv_lookup_conn_next(struct connection *conn) { - struct ebmb_node *node = NULL; + struct eb64_node *node = NULL; struct connection *next_conn = NULL; struct conn_hash_node *hash_node = NULL; - node = ebmb_next_dup(&conn->hash_node->node); + node = eb64_next_dup(&conn->hash_node->node); if (node) { - hash_node = ebmb_entry(node, struct conn_hash_node, node); + hash_node = eb64_entry(node, struct conn_hash_node, node); next_conn = hash_node->conn; } @@ -5846,11 +5846,11 @@ int srv_add_to_idle_list(struct server *srv, struct connection *conn, int is_saf if (is_safe) { conn->flags = (conn->flags & ~CO_FL_LIST_MASK) | CO_FL_SAFE_LIST; - ebmb_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node); _HA_ATOMIC_INC(&srv->curr_safe_nb); } else { conn->flags = (conn->flags & ~CO_FL_LIST_MASK) | CO_FL_IDLE_LIST; - ebmb_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node); _HA_ATOMIC_INC(&srv->curr_idle_nb); } HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); diff --git a/src/ssl_sock.c b/src/ssl_sock.c index cf9c57634d..14732897fd 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -6539,9 +6539,9 @@ leave: HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (conn_in_list == CO_FL_SAFE_LIST) - ebmb_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].safe_conns, &conn->hash_node->node); else - ebmb_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node, sizeof(conn->hash_node->hash)); + eb64_insert(&srv->per_thr[tid].idle_conns, &conn->hash_node->node); HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); } return t; -- 2.39.5