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 1de43356da2ed647e01a7ce2b56749f164f8ac5c..eb02bbcf225719695b40f775f0b06d0ab83b9286 100644 (file)
--- 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 90b47b54b397f4156939cbfd683762722db41edb..4d289e7b347b5eb17fdc8367aff580094f34d0c5 100644 (file)
--- 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 ab4f28a01b223d1ec81286c7e4d69dd9c1e0f937..13152cf1ba321fd4b128658a403591994c584491 100644 (file)
--- 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 dc30c7627072ab4ede1ae79f8013e592a24584c2..45e9eab4126f10a126287cc7f227932defc23ae7 100644 (file)
--- 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 f68c14a173d17a5d4986de90741d31e0c6dfcedb..4a73dbcc802afb6a7fa6110b1d018cdb0aecfd82 100644 (file)
--- 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 c93ddd46578bb03fa64adcf8a5d8c9402decb0fe..8535b6271e1e5e99e4e842b3e6451475b74b8f32 100644 (file)
--- 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 b1be71df52020b3dc3173d74a22d1a8585ab9778..5739d91d918fd8af2806593a14712a501176ed4a 100644 (file)
--- 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 b0fb03678992791922e491508de0df78aaa23caf..a10dfb078a28bc7cef2b009b7974cfcfba3fe247 100644 (file)
--- 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 f35190d361f17f8580de222cd28a4aad152de34f..4b59e906068db422232ef9141ae987c9c53d215b 100644 (file)
--- 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 cf9c57634dc3f008532677b238b0fddf538e1389..14732897fd41beed89bd3e6dc1b3e9c578388507 100644 (file)
--- 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;