[PATCH 0/3] kthread_worker: Fix race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync()

[PATCH 0/3] kthread_worker: Fix race between kthread_mod_delayed_work() and kthread_cancel_delayed_work_sync()

[Petr Mladek]

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

The original fix, from Martin Liu [1], opened discussion [2] about
the return value. It took me some time to realize that the original
patch was not a correct fix.

The problem is that the return value might be used for reference counting
of the queued works. The return value is boolean. It could distinguish
only two situations where the work is:

    + newly queued => inc(refcnt)                   (ret == false)
    + was queued, removed, added => nope(refcntn)   (ret == true)

The original fix, introduced another situation:

    + was queued, removed => dec(refcnt)   (ret == ???)

The proper solution is to remove the work from the list only when
it can be added again. Fortunately, it can be fixed relatively
easily. I have split it into 3 small steps.

[1] https://lore.kernel.org/r/20210513065458.941403-1-liumartin@google.com
[2] https://lore.kernel.org/r/20210520214737.MrGGKbPrJ%akpm@linux-foundation.org


Petr Mladek (3):
  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()
  kthread_worker: Fix return value when kthread_mod_delayed_work() races
    with kthread_cancel_delayed_work_sync()

 kernel/kthread.c | 96 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 33 deletions(-)

-- 
2.26.2

[PATCH 1/3] kthread_worker: Split code for canceling the delayed work timer.

[Petr Mladek]

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.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index fe3f2a40d61e..121a0e1fc659 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -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
-- 
2.26.2

