From 8f2503e0af490ea253d6db1a15b4901437171cc1 Mon Sep 17 00:00:00 2001 From: Prakash Surya Date: Fri, 16 Dec 2011 14:57:31 -0800 Subject: [PATCH] Store copy of tqent_flags prior to servicing task A preallocated taskq_ent_t's tqent_flags must be checked prior to servicing the taskq_ent_t. Once a preallocated taskq entry is serviced, the ownership of the entry is handed back to the caller of taskq_dispatch, thus the entry's contents can potentially be mangled. In particular, this is a problem in the case where a preallocated taskq entry is serviced, and the caller clears it's tqent_flags field. Thus, when the function returns and task_done is called, it looks as though the entry is **not** a preallocated task (when in fact it **is** a preallocated task). In this situation, task_done will place the preallocated taskq_ent_t structure onto the taskq_t's free list. This is a **huge** mistake. If the taskq_ent_t is then freed by the caller of taskq_dispatch, the taskq_t's free list will hold a pointer to garbage data. Even worse, if nothing has over written the freed memory before the pointer is dereferenced, it may still look as though it points to a valid list_head belonging to a taskq_ent_t structure. Thus, the task entry's flags are now copied prior to servicing the task. This copy is then checked to see if it is a preallocated task, and determine if the entry needs to be passed down to the task_done function. Signed-off-by: Prakash Surya Signed-off-by: Brian Behlendorf Closes #71 --- include/sys/taskq.h | 1 + module/spl/spl-taskq.c | 22 ++++++++++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/include/sys/taskq.h b/include/sys/taskq.h index 0a7143375..fec4de8ca 100644 --- a/include/sys/taskq.h +++ b/include/sys/taskq.h @@ -97,6 +97,7 @@ typedef struct taskq_thread { struct task_struct *tqt_thread; taskq_t *tqt_tq; taskqid_t tqt_id; + uintptr_t tqt_flags; } taskq_thread_t; /* Global system-wide dynamic task queue available for all consumers */ diff --git a/module/spl/spl-taskq.c b/module/spl/spl-taskq.c index ccb713c20..ece99aad6 100644 --- a/module/spl/spl-taskq.c +++ b/module/spl/spl-taskq.c @@ -135,11 +135,6 @@ task_done(taskq_t *tq, taskq_ent_t *t) ASSERT(t); ASSERT(spin_is_locked(&tq->tq_lock)); - /* For prealloc'd tasks, we don't free anything. */ - if ((!(tq->tq_flags & TASKQ_DYNAMIC)) && - (t->tqent_flags & TQENT_FLAG_PREALLOC)) - return; - list_del_init(&t->tqent_list); if (tq->tq_nalloc <= tq->tq_minalloc) { @@ -147,6 +142,7 @@ task_done(taskq_t *tq, taskq_ent_t *t) t->tqent_func = NULL; t->tqent_arg = NULL; t->tqent_flags = 0; + list_add_tail(&t->tqent_list, &tq->tq_free_list); } else { task_free(tq, t); @@ -480,10 +476,19 @@ taskq_thread(void *args) if (pend_list) { t = list_entry(pend_list->next, taskq_ent_t, tqent_list); list_del_init(&t->tqent_list); + /* In order to support recursively dispatching a * preallocated taskq_ent_t, tqent_id must be * stored prior to executing tqent_func. */ tqt->tqt_id = t->tqent_id; + + /* We must store a copy of the flags prior to + * servicing the task (servicing a prealloc'd task + * returns the ownership of the tqent back to + * the caller of taskq_dispatch). Thus, + * tqent_flags _may_ change within the call. */ + tqt->tqt_flags = t->tqent_flags; + taskq_insert_in_order(tq, tqt); tq->tq_nactive++; spin_unlock_irqrestore(&tq->tq_lock, tq->tq_lock_flags); @@ -494,7 +499,11 @@ taskq_thread(void *args) spin_lock_irqsave(&tq->tq_lock, tq->tq_lock_flags); tq->tq_nactive--; list_del_init(&tqt->tqt_active_list); - task_done(tq, t); + + /* For prealloc'd tasks, we don't free anything. */ + if ((tq->tq_flags & TASKQ_DYNAMIC) || + !(tqt->tqt_flags & TQENT_FLAG_PREALLOC)) + task_done(tq, t); /* When the current lowest outstanding taskqid is * done calculate the new lowest outstanding id */ @@ -504,6 +513,7 @@ taskq_thread(void *args) } tqt->tqt_id = 0; + tqt->tqt_flags = 0; wake_up_all(&tq->tq_wait_waitq); } -- 2.40.0