From patchwork Fri Nov 18 00:03:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Gleixner X-Patchwork-Id: 9435539 X-Patchwork-Delegate: rui.zhang@intel.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1BCD46047D for ; Fri, 18 Nov 2016 00:07:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0BC48296F5 for ; Fri, 18 Nov 2016 00:07:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 00A0F2970B; Fri, 18 Nov 2016 00:07:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2CBDF296F5 for ; Fri, 18 Nov 2016 00:07:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753184AbcKRAHT (ORCPT ); Thu, 17 Nov 2016 19:07:19 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:50366 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752813AbcKRAGR (ORCPT ); Thu, 17 Nov 2016 19:06:17 -0500 Received: from localhost ([127.0.0.1] helo=[127.0.1.1]) by Galois.linutronix.de with esmtp (Exim 4.80) (envelope-from ) id 1c7Wei-0000gw-3a; Fri, 18 Nov 2016 01:03:48 +0100 Message-Id: <20161117234810.509706590@linutronix.de> User-Agent: quilt/0.63-1 Date: Fri, 18 Nov 2016 00:03:36 -0000 From: Thomas Gleixner To: LKML Cc: Zhang Rui , Eduardo Valentin , linux-pm@vger.kernel.org, Peter Zijlstra , x86@kernel.org, rt@linutronix.de, Borislav Petkov Subject: [patch 08/12] thermal/x86_pkg_temp: Sanitize locking References: <20161117231435.891545908@linutronix.de> MIME-Version: 1.0 Content-Disposition: inline; filename=thermal-x86_pkg_temp--Sanitize-locking.patch Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP The work cancellation code, the thermal zone unregistering, the work code and the interrupt notification function are racy against each other and against cpu hotplug and module exit. The random locking sprinkeled all over the place does not help anything and probably exists to make people feel good. The resulting issues (mainly use after free) are probably hard to trigger, but they clearly exist Protect the package list with a spinlock so it can be accessed from the interrupt notifier and also from the work function. The add/removal code in the hotplug callbacks take the lock for list manipulation. That makes sure that on removal neither the interrupt notifier nor the work function can access the about to be freed package structure anymore. The thermal zone unregistering is another trainwreck. It's not serialized against the work function. So unregistering the zone device can race with the work function and cause havoc. Protect the thermal zone with a mutex, which is held in the work function to make sure that the zone device is not being unregistered concurrently. To solve the module exit issues, we simply invoke the cpu offline callback and let it work its magic. Use proper names for the locks so it's clear what they are for and add a pile of comments to explain the protection rules. It's amazing that fixing the locking and adding 30 lines of comments explaining it still removes more lines than it adds. Signed-off-by: Thomas Gleixner --- drivers/thermal/x86_pkg_temp_thermal.c | 216 +++++++++++++++------------------ 1 file changed, 104 insertions(+), 112 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/drivers/thermal/x86_pkg_temp_thermal.c +++ b/drivers/thermal/x86_pkg_temp_thermal.c @@ -73,7 +73,10 @@ static struct thermal_zone_params pkg_te /* List maintaining number of package instances */ static LIST_HEAD(phy_dev_list); -static DEFINE_MUTEX(phy_dev_list_mutex); +/* Serializes interrupt notification, work and hotplug */ +static DEFINE_SPINLOCK(pkg_temp_lock); +/* Protects zone operation in the work function against hotplug removal */ +static DEFINE_MUTEX(thermal_zone_mutex); /* Interrupt to work function schedule queue */ static DEFINE_PER_CPU(struct delayed_work, pkg_temp_thermal_threshold_work); @@ -81,8 +84,6 @@ static DEFINE_PER_CPU(struct delayed_wor /* To track if the work is already scheduled on a package */ static u8 *pkg_work_scheduled; -/* Spin lock to prevent races with pkg_work_scheduled */ -static spinlock_t pkg_work_lock; static u16 max_phy_id; /* Debug counters to show using debugfs */ @@ -115,21 +116,23 @@ static int pkg_temp_debugfs_init(void) return -ENOENT; } +/* + * Protection: + * + * - cpu hotplug: Read serialized by cpu hotplug lock + * Write must hold pkg_temp_lock + * + * - Other callsites: Must hold pkg_temp_lock + */ static struct pkg_device *pkg_temp_thermal_get_dev(unsigned int cpu) { u16 phys_proc_id = topology_physical_package_id(cpu); struct pkg_device *pkgdev; - mutex_lock(&phy_dev_list_mutex); - - list_for_each_entry(pkgdev, &phy_dev_list, list) - if (pkgdev->phys_proc_id == phys_proc_id) { - mutex_unlock(&phy_dev_list_mutex); + list_for_each_entry(pkgdev, &phy_dev_list, list) { + if (pkgdev->phys_proc_id == phys_proc_id) return pkgdev; - } - - mutex_unlock(&phy_dev_list_mutex); - + } return NULL; } @@ -289,67 +292,66 @@ static inline void disable_pkg_thres_int static void pkg_temp_thermal_threshold_work_fn(struct work_struct *work) { - int cpu = smp_processor_id(); - struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu); - int phy_id = topology_physical_package_id(cpu); - bool notify = false; - unsigned long flags; + struct thermal_zone_device *tzone = NULL; + int phy_id, cpu = smp_processor_id(); + struct pkg_device *pkgdev; u64 msr_val, wr_val; - if (!pkgdev) - return; - - spin_lock_irqsave(&pkg_work_lock, flags); + mutex_lock(&thermal_zone_mutex); + spin_lock_irq(&pkg_temp_lock); ++pkg_work_cnt; - if (unlikely(phy_id > max_phy_id)) { - spin_unlock_irqrestore(&pkg_work_lock, flags); + + pkgdev = pkg_temp_thermal_get_dev(cpu); + if (!pkgdev) { + spin_unlock_irq(&pkg_temp_lock); + mutex_unlock(&thermal_zone_mutex); return; } + pkg_work_scheduled[phy_id] = 0; - spin_unlock_irqrestore(&pkg_work_lock, flags); rdmsrl(MSR_IA32_PACKAGE_THERM_STATUS, msr_val); wr_val = msr_val & ~(THERM_LOG_THRESHOLD0 | THERM_LOG_THRESHOLD1); if (wr_val != msr_val) { wrmsrl(MSR_IA32_PACKAGE_THERM_STATUS, wr_val); - notify = true; + tzone = pkgdev->tzone; } enable_pkg_thres_interrupt(); + spin_unlock_irq(&pkg_temp_lock); - if (notify) { - pr_debug("thermal_zone_device_update\n"); - thermal_zone_device_update(pkgdev->tzone, - THERMAL_EVENT_UNSPECIFIED); - } + /* + * If tzone is not NULL, then thermal_zone_mutex will prevent the + * concurrent removal in the cpu offline callback. + */ + if (tzone) + thermal_zone_device_update(tzone, THERMAL_EVENT_UNSPECIFIED); + + mutex_unlock(&thermal_zone_mutex); } static int pkg_thermal_notify(u64 msr_val) { int cpu = smp_processor_id(); int phy_id = topology_physical_package_id(cpu); + struct pkg_device *pkgdev; unsigned long flags; - /* - * When a package is in interrupted state, all CPU's in that package - * are in the same interrupt state. So scheduling on any one CPU in - * the package is enough and simply return for others. - */ - spin_lock_irqsave(&pkg_work_lock, flags); + spin_lock_irqsave(&pkg_temp_lock, flags); ++pkg_interrupt_cnt; - if (unlikely(phy_id > max_phy_id) || unlikely(!pkg_work_scheduled) || - pkg_work_scheduled[phy_id]) { - disable_pkg_thres_interrupt(); - spin_unlock_irqrestore(&pkg_work_lock, flags); - return -EINVAL; - } - pkg_work_scheduled[phy_id] = 1; - spin_unlock_irqrestore(&pkg_work_lock, flags); disable_pkg_thres_interrupt(); - schedule_delayed_work_on(cpu, + + /* Work is per package, so scheduling it once is enough. */ + pkgdev = pkg_temp_thermal_get_dev(cpu); + if (pkgdev && pkg_work_scheduled && !pkg_work_scheduled[phy_id]) { + pkg_work_scheduled[phy_id] = 1; + schedule_delayed_work_on(cpu, &per_cpu(pkg_temp_thermal_threshold_work, cpu), msecs_to_jiffies(notify_delay_ms)); + } + + spin_unlock_irqrestore(&pkg_temp_lock, flags); return 0; } @@ -373,29 +375,25 @@ static int pkg_temp_thermal_device_add(u err = get_tj_max(cpu, &tj_max); if (err) - goto err_ret; - - mutex_lock(&phy_dev_list_mutex); + return err; pkgdev = kzalloc(sizeof(*pkgdev), GFP_KERNEL); - if (!pkgdev) { - err = -ENOMEM; - goto err_ret_unlock; - } + if (!pkgdev) + return -ENOMEM; - spin_lock_irqsave(&pkg_work_lock, flags); + spin_lock_irqsave(&pkg_temp_lock, flags); if (topology_physical_package_id(cpu) > max_phy_id) max_phy_id = topology_physical_package_id(cpu); temp = krealloc(pkg_work_scheduled, (max_phy_id+1) * sizeof(u8), GFP_ATOMIC); if (!temp) { - spin_unlock_irqrestore(&pkg_work_lock, flags); - err = -ENOMEM; - goto err_ret_free; + spin_unlock_irqrestore(&pkg_temp_lock, flags); + kfree(pkgdev); + return -ENOMEM; } pkg_work_scheduled = temp; pkg_work_scheduled[topology_physical_package_id(cpu)] = 0; - spin_unlock_irqrestore(&pkg_work_lock, flags); + spin_unlock_irqrestore(&pkg_temp_lock, flags); pkgdev->phys_proc_id = topology_physical_package_id(cpu); pkgdev->cpu = cpu; @@ -406,60 +404,78 @@ static int pkg_temp_thermal_device_add(u pkgdev, &tzone_ops, &pkg_temp_tz_params, 0, 0); if (IS_ERR(pkgdev->tzone)) { err = PTR_ERR(pkgdev->tzone); - goto err_ret_free; + kfree(pkgdev); + return err; } /* Store MSR value for package thermal interrupt, to restore at exit */ rdmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, &pkgdev->msr_pkg_therm_low, &pkgdev->msr_pkg_therm_high); + spin_lock_irq(&pkg_temp_lock); list_add_tail(&pkgdev->list, &phy_dev_list); - pr_debug("pkg_temp_thermal_device_add :phy_id %d cpu %d\n", - pkgdev->phys_proc_id, cpu); - - mutex_unlock(&phy_dev_list_mutex); - + spin_unlock_irq(&pkg_temp_lock); return 0; - -err_ret_free: - kfree(pkgdev); -err_ret_unlock: - mutex_unlock(&phy_dev_list_mutex); - -err_ret: - return err; } -static int pkg_temp_thermal_device_remove(unsigned int cpu) +static void put_core_offline(unsigned int cpu) { + int target = cpumask_any_but(topology_core_cpumask(cpu), cpu); struct pkg_device *pkgdev = pkg_temp_thermal_get_dev(cpu); - int target; + bool lastcpu; if (!pkgdev) - return -ENODEV; + return; - mutex_lock(&phy_dev_list_mutex); + lastcpu = target >= nr_cpu_ids; + /* + * Remove the sysfs files, if this is the last cpu in the package + * before doing further cleanups. + */ + if (lastcpu) { + struct thermal_zone_device *tzone = pkgdev->tzone; + + /* + * We must protect against a work function calling + * thermal_zone_update, after/while unregister. We null out + * the pointer under the zone mutex, so the worker function + * won't try to call. + */ + mutex_lock(&thermal_zone_mutex); + pkgdev->tzone = NULL; + mutex_unlock(&thermal_zone_mutex); - target = cpumask_any_but(topology_core_cpumask(cpu), cpu); - /* This might be the last cpu in this package */ - if (target >= nr_cpu_ids) { - thermal_zone_device_unregister(pkgdev->tzone); + thermal_zone_device_unregister(tzone); + } + + /* + * If this is the last CPU in the package, restore the interrupt + * MSR and remove the package reference from the array. + */ + if (lastcpu) { + /* Protect against work and interrupts */ + spin_lock_irq(&pkg_temp_lock); + list_del(&pkgdev->list); /* - * Restore original MSR value for package thermal - * interrupt. + * After this point nothing touches the MSR anymore. We + * must drop the lock to make the cross cpu call. This goes + * away once we move that code to the hotplug state machine. */ + spin_unlock_irq(&pkg_temp_lock); wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, pkgdev->msr_pkg_therm_low, pkgdev->msr_pkg_therm_high); - list_del(&pkgdev->list); kfree(pkgdev); - } else if (pkgdev->cpu == cpu) { - pkgdev->cpu = target; } - mutex_unlock(&phy_dev_list_mutex); - - return 0; + /* + * Note, this is broken when work was really scheduled on the + * outgoing cpu because this will leave the work_scheduled flag set + * and the thermal interrupts disabled. Will be fixed in the next + * step as there is no way to fix it in a sane way with the per cpu + * work nonsense. + */ + cancel_delayed_work_sync(&per_cpu(pkg_temp_thermal_threshold_work, cpu)); } static int get_core_online(unsigned int cpu) @@ -480,15 +496,6 @@ static int get_core_online(unsigned int return pkg_temp_thermal_device_add(cpu); } -static void put_core_offline(unsigned int cpu) -{ - if (!pkg_temp_thermal_device_remove(cpu)) - cancel_delayed_work_sync( - &per_cpu(pkg_temp_thermal_threshold_work, cpu)); - - pr_debug("put_core_offline: cpu %d\n", cpu); -} - static int pkg_temp_thermal_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu) { @@ -523,8 +530,6 @@ static int __init pkg_temp_thermal_init( if (!x86_match_cpu(pkg_temp_thermal_ids)) return -ENODEV; - spin_lock_init(&pkg_work_lock); - cpu_notifier_register_begin(); for_each_online_cpu(i) if (get_core_online(i)) @@ -550,7 +555,6 @@ module_init(pkg_temp_thermal_init) static void __exit pkg_temp_thermal_exit(void) { - struct pkg_device *pkgdev, *n; int i; platform_thermal_package_notify = NULL; @@ -558,20 +562,8 @@ static void __exit pkg_temp_thermal_exit cpu_notifier_register_begin(); __unregister_hotcpu_notifier(&pkg_temp_thermal_notifier); - mutex_lock(&phy_dev_list_mutex); - list_for_each_entry_safe(pkgdev, n, &phy_dev_list, list) { - /* Retore old MSR value for package thermal interrupt */ - wrmsr_on_cpu(pkgdev->cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, - pkgdev->msr_pkg_therm_low, - pkgdev->msr_pkg_therm_high); - thermal_zone_device_unregister(pkgdev->tzone); - list_del(&pkgdev->list); - kfree(pkgdev); - } - mutex_unlock(&phy_dev_list_mutex); for_each_online_cpu(i) - cancel_delayed_work_sync( - &per_cpu(pkg_temp_thermal_threshold_work, i)); + put_core_offline(i); cpu_notifier_register_done(); kfree(pkg_work_scheduled);