Add k/uprobe & fentry & error_injection supported

From: Patrick Stählin <[email protected]>

Needed for kprobes support. Copied and adapted from arm64 code.

Guo Ren fixup pt_regs type for linux-5.8-rc1.

Signed-off-by Patrick Stählin <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Reviewed-by Zong Li <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

---
 arch/riscv/Kconfig              |  1 +
 arch/riscv/include/asm/ptrace.h | 29 ++++++++++++
 arch/riscv/kernel/ptrace.c      | 99 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

arch/riscv/Kconfig +0 -0

index b7821ac..e6424d8b 100644

arch/riscv/Kconfig +0 -0

 config ARCH_MMAP_RND_BITS_MIN
 	default 18 if 64BIT

arch/riscv/include/asm/ptrace.h +29 -0

index ee49f80..23372bb 100644
@@ -8,6 +8,7 @@
 
 #include <uapi/asm/ptrace.h>
 #include <asm/csr.h>
+#include <linux/compiler.h>
 
 #ifndef __ASSEMBLY__
 

@@ -60,6 +61,7 @@ struct pt_regs {
 
 #define user_mode(regs) (((regs)->status & SR_PP) == 0)
 
+#define MAX_REG_OFFSET offsetof(struct pt_regs, orig_a0)
 
 /* Helpers for working with the instruction pointer */
 static inline unsigned long instruction_pointer(struct pt_regs *regs)

@@ -85,6 +87,12 @@ static inline void user_stack_pointer_set(struct pt_regs *regs,
 	regs->sp =  val;
 }
 
+/* Valid only for Kernel mode traps. */
+static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
+{
+	return regs->sp;
+}
+
 /* Helpers for working with the frame pointer */
 static inline unsigned long frame_pointer(struct pt_regs *regs)
 {

@@ -101,6 +109,27 @@ static inline unsigned long regs_return_value(struct pt_regs *regs)
 	return regs->a0;
 }
 
+extern int regs_query_register_offset(const char *name);
+extern unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
+					       unsigned int n);
+
+/**
+ * regs_get_register() - get register value from its offset
+ * @regs:	pt_regs from which register value is gotten
+ * @offset:	offset of the register.
+ *
+ * regs_get_register returns the value of a register whose offset from @regs.
+ * The @offset is the offset of the register in struct pt_regs.
+ * If @offset is bigger than MAX_REG_OFFSET, this returns 0.
+ */
+static inline unsigned long regs_get_register(struct pt_regs *regs,
+					      unsigned int offset)
+{
+	if (unlikely(offset > MAX_REG_OFFSET))
+		return 0;
+
+	return *(unsigned long *)((unsigned long)regs + offset);
+}
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PTRACE_H */

arch/riscv/kernel/ptrace.c +99 -0

index 2d6395f..1a85305 100644
@@ -114,6 +114,105 @@ const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 	return &riscv_user_native_view;
 }
 
+struct pt_regs_offset {
+	const char *name;
+	int offset;
+};
+
+#define REG_OFFSET_NAME(r) {.name = #r, .offset = offsetof(struct pt_regs, r)}
+#define REG_OFFSET_END {.name = NULL, .offset = 0}
+
+static const struct pt_regs_offset regoffset_table[] = {
+	REG_OFFSET_NAME(epc),
+	REG_OFFSET_NAME(ra),
+	REG_OFFSET_NAME(sp),
+	REG_OFFSET_NAME(gp),
+	REG_OFFSET_NAME(tp),
+	REG_OFFSET_NAME(t0),
+	REG_OFFSET_NAME(t1),
+	REG_OFFSET_NAME(t2),
+	REG_OFFSET_NAME(s0),
+	REG_OFFSET_NAME(s1),
+	REG_OFFSET_NAME(a0),
+	REG_OFFSET_NAME(a1),
+	REG_OFFSET_NAME(a2),
+	REG_OFFSET_NAME(a3),
+	REG_OFFSET_NAME(a4),
+	REG_OFFSET_NAME(a5),
+	REG_OFFSET_NAME(a6),
+	REG_OFFSET_NAME(a7),
+	REG_OFFSET_NAME(s2),
+	REG_OFFSET_NAME(s3),
+	REG_OFFSET_NAME(s4),
+	REG_OFFSET_NAME(s5),
+	REG_OFFSET_NAME(s6),
+	REG_OFFSET_NAME(s7),
+	REG_OFFSET_NAME(s8),
+	REG_OFFSET_NAME(s9),
+	REG_OFFSET_NAME(s10),
+	REG_OFFSET_NAME(s11),
+	REG_OFFSET_NAME(t3),
+	REG_OFFSET_NAME(t4),
+	REG_OFFSET_NAME(t5),
+	REG_OFFSET_NAME(t6),
+	REG_OFFSET_NAME(status),
+	REG_OFFSET_NAME(badaddr),
+	REG_OFFSET_NAME(cause),
+	REG_OFFSET_NAME(orig_a0),
+	REG_OFFSET_END,
+};
+
+/**
+ * regs_query_register_offset() - query register offset from its name
+ * @name:	the name of a register
+ *
+ * regs_query_register_offset() returns the offset of a register in struct
+ * pt_regs from its name. If the name is invalid, this returns -EINVAL;
+ */
+int regs_query_register_offset(const char *name)
+{
+	const struct pt_regs_offset *roff;
+
+	for (roff = regoffset_table; roff->name != NULL; roff++)
+		if (!strcmp(roff->name, name))
+			return roff->offset;
+	return -EINVAL;
+}
+
+/**
+ * regs_within_kernel_stack() - check the address in the stack
+ * @regs:      pt_regs which contains kernel stack pointer.
+ * @addr:      address which is checked.
+ *
+ * regs_within_kernel_stack() checks @addr is within the kernel stack page(s).
+ * If @addr is within the kernel stack, it returns true. If not, returns false.
+ */
+static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
+{
+	return (addr & ~(THREAD_SIZE - 1))  ==
+		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1));
+}
+
+/**
+ * regs_get_kernel_stack_nth() - get Nth entry of the stack
+ * @regs:	pt_regs which contains kernel stack pointer.
+ * @n:		stack entry number.
+ *
+ * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
+ * is specified by @regs. If the @n th entry is NOT in the kernel stack,
+ * this returns 0.
+ */
+unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs, unsigned int n)
+{
+	unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
+
+	addr += n;
+	if (regs_within_kernel_stack(regs, (unsigned long)addr))
+		return *addr;
+	else
+		return 0;
+}
+
 void ptrace_disable(struct task_struct *child)
 {
 	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);

Quoted 17 lines · On Sat, 17 Oct 2020 00:06:09 PDT (-0700), [email protected] wrote:

From: Patrick Stählin <[email protected]>

Needed for kprobes support. Copied and adapted from arm64 code.

Guo Ren fixup pt_regs type for linux-5.8-rc1.

Signed-off-by Patrick Stählin <[email protected]>

Signed-off-by Guo Ren <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Reviewed-by Zong Li <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

---
 arch/riscv/Kconfig              |  1 +
 arch/riscv/include/asm/ptrace.h | 29 ++++++++++++
 arch/riscv/kernel/ptrace.c      | 99 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

arch/riscv/Kconfig config RISCV · line 90

Quoted 9 lines

arch/riscv/Kconfig +1 -0

index b7821ac..e6424d8b 100644
@@ -87,6 +87,7 @@ config RISCV
      select SPARSE_IRQ
      select SYSCTL_EXCEPTION_TRACE
      select THREAD_INFO_IN_TASK
