From: Steven Rostedt <rostedt@goodmis.org> To: Masami Hiramatsu <mhiramat@kernel.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: Thu, 29 Oct 2020 09:40:01 -0400 [thread overview] Message-ID: <20201029094001.0cfab7aa@gandalf.local.home> (raw) In-Reply-To: <20201029165803.5f6b401e5bccca4e57c70181@kernel.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Steven Rostedt <rostedt@goodmis.org> To: Masami Hiramatsu <mhiramat@kernel.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: Thu, 29 Oct 2020 09:40:01 -0400 [thread overview] Message-ID: <20201029094001.0cfab7aa@gandalf.local.home> (raw) In-Reply-To: <20201029165803.5f6b401e5bccca4e57c70181@kernel.org> 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
next prev parent reply other threads:[~2020-10-29 13:40 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 [this message] 2020-10-29 13:40 ` Steven Rostedt 2020-11-02 5:08 ` Masami Hiramatsu 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=20201029094001.0cfab7aa@gandalf.local.home \ --to=rostedt@goodmis.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=mhiramat@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=naveen.n.rao@linux.ibm.com \ --cc=paulus@samba.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: linkBe 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.