⬢ DragonFlyBSD Kernel Audit
DF-4919 / fix.diff
← back to finding ↓ download raw
--- a/sys/dev/drm/include/linux/workqueue.h
+++ b/sys/dev/drm/include/linux/workqueue.h
@@ -49,6 +49,15 @@
 	bool	on_queue;
 	bool	running;
 	bool	canceled;
+	/*
+	 * DF-4919: pending-claim (Linux WORK_STRUCT_PENDING equivalent).
+	 * Held atomically from before the work is published as queued
+	 * until it starts running (or is removed/dropped), so that
+	 * cancel_work_sync()/flush_work() cannot render an "idle"
+	 * verdict while an insert or the dequeue->start transition is
+	 * in flight.
+	 */
+	volatile u_int wq_claim;
 };
 
 struct workqueue_worker {
@@ -82,6 +91,8 @@
 	(work)->on_queue = false;		\
 	(work)->running = false;		\
 	(work)->canceled = false;		\
+	(work)->worker = NULL;			\
+	(work)->wq_claim = 0;			\
 } while (0)
 
 #define INIT_WORK_ONSTACK(work, _func)	INIT_WORK(work, _func)
--- a/sys/dev/drm/linux_workqueue.c
+++ b/sys/dev/drm/linux_workqueue.c
@@ -53,6 +53,21 @@
 struct workqueue_struct *system_power_efficient_wq;
 
 /*
+ * DF-4919: coherence lock for the per-work flags (on_queue, running,
+ * canceled, worker).  These are mutated by queue_work() and
+ * process_all_work() under the target worker's lock (or, for the
+ * dequeue->start transition, under worker_lock), but were read WITHOUT
+ * any lock by _cancel_work()/flush_work().  An unlocked
+ * on_queue/running pair can straddle the dequeue->start transition
+ * (on_queue=false ... running=true) or the queue_work() insert
+ * publication and return "the work is idle" while work->func() is
+ * about to execute -- although callers may free the work after
+ * cancel_work_sync()/flush_work() return.
+ */
+static struct spinlock work_flags_lock =
+	SPINLOCK_INITIALIZER(work_flags_lock, "wqflags");
+
+/*
  * Linux now uses these worker pools:
  * - (per cpu) regular
  * - (per cpu) regular high priority
@@ -71,29 +86,50 @@
 	while (STAILQ_FIRST(&worker->ws_list_head)) {
 		work = STAILQ_FIRST(&worker->ws_list_head);
 		STAILQ_REMOVE_HEAD(&worker->ws_list_head, ws_entries);
+
+		/* DF-4919: the dequeue->start transition must be atomic
+		 * against the _cancel_work()/flush_work() fast paths.
+		 */
+		spin_lock(&work_flags_lock);
 		work->on_queue = false;
 
 		/* A work shouldn't be executed concurrently on a single cpu */
-		if (work->running)
+		if (work->running) {
+			/* requeued while running: the requeue's claim is
+			 * dropped along with the queued request (DF-4919) */
+			atomic_store_int(&work->wq_claim, 0);
+			spin_unlock(&work_flags_lock);
 			continue;
+		}
 
 		/* Do not run canceled works */
 		if (work->canceled) {
 			/* XXX: should we allow canceled works to be reenabled ? */
 			work->canceled = false;
+			atomic_store_int(&work->wq_claim, 0);
+			spin_unlock(&work_flags_lock);
 			continue;
 		}
 
 		work->running = true;
+		/*
+		 * DF-4919: the dequeue->start transition is over and the
+		 * work is observably running; release the pending-claim
+		 * so the work can be requeued.
+		 */
+		atomic_store_int(&work->wq_claim, 0);
+		spin_unlock(&work_flags_lock);
 		lockmgr(&worker->worker_lock, LK_RELEASE);
 		work->func(work);
 		lwkt_yield();
 		lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
+		spin_lock(&work_flags_lock);
 		if (work->on_queue == false)
 			work->worker = NULL;
 		didcan = work->canceled;
-		cpu_sfence();
 		work->running = false;
+		spin_unlock(&work_flags_lock);
+		cpu_sfence();
 		if (didcan == true)
 			wakeup(work);
 	}
@@ -126,12 +162,25 @@
 	if (wq->is_draining)
 		return false;
 
+	/*
+	 * DF-4919: claim the right to make this work pending BEFORE
+	 * taking any locks (Linux WORK_STRUCT_PENDING equivalent).
+	 * cancel_work_sync()/flush_work() idle-verdicts also need the
+	 * claim, so they can no longer observe "idle" while an insert
+	 * is in flight.
+	 */
+	if (atomic_cmpset_int(&work->wq_claim, 0, 1) == 0)
+		return false;
+
 	if (wq->num_workers > 1)
 		worker = &(*wq->workers)[mycpuid];
 	else
 		worker = &(*wq->workers)[0];
 
 	lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