[PATCH 2/3] kthread: Prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[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")
Reported-by: Martin Liu <liumartin@google.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 121a0e1fc659..0fccf7d0c6a1 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -1120,8 +1120,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.
@@ -1129,13 +1132,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.
@@ -1188,11 +1186,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:
@@ -1214,7 +1224,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;
-- 
2.26.2

[PATCH 3/3] kthread_worker: Fix return value when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[Petr Mladek]

kthread_mod_delayed_work() might race with
kthread_cancel_delayed_work_sync() or another kthread_mod_delayed_work()
call. The function lets the other operation win when it sees
work->canceling counter set. And it returns @false.

But it should return @true as it is done by the related workqueue API,
see mod_delayed_work_on().

The reason is that the return value might be used for reference counting.
It has to distinguish the case when the number of queued works has changed
or stayed the same.

The change is safe. kthread_mod_delayed_work() return value is not checked
anywhere at the moment.

Link: https://lore.kernel.org/r/20210521163526.GA17916@redhat.com
Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 0fccf7d0c6a1..86ae5f2e6db8 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -1156,14 +1156,14 @@ static bool __kthread_cancel_work(struct kthread_work *work)
  * modify @dwork's timer so that it expires after @delay. If @delay is zero,
  * @work is guaranteed to be queued immediately.
  *
- * Return: %true if @dwork was pending and its timer was modified,
- * %false otherwise.
+ * Return: %false if @dwork was idle and queued, %true otherwise.
  *
  * A special case is when the work is being canceled in parallel.
  * It might be caused either by the real kthread_cancel_delayed_work_sync()
  * or yet another kthread_mod_delayed_work() call. We let the other command
- * win and return %false here. The caller is supposed to synchronize these
- * operations a reasonable way.
+ * win and return %true here. The return value can be used for reference
+ * counting and the number of queued works stays the same. Anyway, the caller
+ * is supposed to synchronize these operations a reasonable way.
  *
  * This function is safe to call from any context including IRQ handler.
  * See __kthread_cancel_work() and kthread_delayed_work_timer_fn()
@@ -1175,13 +1175,15 @@ bool kthread_mod_delayed_work(struct kthread_worker *worker,
 {
 	struct kthread_work *work = &dwork->work;
 	unsigned long flags;
-	int ret = false;
+	int ret;
 
 	raw_spin_lock_irqsave(&worker->lock, flags);
 
 	/* Do not bother with canceling when never queued. */
-	if (!work->worker)
+	if (!work->worker) {
+		ret = false;
 		goto fast_queue;
+	}
 
 	/* Work must not be used with >1 worker, see kthread_queue_work() */
 	WARN_ON_ONCE(work->worker != worker);
@@ -1199,8 +1201,11 @@ bool kthread_mod_delayed_work(struct kthread_worker *worker,
 	 * be used for reference counting.
 	 */
 	kthread_cancel_delayed_work_timer(work, &flags);
-	if (work->canceling)
+	if (work->canceling) {
+		/* The number of works in the queue does not change. */
+		ret = true;
 		goto out;
+	}
 	ret = __kthread_cancel_work(work);
 
 fast_queue:
-- 
2.26.2

Re: [PATCH 2/3] kthread: Prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[Andrew Morton]

On Thu, 10 Jun 2021 15:30:50 +0200 Petr Mladek <pmladek@suse.com> wrote:

> The system might hang with the following backtrace:

Well that's not good.

> Fixes: 9a6b06c8d9a220860468a ("kthread: allow to modify delayed kthread work")
> Reported-by: Martin Liu <liumartin@google.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Was a -stable backport considered?

Re: [PATCH 2/3] kthread: Prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[Petr Mladek]

On Thu 2021-06-10 14:30:30, Andrew Morton wrote:
> On Thu, 10 Jun 2021 15:30:50 +0200 Petr Mladek <pmladek@suse.com> wrote:
> 
> > The system might hang with the following backtrace:
> 
> Well that's not good.

Fortunately, the API users normally synchronize these operations
another way. The race should never happen when the API is used
a reasonable way,
see https://lore.kernel.org/lkml/YKZLnTNOlUQ85F2s@alley/

> > Fixes: 9a6b06c8d9a220860468a ("kthread: allow to modify delayed kthread work")
> > Reported-by: Martin Liu <liumartin@google.com>
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> 
> Was a -stable backport considered?

Good point! It would make sense to backport it. System hang is never
good.

Could you please add Cc: stable@vger.kernel.org or should I resend the
patchset?

Best Regards,
Petr

Re: [PATCH 2/3] kthread: Prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[Andrew Morton]

On Fri, 11 Jun 2021 09:16:52 +0200 Petr Mladek <pmladek@suse.com> wrote:

> On Thu 2021-06-10 14:30:30, Andrew Morton wrote:
> > On Thu, 10 Jun 2021 15:30:50 +0200 Petr Mladek <pmladek@suse.com> wrote:
> > 
> > > The system might hang with the following backtrace:
> > 
> > Well that's not good.
> 
> Fortunately, the API users normally synchronize these operations
> another way. The race should never happen when the API is used
> a reasonable way,
> see https://lore.kernel.org/lkml/YKZLnTNOlUQ85F2s@alley/
> 
> > > Fixes: 9a6b06c8d9a220860468a ("kthread: allow to modify delayed kthread work")
> > > Reported-by: Martin Liu <liumartin@google.com>
> > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > 
> > Was a -stable backport considered?
> 
> Good point! It would make sense to backport it. System hang is never
> good.
> 
> Could you please add Cc: stable@vger.kernel.org or should I resend the
> patchset?

I made that change to patches 1&2.  I don't think patch 3 need be
backported?

Re: [PATCH 2/3] kthread: Prevent deadlock when kthread_mod_delayed_work() races with kthread_cancel_delayed_work_sync()

[Petr Mladek]

On Fri 2021-06-11 13:41:25, Andrew Morton wrote:
> On Fri, 11 Jun 2021 09:16:52 +0200 Petr Mladek <pmladek@suse.com> wrote:
> 
> > On Thu 2021-06-10 14:30:30, Andrew Morton wrote:
> > > On Thu, 10 Jun 2021 15:30:50 +0200 Petr Mladek <pmladek@suse.com> wrote:
> > > 
> > > > The system might hang with the following backtrace:
> > > 
> > > Well that's not good.
> > 
> > Fortunately, the API users normally synchronize these operations
> > another way. The race should never happen when the API is used
> > a reasonable way,
> > see https://lore.kernel.org/lkml/YKZLnTNOlUQ85F2s@alley/
> > 
> > > > Fixes: 9a6b06c8d9a220860468a ("kthread: allow to modify delayed kthread work")
> > > > Reported-by: Martin Liu <liumartin@google.com>
> > > > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > > 
> > > Was a -stable backport considered?
> > 
> > Good point! It would make sense to backport it. System hang is never
> > good.
> > 
> > Could you please add Cc: stable@vger.kernel.org or should I resend the
> > patchset?
> 
> I made that change to patches 1&2.  I don't think patch 3 need be
> backported?

Sounds good to me. Thanks a lot for the update.

Best Regards,
Petr