| 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
| ||
|
Date: Mon, 06 Jul 2009 11:02:11 -0700
From: "Pallipadi, Venkatesh" <venkatesh.pallipadi@...el.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Dave Jones <davej@...hat.com>,
Thomas Renninger <trenn@...e.de>,
"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
"kernel-testers@...r.kernel.org" <kernel-testers@...r.kernel.org>,
Ingo Molnar <mingo@...e.hu>, "rjw@...k.pl" <rjw@...k.pl>,
Dave Young <hidave.darkstar@...il.com>,
Pekka Enberg <penberg@...helsinki.fi>,
"Li, Shaohua" <shaohua.li@...el.com>,
Rusty Russell <rusty@...tcorp.com.au>,
"sven.wegener@...aler.net" <sven.wegener@...aler.net>
Subject: Re: [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
On Fri, 2009-07-03 at 12:41 -0700, Mathieu Desnoyers wrote:
> * Pallipadi, Venkatesh (venkatesh.pallipadi@...el.com) wrote:
> >
> >
> > >-----Original Message-----
> > >From: Mathieu Desnoyers [mailto:mathieu.desnoyers@...ymtl.ca]
> > >Sent: Friday, July 03, 2009 8:25 AM
> > >To: linux-kernel@...r.kernel.org; Pallipadi, Venkatesh; Dave
> > >Jones; Thomas Renninger; cpufreq@...r.kernel.org;
> > >kernel-testers@...r.kernel.org; Ingo Molnar; rjw@...k.pl; Dave
> > >Young; Pekka Enberg
> > >Cc: Mathieu Desnoyers; Li, Shaohua; Rusty Russell;
> > >sven.wegener@...aler.net
> > >Subject: [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
> > >
> > >OK, I've tried to clean it up the best I could, but please
> > >test this with
> > >concurrent cpu hotplug and cpufreq add/remove in loops. I'm
> > >sure we will make
> > >other interesting findings.
> > >
> >
> > This is a good and needed cleanup of cpufreq_add_dev.
> >
> >
> > >This is step one of fixing the overall locking dependency mess
> > >in cpufreq.
> > >
> > >Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
> > >CC: Venkatesh Pallipadi <venkatesh.pallipadi@...el.com>
> > >CC: rjw@...k.pl
> > >CC: mingo@...e.hu
> > >CC: Shaohua Li <shaohua.li@...el.com>
> > >CC: Pekka Enberg <penberg@...helsinki.fi>
> > >CC: Dave Young <hidave.darkstar@...il.com>
> > >CC: "Rafael J. Wysocki" <rjw@...k.pl>
> > >CC: Rusty Russell <rusty@...tcorp.com.au>
> > >CC: sven.wegener@...aler.net
> > >CC: cpufreq@...r.kernel.org
> > >CC: Thomas Renninger <trenn@...e.de>
> > >---
> > > drivers/cpufreq/cpufreq.c | 65
> > >++++++++++++++++++++++++++++------------------
> > > 1 file changed, 40 insertions(+), 25 deletions(-)
> > >
> > >Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c
> > >===================================================================
> > >--- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c
> > >2009-07-02 23:59:08.000000000 -0400
> > >+++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c 2009-07-02
> > >23:59:09.000000000 -0400
> > >@@ -763,6 +763,10 @@ static struct kobj_type ktype_cpufreq =
> > > * cpufreq_add_dev - add a CPU device
> > > *
> > > * Adds the cpufreq interface for a CPU device.
> > >+ *
> > >+ * The Oracle says: try running cpufreq
> > >registration/unregistration concurrently
> > >+ * with with cpu hotplugging and all hell will break loose.
> > >Tried to clean this
> > >+ * mess up, but more thorough testing is needed. - Mathieu
> > > */
> > > static int cpufreq_add_dev(struct sys_device *sys_dev)
> > > {
> > >@@ -806,15 +810,12 @@ static int cpufreq_add_dev(struct sys_de
> > > goto nomem_out;
> > > }
> > > if (!alloc_cpumask_var(&policy->cpus, GFP_KERNEL)) {
> > >- kfree(policy);
> > > ret = -ENOMEM;
> > >- goto nomem_out;
> > >+ goto err_free_policy;
> > > }
> > > if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL)) {
> > >- free_cpumask_var(policy->cpus);
> > >- kfree(policy);
> > > ret = -ENOMEM;
> > >- goto nomem_out;
> > >+ goto err_free_cpumask;
> > > }
> > >
> > > policy->cpu = cpu;
> > >@@ -822,7 +823,8 @@ static int cpufreq_add_dev(struct sys_de
> > >
> > > /* Initially set CPU itself as the policy_cpu */
> > > per_cpu(policy_cpu, cpu) = cpu;
> > >- lock_policy_rwsem_write(cpu);
> > >+ ret = (lock_policy_rwsem_write(cpu) < 0);
> > >+ WARN_ON(ret);
> > >
> > > init_completion(&policy->kobj_unregister);
> > > INIT_WORK(&policy->update, handle_update);
> > >@@ -835,7 +837,7 @@ static int cpufreq_add_dev(struct sys_de
> > > ret = cpufreq_driver->init(policy);
> > > if (ret) {
> > > dprintk("initialization failed\n");
> > >- goto err_out;
> > >+ goto err_unlock_policy;
> > > }
> > > policy->user_policy.min = policy->min;
> > > policy->user_policy.max = policy->max;
> > >@@ -860,15 +862,21 @@ static int cpufreq_add_dev(struct sys_de
> > > /* Check for existing affected CPUs.
> > > * They may not be aware of it due to CPU Hotplug.
> > > */
> > >- managed_policy = cpufreq_cpu_get(j);
> > >/* FIXME: Where is this released? What about error paths? */
> > >+ managed_policy = cpufreq_cpu_get(j);
> > > if (unlikely(managed_policy)) {
> > >
> > > /* Set proper policy_cpu */
> > > unlock_policy_rwsem_write(cpu);
> > > per_cpu(policy_cpu, cpu) = managed_policy->cpu;
> > >
> > >- if (lock_policy_rwsem_write(cpu) < 0)
> > >- goto err_out_driver_exit;
> > >+ if (lock_policy_rwsem_write(cpu) < 0) {
> > >+ /* Should not go through policy
> > >unlock path */
> > >+ if (cpufreq_driver->exit)
> > >+ cpufreq_driver->exit(policy);
> > >+ ret = -EBUSY;
> > >+ cpufreq_cpu_put(managed_policy);
> > >+ goto err_free_cpumask;
> > >+ }
> > >
> > > spin_lock_irqsave(&cpufreq_driver_lock, flags);
> > > cpumask_copy(managed_policy->cpus,
> > >policy->cpus);
> > >@@ -879,12 +887,14 @@ static int cpufreq_add_dev(struct sys_de
> > > ret = sysfs_create_link(&sys_dev->kobj,
> > > &managed_policy->kobj,
> > > "cpufreq");
> > >- if (ret)
> > >- goto err_out_driver_exit;
> > >-
> > >- cpufreq_debug_enable_ratelimit();
> > >- ret = 0;
> > >- goto err_out_driver_exit; /* call
> > >driver->exit() */
> > >+ if (!ret)
> > >+ cpufreq_cpu_put(managed_policy);
> >
> > Looks like cpufreq_cpu_put is needed both with ret and !ret. No?
> >
>
> No. ret == 0 path is a "success path" only creating a symlink, and
> therefore __cpufreq_remove_dev() will take care of calling the
> cpufreq_cpu_put() to decrement the reference count :
>
> static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> {
> ...
>
> if (unlikely(cpu != data->cpu)) {
> dprintk("removing link\n");
> cpumask_clear_cpu(cpu, data->cpus);
> spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> sysfs_remove_link(&sys_dev->kobj, "cpufreq");
> cpufreq_cpu_put(data);
> cpufreq_debug_enable_ratelimit();
> unlock_policy_rwsem_write(cpu);
> return 0;
> }
>
> This is, at least, how I understand what is happening here.
>
Agreed.
Thanks,
Venki
--
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/
Powered by blists - more mailing lists