* [PATCH] smpboot: add missing get_online_cpus() when register @ 2014-07-31 3:30 Lai Jiangshan 2014-08-01 21:54 ` David Rientjes 2015-01-23 10:36 ` [tip:core/urgent] smpboot: Add missing get_online_cpus() in smpboot_register_percpu_thread() tip-bot for Lai Jiangshan 0 siblings, 2 replies; 4+ messages in thread From: Lai Jiangshan @ 2014-07-31 3:30 UTC (permalink / raw) To: linux-kernel Cc: Lai Jiangshan, Thomas Gleixner, Rusty Russell, Peter Zijlstra, Srivatsa S. Bhat, stable If the smpboot_register_percpu_thread() is called after smpboot_create_threads() but before __cpu_up(), the smpboot thread of the online-ing CPU is not created, and it results a bug. So we use get_online_cpus() to prevent it. smpboot_unregister_percpu_thread() travels all possible CPU, it doesn't need get_online_cpus() which is removed in the patch. CC: Thomas Gleixner <tglx@linutronix.de> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> CC: stable@kernel.org Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- kernel/smpboot.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/smpboot.c b/kernel/smpboot.c index eb89e18..8adab87 100644 --- a/kernel/smpboot.c +++ b/kernel/smpboot.c @@ -279,6 +279,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) unsigned int cpu; int ret = 0; + get_online_cpus(); mutex_lock(&smpboot_threads_lock); for_each_online_cpu(cpu) { ret = __smpboot_create_thread(plug_thread, cpu); @@ -291,6 +292,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) list_add(&plug_thread->list, &hotplug_threads); out: mutex_unlock(&smpboot_threads_lock); + put_online_cpus(); return ret; } EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); @@ -303,11 +305,9 @@ EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); */ void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread) { - get_online_cpus(); mutex_lock(&smpboot_threads_lock); list_del(&plug_thread->list); smpboot_destroy_threads(plug_thread); mutex_unlock(&smpboot_threads_lock); - put_online_cpus(); } EXPORT_SYMBOL_GPL(smpboot_unregister_percpu_thread); -- 1.7.4.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] smpboot: add missing get_online_cpus() when register 2014-07-31 3:30 [PATCH] smpboot: add missing get_online_cpus() when register Lai Jiangshan @ 2014-08-01 21:54 ` David Rientjes 2014-08-04 7:33 ` Lai Jiangshan 2015-01-23 10:36 ` [tip:core/urgent] smpboot: Add missing get_online_cpus() in smpboot_register_percpu_thread() tip-bot for Lai Jiangshan 1 sibling, 1 reply; 4+ messages in thread From: David Rientjes @ 2014-08-01 21:54 UTC (permalink / raw) To: Lai Jiangshan Cc: linux-kernel, Thomas Gleixner, Rusty Russell, Peter Zijlstra, Srivatsa S. Bhat, stable On Thu, 31 Jul 2014, Lai Jiangshan wrote: > If the smpboot_register_percpu_thread() is called after smpboot_create_threads() > but before __cpu_up(), the smpboot thread of the online-ing CPU is not created, > and it results a bug. So we use get_online_cpus() to prevent it. > Do you have an example of the bug to include? Maintainers are going to need to understand the implications of the problem before the stable@kernel.org annotation is warranted. > smpboot_unregister_percpu_thread() travels all possible CPU, it doesn't need > get_online_cpus() which is removed in the patch. > > CC: Thomas Gleixner <tglx@linutronix.de> > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> > CC: stable@kernel.org > Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > --- > kernel/smpboot.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/smpboot.c b/kernel/smpboot.c > index eb89e18..8adab87 100644 > --- a/kernel/smpboot.c > +++ b/kernel/smpboot.c > @@ -279,6 +279,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) > unsigned int cpu; > int ret = 0; > > + get_online_cpus(); > mutex_lock(&smpboot_threads_lock); > for_each_online_cpu(cpu) { > ret = __smpboot_create_thread(plug_thread, cpu); > @@ -291,6 +292,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) > list_add(&plug_thread->list, &hotplug_threads); > out: > mutex_unlock(&smpboot_threads_lock); > + put_online_cpus(); > return ret; > } I think the {get,put}_online_cpus() pair should be nested inside the smpboot_threads_lock for better lock ordering since not all cases smpboot_threads_lock will require it. That way, you can also do put_online_cpus() before smpboot_destroy_threads(), which you have already proven doesn't need it: @@ -280,14 +280,17 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) int ret = 0; mutex_lock(&smpboot_threads_lock); + get_online_cpus(); for_each_online_cpu(cpu) { ret = __smpboot_create_thread(plug_thread, cpu); if (ret) { + put_online_cpus(); smpboot_destroy_threads(plug_thread); goto out; } smpboot_unpark_thread(plug_thread, cpu); } + put_online_cpus(); list_add(&plug_thread->list, &hotplug_threads); out: mutex_unlock(&smpboot_threads_lock); > EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); > @@ -303,11 +305,9 @@ EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); > */ > void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread) > { > - get_online_cpus(); > mutex_lock(&smpboot_threads_lock); > list_del(&plug_thread->list); > smpboot_destroy_threads(plug_thread); > mutex_unlock(&smpboot_threads_lock); > - put_online_cpus(); > } > EXPORT_SYMBOL_GPL(smpboot_unregister_percpu_thread); This makes sense. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] smpboot: add missing get_online_cpus() when register 2014-08-01 21:54 ` David Rientjes @ 2014-08-04 7:33 ` Lai Jiangshan 0 siblings, 0 replies; 4+ messages in thread From: Lai Jiangshan @ 2014-08-04 7:33 UTC (permalink / raw) To: David Rientjes Cc: linux-kernel, Thomas Gleixner, Rusty Russell, Peter Zijlstra, Srivatsa S. Bhat, stable On 08/02/2014 05:54 AM, David Rientjes wrote: > On Thu, 31 Jul 2014, Lai Jiangshan wrote: > >> If the smpboot_register_percpu_thread() is called after smpboot_create_threads() >> but before __cpu_up(), the smpboot thread of the online-ing CPU is not created, >> and it results a bug. So we use get_online_cpus() to prevent it. >> > > Do you have an example of the bug to include? Sorry, no, I don't have. > Maintainers are going to > need to understand the implications of the problem before the > stable@kernel.org annotation is warranted. It is possible that smpboot_register_percpu_thread() can be called any time in current kernel. Repeating the module ehca and check while repeating online/offline the CPUs, the bug is possible to hit. I have not such devices to test. Let Thomas make the choice. > >> smpboot_unregister_percpu_thread() travels all possible CPU, it doesn't need >> get_online_cpus() which is removed in the patch. >> >> CC: Thomas Gleixner <tglx@linutronix.de> >> Cc: Rusty Russell <rusty@rustcorp.com.au> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> >> CC: stable@kernel.org >> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> >> --- >> kernel/smpboot.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/smpboot.c b/kernel/smpboot.c >> index eb89e18..8adab87 100644 >> --- a/kernel/smpboot.c >> +++ b/kernel/smpboot.c >> @@ -279,6 +279,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) >> unsigned int cpu; >> int ret = 0; >> >> + get_online_cpus(); >> mutex_lock(&smpboot_threads_lock); >> for_each_online_cpu(cpu) { >> ret = __smpboot_create_thread(plug_thread, cpu); >> @@ -291,6 +292,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) >> list_add(&plug_thread->list, &hotplug_threads); >> out: >> mutex_unlock(&smpboot_threads_lock); >> + put_online_cpus(); >> return ret; >> } > > I think the {get,put}_online_cpus() pair should be nested inside the > smpboot_threads_lock for better lock ordering since not all cases > smpboot_threads_lock will require it. > > That way, you can also do put_online_cpus() before > smpboot_destroy_threads(), which you have already proven doesn't need it: > > @@ -280,14 +280,17 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) > int ret = 0; > > mutex_lock(&smpboot_threads_lock); > + get_online_cpus(); get_online_cpus() can't be nested in smpboot_threads_lock. > for_each_online_cpu(cpu) { > ret = __smpboot_create_thread(plug_thread, cpu); > if (ret) { > + put_online_cpus(); > smpboot_destroy_threads(plug_thread); > goto out; > } > smpboot_unpark_thread(plug_thread, cpu); > } > + put_online_cpus(); > list_add(&plug_thread->list, &hotplug_threads); > out: > mutex_unlock(&smpboot_threads_lock); > >> EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); >> @@ -303,11 +305,9 @@ EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); >> */ >> void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread) >> { >> - get_online_cpus(); >> mutex_lock(&smpboot_threads_lock); >> list_del(&plug_thread->list); >> smpboot_destroy_threads(plug_thread); >> mutex_unlock(&smpboot_threads_lock); >> - put_online_cpus(); >> } >> EXPORT_SYMBOL_GPL(smpboot_unregister_percpu_thread); > > This makes sense. > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tip:core/urgent] smpboot: Add missing get_online_cpus() in smpboot_register_percpu_thread() 2014-07-31 3:30 [PATCH] smpboot: add missing get_online_cpus() when register Lai Jiangshan 2014-08-01 21:54 ` David Rientjes @ 2015-01-23 10:36 ` tip-bot for Lai Jiangshan 1 sibling, 0 replies; 4+ messages in thread From: tip-bot for Lai Jiangshan @ 2015-01-23 10:36 UTC (permalink / raw) To: linux-tip-commits Cc: srivatsa.bhat, rusty, laijs, tglx, linux-kernel, hpa, mingo, peterz, rientjes Commit-ID: 4bee96860a65c3a62d332edac331b3cf936ba3ad Gitweb: http://git.kernel.org/tip/4bee96860a65c3a62d332edac331b3cf936ba3ad Author: Lai Jiangshan <laijs@cn.fujitsu.com> AuthorDate: Thu, 31 Jul 2014 11:30:17 +0800 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Fri, 23 Jan 2015 11:33:51 +0100 smpboot: Add missing get_online_cpus() in smpboot_register_percpu_thread() The following race exists in the smpboot percpu threads management: CPU0 CPU1 cpu_up(2) get_online_cpus(); smpboot_create_threads(2); smpboot_register_percpu_thread(); for_each_online_cpu(); __smpboot_create_thread(); __cpu_up(2); This results in a missing per cpu thread for the newly onlined cpu2 and in a NULL pointer dereference on a consecutive offline of that cpu. Proctect smpboot_register_percpu_thread() with get_online_cpus() to prevent that. [ tglx: Massaged changelog and removed the change in smpboot_unregister_percpu_thread() because that's an optimization and therefor not stable material. ] Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Cc: David Rientjes <rientjes@google.com> Cc: stable@vger.kernel.org Link: http://lkml.kernel.org/r/1406777421-12830-1-git-send-email-laijs@cn.fujitsu.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- kernel/smpboot.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/smpboot.c b/kernel/smpboot.c index f032fb5..40190f2 100644 --- a/kernel/smpboot.c +++ b/kernel/smpboot.c @@ -280,6 +280,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) unsigned int cpu; int ret = 0; + get_online_cpus(); mutex_lock(&smpboot_threads_lock); for_each_online_cpu(cpu) { ret = __smpboot_create_thread(plug_thread, cpu); @@ -292,6 +293,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) list_add(&plug_thread->list, &hotplug_threads); out: mutex_unlock(&smpboot_threads_lock); + put_online_cpus(); return ret; } EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-23 10:37 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-31 3:30 [PATCH] smpboot: add missing get_online_cpus() when register Lai Jiangshan 2014-08-01 21:54 ` David Rientjes 2014-08-04 7:33 ` Lai Jiangshan 2015-01-23 10:36 ` [tip:core/urgent] smpboot: Add missing get_online_cpus() in smpboot_register_percpu_thread() tip-bot for Lai Jiangshan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.