[PATCH] kthread: Teach lockdep about kthread_flush_work() dependencies

[PATCH] kthread: Teach lockdep about kthread_flush_work() dependencies

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 7321 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

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 <dwmw@amazon.co.uk>
---
 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(&params.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


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]