From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754353AbdKAJCh (ORCPT ); Wed, 1 Nov 2017 05:02:37 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:55419 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753393AbdKAJCS (ORCPT ); Wed, 1 Nov 2017 05:02:18 -0400 X-Google-Smtp-Source: ABhQp+QF57E2swl90qemOz80Bo3spmXInVTQ+UAFqgG+wfqulLBs7Y4/9sJFXqr9sWB0Ywg3PFKqaA== Date: Wed, 1 Nov 2017 10:02:14 +0100 From: Ingo Molnar To: Zhou Chengming Cc: mhiramat@kernel.org, rostedt@goodmis.org, hpa@zytor.com, peterz@infradead.org, mingo@redhat.com, davem@davemloft.net, bp@suse.de, jkosina@suse.cz, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] kprobes, x86/alternatives: use text_mutex to protect smp_alt_modules Message-ID: <20171101090214.mo7tuvkfvsca2ik5@gmail.com> References: <1509502822-139988-1-git-send-email-zhouchengming1@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1509502822-139988-1-git-send-email-zhouchengming1@huawei.com> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Zhou Chengming wrote: > Changes: > - Add a comment about text_mutex protecting this on x86. > > Fixes: 2cfa197 "ftrace/alternatives: Introducing *_text_reserved > functions" > > We use alternatives_text_reserved() to check if the address is in > the fixed pieces of alternative reserved, but the problem is that > we don't hold the smp_alt mutex when call this function. So the list > traversal may encounter a deleted list_head if another path is doing > alternatives_smp_module_del(). > > One solution is that we can hold smp_alt mutex before call this > function, but the difficult point is that the callers of this > functions, arch_prepare_kprobe() and arch_prepare_optimized_kprobe(), > are called inside the text_mutex. So we must hold smp_alt mutex > before we go into these arch dependent code. But we can't now, > the smp_alt mutex is the arch dependent part, only x86 has it. > Maybe we can export another arch dependent callback to solve this. > > But there is a simpler way to handle this problem. We can reuse the > text_mutex to protect smp_alt_modules instead of using another mutex. > And all the arch dependent checks of kprobes are inside the text_mutex, > so it's safe now. > > Reviewed-by: Masami Hiramatsu > Signed-off-by: Zhou Chengming > --- > arch/x86/kernel/alternative.c | 24 +++++++++++------------- > kernel/extable.c | 2 ++ > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 3344d33..55abbaa 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -442,7 +442,6 @@ static void alternatives_smp_lock(const s32 *start, const s32 *end, > { > const s32 *poff; > > - mutex_lock(&text_mutex); > for (poff = start; poff < end; poff++) { > u8 *ptr = (u8 *)poff + *poff; > > @@ -452,7 +451,6 @@ static void alternatives_smp_lock(const s32 *start, const s32 *end, > if (*ptr == 0x3e) > text_poke(ptr, ((unsigned char []){0xf0}), 1); > } > - mutex_unlock(&text_mutex); > } > > static void alternatives_smp_unlock(const s32 *start, const s32 *end, > @@ -460,7 +458,6 @@ static void alternatives_smp_unlock(const s32 *start, const s32 *end, > { > const s32 *poff; > > - mutex_lock(&text_mutex); > for (poff = start; poff < end; poff++) { > u8 *ptr = (u8 *)poff + *poff; > > @@ -470,7 +467,6 @@ static void alternatives_smp_unlock(const s32 *start, const s32 *end, > if (*ptr == 0xf0) > text_poke(ptr, ((unsigned char []){0x3E}), 1); > } > - mutex_unlock(&text_mutex); > } > > struct smp_alt_module { > @@ -489,8 +485,7 @@ struct smp_alt_module { > struct list_head next; > }; > static LIST_HEAD(smp_alt_modules); > -static DEFINE_MUTEX(smp_alt); > -static bool uniproc_patched = false; /* protected by smp_alt */ > +static bool uniproc_patched = false; /* protected by text_mutex */ > > void __init_or_module alternatives_smp_module_add(struct module *mod, > char *name, > @@ -499,7 +494,7 @@ void __init_or_module alternatives_smp_module_add(struct module *mod, > { > struct smp_alt_module *smp; > > - mutex_lock(&smp_alt); > + mutex_lock(&text_mutex); > if (!uniproc_patched) > goto unlock; > > @@ -526,14 +521,14 @@ void __init_or_module alternatives_smp_module_add(struct module *mod, > smp_unlock: > alternatives_smp_unlock(locks, locks_end, text, text_end); > unlock: > - mutex_unlock(&smp_alt); > + mutex_unlock(&text_mutex); > } > > void __init_or_module alternatives_smp_module_del(struct module *mod) > { > struct smp_alt_module *item; > > - mutex_lock(&smp_alt); > + mutex_lock(&text_mutex); > list_for_each_entry(item, &smp_alt_modules, next) { > if (mod != item->mod) > continue; > @@ -541,7 +536,7 @@ void __init_or_module alternatives_smp_module_del(struct module *mod) > kfree(item); > break; > } > - mutex_unlock(&smp_alt); > + mutex_unlock(&text_mutex); > } > > void alternatives_enable_smp(void) > @@ -551,7 +546,7 @@ void alternatives_enable_smp(void) > /* Why bother if there are no other CPUs? */ > BUG_ON(num_possible_cpus() == 1); > > - mutex_lock(&smp_alt); > + mutex_lock(&text_mutex); > > if (uniproc_patched) { > pr_info("switching to SMP code\n"); > @@ -563,10 +558,13 @@ void alternatives_enable_smp(void) > mod->text, mod->text_end); > uniproc_patched = false; > } > - mutex_unlock(&smp_alt); > + mutex_unlock(&text_mutex); > } > > -/* Return 1 if the address range is reserved for smp-alternatives */ > +/* > + * Return 1 if the address range is reserved for smp-alternatives. > + * Must hold text_mutex. > + */ > int alternatives_text_reserved(void *start, void *end) > { Please also add this as a lockdep_assert_held() check. Thanks, Ingo