BUG/MAJOR: server: fix deadlock when changing maxconn via agent-check

The server_parse_maxconn_change_request locks the server lock. However,
this function can be called via agent-checks or lua code which already
lock it. This bug has been introduced by the following commit :

  commit 79a88ba3d0
  BUG/MAJOR: server: prevent deadlock when using 'set maxconn server'

This commit tried to fix another deadlock with can occur because
previoulsy server_parse_maxconn_change_request requires the server lock
to be held. However, it may call internally process_srv_queue which also
locks the server lock. The locking policy has thus been updated. The fix
is functional for the CLI 'set maxconn' but fails to address the
agent-check / lua counterparts.

This new issue is fixed in two steps :
- changes from the above commit have been reverted. This means that
  server_parse_maxconn_change_request must again be called with the
  server lock.

- to counter the deadlock fixed by the above commit, process_srv_queue
  now takes an argument to render the server locking optional if the
  caller already held it. This is only used by
  server_parse_maxconn_change_request.

The above commit was subject to backport up to 1.8. Thus this commit
must be backported in every release where it is already present.

include/haproxy/queue.h

@@ -34,7 +34,7 @@ extern struct pool_head *pool_head_pendconn;
 
 struct pendconn *pendconn_add(struct stream *strm);
 int pendconn_dequeue(struct stream *strm);
-void process_srv_queue(struct server *s);
+void process_srv_queue(struct server *s, int server_locked);
 unsigned int srv_dynamic_maxconn(const struct server *s);
 int pendconn_redistribute(struct server *s);
 int pendconn_grab_from_px(struct server *s);

include/haproxy/stream.h

@@ -339,7 +339,7 @@ static inline void stream_choose_redispatch(struct stream *s)
 	      ((s->be->lbprm.algo & BE_LB_KIND) == BE_LB_KIND_RR)))) {
 		sess_change_server(s, NULL);
 		if (may_dequeue_tasks(objt_server(s->target), s->be))
-			process_srv_queue(objt_server(s->target));
+			process_srv_queue(objt_server(s->target), 0);
 
 		s->flags &= ~(SF_DIRECT | SF_ASSIGNED | SF_ADDR_SET);
 		si->state = SI_ST_REQ;

src/backend.c

@@ -819,7 +819,7 @@ out_ok:
 			sess_change_server(s, srv);
 		} else {
 			if (may_dequeue_tasks(conn_slot, s->be))
-				process_srv_queue(conn_slot);
+				process_srv_queue(conn_slot, 0);
 		}
 	}
 
@@ -1831,7 +1831,7 @@ int srv_redispatch_connect(struct stream *s)
 
 		/* release other streams waiting for this server */
 		if (may_dequeue_tasks(srv, s->be))
-			process_srv_queue(srv);
+			process_srv_queue(srv, 0);
 		return 1;
 	}
 	/* if we get here, it's because we got SRV_STATUS_OK, which also
@@ -1907,7 +1907,7 @@ void back_try_conn_req(struct stream *s)
 			/* release other streams waiting for this server */
 			sess_change_server(s, NULL);
 			if (may_dequeue_tasks(srv, s->be))
-				process_srv_queue(srv);
+				process_srv_queue(srv, 0);
 
 			/* Failed and not retryable. */
 			si_shutr(si);
@@ -2235,7 +2235,7 @@ void back_handle_st_cer(struct stream *s)
 		_HA_ATOMIC_INC(&s->be->be_counters.failed_conns);
 		sess_change_server(s, NULL);
 		if (may_dequeue_tasks(objt_server(s->target), s->be))
-			process_srv_queue(objt_server(s->target));
+			process_srv_queue(objt_server(s->target), 0);
 
 		/* shutw is enough so stop a connecting socket */
 		si_shutw(si);

src/cli.c

@@ -2565,7 +2565,7 @@ int pcli_wait_for_response(struct stream *s, struct channel *rep, int an_bit)
 				HA_ATOMIC_DEC(&__objt_server(s->target)->cur_sess);
 			}
 			if (may_dequeue_tasks(__objt_server(s->target), be))
