linux-kernel - RE: [PATCH] cpufreq: governor: Fix prev_load initialization in cpufreq_governor

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: <36DF59CE26D8EE47B0655C516E9CE640287478E9@shsmsx102.ccr.corp.intel.com>
Date:	Mon, 25 Apr 2016 15:45:54 +0000
From:	"Chen, Yu C" <yu.c.chen@...el.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Linux PM list <linux-pm@...r.kernel.org>
Subject: RE: [PATCH] cpufreq: governor: Fix prev_load initialization in
 cpufreq_governor_start()

Hi,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@...ysocki.net]
> Sent: Monday, April 25, 2016 9:08 AM
> To: Linux PM list
> Cc: Linux Kernel Mailing List; Viresh Kumar; Chen, Yu C
> Subject: [PATCH] cpufreq: governor: Fix prev_load initialization in
> cpufreq_governor_start()
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> The way cpufreq_governor_start() initializes j_cdbs->prev_load is questionable.
> 
> First off, j_cdbs->prev_cpu_wall used as a denominator in the computation may
> be zero.  The case this happens is when
> get_cpu_idle_time_us() returns -1 and get_cpu_idle_time_jiffy() used to return
> that number is called exactly at the jiffies_64 wrap time.  It is rather hard to
> trigger that error, but it is not impossible and it will just crash the kernel then.
> 
> Second, j_cdbs->prev_load is computed as the average load during the entire
> time since the system started and it may not reflect the load in the previous
> sampling period (as it is expected to).
> That doesn't play well with the way dbs_update() uses that value.
> Namely, if the update time delta (wall_time) happens do be greater than twice
happens s/do/to be?
> the sampling rate on the first invocation of it, the initial value of j_cdbs-
> >prev_load (which may be completely off) will be returned to the caller as the
> current load (unless it is equal to zero and unless another CPU sharing the same
> policy object has a greater load value).
> 
> For this reason, notice that the prev_load field of struct cpu_dbs_info is only
> used by dbs_update() and only in that one place, so if
> cpufreq_governor_start() is modified to always initialize it to 0, it will make
> dbs_update() always compute the actual load first time it checks the update
> time delta against the doubled sampling rate (after initialization) and there
> won't be any side effects of it.
> 
> Consequently, modify cpufreq_governor_start() as described.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---

Acked-by: Chen Yu <yu.c.chen@...el.com>

thanks,
Yu