* [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
[not found] <20201028115244.995788961@goodmis.org>
@ 2020-10-28 11:52 ` Steven Rostedt
2020-10-29 7:58 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Steven Rostedt @ 2020-10-28 11:52 UTC (permalink / raw)
To: linux-kernel
Cc: James E.J. Bottomley, Guo Ren, linux-csky, H. Peter Anvin,
linux-s390, Helge Deller, x86, Anil S Keshavamurthy,
Christian Borntraeger, Naveen N. Rao, Vasily Gorbik,
Heiko Carstens, Borislav Petkov, Thomas Gleixner, linux-parisc,
Masami Hiramatsu, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
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 assume recursion protection unless
otherwise specified.
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 4bab21c71055..5f7742b225a5 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 (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();
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
2020-10-28 11:52 ` [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
@ 2020-10-29 7:58 ` Masami Hiramatsu
2020-10-29 13:40 ` Steven Rostedt
0 siblings, 1 reply; 4+ messages in thread
From: Masami Hiramatsu @ 2020-10-29 7:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: James E.J. Bottomley, Guo Ren, linux-csky, H. Peter Anvin,
linux-s390, Helge Deller, x86, Anil S Keshavamurthy,
Christian Borntraeger, Naveen N. Rao, Vasily Gorbik,
Heiko Carstens, Borislav Petkov, Thomas Gleixner, linux-parisc,
linux-kernel, Masami Hiramatsu, Paul Mackerras, Andrew Morton,
linuxppc-dev, David S. Miller
Hi Steve,
On Wed, 28 Oct 2020 07:52:49 -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.
So in that case the handlers will be called without preempt disabled?
> The default for ftrace_ops is going to assume recursion protection unless
> otherwise specified.
This seems to skip entier handler if ftrace finds recursion.
I would like to increment the missed counter even in that case.
[...]
e.g.
> 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();
> +
> + 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;
> }
if (bit < 0) {
kprobes_inc_nmissed_count(p);
goto out;
}
>
> @@ -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();
if (bit >= 0)
ftrace_test_recursion_unlock(bit);
> }
> NOKPROBE_SYMBOL(kprobe_ftrace_handler);
>
Or, we can also introduce a support function,
static inline void kprobes_inc_nmissed_ip(unsigned long ip)
{
struct kprobe *p;
preempt_disable_notrace();
p = get_kprobe((kprobe_opcode_t *)ip);
if (p)
kprobes_inc_nmissed_count(p);
preempt_enable_notrace();
}
> diff --git a/arch/parisc/kernel/ftrace.c b/arch/parisc/kernel/ftrace.c
> index 4bab21c71055..5f7742b225a5 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);
(BTW, here is a bug... get_kprobe() must be called with preempt disabled.)
> + int bit;
>
> - if (unlikely(!p) || kprobe_disabled(p))
> + bit = ftrace_test_recursion_trylock();
if (bit < 0) {
kprobes_inc_nmissed_ip(ip);
> return;
}
This may easier for you ?
Thank you,
>
> + preempt_disable_notrace();
> + 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();
> + 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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
2020-10-29 7:58 ` Masami Hiramatsu
@ 2020-10-29 13:40 ` Steven Rostedt
2020-11-02 5:08 ` Masami Hiramatsu
0 siblings, 1 reply; 4+ messages in thread
From: Steven Rostedt @ 2020-10-29 13:40 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: James E.J. Bottomley, Guo Ren, linux-csky, H. Peter Anvin,
linux-s390, Helge Deller, x86, Anil S Keshavamurthy,
Christian Borntraeger, Naveen N. Rao, Vasily Gorbik,
Heiko Carstens, Borislav Petkov, Thomas Gleixner, linux-parisc,
linux-kernel, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
On Thu, 29 Oct 2020 16:58:03 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Hi Steve,
>
> On Wed, 28 Oct 2020 07:52:49 -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.
>
> So in that case the handlers will be called without preempt disabled?
>
>
> > The default for ftrace_ops is going to assume recursion protection unless
> > otherwise specified.
>
> This seems to skip entier handler if ftrace finds recursion.
> I would like to increment the missed counter even in that case.
Note, this code does not change the functionality at this point, because
without having the FL_RECURSION flag set (which kprobes does not even in
this patch), it always gets called from the helper function that does this:
bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX);
if (bit < 0)
return;
preempt_disable_notrace();
op->func(ip, parent_ip, op, regs);
preempt_enable_notrace();
trace_clear_recursion(bit);
Where this function gets called by op->func().
In other words, you don't get that count anyway, and I don't think you want
it. Because it means you traced something that your callback calls.
That bit check is basically a nop, because the last patch in this series
will make the default that everything has recursion protection, but at this
patch the test does this:
/* A previous recursion check was made */
if ((val & TRACE_CONTEXT_MASK) > max)
return 0;
Which would always return true, because this function is called via the
helper that already did the trace_test_and_set_recursion() which, if it
made it this far, the val would always be greater than max.
>
> [...]
> e.g.
>
> > 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();
>
> > +
> > + 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;
> > }
>
> if (bit < 0) {
> kprobes_inc_nmissed_count(p);
> goto out;
> }
If anything called in get_kprobe() or kprobes_inc_nmissed_count() gets
traced here, you have zero recursion protection, and this will crash the
machine with a likely reboot (triple fault).
Note, the recursion handles interrupts and wont stop them. bit < 0 only
happens if you recurse because this function called something that ends up
calling itself. Really, why would you care about missing a kprobe on the
same kprobe?
-- Steve
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
2020-10-29 13:40 ` Steven Rostedt
@ 2020-11-02 5:08 ` Masami Hiramatsu
0 siblings, 0 replies; 4+ messages in thread
From: Masami Hiramatsu @ 2020-11-02 5:08 UTC (permalink / raw)
To: Steven Rostedt
Cc: James E.J. Bottomley, Guo Ren, linux-csky, H. Peter Anvin,
linux-s390, Helge Deller, x86, Anil S Keshavamurthy,
Christian Borntraeger, Naveen N. Rao, Vasily Gorbik,
Heiko Carstens, Borislav Petkov, Thomas Gleixner, linux-parisc,
linux-kernel, Paul Mackerras, Andrew Morton, linuxppc-dev,
David S. Miller
On Thu, 29 Oct 2020 09:40:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 29 Oct 2020 16:58:03 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > Hi Steve,
> >
> > On Wed, 28 Oct 2020 07:52:49 -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.
> >
> > So in that case the handlers will be called without preempt disabled?
> >
> >
> > > The default for ftrace_ops is going to assume recursion protection unless
> > > otherwise specified.
> >
> > This seems to skip entier handler if ftrace finds recursion.
> > I would like to increment the missed counter even in that case.
>
> Note, this code does not change the functionality at this point, because
> without having the FL_RECURSION flag set (which kprobes does not even in
> this patch), it always gets called from the helper function that does this:
>
> bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX);
> if (bit < 0)
> return;
>
> preempt_disable_notrace();
>
> op->func(ip, parent_ip, op, regs);
>
> preempt_enable_notrace();
> trace_clear_recursion(bit);
>
> Where this function gets called by op->func().
>
> In other words, you don't get that count anyway, and I don't think you want
> it. Because it means you traced something that your callback calls.
Got it. So nmissed count increment will be an improvement.
>
> That bit check is basically a nop, because the last patch in this series
> will make the default that everything has recursion protection, but at this
> patch the test does this:
>
> /* A previous recursion check was made */
> if ((val & TRACE_CONTEXT_MASK) > max)
> return 0;
>
> Which would always return true, because this function is called via the
> helper that already did the trace_test_and_set_recursion() which, if it
> made it this far, the val would always be greater than max.
OK, let me check the last patch too.
>
> >
> > [...]
> > e.g.
> >
> > > 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();
> >
> > > +
> > > + 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;
> > > }
> >
> > if (bit < 0) {
> > kprobes_inc_nmissed_count(p);
> > goto out;
> > }
>
> If anything called in get_kprobe() or kprobes_inc_nmissed_count() gets
> traced here, you have zero recursion protection, and this will crash the
> machine with a likely reboot (triple fault).
Oops, ok, those can be traced.
>
> Note, the recursion handles interrupts and wont stop them. bit < 0 only
> happens if you recurse because this function called something that ends up
> calling itself. Really, why would you care about missing a kprobe on the
> same kprobe?
Usually, sw-breakpoint based kprobes will count that case. Moreover, kprobes
shares one ftrace_ops among all kprobes. I guess in that case any kprobes
in kprobes (e.g. recursive call inside kprobe pre_handlers) will be skipped
by ftrace_test_recursion_trylock(), is that correct?
Thank you,
>
> -- Steve
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-02 5:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201028115244.995788961@goodmis.org>
2020-10-28 11:52 ` [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
2020-10-29 7:58 ` Masami Hiramatsu
2020-10-29 13:40 ` Steven Rostedt
2020-11-02 5:08 ` Masami Hiramatsu
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).