Messages in this thread | | | From | Guo Ren <> | Date | Tue, 31 Jan 2023 09:48:29 +0800 | Subject | Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity |
| |
On Mon, Jan 30, 2023 at 11:49 PM Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Bjorn, > > On Mon, Jan 30, 2023 at 04:28:15PM +0100, Björn Töpel wrote: > > Guo Ren <guoren@kernel.org> writes: > > > > >> In the serie of RISCV OPTPROBES [1], it patches a long-jump instructions pair > > >> AUIPC/JALR in kernel text, so in order to ensure other CPUs does not execute > > >> in the instructions that will be modified, it is still need to stop other CPUs > > >> via patch_text API, or you have any better solution to achieve the purpose? > > > - The stop_machine is an expensive way all architectures should > > > avoid, and you could keep that in your OPTPROBES implementation files > > > with static functions. > > > - The stop_machine couldn't work with PREEMPTION, so your > > > implementation needs to work with !PREEMPTION. > > > > ...and stop_machine() with !PREEMPTION is broken as well, when you're > > replacing multiple instructions (see Mark's post at [1]). The > > stop_machine() dance might work when you're replacing *one* instruction, > > not multiple as in the RISC-V case. I'll expand on this in a comment in > > the OPTPROBES v6 series. > > Just to clarify, my comments in [1] were assuming that stop_machine() was not > used, in which case there is a problem with or without PREEMPTION. > > I believe that when using stop_machine(), the !PREEMPTION case is fine, since > stop_machine() schedules work rather than running work in IRQ context on the > back of an IPI, so no CPUs should be mid-sequnce during the patching, and it's > not possible for there to be threads which are preempted mid-sequence. > > That all said, IIUC optprobes is going to disappear once fprobe is ready > everywhere, so that might be moot. The optprobes could be in the middle of a function, but fprobe must be the entry of a function, right?
Does your fprobe here mean: ?
The Linux kernel configuration item CONFIG_FPROBE:
prompt: Kernel Function Probe (fprobe) type: bool depends on: ( CONFIG_FUNCTION_TRACER ) && ( CONFIG_DYNAMIC_FTRACE_WITH_REGS ) && ( CONFIG_HAVE_RETHOOK ) defined in kernel/trace/Kconfig
> > Thanks, > Mark. > > > >> > static void __kprobes arch_prepare_simulate(struct kprobe *p) > > >> > @@ -114,16 +120,23 @@ void *alloc_insn_page(void) > > >> > /* install breakpoint in text */ > > >> > void __kprobes arch_arm_kprobe(struct kprobe *p) > > >> > { > > >> > - if ((p->opcode & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) > > >> > - patch_text(p->addr, __BUG_INSN_32); > > >> > - else > > >> > - patch_text(p->addr, __BUG_INSN_16); > > >> > +#ifdef CONFIG_RISCV_ISA_C > > >> > + u32 opcode = __BUG_INSN_16; > > >> > +#else > > >> > + u32 opcode = __BUG_INSN_32; > > >> > +#endif > > >> > + patch_text_nosync(p->addr, &opcode, GET_INSN_LENGTH(opcode)); > > >> > > >> Sounds good, but it will leave some RVI instruction truncated in kernel text, > > >> i doubt kernel behavior depends on the rest of the truncated instruction, well, > > >> it needs more strict testing to prove my concern :) > > > I do this on purpose, and it doesn't cause any problems. Don't worry; > > > IFU hw must enforce the fetch sequence, and there is no way to execute > > > broken instructions even in the speculative execution path. > > > > This is stretching reality a bit much. ARMv8, e.g., has a chapter in the > > Arm ARM [2] Appendix B "Concurrent modification and execution of > > instructions" (CMODX). *Some* instructions can be replaced concurrently, > > and others cannot without caution. Assuming that that all RISC-V > > implementations can, is a stretch. RISC-V hasn't even specified the > > behavior of CMODX (which is problematic). > > > > If anything it would be more likely that the existing > > "stop_machine()-to-replace-with-ebreak" works (again, replacing one > > instruction does not have the !PREEMPTION issues). Then again, no spec, > > so mostly guessing from my side. :-( > > > > Oh, but the existing "ebreak replace" might be broken like [3]. > > > > > > Björn > > > > > > [1] https://lore.kernel.org/linux-riscv/Y7%2F6AtX5X0+5qF6Y@FVFF77S0Q05N/ > > [2] https://developer.arm.com/documentation/ddi0487/latest > > [3] https://lore.kernel.org/linux-riscv/20230126170607.1489141-2-guoren@kernel.org/
-- Best Regards Guo Ren
| |