linux-kernel - Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160118055034.GC30762@vireshk>
Date:	Mon, 18 Jan 2016 11:20:34 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Juri Lelli <juri.lelli@....com>
Cc:	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	peterz@...radead.org, rjw@...ysocki.net, mturquette@...libre.com,
	steve.muckle@...aro.org, vincent.guittot@...aro.org,
	morten.rasmussen@....com, dietmar.eggemann@....com
Subject: Re: [RFC PATCH 15/19] cpufreq: remove useless usage of
 cpufreq_governor_mutex in __cpufreq_governor

On 15-01-16, 16:30, Juri Lelli wrote:
> But governor_enabled seems to not be checked anymore outside cpufreq.c
> (see also 01/19), as it was in the commit you are referring to.

Okay, I must have told you this earlier but anyway ..

governor_enabled was introduced long back to keep governor state
changes serialized. Because of the complex cases we had in hand
(governor-per-policy or system wide governors, etc.), it failed to do
so. Though simple races were avoided with it, complex ones still came
back to haunt us.

We fixed that by managing state changes within ondemand and
conservative governors instead and that worked very well.

Then I wrote a patch to kill the stupid code around governor_enabled
thing, but I got into few races. Those races happened because of
userspace governor, which was getting into invalid states on some
extreme cases (These were caught using the test-suite I wrote and you
perhaps used it).

And I never came back to fix those corner cases ..

You can try that on ARM or x86 by running following command from my
test-suite (I remember that you are using it, right?):

./runme.sh -f sp1 or sp2 or sp3

Only one of sp1, sp2 or sp3 is required..

> Now that
> users of this should be holding policy->rwsem, so that should suffice
> for protecting governor_enabled, as governor_enabled is only changed
> inside here.

If we can get rid of the rwsem dropping problem, then yeah this can be
killed for sure.

> I run some test on a x86 box I setup and didn't see anything related to
> this. I'll wait to get the first 0-day report anyway.

Okay, so run the above test and make sure you have following enabled
in your configuration:

CONFIG_LOCKDEP_SUPPORT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_PI_LIST=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y

> > > -	mutex_lock(&cpufreq_governor_mutex);
> > >  	if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> > >  	    || (!policy->governor_enabled
> > >  	    && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> > > -		mutex_unlock(&cpufreq_governor_mutex);
> > >  		return -EBUSY;
> > >  	}
> > 
> > Actually the above checks should also be removed as the governors are
> > responsible for maintaining their state machines. But
> > userspace/powersave/performance don't have that support yet and so
> > these checks save them from going into undefined states.
> > 
> > Over that, above and below checks are incomplete..
> > 
> 
> You mean we need an additional patch that extends the checks performed?

Yeah, we need to add some state-management code in
userspace/powersave/performance governors as well.

-- 
viresh