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