[v3,4/6] powerpc/64s: Un-blacklist system_call() from kprobes

It is actually safe to probe system_call() in entry_64.S, but only till
we unset MSR_RI. To allow this, add a new symbol system_call_exit()
after the mtmsrd and blacklist that. Though the mtmsrd instruction
itself is now whitelisted, we won't be allowed to probe on it as we
don't allow probing on rfi and mtmsr instructions (checked for in
arch_prepare_kprobe()).

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_64.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Thu, 22 Jun 2017 00:08:40 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:

> It is actually safe to probe system_call() in entry_64.S, but only till
> we unset MSR_RI. To allow this, add a new symbol system_call_exit()
> after the mtmsrd and blacklist that. Though the mtmsrd instruction
> itself is now whitelisted, we won't be allowed to probe on it as we
> don't allow probing on rfi and mtmsr instructions (checked for in
> arch_prepare_kprobe()).

Can you add a little comment to say probes aren't allowed, and it's
located after the mtmsr in order to avoid contaminating traces?

Also I wonder if a slightly different name would be more instructive?
I don't normally care, but the system_call_common code isn't trivial
to follow. system_call_exit might give the impression that it is the
entire exit path (which would pair with system_call for entry).

Perhaps system_call_exit_notrace? No that sucks too :(

Thanks,
Nick


> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/entry_64.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index ef8e6615b8ba..feeeadc9aa71 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -204,6 +204,7 @@ system_call:			/* label this so stack traces look sane */
>  	mtmsrd	r11,1
>  #endif /* CONFIG_PPC_BOOK3E */
>  
> +system_call_exit:
>  	ld	r9,TI_FLAGS(r12)
>  	li	r11,-MAX_ERRNO
>  	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
> @@ -412,7 +413,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	b	.	/* prevent speculative execution */
>  #endif
>  _ASM_NOKPROBE_SYMBOL(system_call_common);
> -_ASM_NOKPROBE_SYMBOL(system_call);
> +_ASM_NOKPROBE_SYMBOL(system_call_exit);
>  
>  /* Save non-volatile GPRs, if not already saved. */
>  _GLOBAL(save_nvgprs)

Nicholas Piggin <npiggin@gmail.com> writes:

> On Thu, 22 Jun 2017 00:08:40 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
>> It is actually safe to probe system_call() in entry_64.S, but only till
>> we unset MSR_RI. To allow this, add a new symbol system_call_exit()
>> after the mtmsrd and blacklist that. Though the mtmsrd instruction
>> itself is now whitelisted, we won't be allowed to probe on it as we
>> don't allow probing on rfi and mtmsr instructions (checked for in
>> arch_prepare_kprobe()).
>
> Can you add a little comment to say probes aren't allowed, and it's
> located after the mtmsr in order to avoid contaminating traces?
>
> Also I wonder if a slightly different name would be more instructive?
> I don't normally care, but the system_call_common code isn't trivial
> to follow. system_call_exit might give the impression that it is the
> entire exit path (which would pair with system_call for entry).

It is the entire path in the happy case isn't it? I'm not sure I know
what you mean.

> Perhaps system_call_exit_notrace? No that sucks too :(

A bit :D

If you're tracing etc. then you'll be in syscall_exit_work, isn't that
sufficient to differentiate the two?

cheers

On Thu, 22 Jun 2017 21:14:21 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > On Thu, 22 Jun 2017 00:08:40 +0530
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> >  
> >> It is actually safe to probe system_call() in entry_64.S, but only till
> >> we unset MSR_RI. To allow this, add a new symbol system_call_exit()
> >> after the mtmsrd and blacklist that. Though the mtmsrd instruction
> >> itself is now whitelisted, we won't be allowed to probe on it as we
> >> don't allow probing on rfi and mtmsr instructions (checked for in
> >> arch_prepare_kprobe()).  
> >
> > Can you add a little comment to say probes aren't allowed, and it's
> > located after the mtmsr in order to avoid contaminating traces?
> >
> > Also I wonder if a slightly different name would be more instructive?
> > I don't normally care, but the system_call_common code isn't trivial
> > to follow. system_call_exit might give the impression that it is the
> > entire exit path (which would pair with system_call for entry).  
> 
> It is the entire path in the happy case isn't it? I'm not sure I know
> what you mean.

Oh, yes you're right, I thought it was moved down further.

Thanks,
Nick