+     select HAVE_REGS_AND_STACK_ACCESS_API

We alphabetize these now -- I'd usually just fix it, but there's a Signed-off-by issue later in the patch set.

From: Guo Ren <[email protected]>

This patch enables "kprobe & kretprobe" to work with ftrace interface. It utilized software breakpoint as single-step mechanism.

Some instructions which can't be single-step executed must be simulated in kernel execution slot, such as: branch, jal, auipc, la ...

Some instructions should be rejected for probing and we use a blacklist to filter, such as: ecall, ebreak, ...

We use ebreak & c.ebreak to replace origin instruction and the kprobe handler prepares an executable memory slot for out-of-line execution with a copy of the original instruction being probed. In execution slot we add ebreak behind original instruction to simulate a single-setp mechanism.

The patch is based on packi's work [1] and csky's work [2].
- The kprobes_trampoline.S is all from packi's patch
- The single-step mechanism is new designed for riscv without hw single-step trap
- The simulation codes are from csky
- Frankly, all codes refer to other archs' implementation

[1] https://lore.kernel.org/linux-riscv/[email protected]/
[2] https://lore.kernel.org/linux-csky/[email protected]/

Signed-off-by Guo Ren <[email protected]>

Co-Developed-by Patrick Stählin <[email protected]>

Acked-by Masami Hiramatsu <[email protected]>

Tested-by Zong Li <[email protected]>

Reviewed-by Pekka Enberg <[email protected]>

Cc Patrick Stählin <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Björn Töpel <[email protected]>

---
 arch/riscv/Kconfig                            |   2 +
 arch/riscv/include/asm/kprobes.h              |  40 +++
 arch/riscv/include/asm/probes.h               |  24 ++
 arch/riscv/kernel/Makefile                    |   1 +
 arch/riscv/kernel/probes/Makefile             |   4 +
 arch/riscv/kernel/probes/decode-insn.c        |  48 ++++
 arch/riscv/kernel/probes/decode-insn.h        |  18 ++
 arch/riscv/kernel/probes/kprobes.c            | 398 ++++++++++++++++++++++++++
 arch/riscv/kernel/probes/kprobes_trampoline.S |  93 ++++++
 arch/riscv/kernel/probes/simulate-insn.c      |  85 ++++++
 arch/riscv/kernel/probes/simulate-insn.h      |  47 +++
 arch/riscv/kernel/traps.c                     |   9 +
 arch/riscv/mm/fault.c                         |   4 +
 13 files changed, 773 insertions(+)
 create mode 100644 arch/riscv/include/asm/probes.h
 create mode 100644 arch/riscv/kernel/probes/Makefile
 create mode 100644 arch/riscv/kernel/probes/decode-insn.c
 create mode 100644 arch/riscv/kernel/probes/decode-insn.h
 create mode 100644 arch/riscv/kernel/probes/kprobes.c
 create mode 100644 arch/riscv/kernel/probes/kprobes_trampoline.S
 create mode 100644 arch/riscv/kernel/probes/simulate-insn.c
 create mode 100644 arch/riscv/kernel/probes/simulate-insn.h

arch/riscv/Kconfig +2 -0

index e6424d8b..c07214a 100644
@@ -67,6 +67,8 @@ config RISCV
 	select HAVE_FUTEX_CMPXCHG if FUTEX
 	select HAVE_GCC_PLUGINS
 	select HAVE_GENERIC_VDSO if MMU && 64BIT
+	select HAVE_KPROBES
+	select HAVE_KRETPROBES
 	select HAVE_PCI
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS

arch/riscv/include/asm/kprobes.h +40 -0

index 56a98ea3..4647d38 100644
@@ -11,4 +11,44 @@
 
 #include <asm-generic/kprobes.h>
 
+#ifdef CONFIG_KPROBES
+#include <linux/types.h>
+#include <linux/ptrace.h>
+#include <linux/percpu.h>
+
+#define __ARCH_WANT_KPROBES_INSN_SLOT
+#define MAX_INSN_SIZE			2
+
+#define flush_insn_slot(p)		do { } while (0)
+#define kretprobe_blacklist_size	0
+
+#include <asm/probes.h>
+
+struct prev_kprobe {
+	struct kprobe *kp;
+	unsigned int status;
+};
+
+/* Single step context for kprobe */
+struct kprobe_step_ctx {
+	unsigned long ss_pending;
+	unsigned long match_addr;
+};
+
+/* per-cpu kprobe control block */
+struct kprobe_ctlblk {
+	unsigned int kprobe_status;
+	unsigned long saved_status;
+	struct prev_kprobe prev_kprobe;
+	struct kprobe_step_ctx ss_ctx;
+};
+
+void arch_remove_kprobe(struct kprobe *p);
+int kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr);
+bool kprobe_breakpoint_handler(struct pt_regs *regs);
+bool kprobe_single_step_handler(struct pt_regs *regs);
+void kretprobe_trampoline(void);
+void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
+
+#endif /* CONFIG_KPROBES */
 #endif /* _ASM_RISCV_KPROBES_H */

arch/riscv/include/asm/probes.h new file +24 -0

new file mode 100644
index 00000000..a787e6d
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_RISCV_PROBES_H
+#define _ASM_RISCV_PROBES_H
+
+typedef u32 probe_opcode_t;
+typedef bool (probes_handler_t) (u32 opcode, unsigned long addr, struct pt_regs *);
+
+/* architecture specific copy of original instruction */
+struct arch_probe_insn {
+	probe_opcode_t *insn;
+	probes_handler_t *handler;
+	/* restore address after simulation */
+	unsigned long restore;
+};
+
+#ifdef CONFIG_KPROBES
+typedef u32 kprobe_opcode_t;
+struct arch_specific_insn {
+	struct arch_probe_insn api;
+};
+#endif
+
+#endif /* _ASM_RISCV_PROBES_H */

arch/riscv/kernel/Makefile +1 -0

index f14aed2..80958cb 100644
@@ -29,6 +29,7 @@ obj-y	+= riscv_ksyms.o
 obj-y	+= stacktrace.o
 obj-y	+= cacheinfo.o
 obj-y	+= patch.o
+obj-y	+= probes/
 obj-$(CONFIG_MMU) += vdso.o vdso/
 
 obj-$(CONFIG_RISCV_M_MODE)	+= traps_misaligned.o

arch/riscv/kernel/probes/Makefile new file +4 -0

new file mode 100644
index 00000000..8a39507
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_KPROBES)		+= kprobes.o decode-insn.o simulate-insn.o
+obj-$(CONFIG_KPROBES)		+= kprobes_trampoline.o
+CFLAGS_REMOVE_simulate-insn.o = $(CC_FLAGS_FTRACE)

arch/riscv/kernel/probes/decode-insn.c new file +48 -0

