Le 20/05/2021 à 12:54, Naveen N. Rao a écrit :
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.
I'm not completely sure it is worth reverting, on an other hand it is pointless anyway to have
something to convert to a u64 something that cannot be more than 32 bits on a PPC32, so now that we
have ppc_inst_as_ulong() it is as good I think.
Christophe
