Skip to contentSkip to conversation

FAILED: patch "[PATCH] kthread_worker: split code for canceling the delayed work" failed to apply to 4.19-stable tree

PATCH In review [email protected] 2 participants 9 messages Jun 27, 2021 – Jul 8, 2021 Stable branch

lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 34b3d5344719d14fd2185b2d9459b3abcb8cf9d8 Mon Sep 17 00:00:00 2001
From: Petr Mladek <[email protected]>
Date: Thu, 24 Jun 2021 18:39:45 -0700
Subject: [PATCH] kthread_worker: split code for canceling the delayed work
timer

Patch series "kthread_worker: Fix race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync()".

This patchset fixes the race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync() including proper return value handling.

This patch (of 2):

Simple code refactoring as a preparation step for fixing a race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync().

It does not modify the existing behavior.

kernel/kthread.c +29 -17
index fe3f2a40d61e..121a0e1fc659 100644
@@ -1092,6 +1092,33 @@ void kthread_flush_work(struct kthread_work *work)
 }
 EXPORT_SYMBOL_GPL(kthread_flush_work);
 
+/*
+ * Make sure that the timer is neither set nor running and could
+ * not manipulate the work list_head any longer.
+ *
+ * The function is called under worker->lock. The lock is temporary
+ * released but the timer can't be set again in the meantime.
+ */
+static void kthread_cancel_delayed_work_timer(struct kthread_work *work,
+					      unsigned long *flags)
+{
+	struct kthread_delayed_work *dwork =
+		container_of(work, struct kthread_delayed_work, work);
+	struct kthread_worker *worker = work->worker;
+
+	/*
+	 * del_timer_sync() must be called to make sure that the timer
+	 * callback is not running. The lock must be temporary released
+	 * to avoid a deadlock with the callback. In the meantime,
+	 * any queuing is blocked by setting the canceling counter.
+	 */
+	work->canceling++;
+	raw_spin_unlock_irqrestore(&worker->lock, *flags);
+	del_timer_sync(&dwork->timer);
+	raw_spin_lock_irqsave(&worker->lock, *flags);
+	work->canceling--;
+}
+
 /*
  * This function removes the work from the worker queue. Also it makes sure
  * that it won't get queued later via the delayed work's timer.
@@ -1106,23 +1133,8 @@ static bool __kthread_cancel_work(struct kthread_work *work, bool is_dwork,
 				  unsigned long *flags)
 {
 	/* Try to cancel the timer if exists. */
-	if (is_dwork) {
-		struct kthread_delayed_work *dwork =
-			container_of(work, struct kthread_delayed_work, work);
-		struct kthread_worker *worker = work->worker;
-
-		/*
-		 * del_timer_sync() must be called to make sure that the timer
-		 * callback is not running. The lock must be temporary released
-		 * to avoid a deadlock with the callback. In the meantime,
-		 * any queuing is blocked by setting the canceling counter.
-		 */
-		work->canceling++;
-		raw_spin_unlock_irqrestore(&worker->lock, *flags);
-		del_timer_sync(&dwork->timer);
-		raw_spin_lock_irqsave(&worker->lock, *flags);
-		work->canceling--;
-	}
+	if (is_dwork)
+		kthread_cancel_delayed_work_timer(work, flags);
 
 	/*
 	 * Try to remove the work from a worker list. It might either
#1 lore raw

cover kthread_worker: Fix race between kthread_mod_delayed_work()

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2
#2 lore raw

PATCH 1/2 kthread_worker: split code for canceling the delayed work timer

Patch series "kthread_worker: Fix race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync()".

This patchset fixes the race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync() including proper return value handling.

This patch (of 2):

Simple code refactoring as a preparation step for fixing a race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync().

It does not modify the existing behavior.

---
 kernel/kthread.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)
kernel/kthread.c +29 -17
index 81abfac35127..b2688c39c6f1 100644
@@ -1009,6 +1009,33 @@ void kthread_flush_work(struct kthread_work *work)
 }
 EXPORT_SYMBOL_GPL(kthread_flush_work);
 
+/*
+ * Make sure that the timer is neither set nor running and could
+ * not manipulate the work list_head any longer.
+ *
+ * The function is called under worker->lock. The lock is temporary
+ * released but the timer can't be set again in the meantime.
+ */
+static void kthread_cancel_delayed_work_timer(struct kthread_work *work,
+					      unsigned long *flags)
+{
+	struct kthread_delayed_work *dwork =
+		container_of(work, struct kthread_delayed_work, work);
+	struct kthread_worker *worker = work->worker;
+
+	/*
+	 * del_timer_sync() must be called to make sure that the timer
+	 * callback is not running. The lock must be temporary released
+	 * to avoid a deadlock with the callback. In the meantime,
+	 * any queuing is blocked by setting the canceling counter.
+	 */
+	work->canceling++;
+	spin_unlock_irqrestore(&worker->lock, *flags);
+	del_timer_sync(&dwork->timer);
+	spin_lock_irqsave(&worker->lock, *flags);
+	work->canceling--;
+}
+
 /*
  * This function removes the work from the worker queue. Also it makes sure
  * that it won't get queued later via the delayed work's timer.
@@ -1023,23 +1050,8 @@ static bool __kthread_cancel_work(struct kthread_work *work, bool is_dwork,
 				  unsigned long *flags)
 {
 	/* Try to cancel the timer if exists. */