new file mode 100644
index 00000000..0876c30
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+#include <linux/module.h>
+#include <linux/kallsyms.h>
+#include <asm/sections.h>
+
+#include "decode-insn.h"
+#include "simulate-insn.h"
+
+/* Return:
+ *   INSN_REJECTED     If instruction is one not allowed to kprobe,
+ *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
+ */
+enum probe_insn __kprobes
+riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)
+{
+	probe_opcode_t insn = le32_to_cpu(*addr);
+
+	/*
+	 * Reject instructions list:
+	 */
+	RISCV_INSN_REJECTED(system,		insn);
+	RISCV_INSN_REJECTED(fence,		insn);
+
+	/*
+	 * Simulate instructions list:
+	 * TODO: the REJECTED ones below need to be implemented
+	 */
+#ifdef CONFIG_RISCV_ISA_C
+	RISCV_INSN_REJECTED(c_j,		insn);
+	RISCV_INSN_REJECTED(c_jr,		insn);
+	RISCV_INSN_REJECTED(c_jal,		insn);
+	RISCV_INSN_REJECTED(c_jalr,		insn);
+	RISCV_INSN_REJECTED(c_beqz,		insn);
+	RISCV_INSN_REJECTED(c_bnez,		insn);
+	RISCV_INSN_REJECTED(c_ebreak,		insn);
+#endif
+
+	RISCV_INSN_REJECTED(auipc,		insn);
+	RISCV_INSN_REJECTED(branch,		insn);
+
+	RISCV_INSN_SET_SIMULATE(jal,		insn);
+	RISCV_INSN_SET_SIMULATE(jalr,		insn);
+
+	return INSN_GOOD;
+}

arch/riscv/kernel/probes/decode-insn.h new file +18 -0

new file mode 100644
index 00000000..42269a7
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _RISCV_KERNEL_KPROBES_DECODE_INSN_H
+#define _RISCV_KERNEL_KPROBES_DECODE_INSN_H
+
+#include <asm/sections.h>
+#include <asm/kprobes.h>
+
+enum probe_insn {
+	INSN_REJECTED,
+	INSN_GOOD_NO_SLOT,
+	INSN_GOOD,
+};
+
+enum probe_insn __kprobes
+riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *asi);
+
+#endif /* _RISCV_KERNEL_KPROBES_DECODE_INSN_H */

arch/riscv/kernel/probes/kprobes.c new file +398 -0

new file mode 100644
index 00000000..e60893b
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/kprobes.h>
+#include <linux/extable.h>
+#include <linux/slab.h>
+#include <linux/stop_machine.h>
+#include <asm/ptrace.h>
+#include <linux/uaccess.h>
+#include <asm/sections.h>
+#include <asm/cacheflush.h>
+#include <asm/bug.h>
+#include <asm/patch.h>
+
+#include "decode-insn.h"
+
+DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
+DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
+
+static void __kprobes
+post_kprobe_handler(struct kprobe_ctlblk *, struct pt_regs *);
+
+static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
+{
+	unsigned long offset = GET_INSN_LENGTH(p->opcode);
+
+	p->ainsn.api.restore = (unsigned long)p->addr + offset;
+
+	patch_text(p->ainsn.api.insn, p->opcode);
+	patch_text((void *)((unsigned long)(p->ainsn.api.insn) + offset),
+		   __BUG_INSN_32);
+}
+
+static void __kprobes arch_prepare_simulate(struct kprobe *p)
+{
+	p->ainsn.api.restore = 0;
+}
+
+static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (p->ainsn.api.handler)
+		p->ainsn.api.handler((u32)p->opcode,
+					(unsigned long)p->addr, regs);
+
+	post_kprobe_handler(kcb, regs);
+}
+
+int __kprobes arch_prepare_kprobe(struct kprobe *p)
+{
+	unsigned long probe_addr = (unsigned long)p->addr;
+
+	if (probe_addr & 0x1) {
+		pr_warn("Address not aligned.\n");
+
+		return -EINVAL;
+	}
+
+	/* copy instruction */
+	p->opcode = le32_to_cpu(*p->addr);
+
+	/* decode instruction */
+	switch (riscv_probe_decode_insn(p->addr, &p->ainsn.api)) {
+	case INSN_REJECTED:	/* insn not supported */
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:	/* insn need simulation */
+		p->ainsn.api.insn = NULL;
+		break;
+
+	case INSN_GOOD:	/* instruction uses slot */
+		p->ainsn.api.insn = get_insn_slot();
+		if (!p->ainsn.api.insn)
+			return -ENOMEM;
+		break;
+	}
+
+	/* prepare the instruction */
+	if (p->ainsn.api.insn)
+		arch_prepare_ss_slot(p);
+	else
+		arch_prepare_simulate(p);
+
+	return 0;
+}
+
+/* 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);
+}
+
+/* remove breakpoint from text */
+void __kprobes arch_disarm_kprobe(struct kprobe *p)
+{
+	patch_text(p->addr, p->opcode);
+}
+
+void __kprobes arch_remove_kprobe(struct kprobe *p)
+{
+}
+
+static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
+{
+	kcb->prev_kprobe.kp = kprobe_running();
+	kcb->prev_kprobe.status = kcb->kprobe_status;
+}
+
+static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
+{
+	__this_cpu_write(current_kprobe, kcb->prev_kprobe.kp);
+	kcb->kprobe_status = kcb->prev_kprobe.status;
+}
+
+static void __kprobes set_current_kprobe(struct kprobe *p)
+{
+	__this_cpu_write(current_kprobe, p);
+}
+
+/*
+ * Interrupts need to be disabled before single-step mode is set, and not
+ * reenabled until after single-step mode ends.
+ * Without disabling interrupt on local CPU, there is a chance of
+ * interrupt occurrence in the period of exception return and  start of
+ * out-of-line single-step, that result in wrongly single stepping
+ * into the interrupt handler.
+ */
+static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
+						struct pt_regs *regs)
+{
+	kcb->saved_status = regs->status;
+	regs->status &= ~SR_SPIE;
+}
+
+static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
+						struct pt_regs *regs)
+{
+	regs->status = kcb->saved_status;
+}
+
+static void __kprobes
+set_ss_context(struct kprobe_ctlblk *kcb, unsigned long addr, struct kprobe *p)
+{
+	unsigned long offset = GET_INSN_LENGTH(p->opcode);
+
+	kcb->ss_ctx.ss_pending = true;
+	kcb->ss_ctx.match_addr = addr + offset;
+}
+
+static void __kprobes clear_ss_context(struct kprobe_ctlblk *kcb)
+{
+	kcb->ss_ctx.ss_pending = false;
+	kcb->ss_ctx.match_addr = 0;
+}
+
+static void __kprobes setup_singlestep(struct kprobe *p,
+				       struct pt_regs *regs,
+				       struct kprobe_ctlblk *kcb, int reenter)
+{
+	unsigned long slot;
+
+	if (reenter) {
+		save_previous_kprobe(kcb);
+		set_current_kprobe(p);
+		kcb->kprobe_status = KPROBE_REENTER;
+	} else {
+		kcb->kprobe_status = KPROBE_HIT_SS;
+	}
+
+	if (p->ainsn.api.insn) {
+		/* prepare for single stepping */
+		slot = (unsigned long)p->ainsn.api.insn;
+
+		set_ss_context(kcb, slot, p);	/* mark pending ss */
+
+		/* IRQs and single stepping do not mix well. */
+		kprobes_save_local_irqflag(kcb, regs);
+
+		instruction_pointer_set(regs, slot);
+	} else {
+		/* insn simulation */
+		arch_simulate_insn(p, regs);
+	}
+}
+
+static int __kprobes reenter_kprobe(struct kprobe *p,
+				    struct pt_regs *regs,
+				    struct kprobe_ctlblk *kcb)
+{
+	switch (kcb->kprobe_status) {
+	case KPROBE_HIT_SSDONE:
+	case KPROBE_HIT_ACTIVE:
+		kprobes_inc_nmissed_count(p);
+		setup_singlestep(p, regs, kcb, 1);
+		break;
+	case KPROBE_HIT_SS:
+	case KPROBE_REENTER:
+		pr_warn("Unrecoverable kprobe detected.\n");
+		dump_kprobe(p);
+		BUG();
+		break;
+	default:
+		WARN_ON(1);
+		return 0;
+	}
+
+	return 1;
+}
+
+static void __kprobes
+post_kprobe_handler(struct kprobe_ctlblk *kcb, struct pt_regs *regs)
+{
+	struct kprobe *cur = kprobe_running();
+
+	if (!cur)
+		return;
+
+	/* return addr restore if non-branching insn */
+	if (cur->ainsn.api.restore != 0)
+		regs->epc = cur->ainsn.api.restore;
+
+	/* restore back original saved kprobe variables and continue */
+	if (kcb->kprobe_status == KPROBE_REENTER) {
+		restore_previous_kprobe(kcb);
+		return;
+	}
+
+	/* call post handler */
+	kcb->kprobe_status = KPROBE_HIT_SSDONE;
+	if (cur->post_handler)	{
+		/* post_handler can hit breakpoint and single step
+		 * again, so we enable D-flag for recursive exception.
+		 */
+		cur->post_handler(cur, regs, 0);
+	}
+
+	reset_current_kprobe();
+}
+
+int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int trapnr)
+{
+	struct kprobe *cur = kprobe_running();
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	switch (kcb->kprobe_status) {
+	case KPROBE_HIT_SS:
+	case KPROBE_REENTER:
+		/*
+		 * We are here because the instruction being single
+		 * stepped caused a page fault. We reset the current
+		 * kprobe and the ip points back to the probe address
+		 * and allow the page fault handler to continue as a
+		 * normal page fault.
+		 */
+		regs->epc = (unsigned long) cur->addr;
+		if (!instruction_pointer(regs))
+			BUG();
+
+		if (kcb->kprobe_status == KPROBE_REENTER)
+			restore_previous_kprobe(kcb);
+		else
+			reset_current_kprobe();
+
+		break;
+	case KPROBE_HIT_ACTIVE:
+	case KPROBE_HIT_SSDONE:
+		/*
+		 * We increment the nmissed count for accounting,
+		 * we can also use npre/npostfault count for accounting
+		 * these specific fault cases.
+		 */
+		kprobes_inc_nmissed_count(cur);
+
+		/*
+		 * We come here because instructions in the pre/post
+		 * handler caused the page_fault, this could happen
+		 * if handler tries to access user space by
+		 * copy_from_user(), get_user() etc. Let the
+		 * user-specified handler try to fix it first.
+		 */
+		if (cur->fault_handler && cur->fault_handler(cur, regs, trapnr))
+			return 1;
+
+		/*
+		 * In case the user-specified fault handler returned
+		 * zero, try to fix up.
+		 */
+		if (fixup_exception(regs))
+			return 1;
+	}
+	return 0;
+}
+
+bool __kprobes
+kprobe_breakpoint_handler(struct pt_regs *regs)
+{
+	struct kprobe *p, *cur_kprobe;
+	struct kprobe_ctlblk *kcb;
+	unsigned long addr = instruction_pointer(regs);
+
+	kcb = get_kprobe_ctlblk();
+	cur_kprobe = kprobe_running();
+
+	p = get_kprobe((kprobe_opcode_t *) addr);
+
+	if (p) {
+		if (cur_kprobe) {
+			if (reenter_kprobe(p, regs, kcb))
+				return true;
+		} else {
+			/* Probe hit */
+			set_current_kprobe(p);
+			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+
+			/*
+			 * If we have no pre-handler or it returned 0, we
+			 * continue with normal processing.  If we have a
+			 * pre-handler and it returned non-zero, it will
+			 * modify the execution path and no need to single
+			 * stepping. Let's just reset current kprobe and exit.
+			 *
+			 * pre_handler can hit a breakpoint and can step thru
+			 * before return.
+			 */
+			if (!p->pre_handler || !p->pre_handler(p, regs))
+				setup_singlestep(p, regs, kcb, 0);
+			else
+				reset_current_kprobe();
+		}
+		return true;
+	}
+
+	/*
+	 * The breakpoint instruction was removed right
+	 * after we hit it.  Another cpu has removed
+	 * either a probepoint or a debugger breakpoint
+	 * at this address.  In either case, no further
+	 * handling of this interrupt is appropriate.
+	 * Return back to original instruction, and continue.
+	 */
+	return false;
+}
+
+bool __kprobes
+kprobe_single_step_handler(struct pt_regs *regs)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if ((kcb->ss_ctx.ss_pending)
+	    && (kcb->ss_ctx.match_addr == instruction_pointer(regs))) {
+		clear_ss_context(kcb);	/* clear pending ss */
+
+		kprobes_restore_local_irqflag(kcb, regs);
+
+		post_kprobe_handler(kcb, regs);
+		return true;
+	}
+	return false;
+}
+
+/*
+ * Provide a blacklist of symbols identifying ranges which cannot be kprobed.
+ * This blacklist is exposed to userspace via debugfs (kprobes/blacklist).
+ */
+int __init arch_populate_kprobe_blacklist(void)
+{
+	int ret;
+
+	ret = kprobe_add_area_blacklist((unsigned long)__irqentry_text_start,
+					(unsigned long)__irqentry_text_end);
+	return ret;
+}
+
+void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
+{
+	return (void *)kretprobe_trampoline_handler(regs, &kretprobe_trampoline, NULL);
+}
+
+void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
+				      struct pt_regs *regs)
+{
+	ri->ret_addr = (kprobe_opcode_t *)regs->ra;
+	ri->fp = NULL;
+	regs->ra = (unsigned long) &kretprobe_trampoline;
+}
+
+int __kprobes arch_trampoline_kprobe(struct kprobe *p)
+{
+	return 0;
+}
+
+int __init arch_init_kprobes(void)
+{
+	return 0;
+}

