Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] powerpc/kprobes: Blacklist functions running with MMU
 disabled on PPC32
To:     "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Paul Mackerras <paulus@samba.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <3ada1af1b479c5a88bf9f3b8955bf5ba3f32f1ba.1582565849.git.christophe.leroy@c-s.fr>
 <1585299144.f9e0pmxsgv.naveen@linux.ibm.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <9fd6134f-1807-bb49-cf6a-fa49175fdfe5@c-s.fr>
Date:   Sun, 29 Mar 2020 11:48:09 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <1585299144.f9e0pmxsgv.naveen@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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