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.
diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h
index 1de4335..eb02bbc 100644
--- a/include/haproxy/connection-t.h
+++ b/include/haproxy/connection-t.h
@@ -484,7 +484,7 @@
#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 @@
* 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 90b47b5..4d289e7 100644
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -81,7 +81,7 @@
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 ab4f28a..13152cf 100644
--- a/include/haproxy/session.h
+++ b/include/haproxy/session.h
@@ -213,7 +213,7 @@
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 dc30c76..45e9eab 100644
--- a/src/backend.c
+++ b/src/backend.c
@@ -1264,9 +1264,8 @@
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 @@
return SF_ERR_RESOURCE;
}
- srv_conn->hash_node->hash = hash;
+ srv_conn->hash_node->node.key = hash;
}
}
@@ -1778,7 +1777,7 @@
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 f68c14a..4a73dbc 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -52,9 +52,9 @@
/* 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 @@
*/
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 @@
&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 c93ddd4..8535b62 100644
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3069,9 +3069,9 @@
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 @@
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 b1be71d..5739d91 100644
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -3126,9 +3126,9 @@
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 b0fb036..a10dfb0 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3903,9 +3903,9 @@
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 @@
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 f35190d..4b59e90 100644
--- a/src/server.c
+++ b/src/server.c
@@ -5779,11 +5779,11 @@
*/
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_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 @@
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 cf9c576..1473289 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6539,9 +6539,9 @@
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;