Revert "BUG/MEDIUM: connections: permit to permanently remove an idle conn"

The target patch fixes a rare race condition which happen when a MUX IO
handler is working on a connection already moved into the purge list. In
this case, the handler will incorrectly moved back the connection into
the idle list.

To fix this, conn_delete_from_tree() was extended to remove flags along
with the connection from the idle list. This was performed when the
connection is moved into the purge list. However, it introduces another
issue related to the idle server connection accounting. Thus it is
necessary to revert it prior to the incoming newer fix.

This patch must be backported to every version where the original commit
is.

diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 0c25e51840544d2aeda0afba1a63e1746f11519f..7943ee39f866c8352d9c30bbafa5f0c492adb643 100644 (file)
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -83,7 +83,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 connection *conn, int thr, int permanent);
+void conn_delete_from_tree(struct connection *conn, int thr);
 
 void conn_init(struct connection *conn, void *target);
 struct connection *conn_new(void *target);

diff --git a/src/backend.c b/src/backend.c
index ae6eac255ae63fa009062d8cb1e940d52e9f14d0..cac056efb79da314e16f2c1d8594bdb21e6cd5e2 100644 (file)
--- a/src/backend.c
+++ b/src/backend.c
@@ -1319,7 +1319,7 @@ struct connection *conn_backend_get(int reuse_mode,
        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        conn = srv_lookup_conn(is_safe ? &srv->per_thr[tid].safe_conns : &srv->per_thr[tid].idle_conns, hash);
        if (conn)
-               conn_delete_from_tree(conn, tid, 0);
+               conn_delete_from_tree(conn, tid);
 
        /* If we failed to pick a connection from the idle list, let's try again with
         * the safe list.
@@ -1327,7 +1327,7 @@ struct connection *conn_backend_get(int reuse_mode,
        if (!conn && !is_safe && srv->curr_safe_nb > 0) {
                conn = srv_lookup_conn(&srv->per_thr[tid].safe_conns, hash);
                if (conn) {
-                       conn_delete_from_tree(conn, tid, 0);
+                       conn_delete_from_tree(conn, tid);
                        is_safe = 1;
                }
        }
@@ -1388,7 +1388,7 @@ check_tgid:
                        conn = srv_lookup_conn(tree, hash);
                        while (conn) {
                                if (conn->mux->takeover && conn->mux->takeover(conn, i, 0) == 0) {
-                                       conn_delete_from_tree(conn, i, 0);
+                                       conn_delete_from_tree(conn, i);
                                        _HA_ATOMIC_INC(&activity[tid].fd_takeover);
                                        found = 1;
                                        break;
@@ -1490,7 +1490,7 @@ takeover_random_idle_conn(struct ceb_root **root, int curtid)
        conn = ceb64_item_first(root, hash_node.node, hash_node.key, struct connection);
        while (conn) {
                if (conn->mux->takeover && conn->mux->takeover(conn, curtid, 1) == 0) {
-                       conn_delete_from_tree(conn, curtid, 0);
+                       conn_delete_from_tree(conn, curtid);
                        return conn;
                }
                conn = ceb64_item_next(root, hash_node.node, hash_node.key, conn);
@@ -1751,7 +1751,7 @@ int be_reuse_connection(int64_t hash, struct session *sess,
                        if (avail <= 1) {
                                /* no more streams available, remove it from the list */
                                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               conn_delete_from_tree(srv_conn, tid, 0);
+                               conn_delete_from_tree(srv_conn, tid);
                                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                        }
 
@@ -1863,7 +1863,7 @@ int connect_server(struct stream *s)
                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                if (!LIST_ISEMPTY(&srv->per_thr[tid].idle_conn_list)) {
                        tokill_conn = LIST_ELEM(srv->per_thr[tid].idle_conn_list.n, struct connection *, idle_list);
-                       conn_delete_from_tree(tokill_conn, tid, 1);
+                       conn_delete_from_tree(tokill_conn, tid);
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
 
                        /* Release the idle lock before calling mux->destroy.
@@ -1892,7 +1892,7 @@ int connect_server(struct stream *s)
 
                                if (!LIST_ISEMPTY(&srv->per_thr[i].idle_conn_list)) {
                                        tokill_conn = LIST_ELEM(srv->per_thr[i].idle_conn_list.n, struct connection *, idle_list);
-                                       conn_delete_from_tree(tokill_conn, i, 1);
+                                       conn_delete_from_tree(tokill_conn, i);
                                }
                                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[i].idle_conns_lock);
 

diff --git a/src/connection.c b/src/connection.c
index 201cd3497f8647d241ee5758f0b3b7894f1084c1..d2ba279e6546a02a5cea397665233906be2eb382 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -74,13 +74,11 @@ struct conn_tlv_list *conn_get_tlv(struct connection *conn, int type)
 
 /* Remove <conn> idle connection from its attached tree (idle, safe or avail)
  * for the server in the connection's target and thread <thr>. If also present
- * in the secondary server idle list, conn is removed from it. Finally, if
- * <permanent> is non-nul, the idle connection flags are cleared as well so
- * that the connection is not re-inserted later.
+ * in the secondary server idle list, conn is removed from it.
  *
  * Must be called with idle_conns_lock held.
  */
-void conn_delete_from_tree(struct connection *conn, int thr, int permanent)
+void conn_delete_from_tree(struct connection *conn, int thr)
 {
        struct ceb_root **conn_tree;
        struct server *srv = __objt_server(conn->target);
@@ -102,8 +100,6 @@ void conn_delete_from_tree(struct connection *conn, int thr, int permanent)
        }
 
        ceb64_item_delete(conn_tree, hash_node.node, hash_node.key, conn);
-       if (permanent)
-               conn->flags &= ~CO_FL_LIST_MASK;
 }
 
 int conn_create_mux(struct connection *conn, int *closed_connection)