+	/* DF-4919: publish on_queue/worker coherently against the
+	 * cancel/flush fast paths (same spinlock they consult). */
+	spin_lock(&work_flags_lock);
 	work->canceled = false;
 	if (work->on_queue == false || work->running == false) {
 		if (work->on_queue == false) {
@@ -142,7 +191,11 @@
 			wakeup_one(worker);
 		}
 		ret = true;
+	} else {
+		/* unreachable while we hold the claim */
+		atomic_store_int(&work->wq_claim, 0);
 	}
+	spin_unlock(&work_flags_lock);
 	lockmgr(&worker->worker_lock, LK_RELEASE);
 
 	return ret;
@@ -325,13 +378,35 @@
 bool
 flush_work(struct work_struct *work)
 {
+	struct workqueue_worker *worker;
 	int ret = false;
+	bool busy;
 
-	/* XXX: probably unreliable */
-	while (work->on_queue || work->running) {
-		ret = true;
-		/* XXX: use something more intelligent than tsleep() */
-		tsleep(&flush_work, 0, "flshwrk", 1);
+	for (;;) {
+		/* DF-4919: decide from coherent flag reads only. */
+		spin_lock(&work_flags_lock);
+		busy = work->on_queue || work->running;
+		spin_unlock(&work_flags_lock);
+		if (busy) {
+			ret = true;
+			/* XXX: use something more intelligent than tsleep() */
+			tsleep(&flush_work, 0, "flshwrk", 1);
+			continue;
+		}
+		worker = work->worker;
+		if (worker == NULL)
+			break;		/* never queued / fully finished */
+		lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
+		spin_lock(&work_flags_lock);
+		busy = (worker == work->worker) &&
+		    (work->on_queue || work->running);
+		spin_unlock(&work_flags_lock);
+		lockmgr(&worker->worker_lock, LK_RELEASE);
+		if (busy) {
+			ret = true;
+			continue;
+		}
+		break;
 	}
 
 	return ret;
@@ -346,12 +421,28 @@
 	ret = false;
 
 	for (;;) {
+		/*
+		 * DF-4919: every on_queue/running decision must be made
+		 * from coherent flag reads (work_flags_lock); the old
+		 * unlocked "running == false" break could straddle the
+		 * worker's dequeue->start transition or a concurrent
+		 * queue_work() insert and return while the work function
+		 * was about to execute -- after which callers may free
+		 * the work (see the contract above cancel_work_sync),
+		 * turning process_all_work's work->func(work) into a
+		 * use-after-free / arbitrary call.
+		 */
+		spin_lock(&work_flags_lock);
 		if (work->on_queue) {
 			worker = work->worker;
+			spin_unlock(&work_flags_lock);
 			if (worker == NULL)
 				continue;
 			lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
-			if (worker != work->worker || work->on_queue == false) {
+			spin_lock(&work_flags_lock);
+			if (worker != work->worker ||
+			    work->on_queue == false) {
+				spin_unlock(&work_flags_lock);
 				lockmgr(&worker->worker_lock, LK_RELEASE);
 				continue;
 			}
@@ -359,21 +450,51 @@
 				      work_struct, ws_entries);
 			work->on_queue = false;
 			ret = true;
+			spin_unlock(&work_flags_lock);
 			lockmgr(&worker->worker_lock, LK_RELEASE);
+			/* DF-4919: the removed instance held the
+			 * pending-claim; release it so the work can be
+			 * queued again, then retest. */
+			atomic_store_int(&work->wq_claim, 0);
+			continue;
+		} else {
+			spin_unlock(&work_flags_lock);
 		}
-		if (work->running == false)
-			break;
 
+		/*
+		 * DF-4919: the idle verdict additionally requires the
+		 * pending-claim: an in-flight queue_work() insert or the
+		 * worker's dequeue->start transition holds it, so a
+		 * successful claim proves neither is underway.
+		 */
+		if (atomic_cmpset_int(&work->wq_claim, 0, 1)) {
+			spin_lock(&work_flags_lock);
+			if (work->running == false &&
+			    work->on_queue == false) {
+				spin_unlock(&work_flags_lock);
+				atomic_store_int(&work->wq_claim, 0);
+				break;
+			}
+			spin_unlock(&work_flags_lock);
+			atomic_store_int(&work->wq_claim, 0);
+			continue;
+		}
+		/* claim busy: the work is pending or transitioning */
+		spin_lock(&work_flags_lock);
 		worker = work->worker;
+		spin_unlock(&work_flags_lock);
 		if (worker == NULL)
 			continue;
 		lockmgr(&worker->worker_lock, LK_EXCLUSIVE);
+		spin_lock(&work_flags_lock);
 		if (worker != work->worker || work->running == false) {
+			spin_unlock(&work_flags_lock);
 			lockmgr(&worker->worker_lock, LK_RELEASE);
 			continue;
 		}
 		work->canceled = true;
 		ret = true;
+		spin_unlock(&work_flags_lock);
 		if (sync_wait == false) {
 			lockmgr(&worker->worker_lock, LK_RELEASE);
 			break;