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 <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #71

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);
 		}