@@ -231,7 +227,7 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                        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 ff9521bc5fe599c2b5329b31f79c2dc869d91cf8..ba736f37192fe8397c22047ba143f9058e0b05de 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3068,7 +3068,7 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                }
 
@@ -3327,7 +3327,7 @@ struct task *fcgi_timeout_task(struct task *t, void *context, unsigned int state
                 * to steal it from us.
                 */
                if (fconn->conn->flags & CO_FL_LIST_MASK)
-                       conn_delete_from_tree(fconn->conn, tid, 0);
+                       conn_delete_from_tree(fconn->conn, tid);
                else if (fconn->conn->flags & CO_FL_SESS_IDLE)
                        session_detach_idle_conn(fconn->conn->owner, fconn->conn);
 

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 7221434643a0991f4fc0925196569a3f2cd49f4e..60862fc57e8668477b7fe79709ad81be68955548 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -4349,7 +4349,7 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                }
 
@@ -4493,7 +4493,7 @@ struct task *h1_timeout_task(struct task *t, void *context, unsigned int state)
                 * to steal it from us.
                 */
                if (h1c->conn->flags & CO_FL_LIST_MASK)
-                       conn_delete_from_tree(h1c->conn, tid, 1);
+                       conn_delete_from_tree(h1c->conn, tid);
 
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
 

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 54d3c212b0cbdf32b0b21306ef88b04b4bf0f2db..963aa1a8af266b6a52f2d12cf1e546ec924b4095 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4985,7 +4985,7 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                }
 
@@ -5160,7 +5160,7 @@ static int h2_process(struct h2c *h2c)
                /* connections in error must be removed from the idle lists */
                if (conn->flags & CO_FL_LIST_MASK) {
                        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                       conn_delete_from_tree(conn, tid, 1);
+                       conn_delete_from_tree(conn, tid);
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                }
        }
@@ -5168,7 +5168,7 @@ static int h2_process(struct h2c *h2c)
                /* connections in error must be removed from the idle lists */
                if (conn->flags & CO_FL_LIST_MASK) {
                        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                       conn_delete_from_tree(conn, tid, 1);
+                       conn_delete_from_tree(conn, tid);
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                }
        }
@@ -5276,7 +5276,7 @@ struct task *h2_timeout_task(struct task *t, void *context, unsigned int state)
                 * to steal it from us.
                 */
                if (h2c->conn->flags & CO_FL_LIST_MASK)
-                       conn_delete_from_tree(h2c->conn, tid, 1);
+                       conn_delete_from_tree(h2c->conn, tid);
                else if (h2c->conn->flags & CO_FL_SESS_IDLE)
                        session_detach_idle_conn(h2c->conn->owner, h2c->conn);
 
@@ -5358,7 +5358,7 @@ do_leave:
        /* in any case this connection must not be considered idle anymore */
        if (h2c->conn->flags & CO_FL_LIST_MASK) {
                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-               conn_delete_from_tree(h2c->conn, tid, 1);
+               conn_delete_from_tree(h2c->conn, tid);
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }
 

diff --git a/src/mux_quic.c b/src/mux_quic.c
index 135c7d8ecb6928f18311aa5f2d5b57e11853f7e2..da471cc57f7587bcf7ed0180f90118fee90a4ebf 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -3201,7 +3201,7 @@ static void qcc_shutdown(struct qcc *qcc)
 
        /* A connection is not reusable if app layer is closed. */
        if (qcc->flags & QC_CF_IS_BACK)