-				process_srv_queue(__objt_server(s->target));
+				process_srv_queue(__objt_server(s->target), 0);
 		}
 
 		s->target = NULL;

src/queue.c

@@ -337,14 +337,16 @@ static int pendconn_process_next_strm(struct server *srv, struct proxy *px)
 }
 
 /* Manages a server's connection queue. This function will try to dequeue as
- * many pending streams as possible, and wake them up.
+ * many pending streams as possible, and wake them up. <server_locked> must
+ * only be set if the caller already hold the server lock.
  */
-void process_srv_queue(struct server *s)
+void process_srv_queue(struct server *s, int server_locked)
 {
 	struct proxy  *p = s->proxy;
 	int maxconn;
 
-	HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
+	if (!server_locked)
+		HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
 	HA_RWLOCK_WRLOCK(PROXY_LOCK,  &p->lock);
 	maxconn = srv_dynamic_maxconn(s);
 	while (s->served < maxconn) {
@@ -353,7 +355,8 @@ void process_srv_queue(struct server *s)
 			break;
 	}
 	HA_RWLOCK_WRUNLOCK(PROXY_LOCK,  &p->lock);
-	HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
+	if (!server_locked)
+		HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 }
 
 /* Adds the stream <strm> to the pending connection queue of server <strm>->srv

src/server.c

@@ -1812,16 +1812,17 @@ const char *server_parse_maxconn_change_request(struct server *sv,
 	else if (end[0] != '\0')
 		return "Trailing garbage in maxconn string";
 
-	HA_SPIN_LOCK(SERVER_LOCK, &sv->lock);
 	if (sv->maxconn == sv->minconn) { // static maxconn
 		sv->maxconn = sv->minconn = v;
 	} else { // dynamic maxconn
 		sv->maxconn = v;
 	}
-	HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock);
 
+	/* server_parse_maxconn_change_request requires the server lock held.
+	 * Specify it to process_srv_queue to prevent a deadlock.
+	 */
 	if (may_dequeue_tasks(sv, sv->proxy))
-		process_srv_queue(sv);
+		process_srv_queue(sv, 1);
 
 	return NULL;
 }
@@ -4193,10 +4194,14 @@ static int cli_parse_set_maxconn_server(char **args, char *payload, struct appct
 	if (!sv)
 		return 1;
 
+	HA_SPIN_LOCK(SERVER_LOCK, &sv->lock);
+
 	warning = server_parse_maxconn_change_request(sv, args[4]);
 	if (warning)
 		cli_err(appctx, warning);
 
+	HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock);
+
 	return 1;
 }
 

src/stream.c

@@ -623,7 +623,7 @@ static void stream_free(struct stream *s)
 			_HA_ATOMIC_DEC(&__objt_server(s->target)->cur_sess);
 		}
 		if (may_dequeue_tasks(objt_server(s->target), s->be))
-			process_srv_queue(objt_server(s->target));
+			process_srv_queue(objt_server(s->target), 0);
 	}
 
 	if (unlikely(s->srv_conn)) {
@@ -1815,7 +1815,7 @@ struct task *process_stream(struct task *t, void *context, unsigned int state)
 			}
 			sess_change_server(s, NULL);
 			if (may_dequeue_tasks(srv, s->be))
-				process_srv_queue(srv);
+				process_srv_queue(srv, 0);
 		}
 	}
 

src/tcpcheck.c

@@ -937,6 +937,9 @@ enum tcpcheck_eval_ret tcpcheck_agent_expect_reply(struct check *check, struct t
 		cs += strlen("maxconn:");
 
 		TRACE_DEVEL("change server maxconn", CHK_EV_TCPCHK_EXP, check);
+		/* This is safe to call server_parse_maxconn_change_request
+		 * because the server lock is held during the check.
+		 */
 		msg = server_parse_maxconn_change_request(check->server, cs);
 		if (!wrn || !*wrn)
 			wrn = msg;