Re: [PATCH 7/7] powerpc/kprobes: Allow probing on any ftrace address

[Naveen N. Rao]

Masami Hiramatsu wrote:
> On Tue, 18 Jun 2019 20:17:06 +0530
> "Naveen N. Rao" <naveen.n.rao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxx> 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