arch/riscv/kernel/probes/kprobes_trampoline.S new file +93 -0

new file mode 100644
index 00000000..6e85d02
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Author: Patrick Stählin <[email protected]>
+ */
+#include <linux/linkage.h>
+
+#include <asm/asm.h>
+#include <asm/asm-offsets.h>
+
+	.text
+	.altmacro
+
+	.macro save_all_base_regs
+	REG_S x1,  PT_RA(sp)
+	REG_S x3,  PT_GP(sp)
+	REG_S x4,  PT_TP(sp)
+	REG_S x5,  PT_T0(sp)
+	REG_S x6,  PT_T1(sp)
+	REG_S x7,  PT_T2(sp)
+	REG_S x8,  PT_S0(sp)
+	REG_S x9,  PT_S1(sp)
+	REG_S x10, PT_A0(sp)
+	REG_S x11, PT_A1(sp)
+	REG_S x12, PT_A2(sp)
+	REG_S x13, PT_A3(sp)
+	REG_S x14, PT_A4(sp)
+	REG_S x15, PT_A5(sp)
+	REG_S x16, PT_A6(sp)
+	REG_S x17, PT_A7(sp)
+	REG_S x18, PT_S2(sp)
+	REG_S x19, PT_S3(sp)
+	REG_S x20, PT_S4(sp)
+	REG_S x21, PT_S5(sp)
+	REG_S x22, PT_S6(sp)
+	REG_S x23, PT_S7(sp)
+	REG_S x24, PT_S8(sp)
+	REG_S x25, PT_S9(sp)
+	REG_S x26, PT_S10(sp)
+	REG_S x27, PT_S11(sp)
+	REG_S x28, PT_T3(sp)
+	REG_S x29, PT_T4(sp)
+	REG_S x30, PT_T5(sp)
+	REG_S x31, PT_T6(sp)
+	.endm
+
+	.macro restore_all_base_regs
+	REG_L x3,  PT_GP(sp)
+	REG_L x4,  PT_TP(sp)
+	REG_L x5,  PT_T0(sp)
+	REG_L x6,  PT_T1(sp)
+	REG_L x7,  PT_T2(sp)
+	REG_L x8,  PT_S0(sp)
+	REG_L x9,  PT_S1(sp)
+	REG_L x10, PT_A0(sp)
+	REG_L x11, PT_A1(sp)
+	REG_L x12, PT_A2(sp)
+	REG_L x13, PT_A3(sp)
+	REG_L x14, PT_A4(sp)
+	REG_L x15, PT_A5(sp)
+	REG_L x16, PT_A6(sp)
+	REG_L x17, PT_A7(sp)
+	REG_L x18, PT_S2(sp)
+	REG_L x19, PT_S3(sp)
+	REG_L x20, PT_S4(sp)
+	REG_L x21, PT_S5(sp)
+	REG_L x22, PT_S6(sp)
+	REG_L x23, PT_S7(sp)
+	REG_L x24, PT_S8(sp)
+	REG_L x25, PT_S9(sp)
+	REG_L x26, PT_S10(sp)
+	REG_L x27, PT_S11(sp)
+	REG_L x28, PT_T3(sp)
+	REG_L x29, PT_T4(sp)
+	REG_L x30, PT_T5(sp)
+	REG_L x31, PT_T6(sp)
+	.endm
+
+ENTRY(kretprobe_trampoline)
+	addi sp, sp, -(PT_SIZE_ON_STACK)
+	save_all_base_regs
+
+	move a0, sp /* pt_regs */
+
+	call trampoline_probe_handler
+
+	/* use the result as the return-address */
+	move ra, a0
+
+	restore_all_base_regs
+	addi sp, sp, PT_SIZE_ON_STACK
+
+	ret
+ENDPROC(kretprobe_trampoline)

