From a56798ea4d28dd1c203df8e8a23484d4a9fa07bc Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 6 Dec 2022 11:38:18 +0100 Subject: [PATCH] BUG/MEDIUM: checks: do not reschedule a possibly running task on state change MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Aurélien found an issue introduced in 2.7-dev8 with commit d114f4a68 ("MEDIUM: checks: spread the checks load over random threads"), but which in fact has deeper roots. When a server's state is changed via __health_adjust(), if a fastinter setting is set, the task gets rescheduled to run at the new date. The way it's done is not thread safe, as nothing prevents another thread where the task is already running from also updating the expire field in parallel. But since such events are quite rare, this statistically never happens. However, with the commit above, the tasks are no longer required to go to the shared wait queue and are no longer marked as shared between multiple threads. It's just that *any* thread may run them at a time without implying that all of them are allowed to modify them. And this change is sufficient to trigger the BUG_ON() condition in the scheduler that detects the inconsistency between a task queued in one thread and being manipulated in parallel by another one: FATAL: bug condition "task->tid != tid" matched at include/haproxy/task.h:670 call trace(13): | 0x55f61cf520c9 [c6 04 25 01 00 00 00 00]: main-0x2ee7 | 0x55f61d0646e8 [8b 45 08 a8 40 0f 85 65]: back_handle_st_cer+0x78/0x4d7 | 0x55f61cff3e72 [41 0f b6 4f 01 e9 c8 df]: process_stream+0x2252/0x364f | 0x55f61d0d2fab [48 89 c3 48 85 db 74 75]: run_tasks_from_lists+0x34b/0x8c4 | 0x55f61d0d38ad [29 44 24 18 8b 54 24 18]: process_runnable_tasks+0x37d/0x6c6 | 0x55f61d0a22fa [83 3d 0b 63 1e 00 01 0f]: run_poll_loop+0x13a/0x536 | 0x55f61d0a28c9 [48 8b 1d f0 46 19 00 48]: main+0x14d919 | 0x55f61cf56dfe [31 c0 e8 eb 93 1b 00 31]: main+0x1e4e/0x2d5d At first glance it looked like it could be addressed in the scheduler only, but in fact the problem clearly is at the application level, since some shared fields are manipulated without protection. At minima, the task's expiry ought to be touched only under the server's lock. While it's arguable that the scheduler could make such updates easier, changing it alone will not be sufficient here. Looking at the sequencing closer, it becomes obvious that we do not need this task_schedule() at all: a simple task_wakeup() is sufficient for the callee to update its timers. Indeed, the process_chk_con() function already deals with spurious wakeups, and already uses srv_getinter() to calculate the next wakeup date based on the current state. So here, instead of having to queue the task from __health_adjust() to anticipate a new check, we can simply wake the task up and let it decide when it needs to run next. This is much cleaner as the expiry calculation remains performed at a single place, from the task itself, as it should be, and it fixes the problem above. This should be backported to 2.7, but not to older versions where the risks of breakage are higher than the chance to fix something that ever happened. --- src/check.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/check.c b/src/check.c index e548b17c70..e4f9756a34 100644 --- a/src/check.c +++ b/src/check.c @@ -634,7 +634,6 @@ void check_notify_stopping(struct check *check) void __health_adjust(struct server *s, short status) { int failed; - int expire; if (s->observe >= HANA_OBS_SIZE) return; @@ -669,11 +668,6 @@ void __health_adjust(struct server *s, short status) chunk_printf(&trash, "Detected %d consecutive errors, last one was: %s", s->consecutive_errors, get_analyze_status(status)); - if (s->check.fastinter) - expire = tick_add(now_ms, MS_TO_TICKS(s->check.fastinter)); - else - expire = TICK_ETERNITY; - HA_SPIN_LOCK(SERVER_LOCK, &s->lock); switch (s->onerror) { @@ -713,9 +707,12 @@ void __health_adjust(struct server *s, short status) s->consecutive_errors = 0; _HA_ATOMIC_INC(&s->counters.failed_hana); - if (tick_isset(expire) && tick_is_lt(expire, s->check.task->expire)) { - /* requeue check task with new expire */ - task_schedule(s->check.task, expire); + if (s->check.fastinter) { + /* timer might need to be advanced, it might also already be + * running in another thread. Let's just wake the task up, it + * will automatically adjust its timer. + */ + task_wakeup(s->check.task, TASK_WOKEN_MSG); } } -- 2.39.5