-               conn_delete_from_tree(qcc->conn, tid, 1);
+               conn_delete_from_tree(qcc->conn, tid);
 
  out:
        qcc->app_st = QCC_APP_ST_SHUT;
@@ -3404,7 +3404,7 @@ struct task *qcc_io_cb(struct task *t, void *ctx, unsigned int state)
                        if (conn->flags & CO_FL_SESS_IDLE)
                                session_detach_idle_conn(conn->owner, conn);
                        else
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                }
 
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
@@ -3526,7 +3526,7 @@ static struct task *qcc_timeout_task(struct task *t, void *ctx, unsigned int sta
                 * attempts to steal it from us.
                 */
                if (qcc->conn->flags & CO_FL_LIST_MASK)
-                       conn_delete_from_tree(qcc->conn, tid, 1);
+                       conn_delete_from_tree(qcc->conn, tid);
                else if (qcc->conn->flags & CO_FL_SESS_IDLE)
                        session_unown_conn(qcc->conn->owner, qcc->conn);
 

diff --git a/src/mux_spop.c b/src/mux_spop.c
index 00878b66959e360c862a76c725ec422a2640bd67..1307ff6e019a742c6eaba06962bb1ee12e7ffcb8 100644 (file)
--- a/src/mux_spop.c
+++ b/src/mux_spop.c
@@ -2564,7 +2564,7 @@ static struct task *spop_io_cb(struct task *t, void *ctx, unsigned int state)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                }
 
@@ -2674,7 +2674,7 @@ static int spop_process(struct spop_conn *spop_conn)
                /* connections in error must be removed from the idle lists */
                if (conn->flags & CO_FL_LIST_MASK) {
                        HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                       conn_delete_from_tree(conn, tid, 1);
+                       conn_delete_from_tree(conn, tid);
                        HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
                }
        }
@@ -2797,7 +2797,7 @@ static struct task *spop_timeout_task(struct task *t, void *context, unsigned in
                 * to steal it from us.
                 */
                if (spop_conn->conn->flags & CO_FL_LIST_MASK)
-                       conn_delete_from_tree(spop_conn->conn, tid, 1);
+                       conn_delete_from_tree(spop_conn->conn, tid);
                else if (spop_conn->conn->flags & CO_FL_SESS_IDLE)
                        session_detach_idle_conn(spop_conn->conn->owner, spop_conn->conn);
 

diff --git a/src/server.c b/src/server.c
index 60d138c9574deb194c901b963532738ceb878c8d..b61489042d3079bd3683791919b4cd8a0ed70b80 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -7214,7 +7214,7 @@ static int srv_migrate_conns_to_remove(struct server *srv, int thr, int toremove
                        break;
 
                conn = LIST_ELEM(srv->per_thr[thr].idle_conn_list.n, struct connection *, idle_list);
-               conn_delete_from_tree(conn, thr, 1);
+               conn_delete_from_tree(conn, thr);
                MT_LIST_APPEND(&idle_conns[thr].toremove_conns, &conn->toremove_list);
                i++;
        }
@@ -7289,7 +7289,8 @@ void srv_release_conn(struct server *srv, struct connection *conn)
        /* Remove the connection from any tree (safe, idle or available) */
        if (ceb_intree(&conn->hash_node.node)) {
                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-               conn_delete_from_tree(conn, tid, 1);
+               conn_delete_from_tree(conn, tid);
+               conn->flags &= ~CO_FL_LIST_MASK;
                HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
        }
 }
@@ -7382,7 +7383,7 @@ int srv_add_to_idle_list(struct server *srv, struct connection *conn, int is_saf
                _HA_ATOMIC_DEC(&srv->curr_used_conns);
 
                HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-               conn_delete_from_tree(conn, tid, 0);
+               conn_delete_from_tree(conn, tid);
 
                if (is_safe) {
                        conn->flags = (conn->flags & ~CO_FL_LIST_MASK) | CO_FL_SAFE_LIST;
@@ -7542,7 +7543,7 @@ static void srv_close_idle_conns(struct server *srv)
                                                        hash_node.key, struct connection))) {
                                if (conn->ctrl->ctrl_close)
                                        conn->ctrl->ctrl_close(conn);
-                               conn_delete_from_tree(conn, i, 1);
+                               conn_delete_from_tree(conn, i);
                        }
                }
        }

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 673e7ceb88825e4e6428b850a8e6cbbcc0374341..fea5951eec2025969ae2c517afa2deea8ed57ed5 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6817,7 +6817,7 @@ struct task *ssl_sock_io_cb(struct task *t, void *context, unsigned int state)
                                        conn_in_list = 0;
                        }
                        else {
-                               conn_delete_from_tree(conn, tid, 0);
+                               conn_delete_from_tree(conn, tid);
                        }
                }