From: David Woodhouse Make potential deadlocks due to kthread_flush_work() visible to lockdep, like the equivalent deadlocks on workqueues already are. On-stack users of KTHREAD_WORK_INIT move to kthread_init_work(): the static initializer keys the lock class on the work's address, which on the stack is not static. Inspired by Sashiko finding precisely such a deadlock (not that it would have actually been found by lockdep in that case, since it isn't a normal use case that would have been tested): https://lore.kernel.org/all/a654a72765eaf278649b285218b0b1f6f9d45664.camel@infradead.org/ Assisted-by: LLM Signed-off-by: David Woodhouse --- drivers/platform/chrome/cros_ec_spi.c | 8 ++++-- include/linux/kthread.h | 32 +++++++++++++++++++--- kernel/kthread.c | 39 +++++++++++++++++++++++---- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c index 28fa82f8cb07..978eaff6d045 100644 --- a/drivers/platform/chrome/cros_ec_spi.c +++ b/drivers/platform/chrome/cros_ec_spi.c @@ -655,13 +655,17 @@ static int cros_ec_xfer_high_pri(struct cros_ec_device *ec_dev, { struct cros_ec_spi *ec_spi = ec_dev->priv; struct cros_ec_xfer_work_params params = { - .work = KTHREAD_WORK_INIT(params.work, - cros_ec_xfer_high_pri_work), .ec_dev = ec_dev, .ec_msg = ec_msg, .fn = fn, }; + /* + * Not KTHREAD_WORK_INIT: the on-stack work needs a static lockdep + * class key, which kthread_init_work() provides per call site. + */ + kthread_init_work(¶ms.work, cros_ec_xfer_high_pri_work); + /* * This looks a bit ridiculous. Why do the work on a * different thread if we're just going to block waiting for diff --git a/include/linux/kthread.h b/include/linux/kthread.h index a01a474719a7..279b7875b7e0 100644 --- a/include/linux/kthread.h +++ b/include/linux/kthread.h @@ -148,6 +148,9 @@ struct kthread_worker { struct list_head delayed_work_list; struct task_struct *task; struct kthread_work *current_work; +#ifdef CONFIG_LOCKDEP + struct lockdep_map lockdep_map; +#endif }; struct kthread_work { @@ -156,6 +159,9 @@ struct kthread_work { struct kthread_worker *worker; /* Number of canceling calls that are running at the moment. */ int canceling; +#ifdef CONFIG_LOCKDEP + struct lockdep_map lockdep_map; +#endif }; struct kthread_delayed_work { @@ -163,9 +169,17 @@ struct kthread_delayed_work { struct timer_list timer; }; +#ifdef CONFIG_LOCKDEP +#define KTHREAD_WORK_INIT_LOCKDEP_MAP(work) \ + .lockdep_map = STATIC_LOCKDEP_MAP_INIT("(kthread_work_completion)("#work")", &(work)), +#else +#define KTHREAD_WORK_INIT_LOCKDEP_MAP(work) +#endif + #define KTHREAD_WORK_INIT(work, fn) { \ .node = LIST_HEAD_INIT((work).node), \ .func = (fn), \ + KTHREAD_WORK_INIT_LOCKDEP_MAP(work) \ } #define KTHREAD_DELAYED_WORK_INIT(dwork, fn) { \ @@ -182,19 +196,31 @@ struct kthread_delayed_work { KTHREAD_DELAYED_WORK_INIT(dwork, fn) extern void __kthread_init_worker(struct kthread_worker *worker, - const char *name, struct lock_class_key *key); + const char *name, struct lock_class_key *key, + struct lock_class_key *flush_key); #define kthread_init_worker(worker) \ do { \ - static struct lock_class_key __key; \ - __kthread_init_worker((worker), "("#worker")->lock", &__key); \ + static struct lock_class_key __key, __flush_key; \ + __kthread_init_worker((worker), "("#worker")->lock", \ + &__key, &__flush_key); \ } while (0) +#ifdef CONFIG_LOCKDEP +#define __kthread_init_work_lockdep_map(work, _key) \ + lockdep_init_map(&(work)->lockdep_map, \ + "(kthread_work_completion)("#work")", (_key), 0) +#else +#define __kthread_init_work_lockdep_map(work, _key) do { } while (0) +#endif + #define kthread_init_work(work, fn) \ do { \ + static struct lock_class_key __work_key; \ memset((work), 0, sizeof(struct kthread_work)); \ INIT_LIST_HEAD(&(work)->node); \ (work)->func = (fn); \ + __kthread_init_work_lockdep_map((work), &__work_key); \ } while (0) #define kthread_init_delayed_work(dwork, fn) \ diff --git a/kernel/kthread.c b/kernel/kthread.c index 63beb59b7a3d..694599805c5c 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -957,11 +957,13 @@ early_initcall(kthreads_init); void __kthread_init_worker(struct kthread_worker *worker, const char *name, - struct lock_class_key *key) + struct lock_class_key *key, + struct lock_class_key *flush_key) { memset(worker, 0, sizeof(struct kthread_worker)); raw_spin_lock_init(&worker->lock); lockdep_set_class_and_name(&worker->lock, key, name); + lockdep_init_map(&worker->lockdep_map, name, flush_key, 0); INIT_LIST_HEAD(&worker->work_list); INIT_LIST_HEAD(&worker->delayed_work_list); } @@ -1020,9 +1022,22 @@ int kthread_worker_fn(void *worker_ptr) if (work) { kthread_work_func_t func = work->func; +#ifdef CONFIG_LOCKDEP + /* + * Work functions are free to release their own work item, so + * hold a copy of its lockdep map rather than the map itself. + */ + struct lockdep_map lockdep_map; + + lockdep_copy_map(&lockdep_map, &work->lockdep_map); +#endif __set_current_state(TASK_RUNNING); trace_sched_kthread_work_execute_start(work); + lock_map_acquire(&worker->lockdep_map); + lock_map_acquire(&lockdep_map); work->func(work); + lock_map_release(&lockdep_map); + lock_map_release(&worker->lockdep_map); /* * Avoid dereferencing work after this point. The trace * event only cares about the address. @@ -1334,12 +1349,22 @@ static void kthread_flush_work_fn(struct kthread_work *work) void kthread_flush_work(struct kthread_work *work) { struct kthread_flush_work fwork = { - KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn), - COMPLETION_INITIALIZER_ONSTACK(fwork.done), + .done = COMPLETION_INITIALIZER_ONSTACK(fwork.done), }; struct kthread_worker *worker; bool noop = false; + kthread_init_work(&fwork.work, kthread_flush_work_fn); + + /* + * Waiting for the work's completion makes any lock held here part + * of a dependency cycle with any lock its function takes, and with + * anything the function itself waits for. Record the wait even if + * the work turns out to be idle. + */ + lock_map_acquire(&work->lockdep_map); + lock_map_release(&work->lockdep_map); + worker = work->worker; if (!worker) return; @@ -1571,10 +1596,14 @@ EXPORT_SYMBOL_GPL(kthread_cancel_delayed_work_sync); void kthread_flush_worker(struct kthread_worker *worker) { struct kthread_flush_work fwork = { - KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn), - COMPLETION_INITIALIZER_ONSTACK(fwork.done), + .done = COMPLETION_INITIALIZER_ONSTACK(fwork.done), }; + kthread_init_work(&fwork.work, kthread_flush_work_fn); + + lock_map_acquire(&worker->lockdep_map); + lock_map_release(&worker->lockdep_map); + kthread_queue_work(worker, &fwork.work); wait_for_completion(&fwork.done); } -- 2.43.0