From: Christopher Faulet <cfaulet@haproxy.com>
Date: Fri, 16 Oct 2020 14:27:17 +0000 (+0200)
Subject: BUG/MEDIUM: lb: Always lock the server when calling server_{take,drop}_conn
X-Git-Tag: v2.3-dev7~2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=26a52af642c3fcfd6a637aef019b78147c05e126;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: lb: Always lock the server when calling server_{take,drop}_conn

The server lock must be held when server_take_conn() and server_drop_conn()
lbprm callback functions are called. It is a documented prerequisite but it is
not always performed. It only affects leastconn and fas lb algorithm. Others
don't use these callback functions.

A race condition on the next pending effecive weight (next_eweight) may be
encountered with the leastconn lb algorithm. An agent check may set it to 0
while fwlc_srv_reposition() is called. The server is locked during the
next_eweight update. But because the server lock is not acquired when
fwlc_srv_reposition() is called, we may use it to recompute the server key,
leading to a division by 0.

This patch must be backported as far as 1.8.
---

diff --git a/src/backend.c b/src/backend.c
index ba642d9582..34e5e7aaca 100644
--- a/src/backend.c
+++ b/src/backend.c
@@ -1593,8 +1593,11 @@ int connect_server(struct stream *s)
 		s->flags |= SF_CURR_SESS;
 		count = _HA_ATOMIC_ADD(&srv->cur_sess, 1);
 		HA_ATOMIC_UPDATE_MAX(&srv->counters.cur_sess_max, count);
-		if (s->be->lbprm.server_take_conn)
+		if (s->be->lbprm.server_take_conn) {
+			HA_SPIN_LOCK(SERVER_LOCK, &srv->lock);
 			s->be->lbprm.server_take_conn(srv);
+			HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+		}
 	}
 
 	/* Now handle synchronously connected sockets. We know the stream-int
diff --git a/src/stream.c b/src/stream.c
index bcb9acf789..8d6bdb26c6 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -2503,8 +2503,11 @@ void sess_change_server(struct stream *sess, struct server *newsrv)
 		_HA_ATOMIC_SUB(&sess->srv_conn->served, 1);
 		_HA_ATOMIC_SUB(&sess->srv_conn->proxy->served, 1);
 		__ha_barrier_atomic_store();
-		if (sess->srv_conn->proxy->lbprm.server_drop_conn)
+		if (sess->srv_conn->proxy->lbprm.server_drop_conn) {
+			HA_SPIN_LOCK(SERVER_LOCK, &sess->srv_conn->lock);
 			sess->srv_conn->proxy->lbprm.server_drop_conn(sess->srv_conn);
+			HA_SPIN_UNLOCK(SERVER_LOCK, &sess->srv_conn->lock);
+		}
 		stream_del_srv_conn(sess);
 	}
 
@@ -2512,8 +2515,11 @@ void sess_change_server(struct stream *sess, struct server *newsrv)
 		_HA_ATOMIC_ADD(&newsrv->served, 1);
 		_HA_ATOMIC_ADD(&newsrv->proxy->served, 1);
 		__ha_barrier_atomic_store();
-		if (newsrv->proxy->lbprm.server_take_conn)
+		if (newsrv->proxy->lbprm.server_take_conn) {
+			HA_SPIN_LOCK(SERVER_LOCK, &newsrv->lock);
 			newsrv->proxy->lbprm.server_take_conn(newsrv);
+			HA_SPIN_UNLOCK(SERVER_LOCK, &newsrv->lock);
+		}
 		stream_add_srv_conn(sess, newsrv);
 	}
 }