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

[Christophe Leroy <christophe.leroy@c-s.fr>]

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@c-s.fr>
>> ---
>> 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