⬢ DragonFlyBSD Kernel Audit
DF-5169 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/dev/drm/include/linux/workqueue.h b/sys/dev/drm/include/linux/workqueue.h
--- a/sys/dev/drm/include/linux/workqueue.h
+++ b/sys/dev/drm/include/linux/workqueue.h
@@ -54,6 +54,7 @@
 struct workqueue_worker {
 	STAILQ_HEAD(ws_list, work_struct) ws_list_head;
 	struct thread *worker_thread;
+	struct work_struct *running_work;	/* owner of in-flight callback */
 	struct lock worker_lock;
 };
 
diff --git a/sys/dev/drm/linux_workqueue.c b/sys/dev/drm/linux_workqueue.c
--- a/sys/dev/drm/linux_workqueue.c
+++ b/sys/dev/drm/linux_workqueue.c
@@ -62,6 +62,34 @@
  * - unbound high priority
  */
 
+/*
+ * work_is_executing() - authoritative "queued or mid-callback" test.
+ *
+ * The worker-owned worker->running_work pointer is authoritative because
+ * work->running is no longer cleared after the callback returns: a work
+ * item may legally free itself inside func() (self-freeing works are
+ * standard Linux semantics, e.g. i915_clflush_work()), and
+ * process_all_work() must not touch *work once func() has returned.
+ */
+static bool
+work_is_executing(struct work_struct *work)
+{
+	struct workqueue_worker *worker;
+	bool executing;
+
+	if (work->on_queue)
+		return (true);
+	if (work->running == false)
+		return (false);
+	worker = work->worker;
+	if (worker == NULL)
+		return (true);	/* conservatively: about to start */
+	lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
+	executing = (worker->running_work == work);
+	lockmgr(&worker->worker_lock, LK_RELEASE);
+	return (executing);
+}
+
 static inline void
 process_all_work(struct workqueue_worker *worker)
 {
@@ -74,7 +102,7 @@
 		work->on_queue = false;
 
 		/* A work shouldn't be executed concurrently on a single cpu */
-		if (work->running)
+		if (work_is_executing(work))
 			continue;
 
 		/* Do not run canceled works */
@@ -84,16 +112,32 @@
 			continue;
 		}
 
+		/*
+		 * Snapshot cancel state before the callback: after func()
+		 * returns we may no longer touch *work at all (it may have
+		 * freed itself).  running_work is set before running so any
+		 * locked reader that observes running=true also observes
+		 * running_work==work.
+		 */
+		didcan = work->canceled;
+		worker->running_work = work;
 		work->running = true;
 		lockmgr(&worker->worker_lock, LK_RELEASE);
 		work->func(work);
 		lwkt_yield();
 		lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
-		if (work->on_queue == false)
-			work->worker = NULL;
-		didcan = work->canceled;
+
+		/*
+		 * Completion bookkeeping is worker-owned; never touch *work
+		 * past this point.  wakeup() only uses the address as a
+		 * wait-channel key, and the free-after-cancel API contract
+		 * keeps the item alive for any cancel_work_sync() waiter
+		 * until woken (this covers cancels requested before func();
+		 * cancels during func() are picked up by the waiter's own
+		 * 1-tick retest loop).
+		 */
+		worker->running_work = NULL;
 		cpu_sfence();
-		work->running = false;
 		if (didcan == true)
 			wakeup(work);
 	}
@@ -295,7 +339,7 @@
 unsigned int
 work_busy(struct work_struct *work)
 {
-	return (work->on_queue || work->running);
+	return (work_is_executing(work));
 }
 
 static inline void
@@ -313,7 +357,7 @@
 	INIT_WORK(&__flush_work, __flush_work_func);
 
 	queue_work(wq, &__flush_work);
-	while (__flush_work.on_queue || __flush_work.running) {
+	while (work_is_executing(&__flush_work)) {
 		tsleep(&__flush_work, 0, "flshwq", 0);
 	}
 }
@@ -328,7 +372,7 @@
 	int ret = false;
 
 	/* XXX: probably unreliable */
-	while (work->on_queue || work->running) {
+	while (work_is_executing(work)) {
 		ret = true;
 		/* XXX: use something more intelligent than tsleep() */
 		tsleep(&flush_work, 0, "flshwrk", 1);
@@ -361,14 +405,14 @@
 			ret = true;
 			lockmgr(&worker->worker_lock, LK_RELEASE);
 		}
-		if (work->running == false)
+		if (work_is_executing(work) == false)
 			break;
 
 		worker = work->worker;
 		if (worker == NULL)
 			continue;
 		lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
-		if (worker != work->worker || work->running == false) {
+		if (worker != work->worker || worker->running_work != work) {
 			lockmgr(&worker->worker_lock, LK_RELEASE);
 			continue;
 		}