From 0cbcab32a0815c8640e4239dac1922eaf2189de2 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 30 Sep 2021 16:38:09 +0200 Subject: [PATCH] MINOR: tasks: catch TICK_ETERNITY with BUG_ON() in __task_queue() __task_queue() must absolutely not be called with TICK_ETERNITY or it will place a never-expiring node upfront in the timers queue, preventing any timer from expiring until the process is restarted. Code was found to cause this using "task_schedule(task, now_ms)" which does this one millisecond every 49.7 days, so let's add a condition against this. It must never trigger since any process susceptible to trigger it would already accumulate tasks until it dies. An extra test was added in wake_expired_tasks() to detect tasks whose timeout would have been changed after being queued. An improvement over this could be in the future to use a non-scalar type (union/struct) for expiration dates so as to avoid the risk of using them directly like this. But now_ms is already such a valid time and this specific construct would still not be caught. This could even be backported to stable versions to help detect other occurrences if any. (cherry picked from commit 7a9699916a92a98ca5daaff77e2a25f9bec8817d) Signed-off-by: Christopher Faulet (cherry picked from commit aaed609f9189915ef610cc47d8d1930ebb257b9d) Signed-off-by: Christopher Faulet --- include/haproxy/task.h | 4 ++++ src/task.c | 12 +++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/include/haproxy/task.h b/include/haproxy/task.h index 459cc23..d2ac5db 100644 --- a/include/haproxy/task.h +++ b/include/haproxy/task.h @@ -545,6 +545,10 @@ static inline void tasklet_set_tid(struct tasklet *tl, int tid) /* Ensure will be woken up at most at . If the task is already in * the run queue (but not running), nothing is done. It may be used that way * with a delay : task_schedule(task, tick_add(now_ms, delay)); + * It MUST NOT be used with a timer in the past, and even less with + * TICK_ETERNITY (which would block all timers). Note that passing it directly + * now_ms without using tick_add() will definitely make this happen once every + * 49.7 days. */ static inline void task_schedule(struct task *task, int when) { diff --git a/src/task.c b/src/task.c index 92d9630..539f3b5 100644 --- a/src/task.c +++ b/src/task.c @@ -176,7 +176,7 @@ void __task_wakeup(struct task *t, struct eb_root *root) * * Inserts a task into wait queue at the position given by its expiration * date. It does not matter if the task was already in the wait queue or not, - * as it will be unlinked. The task must not have an infinite expiration timer. + * as it will be unlinked. The task MUST NOT have an infinite expiration timer. * Last, tasks must not be queued further than the end of the tree, which is * between and + 2^31 ms (now+24days in 32bit). * @@ -193,6 +193,10 @@ void __task_queue(struct task *task, struct eb_root *wq) (wq == &sched->timers && (task->state & TASK_SHARED_WQ)) || (wq != &timers && wq != &sched->timers)); #endif + /* if this happens the process is doomed anyway, so better catch it now + * so that we have the caller in the stack. + */ + BUG_ON(task->expire == TICK_ETERNITY); if (likely(task_in_wq(task))) __task_unlink_wq(task); @@ -266,7 +270,8 @@ void wake_expired_tasks() __task_queue(task, &tt->timers); } else { - /* task not expired and correctly placed */ + /* task not expired and correctly placed. It may not be eternal. */ + BUG_ON(task->expire == TICK_ETERNITY); break; } } @@ -336,7 +341,8 @@ void wake_expired_tasks() goto lookup_next; } else { - /* task not expired and correctly placed */ + /* task not expired and correctly placed. It may not be eternal. */ + BUG_ON(task->expire == TICK_ETERNITY); break; } } -- 1.7.10.4