From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f70.google.com (mail-wm0-f70.google.com [74.125.82.70]) by kanga.kvack.org (Postfix) with ESMTP id 238A56B0253 for ; Fri, 2 Dec 2016 10:19:40 -0500 (EST) Received: by mail-wm0-f70.google.com with SMTP id y16so3641446wmd.6 for ; Fri, 02 Dec 2016 07:19:40 -0800 (PST) Received: from mail-wj0-f194.google.com (mail-wj0-f194.google.com. [209.85.210.194]) by mx.google.com with ESMTPS id ce10si5733494wjd.29.2016.12.02.07.19.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 02 Dec 2016 07:19:37 -0800 (PST) Received: by mail-wj0-f194.google.com with SMTP id xy5so30642390wjc.1 for ; Fri, 02 Dec 2016 07:19:37 -0800 (PST) Date: Fri, 2 Dec 2016 16:19:36 +0100 From: Michal Hocko Subject: [PATCH] hotplug: make register and unregister notifier API symmetric Message-ID: <20161202151935.GR6830@dhcp22.suse.cz> References: <1480540516-6458-1-git-send-email-yuzhao@google.com> <20161202134604.GA6837@dhcp22.suse.cz> <20161202143848.GP6830@dhcp22.suse.cz> <20161202144440.GQ6830@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Dan Streetman Cc: Yu Zhao , Seth Jennings , Linux-MM , linux-kernel , Avi Kivity , Thomas Gleixner , Ingo Molnar [Let's CC more people - the thread started http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com] On Fri 02-12-16 09:56:26, Dan Streetman wrote: > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko wrote: > > On Fri 02-12-16 15:38:48, Michal Hocko wrote: > >> On Fri 02-12-16 09:24:35, Dan Streetman wrote: > >> > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko wrote: > >> > > On Wed 30-11-16 13:15:16, Yu Zhao wrote: > >> > >> __unregister_cpu_notifier() only removes registered notifier from its > >> > >> linked list when CPU hotplug is configured. If we free registered CPU > >> > >> notifier when HOTPLUG_CPU=n, we corrupt the linked list. > >> > >> > >> > >> To fix the problem, we can either use a static CPU notifier that walks > >> > >> through each pool or just simply disable CPU notifier when CPU hotplug > >> > >> is not configured (which is perfectly safe because the code in question > >> > >> is called after all possible CPUs are online and will remain online > >> > >> until power off). > >> > >> > >> > >> v2: #ifdef for cpu_notifier_register_done during cleanup. > >> > > > >> > > this ifedfery is just ugly as hell. I am also wondering whether it is > >> > > really needed. __register_cpu_notifier and __unregister_cpu_notifier are > >> > > noops for CONFIG_HOTPLUG_CPU=n. So what's exactly that is broken here? > >> > > >> > hmm, that's interesting, __unregister_cpu_notifier is always a noop if > >> > HOTPLUG_CPU=n, but __register_cpu_notifier is only a noop if > >> > HOTPLUG_CPU=n *and* MODULE. If !MODULE, __register_cpu_notifier does > >> > >> OK, I've missed the MODULE part > >> > >> > actually register! This was added by commit > >> > 47e627bc8c9a70392d2049e6af5bd55fae61fe53 ('hotplug: Allow modules to > >> > use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU') and looks > >> > like it's to allow built-ins to register so they can notice during > >> > boot when cpus are initialized. > >> > >> I cannot say I wound understand the motivation but that is not really > >> all that important. > >> > >> > IMHO, that is the real problem - sure, without HOTPLUG_CPU, nobody > >> > should ever get a notification that a cpu is dying, but that doesn't > >> > mean builtins that register notifiers will never unregister their > >> > notifiers and then free them. > >> > >> Yes that is true. That suggests that __unregister_cpu_notifier should > >> the the symmetric thing to the __register_cpu_notifier for > >> CONFIG_MODULE, right? > > > > I meant the following. Completely untested > > agreed, but also needs the non-__ version, and kernel/cpu.c needs > tweaking to move those functions out of the #ifdef CONFIG_HOTPLUG_CPU > section. OK, this is still only compile tested. Yu Zhao, assuming you were able to trigger the original problem could you test with the below patch please? --- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755027AbcLBPTk (ORCPT ); Fri, 2 Dec 2016 10:19:40 -0500 Received: from mail-wj0-f196.google.com ([209.85.210.196]:36778 "EHLO mail-wj0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751201AbcLBPTj (ORCPT ); Fri, 2 Dec 2016 10:19:39 -0500 Date: Fri, 2 Dec 2016 16:19:36 +0100 From: Michal Hocko To: Dan Streetman Cc: Yu Zhao , Seth Jennings , Linux-MM , linux-kernel , Avi Kivity , Thomas Gleixner , Ingo Molnar Subject: [PATCH] hotplug: make register and unregister notifier API symmetric Message-ID: <20161202151935.GR6830@dhcp22.suse.cz> References: <1480540516-6458-1-git-send-email-yuzhao@google.com> <20161202134604.GA6837@dhcp22.suse.cz> <20161202143848.GP6830@dhcp22.suse.cz> <20161202144440.GQ6830@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [Let's CC more people - the thread started http://lkml.kernel.org/r/1480540516-6458-1-git-send-email-yuzhao@google.com] On Fri 02-12-16 09:56:26, Dan Streetman wrote: > On Fri, Dec 2, 2016 at 9:44 AM, Michal Hocko wrote: > > On Fri 02-12-16 15:38:48, Michal Hocko wrote: > >> On Fri 02-12-16 09:24:35, Dan Streetman wrote: > >> > On Fri, Dec 2, 2016 at 8:46 AM, Michal Hocko wrote: > >> > > On Wed 30-11-16 13:15:16, Yu Zhao wrote: > >> > >> __unregister_cpu_notifier() only removes registered notifier from its > >> > >> linked list when CPU hotplug is configured. If we free registered CPU > >> > >> notifier when HOTPLUG_CPU=n, we corrupt the linked list. > >> > >> > >> > >> To fix the problem, we can either use a static CPU notifier that walks > >> > >> through each pool or just simply disable CPU notifier when CPU hotplug > >> > >> is not configured (which is perfectly safe because the code in question > >> > >> is called after all possible CPUs are online and will remain online > >> > >> until power off). > >> > >> > >> > >> v2: #ifdef for cpu_notifier_register_done during cleanup. > >> > > > >> > > this ifedfery is just ugly as hell. I am also wondering whether it is > >> > > really needed. __register_cpu_notifier and __unregister_cpu_notifier are > >> > > noops for CONFIG_HOTPLUG_CPU=n. So what's exactly that is broken here? > >> > > >> > hmm, that's interesting, __unregister_cpu_notifier is always a noop if > >> > HOTPLUG_CPU=n, but __register_cpu_notifier is only a noop if > >> > HOTPLUG_CPU=n *and* MODULE. If !MODULE, __register_cpu_notifier does > >> > >> OK, I've missed the MODULE part > >> > >> > actually register! This was added by commit > >> > 47e627bc8c9a70392d2049e6af5bd55fae61fe53 ('hotplug: Allow modules to > >> > use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU') and looks > >> > like it's to allow built-ins to register so they can notice during > >> > boot when cpus are initialized. > >> > >> I cannot say I wound understand the motivation but that is not really > >> all that important. > >> > >> > IMHO, that is the real problem - sure, without HOTPLUG_CPU, nobody > >> > should ever get a notification that a cpu is dying, but that doesn't > >> > mean builtins that register notifiers will never unregister their > >> > notifiers and then free them. > >> > >> Yes that is true. That suggests that __unregister_cpu_notifier should > >> the the symmetric thing to the __register_cpu_notifier for > >> CONFIG_MODULE, right? > > > > I meant the following. Completely untested > > agreed, but also needs the non-__ version, and kernel/cpu.c needs > tweaking to move those functions out of the #ifdef CONFIG_HOTPLUG_CPU > section. OK, this is still only compile tested. Yu Zhao, assuming you were able to trigger the original problem could you test with the below patch please? --- >>From c812fe4e519914aa37f092d3a0321038fadcdde7 Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Fri, 2 Dec 2016 16:06:56 +0100 Subject: [PATCH] hotplug: make register and unregister notifier API symmetric Yu Zhao has noticed that __unregister_cpu_notifier only unregisters its notifiers when HOTPLUG_CPU=y while the registration might succeed even when HOTPLUG_CPU=n if MODULE is enabled. This means that e.g. zswap might keep a stale notifier on the list on the manual clean up during the pool tear down and thus corrupt the list. Fix this issue by making unregister APIs symmetric to the register so there are no surprises. Fixes: 47e627bc8c9a ("[PATCH] hotplug: Allow modules to use the cpu hotplug notifiers even if !CONFIG_HOTPLUG_CPU") Cc: stable # zswap needs it 4.3+ Reported-by: Yu Zhao Signed-off-by: Michal Hocko --- include/linux/cpu.h | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/include/linux/cpu.h b/include/linux/cpu.h index 797d9c8e9a1b..c8938eb21e34 100644 --- a/include/linux/cpu.h +++ b/include/linux/cpu.h @@ -105,22 +105,16 @@ extern bool cpuhp_tasks_frozen; { .notifier_call = fn, .priority = pri }; \ __register_cpu_notifier(&fn##_nb); \ } -#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */ -#define cpu_notifier(fn, pri) do { (void)(fn); } while (0) -#define __cpu_notifier(fn, pri) do { (void)(fn); } while (0) -#endif /* #else #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */ -#ifdef CONFIG_HOTPLUG_CPU extern int register_cpu_notifier(struct notifier_block *nb); extern int __register_cpu_notifier(struct notifier_block *nb); extern void unregister_cpu_notifier(struct notifier_block *nb); extern void __unregister_cpu_notifier(struct notifier_block *nb); -#else -#ifndef MODULE -extern int register_cpu_notifier(struct notifier_block *nb); -extern int __register_cpu_notifier(struct notifier_block *nb); -#else +#else /* #if defined(CONFIG_HOTPLUG_CPU) || !defined(MODULE) */ +#define cpu_notifier(fn, pri) do { (void)(fn); } while (0) +#define __cpu_notifier(fn, pri) do { (void)(fn); } while (0) + static inline int register_cpu_notifier(struct notifier_block *nb) { return 0; @@ -130,7 +124,6 @@ static inline int __register_cpu_notifier(struct notifier_block *nb) { return 0; } -#endif static inline void unregister_cpu_notifier(struct notifier_block *nb) { -- 2.10.2 -- Michal Hocko SUSE Labs