arch/riscv/kernel/probes/simulate-insn.c new file +85 -0

new file mode 100644
index 00000000..2519ce2
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+
+#include "decode-insn.h"
+#include "simulate-insn.h"
+
+static inline bool rv_insn_reg_get_val(struct pt_regs *regs, u32 index,
+				       unsigned long *ptr)
+{
+	if (index == 0)
+		*ptr = 0;
+	else if (index <= 31)
+		*ptr = *((unsigned long *)regs + index);
+	else
+		return false;
+
+	return true;
+}
+
+static inline bool rv_insn_reg_set_val(struct pt_regs *regs, u32 index,
+				       unsigned long val)
+{
+	if (index == 0)
+		return false;
+	else if (index <= 31)
+		*((unsigned long *)regs + index) = val;
+	else
+		return false;
+
+	return true;
+}
+
+bool __kprobes simulate_jal(u32 opcode, unsigned long addr, struct pt_regs *regs)
+{
+	/*
+	 *     31    30       21    20     19        12 11 7 6      0
+	 * imm [20] | imm[10:1] | imm[11] | imm[19:12] | rd | opcode
+	 *     1         10          1           8       5    JAL/J
+	 */
+	bool ret;
+	u32 imm;
+	u32 index = (opcode >> 7) & 0x1f;
+
+	ret = rv_insn_reg_set_val(regs, index, addr + 4);
+	if (!ret)
+		return ret;
+
+	imm  = ((opcode >> 21) & 0x3ff) << 1;
+	imm |= ((opcode >> 20) & 0x1)   << 11;
+	imm |= ((opcode >> 12) & 0xff)  << 12;
+	imm |= ((opcode >> 31) & 0x1)   << 20;
+
+	instruction_pointer_set(regs, addr + sign_extend32((imm), 20));
+
+	return ret;
+}
+
+bool __kprobes simulate_jalr(u32 opcode, unsigned long addr, struct pt_regs *regs)
+{
+	/*
+	 * 31          20 19 15 14 12 11 7 6      0
+	 *  offset[11:0] | rs1 | 010 | rd | opcode
+	 *      12         5      3    5    JALR/JR
+	 */
+	bool ret;
+	unsigned long base_addr;
+	u32 imm = (opcode >> 20) & 0xfff;
+	u32 rd_index = (opcode >> 7) & 0x1f;
+	u32 rs1_index = (opcode >> 15) & 0x1f;
+
+	ret = rv_insn_reg_set_val(regs, rd_index, addr + 4);
+	if (!ret)
+		return ret;
+
+	ret = rv_insn_reg_get_val(regs, rs1_index, &base_addr);
+	if (!ret)
+		return ret;
+
+	instruction_pointer_set(regs, (base_addr + sign_extend32((imm), 11))&~1);
+
+	return ret;
+}

arch/riscv/kernel/probes/simulate-insn.h new file +47 -0

new file mode 100644
index 00000000..a62d784
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _RISCV_KERNEL_PROBES_SIMULATE_INSN_H
+#define _RISCV_KERNEL_PROBES_SIMULATE_INSN_H
+
+#define __RISCV_INSN_FUNCS(name, mask, val)				\
+static __always_inline bool riscv_insn_is_##name(probe_opcode_t code)	\
+{									\
+	BUILD_BUG_ON(~(mask) & (val));					\
+	return (code & (mask)) == (val);				\
+}									\
+bool simulate_##name(u32 opcode, unsigned long addr,			\
+		     struct pt_regs *regs);
+
+#define RISCV_INSN_REJECTED(name, code)					\
+	do {								\
+		if (riscv_insn_is_##name(code)) {			\
+			return INSN_REJECTED;				\
+		}							\
+	} while (0)
+
+__RISCV_INSN_FUNCS(system,	0x7f, 0x73)
+__RISCV_INSN_FUNCS(fence,	0x7f, 0x0f)
+
+#define RISCV_INSN_SET_SIMULATE(name, code)				\
+	do {								\
+		if (riscv_insn_is_##name(code)) {			\
+			api->handler = simulate_##name;			\
+			return INSN_GOOD_NO_SLOT;			\
+		}							\
+	} while (0)
+
+__RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001)
+__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002)
+__RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001)
+__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002)
+__RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001)
+__RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001)
+__RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002)
+
+__RISCV_INSN_FUNCS(auipc,	0x7f, 0x17)
+__RISCV_INSN_FUNCS(branch,	0x7f, 0x63)
+
+__RISCV_INSN_FUNCS(jal,		0x7f, 0x6f)
+__RISCV_INSN_FUNCS(jalr,	0x707f, 0x67)
+
+#endif /* _RISCV_KERNEL_PROBES_SIMULATE_INSN_H */

arch/riscv/kernel/traps.c +9 -0

index ad14f44..19a788a 100644
@@ -12,6 +12,7 @@
 #include <linux/signal.h>
 #include <linux/kdebug.h>
 #include <linux/uaccess.h>
+#include <linux/kprobes.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/irq.h>

@@ -145,6 +146,14 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
 
 asmlinkage __visible void do_trap_break(struct pt_regs *regs)
 {
+#ifdef CONFIG_KPROBES
+	if (kprobe_single_step_handler(regs))
+		return;
+
+	if (kprobe_breakpoint_handler(regs))
+		return;
+#endif
+
 	if (user_mode(regs))
 		force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->epc);
 #ifdef CONFIG_KGDB

arch/riscv/mm/fault.c +4 -0

index 716d64e..6afd59d 100644
@@ -13,6 +13,7 @@
 #include <linux/perf_event.h>
 #include <linux/signal.h>
 #include <linux/uaccess.h>
