linux-kernel - Re: [PATCH 2/2] cpufreq: serialize calls to __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: <CAKohpon1VsoUnw=t0c9wQ2Zfva5nnXoKjNNNxLXV7Frto1BFug@mail.gmail.com>
Date:	Sun, 1 Sep 2013 21:30:49 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Stephen Boyd <sboyd@...eaurora.org>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	Patch Tracking <patches@...aro.org>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor()

On 1 September 2013 18:58, Rafael J. Wysocki <rjw@...k.pl> wrote:
> On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
>> We can't take a big lock around __cpufreq_governor() as this causes recursive
>> locking for some cases. But calls to this routine must be serialized for every
>> policy.
>
> Care to explain here why it must be serialized?

Yes, added that to the attached patches (Added reported-by too):

commit dc51771506b113b998c49c3be2db0fb88bb92153
Author: Viresh Kumar <viresh.kumar@...aro.org>
Date:   Sat Aug 31 17:48:23 2013 +0530

    cpufreq: serialize calls to __cpufreq_governor()

    We can't take a big lock around __cpufreq_governor() as this
causes recursive
    locking for some cases. But calls to this routine must be
serialized for every
    policy. Otherwise we can see some unpredictable events.

    For example, consider following scenario:

    __cpufreq_remove_dev()
     __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
       policy->governor->governor(policy, CPUFREQ_GOV_STOP);
        cpufreq_governor_dbs()
         case CPUFREQ_GOV_STOP:
          mutex_destroy(&cpu_cdbs->timer_mutex)
          cpu_cdbs->cur_policy = NULL;
      <PREEMPT>
    store()
     __cpufreq_set_policy()
      __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
        policy->governor->governor(policy, CPUFREQ_GOV_LIMITS);
         case CPUFREQ_GOV_LIMITS:
          mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex)
           if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL

    And so store() will eventually result in a crash cur_policy is already NULL;

    Lets introduce another variable which would guarantee serialization here.

   Reported-by: Stephen Boyd <sboyd@...eaurora.org>
   Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>

>> Lets introduce another variable which would guarantee serialization here.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
>> ---
>>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>>  include/linux/cpufreq.h   | 1 +
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index f320a20..4d5723db 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>>                                               policy->cpu, event);
>>
>>       mutex_lock(&cpufreq_governor_lock);
>> -     if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>> +     if (policy->governor_busy ||
>> +             (policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
>
> Again, broken white space, but I can fix it up.

Sorry, what exactly.. Sorry couldn't understand it :(

Download attachment "0001-cpufreq-don-t-allow-governor-limits-to-be-changed-wh.patch" of type "application/octet-stream" (1497 bytes)

Download attachment "0002-cpufreq-serialize-calls-to-__cpufreq_governor.patch" of type "application/octet-stream" (3336 bytes)