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, 10 Feb 2014 14:45:55 +0530 From: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com> To: ego@...ux.vnet.ibm.com CC: Oleg Nesterov <oleg@...hat.com>, paulus@...ba.org, rusty@...tcorp.com.au, peterz@...radead.org, tglx@...utronix.de, akpm@...ux-foundation.org, mingo@...nel.org, paulmck@...ux.vnet.ibm.com, tj@...nel.org, walken@...gle.com, linux@....linux.org.uk, linux-kernel@...r.kernel.org, Toshi Kani <toshi.kani@...com>, "Rafael J. Wysocki" <rafael.j.wysocki@...el.com> Subject: Re: [PATCH 01/51] CPU hotplug: Provide lockless versions of callback registration functions Hi Gautham, On 02/08/2014 12:41 AM, Gautham R Shenoy wrote: > On Thu, Feb 06, 2014 at 07:41:03PM +0100, Oleg Nesterov wrote: >> On 02/06, Srivatsa S. Bhat wrote: >>> >>> The following method of CPU hotplug callback registration is not safe >>> due to the possibility of an ABBA deadlock involving the cpu_add_remove_lock >>> and the cpu_hotplug.lock. >> >> Off-topic, but perhaps it also makes sense to add the lockdep annotations >> later, to catch other similar problems. Currently get_online_cpus() acquires >> nothing from lockdep pov. > > Well, both get/put_online_cpus() as well as cpu_hotplug_begin/end() > take the cpu_hotplug.lock mutex. So ideally the lockdep annotations of > mutex_lock/unlock() should have worked. The reason lockdep doesn't catch the lock-inversion (ABBA) deadlock between cpu_hotplug.lock (from get_online_cpus) and cpu_add_remove_lock (from cpu_maps_update_begin) is because, in the following path, the cpu_add_remove_lock is acquired after *releasing* the cpu_hotplug.lock mutex. get_online_cpus(); // acquire mutex; update counter; release mutex register_cpu_notifier(); // acquire cpu_add_remove_lock ... put_online_cpus(); > If it hasn't, then the > following lockdep annotations to cpu-hotplug locking should do the > trick. > This patch looks good to me. I have a couple of suggestions though.. > Signed-off-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com> > --- > kernel/cpu.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/kernel/cpu.c b/kernel/cpu.c > index deff2e6..3d2dd1c 100644 > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -19,6 +19,7 @@ > #include <linux/mutex.h> > #include <linux/gfp.h> > #include <linux/suspend.h> > +#include <linux/lockdep.h> > > #include "smpboot.h" > > @@ -57,21 +58,34 @@ static struct { > * an ongoing cpu hotplug operation. > */ > int refcount; > + > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > + struct lockdep_map dep_map; > +#endif > } cpu_hotplug = { > .active_writer = NULL, > .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock), > .refcount = 0, > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > + .dep_map = {.name = "cpu_hotplug.lock" }, > +#endif > }; > > +#define cphp_lock_acquire_read(l, s, t, i) lock_acquire_shared_recursive(l, s, t, NULL, i) > +#define cphp_lock_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i) > +#define cphp_lock_release(l, n, i) lock_release(l, n, i) > + Can you make them cpuhp_* instead of cphp_*? That way it would suit better as a short-form of "cpu hotplug". Also, perhaps we could use the lock_map_acquire(), lock_map_acquire_read() and lock_map_release() macros to make the call-sites look neater. Regards, Srivatsa S. Bhat > void get_online_cpus(void) > { > might_sleep(); > if (cpu_hotplug.active_writer == current) > return; > + cphp_lock_acquire_read(&cpu_hotplug.dep_map, 0, 0, _RET_IP_); > mutex_lock(&cpu_hotplug.lock); > cpu_hotplug.refcount++; > mutex_unlock(&cpu_hotplug.lock); > > + > } > EXPORT_SYMBOL_GPL(get_online_cpus); > > @@ -79,6 +93,7 @@ void put_online_cpus(void) > { > if (cpu_hotplug.active_writer == current) > return; > + > mutex_lock(&cpu_hotplug.lock); > > if (WARN_ON(!cpu_hotplug.refcount)) > @@ -87,6 +102,7 @@ void put_online_cpus(void) > if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer)) > wake_up_process(cpu_hotplug.active_writer); > mutex_unlock(&cpu_hotplug.lock); > + cphp_lock_release(&cpu_hotplug.dep_map, 1, _RET_IP_); > > } > EXPORT_SYMBOL_GPL(put_online_cpus); > @@ -117,6 +133,7 @@ void cpu_hotplug_begin(void) > { > cpu_hotplug.active_writer = current; > > + cphp_lock_acquire(&cpu_hotplug.dep_map, 0, 0, _RET_IP_); > for (;;) { > mutex_lock(&cpu_hotplug.lock); > if (likely(!cpu_hotplug.refcount)) > @@ -131,6 +148,7 @@ void cpu_hotplug_done(void) > { > cpu_hotplug.active_writer = NULL; > mutex_unlock(&cpu_hotplug.lock); > + cphp_lock_release(&cpu_hotplug.dep_map, 1, _RET_IP_); > } > > /* > -- 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