linux-kernel - Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

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: <CAKohpomEQsS-WHQpa=_07NQ1FXDB711inrmMgUguD529BA7g1A@mail.gmail.com>
Date:	Mon, 16 Sep 2013 22:38:05 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	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>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Subject: Re: [PATCH 1/2] cpufreq: unlock correct rwsem while updating policy->cpu

On 16 September 2013 21:57, Jon Medhurst (Tixy) <tixy@...aro.org> wrote:
> If I take mainline code and just change the line above to:

You meant this line by above line?

         unlock_policy_rwsem_write(cpu);

>    up_write(&per_cpu(cpu_policy_rwsem, (per_cpu(cpufreq_cpu_data,
> cpu))->last_cpu));
> then the big_little cpufreq driver works for me.

That would be same as: up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu));

>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 43c24aa..c18bf7b 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -952,9 +952,16 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
>>       if (cpu == policy->cpu)
>>               return;
>>
>> +     /* take direct locks as lock_policy_rwsem_write wouldn't work here */
>> +     down_write(&per_cpu(cpu_policy_rwsem, policy->cpu));
>> +     down_write(&per_cpu(cpu_policy_rwsem, cpu));
>> +
>>       policy->last_cpu = policy->cpu;
>>       policy->cpu = cpu;
>>
>> +     up_write(&per_cpu(cpu_policy_rwsem, cpu));
>> +     up_write(&per_cpu(cpu_policy_rwsem, policy->cpu));
>
> You've just overwritten policy->cpu with cpu.

Stupid enough :)

> I tried using
> policy->last_cpu to fix that, but it still doesn't work for me (giving
> the lockdep warning I mentioned.) If I change the code to just lock the
> original policy->cpu lock only, then all is fine.

Fixed my patch now.. find attached.. It mentions why lock for last cpu is
enough here. Copied that here too..

+ /*
+ * Take direct locks as lock_policy_rwsem_write wouldn't work here.
+ * Also lock for last cpu is enough here as contention will happen only
+ * after policy->cpu is changed and after it is changed, other threads
+ * will try to acquire lock for new cpu. And policy is already updated
+ * by then.
+ */

> Also, this locking is now just happening around policy->cpu update,
> whereas before this change, it was locked for the whole of
> update_policy_cpu, i.e. cpufreq_frequency_table_update_policy_cpu and
> the notifier callbacks. Is that change of lock coverage OK?

Yeah, the rwsem is only required for updating policy's fields and so that
should be okay.

Download attachment "0001-cpufreq-unlock-correct-rwsem-while-updating-policy-c.patch" of type "application/octet-stream" (2344 bytes)