From: Willy Tarreau <w@1wt.eu>
Date: Mon, 23 Mar 2020 08:27:28 +0000 (+0100)
Subject: BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection
X-Git-Tag: v2.1.4~13
X-Git-Url: http://git.haproxy.org/?a=commitdiff_plain;h=e04beb23b0d1e6abbb94481612e3a7439f9172a5;p=haproxy-2.1.git

BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection

Commit 4b3f27b ("BUG/MINOR: haproxy/threads: try to make all threads
leave together") improved the soft-stop synchronization but it left a
small race open because it looks at tasks_run_queue, which can drop
to zero then back to one while another thread picks the task from the
run queue to insert it into the tasklet_list. The risk is very low but
not null. In addition the condition didn't consider the possible presence
of signals in the queue.

This patch moves the stopping detection just after the "wake" calculation
which already takes care of the various queues' sizes and signals. It
avoids needlessly duplicating these tests.

The bug was discovered during a code review but will probably never be
observed. This fix may be backported to 2.1 and 2.0 along with the commit
above.

(cherry picked from commit 4f46a354e67f4a7781570f6f4e17738eeca9d5ac)
[wt: context adjustment around call to wake_expired_tasks()]
Signed-off-by: Willy Tarreau <w@1wt.eu>
---

diff --git a/src/haproxy.c b/src/haproxy.c
index 8e7c3a2..6839687 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2753,14 +2753,6 @@ static void run_poll_loop()
 		/* Check if we can expire some tasks */
 		next = wake_expired_tasks();
 
-		if (stopping && tasks_run_queue == 0)
-			_HA_ATOMIC_OR(&stopping_thread_mask, tid_bit);
-
-		/* stop when there's nothing left to do */
-		if ((jobs - unstoppable_jobs) == 0 && tasks_run_queue == 0 &&
-		    (stopping_thread_mask & all_threads_mask) == all_threads_mask)
-			break;
-
 		/* also stop  if we failed to cleanly stop all tasks */
 		if (killed > 1)
 			break;
@@ -2781,6 +2773,16 @@ static void run_poll_loop()
 				wake = 0;
 		}
 
+		if (!wake) {
+			if (stopping)
+				_HA_ATOMIC_OR(&stopping_thread_mask, tid_bit);
+
+			/* stop when there's nothing left to do */
+			if ((jobs - unstoppable_jobs) == 0 &&
+			    (stopping_thread_mask & all_threads_mask) == all_threads_mask)
+				break;
+		}
+
 		/* The poller will ensure it returns around <next> */
 		cur_poller.poll(&cur_poller, next, wake);