Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753413AbbGIPRO (ORCPT ); Thu, 9 Jul 2015 11:17:14 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:32864 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752989AbbGIPQy (ORCPT ); Thu, 9 Jul 2015 11:16:54 -0400 From: Frederic Weisbecker To: LKML Cc: Frederic Weisbecker , Peter Zijlstra , Chris Metcalf , Thomas Gleixner , Don Zickus , Ulrich Obergfell , Andrew Morton Subject: [PATCH 2/4] smpboot: Make cleanup to mirror setup Date: Thu, 9 Jul 2015 17:16:30 +0200 Message-Id: <1436454992-21462-3-git-send-email-fweisbec@gmail.com> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1436454992-21462-1-git-send-email-fweisbec@gmail.com> References: <1436454992-21462-1-git-send-email-fweisbec@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4007 Lines: 99 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 ressources 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: [ 112.645556] WATCHDOG disable 0 [ 112.648765] WATCHDOG disable 1 [ 112.651891] WATCHDOG disable 2 [ 112.654953] BUG: unable to handle kernel NULL pointer dereference at (null) [ 112.662808] IP: [] hrtimer_active+0x26/0x60 [...] [ 112.815078] Call Trace: [ 112.817523] [] hrtimer_try_to_cancel+0x1c/0x280 [ 112.823697] [] hrtimer_cancel+0x1d/0x30 [ 112.829172] [] watchdog_disable+0x56/0x70 [ 112.834818] [] watchdog_cleanup+0xe/0x10 [ 112.840381] [] smpboot_thread_fn+0x23c/0x2c0 [ 112.846296] [] ? sort_range+0x30/0x30 [ 112.851596] [] kthread+0xf8/0x110 [ 112.856550] [] ? kthread_create_on_node+0x210/0x210 [ 112.863065] [] ret_from_fork+0x3f/0x70 [ 112.868460] [] ? kthread_create_on_node+0x210/0x210 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 lets 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. Cc: Andrew Morton Cc: Chris Metcalf Cc: Don Zickus Cc: Peter Zijlstra Cc: Thomas Gleixner Cc: Ulrich Obergfell Signed-off-by: Frederic Weisbecker --- kernel/smpboot.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/kernel/smpboot.c b/kernel/smpboot.c index 71aa90b..60aa858 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); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/