linux-kernel - Re: [PATCH v2] cputime: fix invalid gtime

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151029033735.GA30367@lerouge>
Date:	Thu, 29 Oct 2015 04:37:37 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] cputime: fix invalid gtime

On Thu, Oct 29, 2015 at 01:10:01AM +0000, Hiroshi Shimamoto wrote:
> > Obviously I completely messed up there. And task_cputime() has a similar issue
> > but it happens to work due to vtime_snap_whence set to VTIME_SLEEPING when vtime
> > doesn't run. Still it works at the cost of a seqcount read operation.
> > 
> > Do you think you could fix it too (along with task_cputime_scaled())? I think those
> > patches will also need a stable tag.
> 
> Do you mean that task_cputime() and task_cputime_scaled() don't hit invalid behavior
> but have some extra operation cost which could be removed?

Exactly.

> 
> Will look into it, and send patches with stable tag.

Thanks a lot!

Oh and another detail: vtime_accounting_enabled() checks if vtime
accounting is done precisely on the current CPU. That's what we want to check
when we account the time but not when we want to read the cputime of a task.

For example, CPU 0 never has vtime_accounting_enabled() because it plays the
role of timekeeper and as such it keeps the tick periodic. So if task A runs on
CPU 1 that has vtime accounting on, and we read the cputime of task A from CPU 0,
vtime_accounting_enabled() will be false whereas we need to compute the delta.

So vtime_accounting_enabled() isn't suitable to check if vtime is running on _some_
CPU such that we can't return utime/stime with a raw read.

Ideally we shoud rename vtime_accounting_enabled() to vtime_accounting_cpu_enabled()
and have vtime_accounting_enabled() to check if vtime runs somewhere. But that would
be too much an invasive change for a stable patch. So lets just use
context_tracking_is_enabled() for now instead.

I think task_gtime() is also buggy when the target task runs in a CPU that doesn't do
vtime accounting (whereas another CPU does vtime accounting). We should fix that with
using a new VTIME_GUEST value instead of (or along with) PF_VCPU. But that's another story,
your fix is much more important for now.

> 
> thanks,
> Hiroshi
> 
> > 
> > Thanks!
> > 
> > 
> > 
> > >  	do {
> > >  		seq = read_seqbegin(&t->vtime_seqlock);
> > >
> > > --
> > > 1.8.3.1
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/