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

[Christophe Leroy]

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 ?

> 
> The offending instruction is:
>      81 24 00 00     lwz     r9,0(r4)
> 
> Here, we are trying to load the second argument to emulate_step():
> struct ppc_inst, which is the instruction to be emulated. On ppc64,
> structures are passed in registers when passed by value. However, per
> the ppc32 ABI, structures are always passed to functions as pointers.

Yes, and that was one of the reasons I was reluctant to that new 'struct ppc_inst', as it means 
stack frames, copies, etc ... whereas we could have just changed it from 'unsigned int' to 'unsigned 
long'.
Ok, now we have it, so let's use it, but use it correctly, see my clean-up series 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=244719&state=* (v2 on the way).

> This isn't being adhered to when setting up the call to emulate_step()
> in the optprobe trampoline. Fix the same.
> 
> Fixes: eacf4c0202654a ("powerpc: Enable OPTPROBES on PPC32")
> Signed-off-by: Naveen N. Rao <naveen.n.rao at linux.vnet.ibm.com>
> ---
>   arch/powerpc/kernel/optprobes.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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.

Christophe