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
| ||
|
Message-ID: <7F861DC0615E0C47A872E6F3C5FCDDBD05F55194@BPXM14GP.gisp.nec.co.jp> Date: Thu, 29 Oct 2015 04:30:20 +0000 From: Hiroshi Shimamoto <h-shimamoto@...jp.nec.com> To: Frederic Weisbecker <fweisbec@...il.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 > 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. I see the point, vtime accounting can be enabled on dedicated cpu and there is no guarantee the reading thread is on the same state. > > 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 have dig the code. And my understanding is that vtime_accounting_enabled() does check global flag with context_tracking_is_enabled() and then check current cpu state with context_tracking_cpu_is_enabled(). For now, we just check global flag to fix current issue instead of checking both in vtime_accounting_enabled(). In future we should fix more precisely. Is that correct? thanks, Hiroshi > > 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/
Powered by blists - more mailing lists