-	if (is_dwork) {
-		struct kthread_delayed_work *dwork =
-			container_of(work, struct kthread_delayed_work, work);
-		struct kthread_worker *worker = work->worker;
-
-		/*
-		 * del_timer_sync() must be called to make sure that the timer
-		 * callback is not running. The lock must be temporary released
-		 * to avoid a deadlock with the callback. In the meantime,
-		 * any queuing is blocked by setting the canceling counter.
-		 */
-		work->canceling++;
-		spin_unlock_irqrestore(&worker->lock, *flags);
-		del_timer_sync(&dwork->timer);
-		spin_lock_irqsave(&worker->lock, *flags);
-		work->canceling--;
-	}
+	if (is_dwork)
+		kthread_cancel_delayed_work_timer(work, flags);
 
 	/*
 	 * Try to remove the work from a worker list. It might either
Signature · 1 line
2.26.2
#3 lore raw

PATCH 2/2 kthread: prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

In reply to Petr Mladek

The system might hang with the following backtrace:

	schedule+0x80/0x100
	schedule_timeout+0x48/0x138
	wait_for_common+0xa4/0x134
	wait_for_completion+0x1c/0x2c
	kthread_flush_work+0x114/0x1cc
	kthread_cancel_work_sync.llvm.16514401384283632983+0xe8/0x144
	kthread_cancel_delayed_work_sync+0x18/0x2c
	xxxx_pm_notify+0xb0/0xd8
	blocking_notifier_call_chain_robust+0x80/0x194
	pm_notifier_call_chain_robust+0x28/0x4c
	suspend_prepare+0x40/0x260
	enter_state+0x80/0x3f4
	pm_suspend+0x60/0xdc
	state_store+0x108/0x144
	kobj_attr_store+0x38/0x88
	sysfs_kf_write+0x64/0xc0
	kernfs_fop_write_iter+0x108/0x1d0
	vfs_write+0x2f4/0x368
	ksys_write+0x7c/0xec

It is caused by the following race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync():

CPU0 CPU1

Context: Thread A Context: Thread B

kthread_mod_delayed_work()
spin_lock()
__kthread_cancel_work()

     spin_unlock()
     del_timer_sync()
				kthread_cancel_delayed_work_sync()
				  spin_lock()
				  __kthread_cancel_work()
				    spin_unlock()
				    del_timer_sync()
				    spin_lock()

				  work->canceling++
				  spin_unlock
     spin_lock()
   queue_delayed_work()
     // dwork is put into the worker->delayed_work_list

spin_unlock()

				  kthread_flush_work()
     // flush_work is put at the tail of the dwork

				    wait_for_completion()

Context: IRQ

  kthread_delayed_work_timer_fn()
    spin_lock()
    list_del_init(&work->node);
    spin_unlock()

BANG: flush_work is not longer linked and will never get proceed.

The problem is that kthread_mod_delayed_work() checks work->canceling flag before canceling the timer.

A simple solution is to (re)check work->canceling after __kthread_cancel_work(). But then it is not clear what should be returned when __kthread_cancel_work() removed the work from the queue (list) and it can't queue it again with the new @delay.

The return value might be used for reference counting. The caller has to know whether a new work has been queued or an existing one was replaced.

The proper solution is that kthread_mod_delayed_work() will remove the work from the queue (list) _only_ when work->canceling is not set. The flag must be checked after the timer is stopped and the remaining operations can be done under worker->lock.

Note that kthread_mod_delayed_work() could remove the timer and then bail out. It is fine. The other canceling caller needs to cancel the timer as well. The important thing is that the queue (list) manipulation is done atomically under worker->lock.

Fixes 9a6b06c8d9a220860468a ("kthread: allow to modify delayed kthread work")
Cc Minchan Kim <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc Nick Desaulniers <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Tejun Heo <[email protected]>
---
 kernel/kthread.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)
kernel/kthread.c +24 -11
index b2688c39c6f1..9750f4f7f901 100644
@@ -1037,8 +1037,11 @@ static void kthread_cancel_delayed_work_timer(struct kthread_work *work,
 }
 
 /*
- * This function removes the work from the worker queue. Also it makes sure
- * that it won't get queued later via the delayed work's timer.
+ * This function removes the work from the worker queue.
+ *
+ * It is called under worker->lock. The caller must make sure that
+ * the timer used by delayed work is not running, e.g. by calling
+ * kthread_cancel_delayed_work_timer().
  *
  * The work might still be in use when this function finishes. See the
  * current_work proceed by the worker.
@@ -1046,13 +1049,8 @@ static void kthread_cancel_delayed_work_timer(struct kthread_work *work,
  * Return: %true if @work was pending and successfully canceled,
  *	%false if @work was not pending
  */
