[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9fd6134f-1807-bb49-cf6a-fa49175fdfe5@c-s.fr>
Date: Sun, 29 Mar 2020 11:48:09 +0200
From: Christophe Leroy <christophe.leroy@....fr>
To: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Michael Ellerman <mpe@...erman.id.au>,
Paul Mackerras <paulus@...ba.org>
Cc: linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2] powerpc/kprobes: Blacklist functions running with MMU
disabled on PPC32
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@....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
Powered by blists - more mailing lists