From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
Guo Ren <guoren@kernel.org>,
linux-csky@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
Miroslav Benes <mbenes@suse.cz>, Ingo Molnar <mingo@kernel.org>,
linux-s390@vger.kernel.org, Helge Deller <deller@gmx.de>,
x86@kernel.org,
Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
Christian Borntraeger <borntraeger@de.ibm.com>,
"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
Petr Mladek <pmladek@suse.com>, Vasily Gorbik <gor@linux.ibm.com>,
Heiko Carstens <hca@linux.ibm.com>,
Jiri Kosina <jikos@kernel.org>, Borislav Petkov <bp@alien8.de>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
linux-parisc@vger.kernel.org, linux-kernel@vger.kernel.org,
Masami Hiramatsu <mhiramat@kernel.org>,
Paul Mackerras <paulus@samba.org>,
Andrew Morton <akpm@linux-foundation.org>,
linuxppc-dev@lists.ozlabs.org,
"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH 05/11 v2] kprobes/ftrace: Add recursion protection to the ftrace callback
Date: Tue, 3 Nov 2020 20:22:57 +0900 [thread overview]
Message-ID: <20201103202257.029364fd78492fd8efc360dc@kernel.org> (raw)
In-Reply-To: <20201030214013.824581418@goodmis.org>
On Fri, 30 Oct 2020 17:31:47 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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 <akpm@linux-foundation.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Guo Ren <guoren@kernel.org>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Heiko Carstens <hca@linux.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> 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) <rostedt@goodmis.org>
> ---
> 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 <mhiramat@kernel.org>
next prev parent reply other threads:[~2020-11-03 13:37 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20201030213142.096102821@goodmis.org>
2020-10-30 21:31 ` [PATCH 05/11 v2] kprobes/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
2020-11-03 11:22 ` Masami Hiramatsu [this message]
2020-11-04 18:46 ` [PATCH 05/11 v2.1] " Steven Rostedt
2020-10-30 21:31 ` [PATCH 11/11 v2] ftrace: Add recording of functions that caused recursion Steven Rostedt
2020-11-02 16:41 ` Petr Mladek
2020-11-02 17:09 ` Steven Rostedt
2020-11-02 17:19 ` Steven Rostedt
2020-11-03 10:40 ` Petr Mladek
2020-11-02 17:37 ` Steven Rostedt
2020-11-02 17:46 ` Steven Rostedt
2020-11-02 19:23 ` [PATCH 11/11 v2.2] " Steven Rostedt
2020-11-03 14:10 ` Petr Mladek
2020-11-03 16:14 ` Steven Rostedt
2020-11-04 19:13 ` Steven Rostedt
2020-11-02 19:14 ` [PATCH 11/11 v2.1] " Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201103202257.029364fd78492fd8efc360dc@kernel.org \
--to=mhiramat@kernel.org \
--cc=James.Bottomley@HansenPartnership.com \
--cc=akpm@linux-foundation.org \
--cc=anil.s.keshavamurthy@intel.com \
--cc=borntraeger@de.ibm.com \
--cc=bp@alien8.de \
--cc=davem@davemloft.net \
--cc=deller@gmx.de \
--cc=gor@linux.ibm.com \
--cc=guoren@kernel.org \
--cc=hca@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-csky@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mbenes@suse.cz \
--cc=mingo@kernel.org \
--cc=naveen.n.rao@linux.ibm.com \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).