linux-kernel - Re: [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_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: <CAAObsKCGJ1NETGD1vcDzEEREjA=8cZn0PY15+OVyK6cN+eHWhw@mail.gmail.com>
Date:	Fri, 31 Oct 2014 09:53:52 +0100
From:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH v2] cpufreq: Guard against not-yet-initialized policies in cpufreq_cpu_get()

On 29 October 2014 22:24, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> On Wednesday, October 29, 2014 04:45:47 PM Tomeu Vizoso wrote:
>> There's a substantial window of opportunity from the time the policy objects
>> are created until they are initialized, causing this:
>>
>> WARNING: CPU: 1 PID: 64 at include/linux/kref.h:47 kobject_get+0x64/0x70()
>> Modules linked in:
>> CPU: 1 PID: 64 Comm: irq/77-tegra-ac Not tainted 3.18.0-rc1-next-20141027ccu-00049-g21a0041 #248
>> [<c0016fac>] (unwind_backtrace) from [<c001272c>] (show_stack+0x10/0x14)
>> [<c001272c>] (show_stack) from [<c0601920>] (dump_stack+0x98/0xd8)
>> [<c0601920>] (dump_stack) from [<c00288d8>] (warn_slowpath_common+0x70/0x8c)
>> [<c00288d8>] (warn_slowpath_common) from [<c0028990>] (warn_slowpath_null+0x1c/0x24)
>> [<c0028990>] (warn_slowpath_null) from [<c02227bc>] (kobject_get+0x64/0x70)
>> [<c02227bc>] (kobject_get) from [<c03e5238>] (cpufreq_cpu_get+0x88/0xc8)
>> [<c03e5238>] (cpufreq_cpu_get) from [<c03e52ec>] (cpufreq_get+0xc/0x64)
>> [<c03e52ec>] (cpufreq_get) from [<c0287818>] (actmon_thread_isr+0x140/0x1a4)
>> [<c0287818>] (actmon_thread_isr) from [<c0068a68>] (irq_thread_fn+0x1c/0x40)
>> [<c0068a68>] (irq_thread_fn) from [<c0068d84>] (irq_thread+0x134/0x174)
>> [<c0068d84>] (irq_thread) from [<c0040284>] (kthread+0xdc/0xf4)
>> [<c0040284>] (kthread) from [<c000f4b8>] (ret_from_fork+0x14/0x3c)
>>
>> This commit checks that initialization has finished before trying to do
>> anything with the policy.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
>> ---
>>  drivers/cpufreq/cpufreq.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 644b54e..7b84d1a 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -48,6 +48,9 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
>>  /* Flag to suspend/resume CPUFreq governors */
>>  static bool cpufreq_suspended;
>>
>> +/* Flag that tells whether initialization is completed */
>> +static atomic_t cpufreq_initialized = ATOMIC_INIT(0);
>> +
>>  static inline bool has_target(void)
>>  {
>>       return cpufreq_driver->target_index || cpufreq_driver->target;
>> @@ -211,7 +214,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>>       /* get the cpufreq driver */
>>       read_lock_irqsave(&cpufreq_driver_lock, flags);
>>
>> -     if (cpufreq_driver) {
>> +     if (atomic_read(&cpufreq_initialized)) {
>
> The atomics don't help you here, because they don't make race conditions go
> away.  Memory barriers would be needed for that, but then there should be an
> alternative way to address the problem at hand.

Yeah, now I'm not sure that atomic is even needed here, as
cpufreq_cpu_get is already returning NULL if the cpufreq_driver hasn't
been registered yet, so the callers should be already retrying.

>>               /* get the CPU */
>>               policy = per_cpu(cpufreq_cpu_data, cpu);
>>               if (policy)
>> @@ -1289,6 +1292,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
>>       kobject_uevent(&policy->kobj, KOBJ_ADD);
>>       up_read(&cpufreq_rwsem);
>>
>> +     atomic_set(&cpufreq_initialized, 1);
>> +
>
> __cpufreq_add_dev() can be run for many times.  Why is the first one only
> relevant?

I see now. What about having the flag in struct cpufreq_policy instead?

TBH, I find the initialization sequence in cpufreq quite daunting, so
any guidance on this will be appreciated.

Regards,

Tomeu

>>       pr_debug("initialization complete\n");
>>
>>       return 0;
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/