Re: [PATCH] powerpc/kprobes: Pass ppc_inst as a pointer to emulate_step() on ppc32

["Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>]

Christophe Leroy wrote:
> 
> 
> Le 20/05/2021 à 09:29, Naveen N. Rao a écrit :
>> Trying to use a kprobe on ppc32 results in the below splat:
>>      BUG: Unable to handle kernel data access on read at 0x7c0802a6
>>      Faulting instruction address: 0xc002e9f0
>>      Oops: Kernel access of bad area, sig: 11 [#1]
>>      BE PAGE_SIZE=4K PowerPC 44x Platform
>>      Modules linked in:
>>      CPU: 0 PID: 89 Comm: sh Not tainted 5.13.0-rc1-01824-g3a81c0495fdb #7
>>      NIP:  c002e9f0 LR: c0011858 CTR: 00008a47
>>      REGS: c292fd50 TRAP: 0300   Not tainted  (5.13.0-rc1-01824-g3a81c0495fdb)
>>      MSR:  00009000 <EE,ME>  CR: 24002002  XER: 20000000
>>      DEAR: 7c0802a6 ESR: 00000000
>>      <snip>
>>      NIP [c002e9f0] emulate_step+0x28/0x324
>>      LR [c0011858] optinsn_slot+0x128/0x10000
>>      Call Trace:
>>       opt_pre_handler+0x7c/0xb4 (unreliable)
>>       optinsn_slot+0x128/0x10000
>>       ret_from_syscall+0x0/0x28
> 
> I remember running some kprobe tests before submitting the patch, how did I miss that ?
> Is there anything special to do to activate the use of optprobes and/or to hit this bug ?

Yeah, I was surprised when I hit this. One of the requirements we have 
for optprobes on powerpc is that the instruction should be a compute 
instruction (no load/store -- emulate_update_regs() should be enough) 
with the exception of conditional branches. It's possible that you ended 
up probing an instruction that couldn't be optimized.

An easy way to confirm if a probe has been optimized is to look at 
kprobes/list in debugfs, and to watch out for [OPTIMIZED] flag there.

>> diff --git a/arch/powerpc/kernel/optprobes.c 
>> b/arch/powerpc/kernel/optprobes.c
>> index cdf87086fa33a0..2bc53fa48a1b33 100644
>> --- a/arch/powerpc/kernel/optprobes.c
>> +++ b/arch/powerpc/kernel/optprobes.c
>> @@ -281,8 +281,12 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>>   	/*
>>   	 * 3. load instruction to be emulated into relevant register, and
>>   	 */
>> -	temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>> -	patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>> +	if (IS_ENABLED(CONFIG_PPC64)) {
>> +		temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>> +		patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>> +	} else {
>> +		patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
>> +	}
> 
> It means commit https://github.com/linuxppc/linux/commit/693557ebf407a85ea400a0b501bb97687d8f4856 
> was not necessary and may be reverted.

Indeed, I will send a revert for it.


- Naveen