From 5869b5064b0950afc447610c0f5d4134b71d9e94 Mon Sep 17 00:00:00 2001 From: Frederic Weisbecker Date: Fri, 4 Sep 2015 15:45:00 -0700 Subject: smpboot: fix memory leak on error handling The cpumask is allocated before threads get created. If the latter step fails, we need to free the cpumask. Signed-off-by: Frederic Weisbecker Reviewed-by: Chris Metcalf Reviewed-by: Thomas Gleixner Cc: Chris Metcalf Cc: Don Zickus Cc: Peter Zijlstra Cc: Ulrich Obergfell Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/smpboot.c | 1 + 1 file changed, 1 insertion(+) (limited to 'kernel/smpboot.c') diff --git a/kernel/smpboot.c b/kernel/smpboot.c index 7c434c39f02a..71aa90b69f8f 100644 --- a/kernel/smpboot.c +++ b/kernel/smpboot.c @@ -301,6 +301,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) ret = __smpboot_create_thread(plug_thread, cpu); if (ret) { smpboot_destroy_threads(plug_thread); + free_cpumask_var(plug_thread->cpumask); goto out; } smpboot_unpark_thread(plug_thread, cpu); -- cgit v1.2.3 From 3dd08c0c918f9bf058572ddbf26e7d6fb5674a5c Mon Sep 17 00:00:00 2001 From: Frederic Weisbecker Date: Fri, 4 Sep 2015 15:45:03 -0700 Subject: smpboot: make cleanup to mirror setup The per-cpu kthread cleanup() callback is the mirror of the setup() callback. When the per-cpu kthread is started, it first calls setup() to initialize the resources which are then released by cleanup() when the kthread exits. Now since the introduction of a per-cpu kthread cpumask, the kthreads excluded by the cpumask on boot may happen to be parked immediately after their creation without taking the setup() stage, waiting to be asked to unpark to do so. Then when smpboot_unregister_percpu_thread() is later called, the kthread is stopped without having ever called setup(). But this triggers a bug as the kthread unconditionally calls cleanup() on exit but this doesn't mirror any setup(). Thus the kernel crashes because we try to free resources that haven't been initialized, as in the watchdog case: WATCHDOG disable 0 WATCHDOG disable 1 WATCHDOG disable 2 BUG: unable to handle kernel NULL pointer dereference at (null) IP: hrtimer_active+0x26/0x60 [...] Call Trace: hrtimer_try_to_cancel+0x1c/0x280 hrtimer_cancel+0x1d/0x30 watchdog_disable+0x56/0x70 watchdog_cleanup+0xe/0x10 smpboot_thread_fn+0x23c/0x2c0 kthread+0xf8/0x110 ret_from_fork+0x3f/0x70 This bug is currently masked with explicit kthread unparking before kthread_stop() on smpboot_destroy_threads(). This forces a call to setup() and then unpark(). We could fix this by unconditionally calling setup() on kthread entry. But setup() isn't always cheap. In the case of watchdog it launches hrtimer, perf events, etc... So we may as well like to skip it if there are chances the kthread will never be used, as in a reduced cpumask value. So let's simply do a state machine check before calling cleanup() that makes sure setup() has been called before mirroring it. And remove the nasty hack workaround. Signed-off-by: Frederic Weisbecker Reviewed-by: Chris Metcalf Reviewed-by: Thomas Gleixner Cc: Chris Metcalf Cc: Don Zickus Cc: Peter Zijlstra Cc: Ulrich Obergfell Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/smpboot.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) (limited to 'kernel/smpboot.c') diff --git a/kernel/smpboot.c b/kernel/smpboot.c index 71aa90b69f8f..60aa858a6a07 100644 --- a/kernel/smpboot.c +++ b/kernel/smpboot.c @@ -113,7 +113,8 @@ static int smpboot_thread_fn(void *data) if (kthread_should_stop()) { __set_current_state(TASK_RUNNING); preempt_enable(); - if (ht->cleanup) + /* cleanup must mirror setup */ + if (ht->cleanup && td->status != HP_THREAD_NONE) ht->cleanup(td->cpu, cpu_online(td->cpu)); kfree(td); return 0; @@ -259,15 +260,6 @@ static void smpboot_destroy_threads(struct smp_hotplug_thread *ht) { unsigned int cpu; - /* Unpark any threads that were voluntarily parked. */ - for_each_cpu_not(cpu, ht->cpumask) { - if (cpu_online(cpu)) { - struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu); - if (tsk) - kthread_unpark(tsk); - } - } - /* We need to destroy also the parked threads of offline cpus */ for_each_possible_cpu(cpu) { struct task_struct *tsk = *per_cpu_ptr(ht->store, cpu); -- cgit v1.2.3 From 230ec93909f00678401cb2d63b8b95f1dea68e40 Mon Sep 17 00:00:00 2001 From: Frederic Weisbecker Date: Fri, 4 Sep 2015 15:45:06 -0700 Subject: smpboot: allow passing the cpumask on per-cpu thread registration It makes the registration cheaper and simpler for the smpboot per-cpu kthread users that don't need to always update the cpumask after threads creation. [sfr@canb.auug.org.au: fix for allow passing the cpumask on per-cpu thread registration] Signed-off-by: Frederic Weisbecker Reviewed-by: Chris Metcalf Reviewed-by: Thomas Gleixner Cc: Chris Metcalf Cc: Don Zickus Cc: Peter Zijlstra Cc: Ulrich Obergfell Signed-off-by: Stephen Rothwell Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/smpboot.h | 11 ++++++++++- kernel/smpboot.c | 14 +++++++++----- kernel/watchdog.c | 9 +++------ 3 files changed, 22 insertions(+), 12 deletions(-) (limited to 'kernel/smpboot.c') diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h index da3c593f9845..e6109a6cd8f6 100644 --- a/include/linux/smpboot.h +++ b/include/linux/smpboot.h @@ -48,7 +48,16 @@ struct smp_hotplug_thread { const char *thread_comm; }; -int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread); +int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread, + const struct cpumask *cpumask); + +static inline int +smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) +{ + return smpboot_register_percpu_thread_cpumask(plug_thread, + cpu_possible_mask); +} + void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread); int smpboot_update_cpumask_percpu_thread(struct smp_hotplug_thread *plug_thread, const struct cpumask *); diff --git a/kernel/smpboot.c b/kernel/smpboot.c index 60aa858a6a07..a818cbc73e14 100644 --- a/kernel/smpboot.c +++ b/kernel/smpboot.c @@ -273,19 +273,22 @@ static void smpboot_destroy_threads(struct smp_hotplug_thread *ht) } /** - * smpboot_register_percpu_thread - Register a per_cpu thread related to hotplug + * smpboot_register_percpu_thread_cpumask - Register a per_cpu thread related + * to hotplug * @plug_thread: Hotplug thread descriptor + * @cpumask: The cpumask where threads run * * Creates and starts the threads on all online cpus. */ -int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) +int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread, + const struct cpumask *cpumask) { unsigned int cpu; int ret = 0; if (!alloc_cpumask_var(&plug_thread->cpumask, GFP_KERNEL)) return -ENOMEM; - cpumask_copy(plug_thread->cpumask, cpu_possible_mask); + cpumask_copy(plug_thread->cpumask, cpumask); get_online_cpus(); mutex_lock(&smpboot_threads_lock); @@ -296,7 +299,8 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread) free_cpumask_var(plug_thread->cpumask); goto out; } - smpboot_unpark_thread(plug_thread, cpu); + if (cpumask_test_cpu(cpu, cpumask)) + smpboot_unpark_thread(plug_thread, cpu); } list_add(&plug_thread->list, &hotplug_threads); out: @@ -304,7 +308,7 @@ out: put_online_cpus(); return ret; } -EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread); +EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread_cpumask); /** * smpboot_unregister_percpu_thread - Unregister a per_cpu thread related to hotplug diff --git a/kernel/watchdog.c b/kernel/watchdog.c index a6ffa43f2993..e5bb86fb0ea5 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -713,15 +713,12 @@ static int watchdog_enable_all_cpus(void) int err = 0; if (!watchdog_running) { - err = smpboot_register_percpu_thread(&watchdog_threads); + err = smpboot_register_percpu_thread_cpumask(&watchdog_threads, + &watchdog_cpumask); if (err) pr_err("Failed to create watchdog threads, disabled\n"); - else { - if (smpboot_update_cpumask_percpu_thread( - &watchdog_threads, &watchdog_cpumask)) - pr_err("Failed to set cpumask for watchdog threads\n"); + else watchdog_running = 1; - } } else { /* * Enable/disable the lockup detectors or -- cgit v1.2.3