linux-kernel - Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY

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]

Date:	Tue, 11 Nov 2014 07:15:29 -0500
From:	Prarit Bhargava <prarit@...hat.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Robert Schöne <robert.schoene@...dresden.de>,
	Stephen Boyd <sboyd@...eaurora.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 2/5] cpufreq, fix locking around CPUFREQ_GOV_POLICY_EXIT
 calls



On 11/10/2014 10:37 PM, Viresh Kumar wrote:
> On 10 November 2014 17:56, Prarit Bhargava <prarit@...hat.com> wrote:
> 
>>> I still fail to understand why ? What will the _trylock() change ?
>>
>> viresh, afaict read_trylock will return 0 when busy with write:
> 
> Yes..
> 
>> static inline int queue_read_trylock(struct qrwlock *lock)
>> {
>>         u32 cnts;
>>
>>         cnts = atomic_read(&lock->cnts);
>>         if (likely(!(cnts & _QW_WMASK))) {
>>
>> so the deadlock will not occur.  the show() is opened, write lock is taken, and
>> if the thread is rescheduled and takes read lock the trylock will return 0, and
>> the thread will return -EBUSY to userspace avoiding the deadlock.
> 
> Which deadlock? 

the deadlock in commit 955ef4833574636819cd269cfbae12f79cbde63a

[   75.471265]        CPU0                    CPU1
[   75.476327]        ----                    ----
[   75.481385]   lock(&policy->rwsem);
[   75.485307]                                lock(s_active#219);
[   75.491857]                                lock(&policy->rwsem);
[   75.498592]   lock(s_active#219);
[   75.502331]
[   75.502331]  *** DEADLOCK ***

And also your changelog talks about accessing invalid pointers
> without the trylock change, how can that be possible? After the read
> lock is taken,
> all the pointers should be valid.

consider the following very simple case:

the governor is ondemand.  cpu 0 reads cpuinfo_cur_freq. cpu0 expects to get the
current cpu freq for the ondemand governor.

simultaneously, cpu1 changes the governor from ondemand to userspace.

the two threads will race for the policy->mutex

suppose cpu0 gets it first.  then there is no problem.  the userspace program
for cpu0 gets exactly the data it is expecting.

Now suppose cpu1 gets the lock and starts to write ... cpu0 is blocked.

cpu1 completes the governor change, and cpu0 gets the mutex ... and returns
bogus data at this point.

P.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/