Message ID | d13eccaed4be643409abed29565dbdd8c7d5af6a.1498069502.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
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
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)
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(-)