-static bool __kthread_cancel_work(struct kthread_work *work, bool is_dwork,
-				  unsigned long *flags)
+static bool __kthread_cancel_work(struct kthread_work *work)
 {
-	/* Try to cancel the timer if exists. */
-	if (is_dwork)
-		kthread_cancel_delayed_work_timer(work, flags);
-
 	/*
 	 * Try to remove the work from a worker list. It might either
 	 * be from worker->work_list or from worker->delayed_work_list.
@@ -1105,11 +1103,23 @@ bool kthread_mod_delayed_work(struct kthread_worker *worker,
 	/* Work must not be used with >1 worker, see kthread_queue_work() */
 	WARN_ON_ONCE(work->worker != worker);
 
-	/* Do not fight with another command that is canceling this work. */
+	/*
+	 * Temporary cancel the work but do not fight with another command
+	 * that is canceling the work as well.
+	 *
+	 * It is a bit tricky because of possible races with another
+	 * mod_delayed_work() and cancel_delayed_work() callers.
+	 *
+	 * The timer must be canceled first because worker->lock is released
+	 * when doing so. But the work can be removed from the queue (list)
+	 * only when it can be queued again so that the return value can
+	 * be used for reference counting.
+	 */
+	kthread_cancel_delayed_work_timer(work, &flags);
 	if (work->canceling)
 		goto out;
+	ret = __kthread_cancel_work(work);
 
-	ret = __kthread_cancel_work(work, true, &flags);
 fast_queue:
 	__kthread_queue_delayed_work(worker, dwork, delay);
 out:
@@ -1131,7 +1141,10 @@ static bool __kthread_cancel_work_sync(struct kthread_work *work, bool is_dwork)
 	/* Work must not be used with >1 worker, see kthread_queue_work(). */
 	WARN_ON_ONCE(work->worker != worker);
 
-	ret = __kthread_cancel_work(work, is_dwork, &flags);
+	if (is_dwork)
+		kthread_cancel_delayed_work_timer(work, &flags);
+
+	ret = __kthread_cancel_work(work);
 
 	if (worker->current_work != work)
 		goto out_fast;
Signature · 1 line
2.26.2
author maintainer
#4 lore raw

Re: [PATCH 0/2] kthread_worker: Fix race between kthread_mod_delayed_work()

In reply to Petr Mladek
Quoted 9 lines · Petr Mladek

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Quoted 11 lines

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2

What is the original git commit ids of these patches in Linus's tree?

thanks,

greg k-h

#5 lore raw

Re: [PATCH 0/2] kthread_worker: Fix race between kthread_mod_delayed_work()

Quoted 15 lines · Greg KH
Quoted 14 lines · Petr Mladek

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

Quoted 8 lines
Quoted 6 lines
 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2

What is the original git commit ids of these patches in Linus's tree?

commit 34b3d5344719d14fd2185b2d9459b3abcb8cf9d8 ("kthread_worker: split code for canceling the delayed work timer")
commit 5fa54346caf67b4b1b10b1f390316ae466da4d53 ("kthread: prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync())"

The original commits have already been taken for the newer stable trees.

I am sorry for the inconvenience.

Best Regards,
Petr

author maintainer
#6 lore raw

Re: [PATCH 0/2] kthread_worker: Fix race between kthread_mod_delayed_work()

Quoted 23 lines · Petr Mladek
Quoted 22 lines · Greg KH
Quoted 20 lines · Petr Mladek

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2
Quoted 8 lines
Quoted 1 line

What is the original git commit ids of these patches in Linus's tree?

commit 34b3d5344719d14fd2185b2d9459b3abcb8cf9d8 ("kthread_worker: split code for canceling the delayed work timer")
commit 5fa54346caf67b4b1b10b1f390316ae466da4d53 ("kthread: prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync())"

The original commits have already been taken for the newer stable trees.

I am sorry for the inconvenience.

What about backports for 5.4, 5.10, and 5.12? We can not take patches only for older kernels, otherwise people upgrading to newer ones would have a regression. Please can you submit these patches for those trees too? Until that, I can not take these.

thanks,

greg k-h

#7 lore raw

Re: [PATCH 0/2] kthread_worker: Fix race between kthread_mod_delayed_work()

Quoted 29 lines · Greg KH
Quoted 28 lines · Petr Mladek
Quoted 15 lines · Greg KH
Quoted 14 lines · Petr Mladek

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

Quoted 8 lines
Quoted 6 lines
 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2

What is the original git commit ids of these patches in Linus's tree?

commit 34b3d5344719d14fd2185b2d9459b3abcb8cf9d8 ("kthread_worker: split code for canceling the delayed work timer")
commit 5fa54346caf67b4b1b10b1f390316ae466da4d53 ("kthread: prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync())"

Quoted 8 lines
Quoted 3 lines

The original commits have already been taken for the newer stable trees.

I am sorry for the inconvenience.

What about backports for 5.4, 5.10, and 5.12? We can not take patches only for older kernels, otherwise people upgrading to newer ones would have a regression. Please can you submit these patches for those trees too? Until that, I can not take these.

The patches for 5.4, 5.10, and 5.12 did not need any changes. My understanding is that the original commits have already been queued for these never code trees. See the mail in stable-commits:

for 5.4:

https://www.spinics.net/lists/stable-commits/msg203482.html
https://www.spinics.net/lists/stable-commits/msg203481.html

for 5.10:

https://www.spinics.net/lists/stable-commits/msg203486.html
https://www.spinics.net/lists/stable-commits/msg203485.html

for 5.12:

https://www.spinics.net/lists/stable-commits/msg203494.html
https://www.spinics.net/lists/stable-commits/msg203493.html

I am sorry, I should have mentioned this in the cover letter.

Best Regards,
Petr

author maintainer
#8 lore raw

Re: [PATCH 0/2] kthread_worker: Fix race between kthread_mod_delayed_work()

Quoted 53 lines · Petr Mladek
Quoted 29 lines · Greg KH
Quoted 28 lines · Petr Mladek
Quoted 15 lines · Greg KH
Quoted 14 lines · Petr Mladek

This is backport of the series for the following stable trees:

+ 4.9
+ 4.14
+ 4.19

The orignal series did not apply because of a conflict with the commit ("kthread: Convert worker lock to raw spinlock").

Petr Mladek (2):
kthread_worker: split code for canceling the delayed work timer
kthread: prevent deadlock when kthread_mod_delayed_work() races with
kthread_cancel_delayed_work_sync()

Quoted 8 lines
Quoted 6 lines
 kernel/kthread.c | 77 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 26 deletions(-)
Signature · 1 line
2.26.2

What is the original git commit ids of these patches in Linus's tree?

commit 34b3d5344719d14fd2185b2d9459b3abcb8cf9d8 ("kthread_worker: split code for canceling the delayed work timer")
commit 5fa54346caf67b4b1b10b1f390316ae466da4d53 ("kthread: prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync())"

Quoted 8 lines
Quoted 3 lines

The original commits have already been taken for the newer stable trees.

I am sorry for the inconvenience.

What about backports for 5.4, 5.10, and 5.12? We can not take patches only for older kernels, otherwise people upgrading to newer ones would have a regression. Please can you submit these patches for those trees too? Until that, I can not take these.

The patches for 5.4, 5.10, and 5.12 did not need any changes. My understanding is that the original commits have already been queued for these never code trees. See the mail in stable-commits:

for 5.4:

https://www.spinics.net/lists/stable-commits/msg203482.html
https://www.spinics.net/lists/stable-commits/msg203481.html

for 5.10:

https://www.spinics.net/lists/stable-commits/msg203486.html
https://www.spinics.net/lists/stable-commits/msg203485.html

Quoted 6 lines

for 5.12:

https://www.spinics.net/lists/stable-commits/msg203494.html
https://www.spinics.net/lists/stable-commits/msg203493.html

I am sorry, I should have mentioned this in the cover letter.

No worries, all now queued up, thanks.

greg k-h