From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> To: Masami Hiramatsu <mhiramat@kernel.org>, Joe Perches <joe@perches.com> Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org>, Michael Ellerman <mpe@ellerman.id.au>, Nicholas Piggin <npiggin@gmail.com>, Steven Rostedt <rostedt@goodmis.org> Subject: Re: [PATCH 7/7] powerpc/kprobes: Allow probing on any ftrace address Date: Wed, 26 Jun 2019 15:09:56 +0530 [thread overview] Message-ID: <1561541820.15ifr1qex2.naveen@linux.ibm.com> (raw) In-Reply-To: <20190621235034.acc00fc3e2b2c7e89caa1fd5@kernel.org> Masami Hiramatsu wrote: > On Tue, 18 Jun 2019 20:17:06 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> With KPROBES_ON_FTRACE, kprobe is allowed to be inserted on instructions >> that branch to _mcount (referred to as ftrace location). With >> -mprofile-kernel, we now include the preceding 'mflr r0' as being part >> of the ftrace location. >> >> However, by default, probing on an instruction that is not actually the >> branch to _mcount() is prohibited, as that is considered to not be at an >> instruction boundary. This is not the case on powerpc, so allow the same >> by overriding arch_check_ftrace_location() >> >> In addition, we update kprobe_ftrace_handler() to detect this scenarios >> and to pass the proper nip to the pre and post probe handlers. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> arch/powerpc/kernel/kprobes-ftrace.c | 30 ++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c >> index 972cb28174b2..6a0bd3c16cb6 100644 >> --- a/arch/powerpc/kernel/kprobes-ftrace.c >> +++ b/arch/powerpc/kernel/kprobes-ftrace.c >> @@ -12,14 +12,34 @@ >> #include <linux/preempt.h> >> #include <linux/ftrace.h> >> >> +/* >> + * With -mprofile-kernel, we patch two instructions -- the branch to _mcount >> + * as well as the preceding 'mflr r0'. Both these instructions are claimed >> + * by ftrace and we should allow probing on either instruction. >> + */ >> +int arch_check_ftrace_location(struct kprobe *p) >> +{ >> + if (ftrace_location((unsigned long)p->addr)) >> + p->flags |= KPROBE_FLAG_FTRACE; >> + return 0; >> +} >> + >> /* Ftrace callback handler for kprobes */ >> void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, >> struct ftrace_ops *ops, struct pt_regs *regs) >> { >> struct kprobe *p; >> + int mflr_kprobe = 0; >> struct kprobe_ctlblk *kcb; >> >> p = get_kprobe((kprobe_opcode_t *)nip); >> + if (unlikely(!p)) { > > Hmm, is this really unlikely? If we put a kprobe on the second instruction address, > we will see p == NULL always. > >> + p = get_kprobe((kprobe_opcode_t *)(nip - MCOUNT_INSN_SIZE)); >> + if (!p) > > Here will be unlikely, because we can not find kprobe at both of nip and > nip - MCOUNT_INSN_SIZE. > >> + return; >> + mflr_kprobe = 1; >> + } >> + >> if (unlikely(!p) || kprobe_disabled(p)) > > "unlikely(!p)" is not needed here. ... Joe Perches wrote: > On Fri, 2019-06-21 at 23:50 +0900, Masami Hiramatsu wrote: >> On Tue, 18 Jun 2019 20:17:06 +0530 >> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > trivia: > >> > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c > [] >> > @@ -57,6 +82,11 @@ NOKPROBE_SYMBOL(kprobe_ftrace_handler); >> > >> > int arch_prepare_kprobe_ftrace(struct kprobe *p) >> > { >> > + if ((unsigned long)p->addr & 0x03) { >> > + printk("Attempt to register kprobe at an unaligned address\n"); > > Please use the appropriate KERN_<LEVEL> or pr_<level> > All good points. Thanks for the review. - Naveen
WARNING: multiple messages have this Message-ID (diff)
From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> To: Masami Hiramatsu <mhiramat@kernel.org>, Joe Perches <joe@perches.com> Cc: linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>, Nicholas Piggin <npiggin@gmail.com>, linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org> Subject: Re: [PATCH 7/7] powerpc/kprobes: Allow probing on any ftrace address Date: Wed, 26 Jun 2019 15:09:56 +0530 [thread overview] Message-ID: <1561541820.15ifr1qex2.naveen@linux.ibm.com> (raw) In-Reply-To: <20190621235034.acc00fc3e2b2c7e89caa1fd5@kernel.org> Masami Hiramatsu wrote: > On Tue, 18 Jun 2019 20:17:06 +0530 > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > >> With KPROBES_ON_FTRACE, kprobe is allowed to be inserted on instructions >> that branch to _mcount (referred to as ftrace location). With >> -mprofile-kernel, we now include the preceding 'mflr r0' as being part >> of the ftrace location. >> >> However, by default, probing on an instruction that is not actually the >> branch to _mcount() is prohibited, as that is considered to not be at an >> instruction boundary. This is not the case on powerpc, so allow the same >> by overriding arch_check_ftrace_location() >> >> In addition, we update kprobe_ftrace_handler() to detect this scenarios >> and to pass the proper nip to the pre and post probe handlers. >> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> arch/powerpc/kernel/kprobes-ftrace.c | 30 ++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c >> index 972cb28174b2..6a0bd3c16cb6 100644 >> --- a/arch/powerpc/kernel/kprobes-ftrace.c >> +++ b/arch/powerpc/kernel/kprobes-ftrace.c >> @@ -12,14 +12,34 @@ >> #include <linux/preempt.h> >> #include <linux/ftrace.h> >> >> +/* >> + * With -mprofile-kernel, we patch two instructions -- the branch to _mcount >> + * as well as the preceding 'mflr r0'. Both these instructions are claimed >> + * by ftrace and we should allow probing on either instruction. >> + */ >> +int arch_check_ftrace_location(struct kprobe *p) >> +{ >> + if (ftrace_location((unsigned long)p->addr)) >> + p->flags |= KPROBE_FLAG_FTRACE; >> + return 0; >> +} >> + >> /* Ftrace callback handler for kprobes */ >> void kprobe_ftrace_handler(unsigned long nip, unsigned long parent_nip, >> struct ftrace_ops *ops, struct pt_regs *regs) >> { >> struct kprobe *p; >> + int mflr_kprobe = 0; >> struct kprobe_ctlblk *kcb; >> >> p = get_kprobe((kprobe_opcode_t *)nip); >> + if (unlikely(!p)) { > > Hmm, is this really unlikely? If we put a kprobe on the second instruction address, > we will see p == NULL always. > >> + p = get_kprobe((kprobe_opcode_t *)(nip - MCOUNT_INSN_SIZE)); >> + if (!p) > > Here will be unlikely, because we can not find kprobe at both of nip and > nip - MCOUNT_INSN_SIZE. > >> + return; >> + mflr_kprobe = 1; >> + } >> + >> if (unlikely(!p) || kprobe_disabled(p)) > > "unlikely(!p)" is not needed here. ... Joe Perches wrote: > On Fri, 2019-06-21 at 23:50 +0900, Masami Hiramatsu wrote: >> On Tue, 18 Jun 2019 20:17:06 +0530 >> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote: > > trivia: > >> > diff --git a/arch/powerpc/kernel/kprobes-ftrace.c b/arch/powerpc/kernel/kprobes-ftrace.c > [] >> > @@ -57,6 +82,11 @@ NOKPROBE_SYMBOL(kprobe_ftrace_handler); >> > >> > int arch_prepare_kprobe_ftrace(struct kprobe *p) >> > { >> > + if ((unsigned long)p->addr & 0x03) { >> > + printk("Attempt to register kprobe at an unaligned address\n"); > > Please use the appropriate KERN_<LEVEL> or pr_<level> > All good points. Thanks for the review. - Naveen
next prev parent reply other threads:[~2019-06-26 9:40 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-18 14:46 [PATCH 0/7] powerpc/ftrace: Patch out -mprofile-kernel instructions Naveen N. Rao 2019-06-18 14:46 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 1/7] ftrace: Expose flags used for ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 2/7] x86/ftrace: Fix use of flags in ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 3/7] ftrace: Expose __ftrace_replace_code() Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-19 5:14 ` Michael Ellerman 2019-06-19 7:10 ` Nicholas Piggin 2019-06-19 7:10 ` Nicholas Piggin 2019-06-19 9:53 ` Naveen N. Rao 2019-06-19 9:53 ` Naveen N. Rao 2019-06-19 10:41 ` Nicholas Piggin 2019-06-19 10:41 ` Nicholas Piggin 2019-06-19 17:14 ` Naveen N. Rao 2019-06-19 17:14 ` Naveen N. Rao 2019-06-18 14:47 ` [PATCH 5/7] powerpc/ftrace: Update ftrace_location() for powerpc -mprofile-kernel Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-18 15:45 ` Steven Rostedt 2019-06-18 15:45 ` Steven Rostedt 2019-06-18 18:11 ` Naveen N. Rao 2019-06-18 18:11 ` Naveen N. Rao 2019-06-18 18:23 ` Naveen N. Rao 2019-06-18 18:23 ` Naveen N. Rao 2019-06-18 18:32 ` Steven Rostedt 2019-06-18 18:32 ` Steven Rostedt 2019-06-19 7:56 ` Naveen N. Rao 2019-06-19 7:56 ` Naveen N. Rao 2019-06-19 9:28 ` Steven Rostedt 2019-06-19 9:28 ` Steven Rostedt 2019-06-18 14:47 ` [PATCH 6/7] kprobes/ftrace: Use ftrace_location() when [dis]arming probes Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-21 14:41 ` Masami Hiramatsu 2019-06-21 14:41 ` Masami Hiramatsu 2019-06-18 14:47 ` [PATCH 7/7] powerpc/kprobes: Allow probing on any ftrace address Naveen N. Rao 2019-06-18 14:47 ` Naveen N. Rao 2019-06-21 14:50 ` Masami Hiramatsu 2019-06-21 14:50 ` Masami Hiramatsu 2019-06-22 3:49 ` Joe Perches 2019-06-22 3:49 ` Joe Perches 2019-06-26 9:39 ` Naveen N. Rao [this message] 2019-06-26 9:39 ` Naveen N. Rao
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=1561541820.15ifr1qex2.naveen@linux.ibm.com \ --to=naveen.n.rao@linux.vnet.ibm.com \ --cc=joe@perches.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mhiramat@kernel.org \ --cc=mingo@kernel.org \ --cc=mpe@ellerman.id.au \ --cc=npiggin@gmail.com \ --cc=rostedt@goodmis.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.