[PATCH v13 05/10] arm64: Kprobes with single stepping support

On Mon, 13 Jun 2016 00:10:29 -0400
David Long [off-list ref] wrote:

quoted

quoted

---
  arch/arm64/Kconfig                      |   1 +
  arch/arm64/include/asm/debug-monitors.h |   5 +
  arch/arm64/include/asm/insn.h           |   4 +-
  arch/arm64/include/asm/kprobes.h        |  60 ++++
  arch/arm64/include/asm/probes.h         |  44 +++
  arch/arm64/kernel/Makefile              |   1 +
  arch/arm64/kernel/debug-monitors.c      |  18 +-
  arch/arm64/kernel/kprobes-arm64.c       | 144 +++++++++
  arch/arm64/kernel/kprobes-arm64.h       |  35 +++
  arch/arm64/kernel/kprobes.c             | 526 ++++++++++++++++++++++++++++++++

Not sure why kprobes.c and kprobes-arm64.c are splitted.

This comes from the model of the arm32 kprobes code where handling of 
the low-level instruction simulation is implemented in separate files 
for 32-bit vs. thumb instructions.  It should make a little more sense 
in the future when additional instruction simulation code will hopefully 
be added for those instructions we cannot currently single-step 
out-of-line.  It also probably *could* be merged into one file.

Hmm, at least the name of arch/arm64/kernel/kprobes-arm64.c is
meaningless. As we've done in x86, I think we can make it 
arch/arm64/kernel/kprobes/decode-insn.{c,h}


[..]

quoted

quoted

+
+/* Return:
+ *   INSN_REJECTED     If instruction is one not allowed to kprobe,
+ *   INSN_GOOD         If instruction is supported and uses instruction slot,
+ *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.

Is there any chance to return INSN_GOOD_NO_SLOT?

Ah, that gets used later when simulation support is added.  I've removed 
this enum value from this commit and will add it to the later one. 
Please no one complain about using an enum instead of a bool, it will 
eventually have three possible values.

OK :)

[..]

quoted

quoted

+enum kprobe_insn __kprobes
+arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi)
+{
+	enum kprobe_insn decoded;
+	kprobe_opcode_t insn = le32_to_cpu(*addr);
+	kprobe_opcode_t *scan_start = addr - 1;
+	kprobe_opcode_t *scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE;
+#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
+	struct module *mod;
+#endif
+
+	if (addr >= (kprobe_opcode_t *)_text &&
+	    scan_end < (kprobe_opcode_t *)_text)
+		scan_end = (kprobe_opcode_t *)_text;
+#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
+	else {
+		preempt_disable();
+		mod = __module_address((unsigned long)addr);
+		if (mod && within_module_init((unsigned long)addr, mod) &&
+			!within_module_init((unsigned long)scan_end, mod))
+			scan_end = (kprobe_opcode_t *)mod->init_layout.base;
+		else if (mod && within_module_core((unsigned long)addr, mod) &&
+			!within_module_core((unsigned long)scan_end, mod))
+			scan_end = (kprobe_opcode_t *)mod->core_layout.base;

What happen if mod == NULL? it should be return error, isn't it?

No, it should be fine.  It just means it didn't have to do either of the 
extra checks to limit the end of the search through the code to the 
boundary of one of the corresponding module text sections. It means the 
instruction is in the regular kernel (non-module) text segment.

Ah, I see. It is OK then. :)

Thank you,


-- 
Masami Hiramatsu [off-list ref]