All of lore.kernel.org
 help / color / mirror / Atom feed
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Guo Ren <guoren@kernel.org>,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Helge Deller <deller@gmx.de>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	"Naveen N. Rao" <naveen.n.rao@linux.ibm.com>,
	Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	linux-csky@vger.kernel.org, linux-parisc@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org
Subject: Re: [PATCH 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
Date: Mon, 2 Nov 2020 14:08:07 +0900	[thread overview]
Message-ID: <20201102140807.05ca1c9e33a96b34d3fffd35@kernel.org> (raw)
In-Reply-To: <20201029094001.0cfab7aa@gandalf.local.home>

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>

WARNING: multiple messages have this Message-ID
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Guo Ren <guoren@kernel.org>,
	linux-csky@vger.kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	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>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	Borislav Petkov <bp@alien8.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-parisc@vger.kernel.org, linux-kernel@vger.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 5/9] kprobes/ftrace: Add recursion protection to the ftrace callback
Date: Mon, 2 Nov 2020 14:08:07 +0900	[thread overview]
Message-ID: <20201102140807.05ca1c9e33a96b34d3fffd35@kernel.org> (raw)
In-Reply-To: <20201029094001.0cfab7aa@gandalf.local.home>

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>

  reply	other threads:[~2020-11-02  5:08 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-28 11:52 [PATCH 0/9] ftrace: Have callbacks handle their own recursion Steven Rostedt
2020-10-28 11:52 ` [PATCH 1/9] ftrace: Move the recursion testing into global headers Steven Rostedt
2020-10-30  9:13   ` Miroslav Benes
2020-10-30 12:30     ` Steven Rostedt
2020-10-28 11:52 ` [PATCH 2/9] ftrace: Add ftrace_test_recursion_trylock() helper function Steven Rostedt
2020-10-28 11:52 ` [PATCH 3/9] ftrace: Optimize testing what context current is in Steven Rostedt
2020-10-28 11:52 ` [PATCH 4/9] pstore/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
2020-10-28 15:59   ` Kees Cook
2020-10-28 11:52 ` [PATCH 5/9] kprobes/ftrace: " Steven Rostedt
2020-10-28 11:52   ` Steven Rostedt
2020-10-29  7:58   ` Masami Hiramatsu
2020-10-29  7:58     ` Masami Hiramatsu
2020-10-29 13:40     ` Steven Rostedt
2020-10-29 13:40       ` Steven Rostedt
2020-11-02  5:08       ` Masami Hiramatsu [this message]
2020-11-02  5:08         ` Masami Hiramatsu
2020-10-28 11:52 ` [PATCH 6/9] livepatch/ftrace: " Steven Rostedt
2020-10-29 13:51   ` Miroslav Benes
2020-10-29 14:37     ` Steven Rostedt
2020-10-30 12:28       ` Steven Rostedt
2020-10-29 14:57     ` Petr Mladek
2020-10-29 15:03       ` Miroslav Benes
2020-10-29 18:24       ` Steven Rostedt
2020-10-30  9:48         ` Miroslav Benes
2020-10-30 10:41           ` Petr Mladek
2020-10-28 11:52 ` [PATCH 7/9] perf/ftrace: " Steven Rostedt
2020-10-28 11:52 ` [PATCH 8/9] perf/ftrace: Check for rcu_is_watching() in callback function Steven Rostedt
2020-10-28 11:52 ` [PATCH 9/9] ftrace: Reverse what the RECURSION flag means in the ftrace_ops 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=20201102140807.05ca1c9e33a96b34d3fffd35@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=akpm@linux-foundation.org \
    --cc=anil.s.keshavamurthy@intel.com \
    --cc=benh@kernel.crashing.org \
    --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=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=mpe@ellerman.id.au \
    --cc=naveen.n.rao@linux.ibm.com \
    --cc=paulus@samba.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.