[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 --]

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

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on chrome-platform/for-firmware-next linus/master v7.3 next-20261019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/kthread-Teach-lockdep-about-kthread_flush_work-dependencies/20261020-160400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/6dee36e290c2a3b85dcd7cdba2cab72fe57b25c4.camel%40infradead.org
patch subject: [PATCH] kthread: Teach lockdep about kthread_flush_work() dependencies
config: arm64-allnoconfig (https://download.01.org/0day-ci/archive/20261021/202610211501.YBj5GUfA-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 16.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261021/202610211501.YBj5GUfA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202610211501.YBj5GUfA-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from kernel/rcu/tree.c:45:
   kernel/rcu/tree_exp.h: In function 'sync_rcu_exp_select_cpus_queue_work':
>> include/linux/kthread.h:219:46: warning: unused variable '__work_key' [-Wunused-variable]
     219 |                 static struct lock_class_key __work_key;                \
         |                                              ^~~~~~~~~~
   kernel/rcu/tree_exp.h:470:9: note: in expansion of macro 'kthread_init_work'
     470 |         kthread_init_work(&rnp->rew.rew_work, sync_rcu_exp_select_node_cpus);
         |         ^~~~~~~~~~~~~~~~~
   kernel/rcu/tree_exp.h: In function 'synchronize_rcu_expedited_queue_work':
>> include/linux/kthread.h:219:46: warning: unused variable '__work_key' [-Wunused-variable]
     219 |                 static struct lock_class_key __work_key;                \
         |                                              ^~~~~~~~~~
   kernel/rcu/tree_exp.h:497:9: note: in expansion of macro 'kthread_init_work'
     497 |         kthread_init_work(&rew->rew_work, wait_rcu_exp_gp);
         |         ^~~~~~~~~~~~~~~~~


vim +/__work_key +219 include/linux/kthread.h

   216	
   217	#define kthread_init_work(work, fn)					\
   218		do {								\
 > 219			static struct lock_class_key __work_key;		\
   220			memset((work), 0, sizeof(struct kthread_work));		\
   221			INIT_LIST_HEAD(&(work)->node);				\
   222			(work)->func = (fn);					\
   223			__kthread_init_work_lockdep_map((work), &__work_key);	\
   224		} while (0)
   225	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on chrome-platform/for-firmware-next linus/master v7.3 next-20261019]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/kthread-Teach-lockdep-about-kthread_flush_work-dependencies/20261020-160400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
patch link:    https://lore.kernel.org/r/6dee36e290c2a3b85dcd7cdba2cab72fe57b25c4.camel%40infradead.org
patch subject: [PATCH] kthread: Teach lockdep about kthread_flush_work() dependencies
config: s390-defconfig (https://download.01.org/0day-ci/archive/20261021/202610211757.UM0pu0Gh-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261021/202610211757.UM0pu0Gh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202610211757.UM0pu0Gh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/vdpa/vdpa_sim/vdpa_sim.c:71:2: warning: unused variable '__work_key' [-Wunused-variable]
      71 |         kthread_init_work(work, vdpasim_mm_work_fn);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/kthread.h:219:32: note: expanded from macro 'kthread_init_work'
     219 |                 static struct lock_class_key __work_key;                \
         |                                              ^~~~~~~~~~
   drivers/vdpa/vdpa_sim/vdpa_sim.c:234:2: warning: unused variable '__work_key' [-Wunused-variable]
     234 |         kthread_init_work(&vdpasim->work, vdpasim_work_fn);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/kthread.h:219:32: note: expanded from macro 'kthread_init_work'
     219 |                 static struct lock_class_key __work_key;                \
         |                                              ^~~~~~~~~~
   2 warnings generated.


vim +/__work_key +71 drivers/vdpa/vdpa_sim/vdpa_sim.c

4bb94d2de2fa90a Stefano Garzarella 2023-04-04  65  
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  66  static void vdpasim_worker_change_mm_sync(struct vdpasim *vdpasim,
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  67  					  struct vdpasim_mm_work *mm_work)
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  68  {
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  69  	struct kthread_work *work = &mm_work->work;
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  70  
4bb94d2de2fa90a Stefano Garzarella 2023-04-04 @71  	kthread_init_work(work, vdpasim_mm_work_fn);
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  72  	kthread_queue_work(vdpasim->worker, work);
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  73  
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  74  	kthread_flush_work(work);
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  75  }
4bb94d2de2fa90a Stefano Garzarella 2023-04-04  76  

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki