commit b1918b176e234ddfda6380ccd8e9322523eba194
author Aurelien DARRAGON <adarragon@haproxy.com> Mon Feb 06 17:19:58 2023 +0100
committer Christopher Faulet <cfaulet@haproxy.com> Mon Jul 24 10:17:52 2023 +0200
tree a183b4b967ea7071b8a2caed387839288c506bb3
parent 3e931a4dc226f44a0ee257fa97bb4366d74fa69a

MINOR: listener: workaround for closing a tiny race between resume_listener() and stopping

This is an alternative fix that tries to address the same issue as
d1ebee177 ("BUG/MINOR: listener: close tiny race between
resume_listener() and stopping") while allowing resume_listener() to be
more versatile.

Indeed, because of the previous fix, resume_listener() is not able to
rebind stopped listeners, and this breaks the original behavior that is
documented in the function description:

 "If the listener was only in the assigned
  state, it's totally rebound. This can happen if a pause() has completely
  stopped it. If the resume fails, 0 is returned and an error might be
  displayed."

With relax_listener(), we now make sure to check l->state under the
listener lock so we don't call resume_listener() when the conditions are not
met.

As such, concurrently stopped listeners may not be rebound using
relax_listener().

Note: the documented race can't happen since 1b927eb3c ("MEDIUM: proto: stop
protocols under thread isolation during soft stop"), but older versions are
concerned as 1b927eb3c was not marked for backports.
Moreover, the patch also prevents the race between protocol_pause_all() and
resuming from LIMITED or FULL states.

This commit depends on:
  - "MINOR: listener: add relax_listener() function"

This should be backported with d1ebee177 up to 2.4
(d1ebee177 is marked to be backported for all stable versions but the current
patch does not apply for versions < 2.4)

(cherry picked from commit f5d98938ad0edf85ae4d10a6d6c088915c1faaed)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 3353ccc853dcfd547d5cdd7561a2af24eaad9887)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit c2d190000824da34deafcdc82a7ef3c495752b10)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit b84a6bb601180104659179b03f1ba107c6633aee)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

src/listener.c

diff --git a/src/listener.c b/src/listener.c
index 84064d6..28c3e63 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -508,10 +508,6 @@
 	if (l->state == LI_READY)
 		goto end;
 
-	/* the listener might have been stopped in parallel */
-	if (l->state < LI_PAUSED)
-		goto end;
-
 	if (l->rx.proto->resume)
 		ret = l->rx.proto->resume(l);
 
@@ -567,7 +563,7 @@
 }
 
 /* Marks a ready listener as full so that the stream code tries to re-enable
- * it upon next close() using resume_listener().
+ * it upon next close() using relax_listener().
  */
 static void listener_full(struct listener *l)
 {
@@ -605,7 +601,7 @@
 		/* This cannot fail because the listeners are by definition in
 		 * the LI_LIMITED state.
 		 */
-		resume_listener(listener, 0, 0);
+		relax_listener(listener, 0, 0);
 	}
 }
 
@@ -618,7 +614,7 @@
 		/* This cannot fail because the listeners are by definition in
 		 * the LI_LIMITED state.
 		 */
-		resume_listener(listener, 0, 0);
+		relax_listener(listener, 0, 0);
 	}
 }
 
@@ -1144,7 +1140,7 @@
 	      (!tick_isset(global_listener_queue_task->expire) ||
 	       tick_is_expired(global_listener_queue_task->expire, now_ms))))) {
 		/* at least one thread has to this when quitting */
-		resume_listener(l, 0, 0);
+		relax_listener(l, 0, 0);
 
 		/* Dequeues all of the listeners waiting for a resource */
 		dequeue_all_listeners();
@@ -1203,7 +1199,7 @@
 	_HA_ATOMIC_DEC(&l->thr_conn[tid]);
 
 	if (l->state == LI_FULL || l->state == LI_LIMITED)
-		resume_listener(l, 0, 0);
+		relax_listener(l, 0, 0);
 
 	/* Dequeues all of the listeners waiting for a resource */
 	dequeue_all_listeners();