[PATCH v2 2/3] powerpc/kprobes: un-blacklist system_call() from kprobes

[Naveen N. Rao]

On 2017/04/27 08:19PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao at linux.vnet.ibm.com> writes:
> 
> > It is actually safe to probe system_call() in entry_64.S, but only till
> > .Lsyscall_exit. To allow this, convert .Lsyscall_exit to a non-local
> > symbol __system_call() and blacklist that symbol, rather than
> > system_call().
> 
> I'm not sure I like this. The reason we made it a local symbol in the
> first place is because it made backtraces look odd:
> 
>   commit 4c3b21686111e0ac6018469dacbc5549f9915cf8
>   Author:     Michael Ellerman <mpe at ellerman.id.au>
>   AuthorDate: Fri Dec 5 21:16:59 2014 +1100
>   
>   powerpc/kernel: Make syscall_exit a local label
>   
>   Currently when we back trace something that is in a syscall we see
>   something like this:
>   
>   [c000000000000000] [c000000000000000] SyS_read+0x6c/0x110
>   [c000000000000000] [c000000000000000] syscall_exit+0x0/0x98
>   
>   Although it's entirely correct, seeing syscall_exit at the bottom can be
>   confusing - we were exiting from a syscall and then called SyS_read() ?
>   
>   If we instead change syscall_exit to be a local label we get something
>   more intuitive:
>   
>   [c0000001fa46fde0] [c00000000026719c] SyS_read+0x6c/0x110
>   [c0000001fa46fe30] [c000000000009264] system_call+0x38/0xd0
>   
>   ie. we were handling a system call, and it was SyS_read().
> 
> 
> I think you know that, although you didn't mention it in the change log,
> because you've called the new symbol __system_call. But that is not a
> great name either because that's not what it does.

Yes, you're right. I used __system_call since I felt that it won't cause 
confusion like syscall_exit did. I agree it's not a great name, but we 
need _some_ label other than system_call if we want to allow probing at 
this point.

Also, if I'm reading this right, there is no other place to probe if we 
want to capture all system call entries.

So, I felt this would be good to have.

> 
> > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> > index 380361c0bb6a..e030ce34dd66 100644
> > --- a/arch/powerpc/kernel/entry_64.S
> > +++ b/arch/powerpc/kernel/entry_64.S
> > @@ -176,7 +176,7 @@ system_call:			/* label this so stack traces look sane */
> >  	mtctr   r12
> >  	bctrl			/* Call handler */
> >  
> > -.Lsyscall_exit:
> > +__system_call:
> >  	std	r3,RESULT(r1)
> >  	CURRENT_THREAD_INFO(r12, r1)
>   
> Why can't we kprobe the std and the rotate to current thread info?
> 
> Is the real no-probe point just here, prior to the clearing of MSR_RI ?
> 
> 	ld	r8,_MSR(r1)
> #ifdef CONFIG_PPC_BOOK3S
> 	/* No MSR:RI on BookE */

We can probe at all those places, just not once MSR_RI is unset. So, the 
no-probe point is just *after* the mtmsrd.

However, for kprobe blacklisting, the granularity is at a function level 
(or ASM labels). As such, we will have to blacklist all of 
syscall_exit/__system_call.


Regards,
Naveen