linux-kernel - Re: [PATCH misc 1/2] workqueue: Add check for clocks going backwards to wq_worker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1452aae8-c8d7-43a2-979a-5b3878ddc2fa@paulmck-laptop>
Date: Fri, 2 Aug 2024 09:39:19 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Rik van Riel <riel@...riel.com>
Cc: linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Breno Leitao <leitao@...ian.org>,
	Anhad Jai Singh <ffledgling@...a.com>,
	Oleg Nesterov <oleg@...hat.com>, Jens Axboe <axboe@...nel.dk>,
	Christian Brauner <brauner@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	Chris Mason <clm@...com>
Subject: Re: [PATCH misc 1/2] workqueue: Add check for clocks going backwards
 to wq_worker_tick()

On Fri, Aug 02, 2024 at 12:06:06PM -0400, Rik van Riel wrote:
> On Thu, 2024-08-01 at 17:30 -0700, Paul E. McKenney wrote:
> > 
> > +++ b/kernel/workqueue.c
> > @@ -1482,6 +1482,7 @@ void wq_worker_tick(struct task_struct *task)
> >  	 * If the current worker is concurrency managed and hogged
> > the CPU for
> >  	 * longer than wq_cpu_intensive_thresh_us, it's
> > automatically marked
> >  	 * CPU_INTENSIVE to avoid stalling other concurrency-managed
> > work items.
> > +	 * If the time is negative, ignore, assuming a backwards
> > clock.
> >  	 *
> >  	 * Set @worker->sleeping means that @worker is in the
> > process of
> >  	 * switching out voluntarily and won't be contributing to
> > @@ -1491,6 +1492,7 @@ void wq_worker_tick(struct task_struct *task)
> >  	 * We probably want to make this prettier in the future.
> >  	 */
> >  	if ((worker->flags & WORKER_NOT_RUNNING) ||
> > READ_ONCE(worker->sleeping) ||
> > +	    WARN_ON_ONCE((s64)(worker->task->se.sum_exec_runtime -
> > worker->current_at) < 0) ||
> >  	    worker->task->se.sum_exec_runtime - worker->current_at <
> >  	    wq_cpu_intensive_thresh_us * NSEC_PER_USEC)
> >  		return;
> 
> What is the code path by which sum_exec_runtime could go backward
> in time, if the TSC and sched_clock() jump backward?
> 
> Might it make sense to check in the place where sum_exec_runtime is
> updated, instead, and catch a wider net?
> 
> On the flip side, the run time increments are "fairly large" in
> number of TSC cycles, while most of the negative TSC jumps we 
> have seen are quite small, so even that wider net might not catch
> much because of how coarse these updates typically are...

Good points!  Even more telling, this patch didn't catch anything during
Breno's tests.  I will drop it with a workqueue.2024.08.01 branch in
-rcu in case someone needs it later.

							Thanx, Paul

> All Rights Reversed.