* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).