+#include <linux/kprobes.h>
 
 #include <asm/ptrace.h>
 #include <asm/tlbflush.h>

@@ -39,6 +40,9 @@ asmlinkage void do_page_fault(struct pt_regs *regs)
 	tsk = current;
 	mm = tsk->mm;
 
+	if (kprobe_page_fault(regs, cause))
+		return;
+
 	/*
 	 * Fault-in kernel-space virtual memory on-demand.
 	 * The 'reference' page table is init_mm.pgd.

Quoted 26 lines · On Sat, 17 Oct 2020 00:06:13 PDT (-0700), [email protected] wrote:

From: Guo Ren <[email protected]>

This patch enables "kprobe & kretprobe" to work with ftrace interface. It utilized software breakpoint as single-step mechanism.

Some instructions which can't be single-step executed must be simulated in kernel execution slot, such as: branch, jal, auipc, la ...

Some instructions should be rejected for probing and we use a blacklist to filter, such as: ecall, ebreak, ...

We use ebreak & c.ebreak to replace origin instruction and the kprobe handler prepares an executable memory slot for out-of-line execution with a copy of the original instruction being probed. In execution slot we add ebreak behind original instruction to simulate a single-setp mechanism.

The patch is based on packi's work [1] and csky's work [2].
- The kprobes_trampoline.S is all from packi's patch
- The single-step mechanism is new designed for riscv without hw single-step trap
- The simulation codes are from csky
- Frankly, all codes refer to other archs' implementation

Quoted 5 lines

[1] https://lore.kernel.org/linux-riscv/[email protected]/
[2] https://lore.kernel.org/linux-csky/[email protected]/

Signed-off-by Guo Ren <[email protected]>

Co-Developed-by Patrick Stählin <[email protected]>

Checkpatch says

WARNING: 'Co-developed-by:' is the preferred signature form #108:

Co-Developed-by Patrick Stählin <[email protected]>

WARNING: Co-developed-by: must be immediately followed by Signed-off-by:
#108:

Co-Developed-by Patrick Stählin <[email protected]>

Acked-by Masami Hiramatsu <[email protected]>

There's some other checkpatch warnings throughout the patch set, but this is one I'm not supposed to fix up. Can one of you re-post the patches with the correct DCO?

From: Guo Ren <[email protected]>

It will cause warning messages:
echo function_graph > /sys/kernel/debug/tracing/current_tracer

[   47.691397] ------------[ cut here ]------------
[   47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x182/0x19a
[   47.694483] Modules linked in:
[   47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.698228] epc: ffffffe000204530 ra : ffffffe00020452c sp : ffffffe0023ffc20 gp : ffffffe0013e1fe0 tp : ffffffe0023e4e00 t0 : 0000000000000000
[   47.701872]  t1 : 000000000000000e t2 : 000000000000001b s0 : ffffffe0023ffc70 s1 : ffffffe000206850 a0 : 0000000000000000 a1 : ffffffffffffffff
[   47.705550]  a2 : 0000000000000000 a3 : ffffffe03af7c5e8 a4 : 0000000000000000 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000150b02d8
[   47.709159]  s2 : 0000000000000008 s3 : 0000000000000858 s4 : ffffffe0023ffc98 s5 : 0000000000000850 s6 : 0000000000000003 s7 : 0000000000000002
[   47.714135]  s8 : 0000000000000004 s9 : 0000000000000001 s10: 0000000000000001 s11: 0000000000000003 t3 : ffffffffffffe000 t4 : 0000000000d86254
[   47.716574]  t5 : 0000000000000005 t6 : 0000000000040000status: 0000000000000100 badaddr: 0000000000000000 cause: 0000000000000003
[   47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.722074] Call Trace:
[   47.722561] [<ffffffe000203c88>] walk_stackframe+0x0/0xc2
[   47.724608] [<ffffffe000b2020a>] show_stack+0x46/0x52
[   47.726246] [<ffffffe000b23c24>] dump_stack+0x90/0xb6
[   47.727672] [<ffffffe00020c9c4>] __warn+0x98/0xfa
[   47.729131] [<ffffffe0005f4be0>] report_bug+0xaa/0x11e
[   47.730624] [<ffffffe000203b74>] do_trap_break+0x96/0xfe
[   47.732448] [<ffffffe000201daa>] ret_from_exception+0x0/0x14
[   47.734341] [<ffffffe00020452c>] patch_insn_write+0x17e/0x19a
[   47.737235] irq event stamp: 39
[   47.738521] hardirqs last  enabled at (39): [<ffffffe000201d0a>] _save_context+0xa2/0xe6
[   47.741055] hardirqs last disabled at (38): [<ffffffe0002c2404>] multi_cpu_stop+0x130/0x166
[   47.743551] softirqs last  enabled at (0): [<ffffffe00020a4d2>] copy_process+0x430/0x1316
[   47.746031] softirqs last disabled at (0): [<0000000000000000>] 0x0
[   47.748617] ---[ end trace 88a1054faa6524ef ]---

Because the path of stop_machine(__ftrace_modify_code)-> ftrace_modify_all_code->...->patch_insn_write has no pair of lock&unlock text_mutex, so we shouldn't put assert here.

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/kernel/patch.c | 7 -------
 1 file changed, 7 deletions(-)

arch/riscv/kernel/patch.c +0 -7

index 0b55287..2ee5063 100644
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
 	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
-	/*
-	 * Before reaching here, it was expected to lock the text_mutex
-	 * already, so we don't need to give another lock here and could
-	 * ensure that it was safe between each cores.
-	 */
-	lockdep_assert_held(&text_mutex);
-
 	if (across_pages)
 		patch_map(addr + len, FIX_TEXT_POKE1);
 

Signature · 1 line

2.7.4

Quoted 31 lines · On Sat, 17 Oct 2020 07:06:17 +0000 [email protected] wrote:

From: Guo Ren <[email protected]>

It will cause warning messages:
echo function_graph > /sys/kernel/debug/tracing/current_tracer

