Re: [PATCH v2] powerpc/kprobes: Blacklist functions running with MMU disabled on PPC32

[Christophe Leroy]

Le 27/03/2020 Ã 10:07, Naveen N. Rao a ÃcritÂ:
> Christophe Leroy wrote:
> > kprobe does not handle events happening in real mode, all
> > functions running with MMU disabled have to be blacklisted.
> >
> > As already done for PPC64, do it for PPC32.
> >
> > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> > ---
> > v2:
> > - Don't rename nonrecoverable as local, mark it noprobe instead.
> > - Add missing linux/kprobes.h include in pq2.c
> > ---
> > Âarch/powerpc/include/asm/ppc_asm.hÂÂÂÂÂÂÂÂÂÂ | 10 +++
> > Âarch/powerpc/kernel/cpu_setup_6xx.SÂÂÂÂÂÂÂÂÂ |Â 4 +-
> > Âarch/powerpc/kernel/entry_32.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 65 ++++++++------------
> > Âarch/powerpc/kernel/fpu.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> > Âarch/powerpc/kernel/idle_6xx.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +-
> > Âarch/powerpc/kernel/idle_e500.SÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +-
> > Âarch/powerpc/kernel/l2cr_6xx.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +-
> > Âarch/powerpc/kernel/misc.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +
> > Âarch/powerpc/kernel/misc_32.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 4 +-
> > Âarch/powerpc/kernel/swsusp_32.SÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 6 +-
> > Âarch/powerpc/kernel/vector.SÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> > Âarch/powerpc/mm/book3s32/hash_low.SÂÂÂÂÂÂÂÂÂ | 38 ++++++------
> > Âarch/powerpc/mm/mem.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +
> > Âarch/powerpc/platforms/52xx/lite5200_sleep.S |Â 2 +
> > Âarch/powerpc/platforms/82xx/pq2.cÂÂÂÂÂÂÂÂÂÂÂ |Â 3 +
> > Âarch/powerpc/platforms/83xx/suspend-asm.SÂÂÂ |Â 1 +
> > Âarch/powerpc/platforms/powermac/cache.SÂÂÂÂÂ |Â 2 +
> > Âarch/powerpc/platforms/powermac/sleep.SÂÂÂÂÂ | 13 ++--
> > Â18 files changed, 86 insertions(+), 74 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ppc_asm.h 
> > b/arch/powerpc/include/asm/ppc_asm.h
> > index 6b03dff61a05..e8f34ba89497 100644
> > --- a/arch/powerpc/include/asm/ppc_asm.h
> > +++ b/arch/powerpc/include/asm/ppc_asm.h
> > @@ -267,8 +267,18 @@ GLUE(.,name):
> > ÂÂÂÂ .pushsection "_kprobe_blacklist","aw";ÂÂÂÂÂÂÂ \
> > ÂÂÂÂ PPC_LONG (entry) ;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > ÂÂÂÂ .popsection
> > +#define _NOKPROBE_ENTRY(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _ASM_NOKPROBE_SYMBOL(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _ENTRY(entry)
> > +#define _NOKPROBE_GLOBAL(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _ASM_NOKPROBE_SYMBOL(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _GLOBAL(entry)
> > Â#else
> > Â#define _ASM_NOKPROBE_SYMBOL(entry)
> > +#define _NOKPROBE_ENTRY(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _ENTRY(entry)
> > +#define _NOKPROBE_GLOBAL(entry)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> > +ÂÂÂ _GLOBAL(entry)
> > Â#endif
>
> Michael hasn't preferred including NOKPROBE variants of those macros 
> previously, since he would like to see some cleanups there:
> https://patchwork.ozlabs.org/patch/696138/

Ok

[...]

> > @@ -194,8 +188,7 @@ transfer_to_handler:
> > ÂÂÂÂ bt-ÂÂÂ 31-TLF_NAPPING,4f
> > ÂÂÂÂ bt-ÂÂÂ 31-TLF_SLEEPING,7f
> > Â#endif /* CONFIG_PPC_BOOK3S_32 || CONFIG_E500 */
> > -ÂÂÂ .globl transfer_to_handler_cont
> > -transfer_to_handler_cont:
> > +_NOKPROBE_ENTRY(transfer_to_handler_cont)
> > Â3:
> > ÂÂÂÂ mflrÂÂÂ r9
> > ÂÂÂÂ tovirt_novmstack r2, r2ÂÂÂÂ /* set r2 to current */
> > @@ -297,6 +290,7 @@ reenable_mmu:
> > Â * On kernel stack overflow, load up an initial stack pointer
> > Â * and call StackOverflow(regs), which should not return.
> > Â */
> > +_ASM_NOKPROBE_SYMBOL(stack_ovf)
> > Âstack_ovf:
>
> The current convention is to add the NOKPROBE annotation at the _end_ of 
> the associated function/symbol...

Ok. For big functions that looks unpractical, but I'll do that.

[...]

> > @@ -1033,17 +1026,15 @@ exc_exit_restart_end:
> > ÂÂÂÂ liÂÂÂ r10, 0
> > ÂÂÂÂ stwÂÂÂ r10, 8(r1)
> > ÂÂÂÂ REST_2GPRS(9, r1)
> > -ÂÂÂ .globl exc_exit_restart
> > +_NOKPROBE_ENTRY(exc_exit_restart)
> > Âexc_exit_restart:
> > ÂÂÂÂ lwzÂÂÂ r11,_NIP(r1)
> > ÂÂÂÂ lwzÂÂÂ r12,_MSR(r1)
> > -exc_exit_start:
> > ÂÂÂÂ mtsprÂÂÂ SPRN_SRR0,r11
> > ÂÂÂÂ mtsprÂÂÂ SPRN_SRR1,r12
> > ÂÂÂÂ REST_2GPRS(11, r1)
> > ÂÂÂÂ lwzÂÂÂ r1,GPR1(r1)
> > -ÂÂÂ .globl exc_exit_restart_end
> > -exc_exit_restart_end:
> > +.Lexc_exit_restart_end:
>
> I think it would be good to break this into smaller patches to handle 
> specific code paths, if possible. At the very least, it would be good to 
> move changes to symbol visibility to a separate patch since this also 
> changes the names printed in a backtrace.

Ok.

I removed most symbol visibility changes. I only kept the ones in 
book3s32/hash_low.S and did a separate patch for it.

I split into patches per platform, then one bigger for everything in 
arch/powerpc/kernel/ except entries, then I did one for exception entry, 
one for syscall exit and one for exception exit.

Christophe