From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A4DBC388F2 for ; Tue, 3 Nov 2020 11:23:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2696422264 for ; Tue, 3 Nov 2020 11:23:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1604402587; bh=miw1KmiC65f8YnprkC3dEffuvVzO9uPuxKpkGSzVRd4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=LBnQvnKegxzBgQ/g+xk/v9Ll2CI8uvarc+A3GQ17R+A7M3V97drt0TQqHqt+2XsT+ mUgcWu7VFqSUxQFZKR0dRVmKNk3+hTsH2uYWW2naPr8uhmazJv+K3PSWsI2tRI4l0H T/bVfqSGFkxDzdWBIhTuyjoQHB3DtPXAoRDyzbUI= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728531AbgKCLXG (ORCPT ); Tue, 3 Nov 2020 06:23:06 -0500 Received: from mail.kernel.org ([198.145.29.99]:45382 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728092AbgKCLXF (ORCPT ); Tue, 3 Nov 2020 06:23:05 -0500 Received: from devnote2 (NE2965lan1.rev.em-net.ne.jp [210.141.244.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9561D20731; Tue, 3 Nov 2020 11:22:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1604402584; bh=miw1KmiC65f8YnprkC3dEffuvVzO9uPuxKpkGSzVRd4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=RbJ7yISctA2KuiVcvuzg/xkeLNmB1YRY1HOsjzx9flqELBpJT1V9eFqxkOc8y30nl f9AVijA5fewrw/jyjA/6vkbIYD43mSjj+o9xEcbJcu99j9cYQ7JyXEnkfrbMcLtlkq iB4AtvCZiedYkRfwXzZKpJ9seDNbEaP4BeKC2E9w= Date: Tue, 3 Nov 2020 20:22:57 +0900 From: Masami Hiramatsu To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, Masami Hiramatsu , Andrew Morton , Peter Zijlstra , Ingo Molnar , Josh Poimboeuf , Jiri Kosina , Miroslav Benes , Petr Mladek , Guo Ren , "James E.J. Bottomley" , Helge Deller , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Heiko Carstens , Vasily Gorbik , Christian Borntraeger , Thomas Gleixner , Borislav Petkov , x86@kernel.org, "H. Peter Anvin" , "Naveen N. Rao" , Anil S Keshavamurthy , "David S. Miller" , linux-csky@vger.kernel.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org Subject: Re: [PATCH 05/11 v2] kprobes/ftrace: Add recursion protection to the ftrace callback Message-Id: <20201103202257.029364fd78492fd8efc360dc@kernel.org> In-Reply-To: <20201030214013.824581418@goodmis.org> References: <20201030213142.096102821@goodmis.org> <20201030214013.824581418@goodmis.org> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 30 Oct 2020 17:31:47 -0400 Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > If a ftrace callback does not supply its own recursion protection and > does not set the RECURSION_SAFE flag in its ftrace_ops, then ftrace will > make a helper trampoline to do so before calling the callback instead of > just calling the callback directly. > > The default for ftrace_ops is going to change. It will expect that handlers > provide their own recursion protection, unless its ftrace_ops states > otherwise. > > Link: https://lkml.kernel.org/r/20201028115613.140212174@goodmis.org > > Cc: Andrew Morton > Cc: Masami Hiramatsu > Cc: Guo Ren > Cc: "James E.J. Bottomley" > Cc: Helge Deller > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Heiko Carstens > Cc: Vasily Gorbik > Cc: Christian Borntraeger > Cc: Thomas Gleixner > Cc: Borislav Petkov > Cc: x86@kernel.org > Cc: "H. Peter Anvin" > Cc: "Naveen N. Rao" > Cc: Anil S Keshavamurthy > Cc: "David S. Miller" > Cc: linux-csky@vger.kernel.org > Cc: linux-parisc@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-s390@vger.kernel.org > Signed-off-by: Steven Rostedt (VMware) > --- > arch/csky/kernel/probes/ftrace.c | 12 ++++++++++-- > arch/parisc/kernel/ftrace.c | 13 +++++++++++-- > arch/powerpc/kernel/kprobes-ftrace.c | 11 ++++++++++- > arch/s390/kernel/ftrace.c | 13 +++++++++++-- > arch/x86/kernel/kprobes/ftrace.c | 12 ++++++++++-- > 5 files changed, 52 insertions(+), 9 deletions(-) > > diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c > index 5264763d05be..5eb2604fdf71 100644 > --- a/arch/csky/kernel/probes/ftrace.c > +++ b/arch/csky/kernel/probes/ftrace.c > @@ -13,16 +13,21 @@ int arch_check_ftrace_location(struct kprobe *p) > void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *ops, struct pt_regs *regs) > { > + int bit; > bool lr_saver = false; > struct kprobe *p; > struct kprobe_ctlblk *kcb; > > - /* Preempt is disabled by ftrace */ > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > + return; > + > + preempt_disable_notrace(); > p = get_kprobe((kprobe_opcode_t *)ip); > if (!p) { > p = get_kprobe((kprobe_opcode_t *)(ip - MCOUNT_INSN_SIZE)); > if (unlikely(!p) || kprobe_disabled(p)) > - return; > + goto out; > lr_saver = true; > } > > @@ -56,6 +61,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > */ > __this_cpu_write(current_kprobe, NULL); > } > +out: > + preempt_enable_notrace(); > + ftrace_test_recursion_unlock(bit); > } > NOKPROBE_SYMBOL(kprobe_ftrace_handler); > > diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c > index 63e3ecb9da81..4b1fdf15662c 100644 > --- a/arch/parisc/kernel/ftrace.c > +++ b/arch/parisc/kernel/ftrace.c > @@ -208,13 +208,19 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > { > struct kprobe_ctlblk *kcb; > struct kprobe *p = get_kprobe((kprobe_opcode_t *)ip); > + int bit; > > - if (unlikely(!p) || kprobe_disabled(p)) > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > return; > > + preempt_disable_notrace(); If we disable preempt here, we also move the get_kprobe() here as below. (get_kprobe() accesses percpu variable) p = get_kprobe((kprobe_opcode_t *)ip); > + if (unlikely(!p) || kprobe_disabled(p)) > + goto out; > + > if (kprobe_running()) { > kprobes_inc_nmissed_count(p); > - return; > + goto out; > } > > __this_cpu_write(current_kprobe, p); > @@ -235,6 +241,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > } > } > __this_cpu_write(current_kprobe, NULL); > +out: > + preempt_enable_notrace(); > + ftrace_test_recursion_unlock(bit); > } > NOKPROBE_SYMBOL(kprobe_ftrace_handler); > > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c > index 972cb28174b2..5df8d50c65ae 100644 > --- a/arch/powerpc/kernel/kprobes-ftrace.c > +++ b/arch/powerpc/kernel/kprobes-ftrace.c > @@ -18,10 +18,16 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, > { > struct kprobe *p; > struct kprobe_ctlblk *kcb; > + int bit; > > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > + return; > + > + preempt_disable_notrace(); > p = get_kprobe((kprobe_opcode_t *)nip); > if (unlikely(!p) || kprobe_disabled(p)) > - return; > + goto out; > > kcb = get_kprobe_ctlblk(); > if (kprobe_running()) { > @@ -52,6 +58,9 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, > */ > __this_cpu_write(current_kprobe, NULL); > } > +out: > + preempt_enable_notrace(); > + ftrace_test_recursion_unlock(bit); > } > NOKPROBE_SYMBOL(kprobe_ftrace_handler); > > diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c > index b388e87a08bf..88466d7fb6b2 100644 > --- a/arch/s390/kernel/ftrace.c > +++ b/arch/s390/kernel/ftrace.c > @@ -202,13 +202,19 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > { > struct kprobe_ctlblk *kcb; > struct kprobe *p = get_kprobe((kprobe_opcode_t *)ip); > + int bit; > > - if (unlikely(!p) || kprobe_disabled(p)) > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > return; > > + preempt_disable_notrace(); Ditto. Others look good to me. Thank you, > + if (unlikely(!p) || kprobe_disabled(p)) > + goto out; > + > if (kprobe_running()) { > kprobes_inc_nmissed_count(p); > - return; > + goto out; > } > > __this_cpu_write(current_kprobe, p); > @@ -228,6 +234,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > } > } > __this_cpu_write(current_kprobe, NULL); > +out: > + preempt_enable_notrace(); > + ftrace_test_recursion_unlock(bit); > } > NOKPROBE_SYMBOL(kprobe_ftrace_handler); > > diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c > index 681a4b36e9bb..a40a6cdfcca3 100644 > --- a/arch/x86/kernel/kprobes/ftrace.c > +++ b/arch/x86/kernel/kprobes/ftrace.c > @@ -18,11 +18,16 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > { > struct kprobe *p; > struct kprobe_ctlblk *kcb; > + int bit; > > - /* Preempt is disabled by ftrace */ > + bit = ftrace_test_recursion_trylock(); > + if (bit < 0) > + return; > + > + preempt_disable_notrace(); > p = get_kprobe((kprobe_opcode_t *)ip); > if (unlikely(!p) || kprobe_disabled(p)) > - return; > + goto out; > > kcb = get_kprobe_ctlblk(); > if (kprobe_running()) { > @@ -52,6 +57,9 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip, > */ > __this_cpu_write(current_kprobe, NULL); > } > +out: > + preempt_enable_notrace(); > + ftrace_test_recursion_unlock(bit); > } > NOKPROBE_SYMBOL(kprobe_ftrace_handler); > > -- > 2.28.0 > > -- Masami Hiramatsu