linux-kernel - Re: [PATCH] cpufreq: Drop the 'initialized' field from struct cpufreq

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]

Date:	Thu, 19 May 2016 07:40:23 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Linux PM list <linux-pm@...r.kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpufreq: Drop the 'initialized' field from struct
 cpufreq_governor

On 18-05-16, 22:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> The 'initialized' field in struct cpufreq_governor is only used by
> the conservative governor (as a usage counter) and the way that
> happens is far from straightforward and arguably incorrect.
> 
> Namely, the value of 'initialized' is checked by
> cpufreq_dbs_governor_init() and cpufreq_dbs_governor_exit() and
> the results of those checks are passed (as the second argument) to
> the ->init() and ->exit() callbacks in struct dbs_governor.  Those
> callbacks are only implemented by the ondemand and conservative
> governors and ondemand doesn't use their second argument at all.
> In turn, the conservative governor uses it to decide whether or not
> to either register or unregister a transition notifier.
> 
> That whole mechanism is not only unnecessarily convoluted, but also
> racy, because the 'initialized' field of struct cpufreq_governor is
> updated in cpufreq_init_governor() and cpufreq_exit_governor() under
> policy->rwsem which doesn't help if one of these functions is run
> twice in parallel for different policies (which isn't impossible in
> principle), for example.
> 
> Instead of it, add a proper usage counter to the conservative
> governor and update it from cs_init() and cs_exit() which is
> guaranteed to be non-racy, as those functions are only called
> under gov_dbs_data_mutex which is global.
> 
> With that in place, drop the 'initialized' field from struct
> cpufreq_governor as it is not used any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
> 
> This is on top of https://patchwork.kernel.org/patch/9094021/
> 
> ---
>  drivers/cpufreq/cpufreq.c              |    3 --
>  drivers/cpufreq/cpufreq_conservative.c |   44 +++++++++++++++++++++------------
>  drivers/cpufreq/cpufreq_governor.c     |    6 ++--
>  drivers/cpufreq/cpufreq_governor.h     |    4 +--
>  drivers/cpufreq/cpufreq_ondemand.c     |    4 +--
>  include/linux/cpufreq.h                |    1 
>  6 files changed, 36 insertions(+), 26 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@...aro.org>

-- 
viresh