From a70bfaaf8b411dcd745db7d432d043a2d6f291ea Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 17 Apr 2019 21:58:23 +0200 Subject: [PATCH] BUG/MAJOR: task: make sure never to delete a queued task Commit 0c7a4b6 ("MINOR: tasks: Don't set the TASK_RUNNING flag when adding in the tasklet list.") revealed a hole in the way tasks may be freed : they could be removed while in the run queue when the TASK_QUEUED flag was present but not the TASK_RUNNING one. But it seems the issue was emphasized by commit cde7902 ("MEDIUM: tasks: improve fairness between the local and global queues") though the code it replaces was already affected given how late the TASK_RUNNING flag was set after removal from the global queue. At the moment the task is picked from the global run queue, if it is the last one, the global run queue lock is dropped, and then the TASK_RUNNING flag was added. In the mean time another thread might have performed a task_free(), and immediately after, the TASK_RUNNING flag was re-added to the task, which was then added to the tasklet list. The unprotected window was extremely faint but does definitely exist and inconsistent task lists have been observed a few times during very intensive tests over the last few days. From this point various options are possible, the task might have been re-allocated while running, and assigned state 0 and/or state QUEUED while it was still running, resulting in the tast not being put back into the tree. This commit simply makes sure that tests on TASK_RUNNING before removing the task also cover TASK_QUEUED. It must be backported to 1.9 along with the previous ones touching that area. --- include/proto/applet.h | 2 +- include/proto/task.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/proto/applet.h b/include/proto/applet.h index efcc52544..606f4b1bf 100644 --- a/include/proto/applet.h +++ b/include/proto/applet.h @@ -103,7 +103,7 @@ static inline void appctx_free(struct appctx *appctx) /* The task is supposed to be run on this thread, so we can just * check if it's running already (or about to run) or not */ - if (!(appctx->t->state & TASK_RUNNING)) + if (!(appctx->t->state & (TASK_QUEUED | TASK_RUNNING))) __appctx_free(appctx); else { /* if it's running, or about to run, defer the freeing diff --git a/include/proto/task.h b/include/proto/task.h index 1a89f2038..361ea8538 100644 --- a/include/proto/task.h +++ b/include/proto/task.h @@ -355,7 +355,7 @@ static inline void task_free(struct task *t) /* There's no need to protect t->state with a lock, as the task * has to run on the current thread. */ - if (t == curr_task || !(t->state & TASK_RUNNING)) + if (t == curr_task || !(t->state & (TASK_QUEUED | TASK_RUNNING))) __task_free(t); else t->process = NULL;