[   47.691397] ------------[ cut here ]------------
[   47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x182/0x19a
[   47.694483] Modules linked in:
[   47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.698228] epc: ffffffe000204530 ra : ffffffe00020452c sp : ffffffe0023ffc20 gp : ffffffe0013e1fe0 tp : ffffffe0023e4e00 t0 : 0000000000000000
[   47.701872]  t1 : 000000000000000e t2 : 000000000000001b s0 : ffffffe0023ffc70 s1 : ffffffe000206850 a0 : 0000000000000000 a1 : ffffffffffffffff
[   47.705550]  a2 : 0000000000000000 a3 : ffffffe03af7c5e8 a4 : 0000000000000000 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000150b02d8
[   47.709159]  s2 : 0000000000000008 s3 : 0000000000000858 s4 : ffffffe0023ffc98 s5 : 0000000000000850 s6 : 0000000000000003 s7 : 0000000000000002
[   47.714135]  s8 : 0000000000000004 s9 : 0000000000000001 s10: 0000000000000001 s11: 0000000000000003 t3 : ffffffffffffe000 t4 : 0000000000d86254
[   47.716574]  t5 : 0000000000000005 t6 : 0000000000040000status: 0000000000000100 badaddr: 0000000000000000 cause: 0000000000000003
[   47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.722074] Call Trace:
[   47.722561] [<ffffffe000203c88>] walk_stackframe+0x0/0xc2
[   47.724608] [<ffffffe000b2020a>] show_stack+0x46/0x52
[   47.726246] [<ffffffe000b23c24>] dump_stack+0x90/0xb6
[   47.727672] [<ffffffe00020c9c4>] __warn+0x98/0xfa
[   47.729131] [<ffffffe0005f4be0>] report_bug+0xaa/0x11e
[   47.730624] [<ffffffe000203b74>] do_trap_break+0x96/0xfe
[   47.732448] [<ffffffe000201daa>] ret_from_exception+0x0/0x14
[   47.734341] [<ffffffe00020452c>] patch_insn_write+0x17e/0x19a
[   47.737235] irq event stamp: 39
[   47.738521] hardirqs last  enabled at (39): [<ffffffe000201d0a>] _save_context+0xa2/0xe6
[   47.741055] hardirqs last disabled at (38): [<ffffffe0002c2404>] multi_cpu_stop+0x130/0x166
[   47.743551] softirqs last  enabled at (0): [<ffffffe00020a4d2>] copy_process+0x430/0x1316
[   47.746031] softirqs last disabled at (0): [<0000000000000000>] 0x0
[   47.748617] ---[ end trace 88a1054faa6524ef ]---

Quoted 3 lines

Because the path of stop_machine(__ftrace_modify_code)-> ftrace_modify_all_code->...->patch_insn_write has no pair of lock&unlock text_mutex, so we shouldn't put assert here.

Hm, are there any chance to get the text_mutex in that path?
There are ftrace_arch_code_modify_prepare/post_process() in arch/riscv/kernel/ftrace.c, aren't those used?

Thank you,

Quoted 25 lines

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/kernel/patch.c | 7 -------
 1 file changed, 7 deletions(-)

arch/riscv/kernel/patch.c +0 -8

index 0b55287..2ee5063 100644
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
      bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
      int ret;
 
-     /*
-      * Before reaching here, it was expected to lock the text_mutex
-      * already, so we don't need to give another lock here and could
-      * ensure that it was safe between each cores.
-      */
-     lockdep_assert_held(&text_mutex);
-
      if (across_pages)
              patch_map(addr + len, FIX_TEXT_POKE1);
 
--

Quoted 2 lines

2.7.4

--
Masami Hiramatsu <[email protected]>

Quoted 31 lines · On Sat, 17 Oct 2020 07:06:17 +0000 [email protected] wrote:

From: Guo Ren <[email protected]>

It will cause warning messages:
echo function_graph > /sys/kernel/debug/tracing/current_tracer

[   47.691397] ------------[ cut here ]------------
[   47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x182/0x19a
[   47.694483] Modules linked in:
[   47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.698228] epc: ffffffe000204530 ra : ffffffe00020452c sp : ffffffe0023ffc20 gp : ffffffe0013e1fe0 tp : ffffffe0023e4e00 t0 : 0000000000000000
[   47.701872]  t1 : 000000000000000e t2 : 000000000000001b s0 : ffffffe0023ffc70 s1 : ffffffe000206850 a0 : 0000000000000000 a1 : ffffffffffffffff
[   47.705550]  a2 : 0000000000000000 a3 : ffffffe03af7c5e8 a4 : 0000000000000000 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000150b02d8
[   47.709159]  s2 : 0000000000000008 s3 : 0000000000000858 s4 : ffffffe0023ffc98 s5 : 0000000000000850 s6 : 0000000000000003 s7 : 0000000000000002
[   47.714135]  s8 : 0000000000000004 s9 : 0000000000000001 s10: 0000000000000001 s11: 0000000000000003 t3 : ffffffffffffe000 t4 : 0000000000d86254
[   47.716574]  t5 : 0000000000000005 t6 : 0000000000040000status: 0000000000000100 badaddr: 0000000000000000 cause: 0000000000000003
[   47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.722074] Call Trace:
[   47.722561] [<ffffffe000203c88>] walk_stackframe+0x0/0xc2
[   47.724608] [<ffffffe000b2020a>] show_stack+0x46/0x52
[   47.726246] [<ffffffe000b23c24>] dump_stack+0x90/0xb6
[   47.727672] [<ffffffe00020c9c4>] __warn+0x98/0xfa
[   47.729131] [<ffffffe0005f4be0>] report_bug+0xaa/0x11e
[   47.730624] [<ffffffe000203b74>] do_trap_break+0x96/0xfe
[   47.732448] [<ffffffe000201daa>] ret_from_exception+0x0/0x14
[   47.734341] [<ffffffe00020452c>] patch_insn_write+0x17e/0x19a
[   47.737235] irq event stamp: 39
[   47.738521] hardirqs last  enabled at (39): [<ffffffe000201d0a>] _save_context+0xa2/0xe6
[   47.741055] hardirqs last disabled at (38): [<ffffffe0002c2404>] multi_cpu_stop+0x130/0x166
[   47.743551] softirqs last  enabled at (0): [<ffffffe00020a4d2>] copy_process+0x430/0x1316
[   47.746031] softirqs last disabled at (0): [<0000000000000000>] 0x0
[   47.748617] ---[ end trace 88a1054faa6524ef ]---

Quoted 3 lines

Because the path of stop_machine(__ftrace_modify_code)-> ftrace_modify_all_code->...->patch_insn_write has no pair of lock&unlock text_mutex, so we shouldn't put assert here.

Hm, are there any chance to get the text_mutex in that path?
There are ftrace_arch_code_modify_prepare/post_process() in arch/riscv/kernel/ftrace.c, aren't those used?

Thank you,

Quoted 25 lines

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/kernel/patch.c | 7 -------
 1 file changed, 7 deletions(-)

arch/riscv/kernel/patch.c +0 -8

index 0b55287..2ee5063 100644
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
      bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
      int ret;
 
-     /*
-      * Before reaching here, it was expected to lock the text_mutex
-      * already, so we don't need to give another lock here and could
-      * ensure that it was safe between each cores.
-      */
-     lockdep_assert_held(&text_mutex);
-
      if (across_pages)
              patch_map(addr + len, FIX_TEXT_POKE1);
 
--

Quoted 2 lines

2.7.4

--
Masami Hiramatsu <[email protected]>

From: Guo Ren <[email protected]>

It will cause warning messages:
echo function_graph > /sys/kernel/debug/tracing/current_tracer

[   47.691397] ------------[ cut here ]------------
[   47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x182/0x19a
[   47.694483] Modules linked in:
[   47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.698228] epc: ffffffe000204530 ra : ffffffe00020452c sp : ffffffe0023ffc20 gp : ffffffe0013e1fe0 tp : ffffffe0023e4e00 t0 : 0000000000000000
[   47.701872]  t1 : 000000000000000e t2 : 000000000000001b s0 : ffffffe0023ffc70 s1 : ffffffe000206850 a0 : 0000000000000000 a1 : ffffffffffffffff
[   47.705550]  a2 : 0000000000000000 a3 : ffffffe03af7c5e8 a4 : 0000000000000000 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000150b02d8
[   47.709159]  s2 : 0000000000000008 s3 : 0000000000000858 s4 : ffffffe0023ffc98 s5 : 0000000000000850 s6 : 0000000000000003 s7 : 0000000000000002
[   47.714135]  s8 : 0000000000000004 s9 : 0000000000000001 s10: 0000000000000001 s11: 0000000000000003 t3 : ffffffffffffe000 t4 : 0000000000d86254
[   47.716574]  t5 : 0000000000000005 t6 : 0000000000040000status: 0000000000000100 badaddr: 0000000000000000 cause: 0000000000000003
[   47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.722074] Call Trace:
[   47.722561] [<ffffffe000203c88>] walk_stackframe+0x0/0xc2
[   47.724608] [<ffffffe000b2020a>] show_stack+0x46/0x52
[   47.726246] [<ffffffe000b23c24>] dump_stack+0x90/0xb6
[   47.727672] [<ffffffe00020c9c4>] __warn+0x98/0xfa
[   47.729131] [<ffffffe0005f4be0>] report_bug+0xaa/0x11e
[   47.730624] [<ffffffe000203b74>] do_trap_break+0x96/0xfe
[   47.732448] [<ffffffe000201daa>] ret_from_exception+0x0/0x14
[   47.734341] [<ffffffe00020452c>] patch_insn_write+0x17e/0x19a
[   47.737235] irq event stamp: 39
[   47.738521] hardirqs last  enabled at (39): [<ffffffe000201d0a>] _save_context+0xa2/0xe6
[   47.741055] hardirqs last disabled at (38): [<ffffffe0002c2404>] multi_cpu_stop+0x130/0x166
[   47.743551] softirqs last  enabled at (0): [<ffffffe00020a4d2>] copy_process+0x430/0x1316
[   47.746031] softirqs last disabled at (0): [<0000000000000000>] 0x0
[   47.748617] ---[ end trace 88a1054faa6524ef ]---

Because the path of stop_machine(__ftrace_modify_code)-> ftrace_modify_all_code->...->patch_insn_write has no pair of lock&unlock text_mutex, so we shouldn't put assert here.

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/kernel/patch.c | 7 -------
 1 file changed, 7 deletions(-)

arch/riscv/kernel/patch.c +0 -8

index 0b55287..2ee5063 100644
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
        bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
        int ret;
 
-       /*
-        * Before reaching here, it was expected to lock the text_mutex
-        * already, so we don't need to give another lock here and could
-        * ensure that it was safe between each cores.
-        */
-       lockdep_assert_held(&text_mutex);
-
        if (across_pages)
                patch_map(addr + len, FIX_TEXT_POKE1);
 
--

2.7.4

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 32 lines · On Sat, Oct 17, 2020 at 12:07 AM <[email protected]> wrote:

From: Guo Ren <[email protected]>

It will cause warning messages:
echo function_graph > /sys/kernel/debug/tracing/current_tracer

[   47.691397] ------------[ cut here ]------------
[   47.692899] WARNING: CPU: 0 PID: 11 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x182/0x19a
[   47.694483] Modules linked in:
[   47.695754] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.698228] epc: ffffffe000204530 ra : ffffffe00020452c sp : ffffffe0023ffc20 gp : ffffffe0013e1fe0 tp : ffffffe0023e4e00 t0 : 0000000000000000
[   47.701872]  t1 : 000000000000000e t2 : 000000000000001b s0 : ffffffe0023ffc70 s1 : ffffffe000206850 a0 : 0000000000000000 a1 : ffffffffffffffff
[   47.705550]  a2 : 0000000000000000 a3 : ffffffe03af7c5e8 a4 : 0000000000000000 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 00000000150b02d8
[   47.709159]  s2 : 0000000000000008 s3 : 0000000000000858 s4 : ffffffe0023ffc98 s5 : 0000000000000850 s6 : 0000000000000003 s7 : 0000000000000002
[   47.714135]  s8 : 0000000000000004 s9 : 0000000000000001 s10: 0000000000000001 s11: 0000000000000003 t3 : ffffffffffffe000 t4 : 0000000000d86254
[   47.716574]  t5 : 0000000000000005 t6 : 0000000000040000status: 0000000000000100 badaddr: 0000000000000000 cause: 0000000000000003
[   47.720019] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.9.0-11367-g1054335 #132
[   47.722074] Call Trace:
[   47.722561] [<ffffffe000203c88>] walk_stackframe+0x0/0xc2
[   47.724608] [<ffffffe000b2020a>] show_stack+0x46/0x52
[   47.726246] [<ffffffe000b23c24>] dump_stack+0x90/0xb6
[   47.727672] [<ffffffe00020c9c4>] __warn+0x98/0xfa
[   47.729131] [<ffffffe0005f4be0>] report_bug+0xaa/0x11e
[   47.730624] [<ffffffe000203b74>] do_trap_break+0x96/0xfe
[   47.732448] [<ffffffe000201daa>] ret_from_exception+0x0/0x14
[   47.734341] [<ffffffe00020452c>] patch_insn_write+0x17e/0x19a
[   47.737235] irq event stamp: 39
[   47.738521] hardirqs last  enabled at (39): [<ffffffe000201d0a>] _save_context+0xa2/0xe6
[   47.741055] hardirqs last disabled at (38): [<ffffffe0002c2404>] multi_cpu_stop+0x130/0x166
[   47.743551] softirqs last  enabled at (0): [<ffffffe00020a4d2>] copy_process+0x430/0x1316
[   47.746031] softirqs last disabled at (0): [<0000000000000000>] 0x0
[   47.748617] ---[ end trace 88a1054faa6524ef ]---

Quoted 4 lines

Because the path of stop_machine(__ftrace_modify_code)-> ftrace_modify_all_code->...->patch_insn_write has no pair of lock&unlock text_mutex, so we shouldn't put assert here.

What platform are you testing ? We are seeing a crash while enabling any of the tracers multiple times
on Qemu/HiFive Unleashed.

http://lists.infradead.org/pipermail/linux-riscv/2020-October/002354.html

As per the thread, Zong agreed to take a look into that. @Zong Li : Do you have any update regarding the ftrace issue.

Quoted 27 lines

Signed-off-by Guo Ren <[email protected]>

---
 arch/riscv/kernel/patch.c | 7 -------
 1 file changed, 7 deletions(-)

arch/riscv/kernel/patch.c +0 -8

index 0b55287..2ee5063 100644
@@ -55,13 +55,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
        bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
        int ret;
 
-       /*
-        * Before reaching here, it was expected to lock the text_mutex
-        * already, so we don't need to give another lock here and could
-        * ensure that it was safe between each cores.
-        */
-       lockdep_assert_held(&text_mutex);
-
        if (across_pages)
                patch_map(addr + len, FIX_TEXT_POKE1);
 
--

2.7.4

Quoted 4 lines

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Regards,
Atish

Quoted 3 lines

What platform are you testing ? We are seeing a crash while enabling any of the tracers multiple times
on Qemu/HiFive Unleashed.

I use qemu for testing. I've changed dynamic ftrace mechanism from mcount to -fpatchable-entry.

The problem is made by the lockdep checking of text_mutex.

Quoted 6 lines · On Tue, 20 Oct 2020 19:18:01 +0800 Guo Ren <[email protected]> wrote:

Quoted 3 lines

What platform are you testing ? We are seeing a crash while enabling any of the tracers multiple times
on Qemu/HiFive Unleashed.

I use qemu for testing. I've changed dynamic ftrace mechanism from mcount to -fpatchable-entry.

Quoted 1 line

The problem is made by the lockdep checking of text_mutex.

If you are switching to "patchable-entry" you shouldn't need to use stop_machine for the updates. No?

-- Steve

Quoted 6 lines · On Tue, 20 Oct 2020 19:18:01 +0800 Guo Ren <[email protected]> wrote:

Quoted 3 lines

What platform are you testing ? We are seeing a crash while enabling any of the tracers multiple times
on Qemu/HiFive Unleashed.

I use qemu for testing. I've changed dynamic ftrace mechanism from mcount to -fpatchable-entry.

Quoted 1 line

The problem is made by the lockdep checking of text_mutex.

If you are switching to "patchable-entry" you shouldn't need to use stop_machine for the updates. No?

-- Steve