Nam Cao <namcaov@xxxxxxxxx> writes: > On Sun, Aug 27, 2023 at 10:11:25AM +0200, Björn Töpel wrote: >> The default implementation of is_trap_insn() which RISC-V is using calls >> is_swbp_insn(), which is doing what your patch does. Your patch does not >> address the issue. > > is_swbp_insn() does this: > > #ifdef CONFIG_RISCV_ISA_C > return (*insn & 0xffff) == UPROBE_SWBP_INSN; > #else > return *insn == UPROBE_SWBP_INSN; > #endif > > ...so it doesn't even check for 32-bit ebreak if C extension is on. My patch > is not the same. Ah, was too quick. AFAIU uprobes *always* uses c.ebreak when CONFIG_RISCV_ISA_C is set, and ebreak otherwise. That's the reason is_swbp_insn() is implemented like that. If that's not the case, there's a another bug, that your patches addresses. In that case we need an arch specific set_swbp() implementation together with your patch. Guo, thoughts? ...text patching on RISC-V is still a big WIP. > But okay, if it doesn't solve the problem, then I must be wrong somewhere. Yes, this bug is another issue. >> We're taking an ebreak trap from kernel space. In this case we should >> never look for a userland (uprobe) handler at all, only the kprobe >> handlers should be considered. >> >> In this case, the TIF_UPROBE is incorrectly set, and incorrectly (not) >> handled in the "common entry" exit path, which takes us to the infinite >> loop. > > This change makes a lot of sense, no reason to check for uprobes if exception > comes from the kernel. Ok! I sent a patch proper for this. Björn
