ARM64: Uprobe support added

This patch adds support for uprobe on ARM64 architecture.

arch/arm64/kernel/probes/uprobes.c new file +4 -0

+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+

Quoted 2 lines · Pratyush Anand

This patch adds support for uprobe on ARM64 architecture.

I know nothing about ARM, so I can't actually review this change. But it looks good to me ;)

Just one note,

Quoted 12 lines

arch/arm64/kernel/probes/uprobes.c new file +12 -0

+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	/* saved fault code is restored in post_xol */
+	utask->autask.saved_fault_code = current->thread.fault_code;
+
+	/* An invalid fault code between pre/post xol event */
+	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	user_enable_single_step(current);

I don't think we want user_{enable,disable{_single_step in the long term, please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs the same.

However, I agree we can do this later and initial version can use these ptrace helpers.

ARM64 kprobe support is lying in torvalds/linux.git:master now. Therefore sending my uprobe patches which were dependent on decode-insn code of kprobe patches.

Unit tests for following have been done so far and they have been found working.
1. Step-able instructions, like sub, ldr, add etc.
2. Simulation-able like ret, cbnz, cbz etc.
3. uretprobe
4. Reject-able instructions like sev, wfe etc.
5. trapped and abort xol path
6. probe at unaligned user address.
7. longjump test cases

Currently it does not support aarch32 instruction probing.

RFC patches were sent long back, and all review comments for RFCs have been incorporated. RFCs were here: https://lwn.net/Articles/648514/

Pratyush Anand (5):
arm64: kprobe: protect/rename few definitions to be reused by uprobe
arm64: kgdb_step_brk_fn: ignore other's exception
arm64: Handle TRAP_HWBRKPT for user mode as well
arm64: Handle TRAP_BRKPT for user mode as well
arm64: Add uprobe support

 arch/arm64/Kconfig                      |   3 +
 arch/arm64/include/asm/debug-monitors.h |   3 +
 arch/arm64/include/asm/probes.h         |  23 ++--
 arch/arm64/include/asm/ptrace.h         |   8 ++
 arch/arm64/include/asm/thread_info.h    |   5 +-
 arch/arm64/include/asm/uprobes.h        |  37 ++++++
 arch/arm64/kernel/debug-monitors.c      |  40 +++---
 arch/arm64/kernel/entry.S               |   6 +-
 arch/arm64/kernel/kgdb.c                |   3 +
 arch/arm64/kernel/probes/Makefile       |   2 +
 arch/arm64/kernel/probes/decode-insn.c  |  31 ++---
 arch/arm64/kernel/probes/decode-insn.h  |   8 +-
 arch/arm64/kernel/probes/kprobes.c      |  36 ++---
 arch/arm64/kernel/probes/uprobes.c      | 227 ++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c              |   4 +-
 arch/arm64/mm/flush.c                   |   6 +
 16 files changed, 378 insertions(+), 64 deletions(-)
 create mode 100644 arch/arm64/include/asm/uprobes.h
 create mode 100644 arch/arm64/kernel/probes/uprobes.c

Signature · 1 line

2.5.5

Quoted 6 lines

I don't think we want user_{enable,disable{_single_step in the long term, please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs the same.

IIUC, then you mean that TIF_SINGLESTEP is a per task flag,

while
arch_uprobe_pre/post_xol() should enable/disable single stepping using a per uprobe_task,

and we should have a flag in "struct arch_uprobe_task" to handle this, right?

Quoted 2 lines

However, I agree we can do this later and initial version can use these ptrace helpers.

Yes, I would also like to do that change latter, because these set of patches have already been tested heavily with systemtap, so it would be better to go with an incremental changes latter on.

Quoted 7 lines · Pratyush Anand

Quoted 6 lines

I don't think we want user_{enable,disable{_single_step in the long term, please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs the same.

Quoted 1 line

IIUC, then you mean that TIF_SINGLESTEP is a per task flag,

Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse each other. And uprobes simply doesn't need to set/clear it.

Quoted 3 lines

while
arch_uprobe_pre/post_xol() should enable/disable single stepping using a per uprobe_task,

I can't really answer since I know nothing about arm. x86 just needs to set X86_EFLAGS_TF, I guess arm needs to modify some register too?

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Pratyush Anand

Quoted 6 lines

I don't think we want user_{enable,disable{_single_step in the long term, please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs the same.

Quoted 4 lines

Quoted 1 line

IIUC, then you mean that TIF_SINGLESTEP is a per task flag,

Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse each other. And uprobes simply doesn't need to set/clear it.

We're already using it for kprobes, hw_breakpoint and kgdb as well as ptrace, so I'd rather uprobes either followed existing practice, or we converted everybody off the current code.

In what way do things get confused?

Quoted 2 lines

I can't really answer since I know nothing about arm. x86 just needs to set X86_EFLAGS_TF, I guess arm needs to modify some register too?

We have {user,kernel}_{enable,disable}_single_step for managing the various registers controlling the single-step state machine on arm64.

arch/arm64/kernel/debug-monitors.c +2 -0

@@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
 static int single_step_handler(unsigned long addr, unsigned int esr,
 			       struct pt_regs *regs)
 {
+	bool handler_found = false;
+
 	/*
 	 * If we are stepping a pending breakpoint, call the hw_breakpoint
 	 * handler first.

@@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	if (!reinstall_suspended_bps(regs))
 		return 0;
 

arch/arm64/kernel/debug-monitors.c +5 -6

@@ -326,16 +326,20 @@ NOKPROBE_SYMBOL(call_break_hook);
 static int brk_handler(unsigned long addr, unsigned int esr,
 		       struct pt_regs *regs)
 {
-	if (user_mode(regs)) {
-		send_user_sigtrap(TRAP_BRKPT);
-	}
+	bool handler_found = false;
+
 #ifdef	CONFIG_KPROBES
-	else if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
-		if (kprobe_breakpoint_handler(regs, esr) != DBG_HOOK_HANDLED)
-			return -EFAULT;
+	if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
+		if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
+			handler_found = true;
 	}
 #endif

Quoted 15 lines · Pratyush Anand

arch/arm64/kernel/debug-monitors.c +2 -0

@@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
  static int single_step_handler(unsigned long addr, unsigned int esr,
  			       struct pt_regs *regs)
  {
+	bool handler_found = false;
+
  	/*
  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
  	 * handler first.

@@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
  	if (!reinstall_suspended_bps(regs))
  		return 0;
 

arch/arm64/kernel/debug-monitors.c static int single_step_handler(unsigned long addr, unsigned int esr, · line 266

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +8 -1

-	if (user_mode(regs)) {
+#ifdef	CONFIG_KPROBES
+	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+#endif
+	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+
+	if (!handler_found && user_mode(regs)) {
  		send_user_sigtrap(TRAP_HWBKPT);

Could we register kprobe_single_step_handler() via register_set_hook() and only invoke call_step_hook() above?

Quoted 16 lines · Catalin Marinas

Quoted 15 lines · Pratyush Anand

arch/arm64/kernel/debug-monitors.c +2 -0

@@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
  static int single_step_handler(unsigned long addr, unsigned int esr,
  			       struct pt_regs *regs)
  {
+	bool handler_found = false;
+
  	/*
  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
  	 * handler first.

@@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
  	if (!reinstall_suspended_bps(regs))
  		return 0;
 

Quoted 14 lines

arch/arm64/kernel/debug-monitors.c static int single_step_handler(unsigned long addr, unsigned int esr, · line 266

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +8 -1

-	if (user_mode(regs)) {
+#ifdef	CONFIG_KPROBES
+	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+#endif
+	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+
+	if (!handler_found && user_mode(regs)) {
  		send_user_sigtrap(TRAP_HWBKPT);

Could we register kprobe_single_step_handler() via register_set_hook() and only invoke call_step_hook() above?

I seem to recall a criticism of doing that in a much earlier kprobes64 patch of mine. The concern was that it would cause unnecessarily more kernel functions to be kprobes-blacklisted. Hence the hardcoded check and call.

Quoted 16 lines · Catalin Marinas

Quoted 15 lines · Pratyush Anand

arch/arm64/kernel/debug-monitors.c +2 -0

@@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
  static int single_step_handler(unsigned long addr, unsigned int esr,
  			       struct pt_regs *regs)
  {
+	bool handler_found = false;
+
  	/*
  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
  	 * handler first.

@@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
  	if (!reinstall_suspended_bps(regs))
  		return 0;
 

Quoted 13 lines

arch/arm64/kernel/debug-monitors.c static int single_step_handler(unsigned long addr, unsigned int esr, · line 266

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +8 -1

-	if (user_mode(regs)) {
+#ifdef	CONFIG_KPROBES
+	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+#endif
+	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+
+	if (!handler_found && user_mode(regs)) {
  		send_user_sigtrap(TRAP_HWBKPT);

Could we register kprobe_single_step_handler() via register_set_hook() and only invoke call_step_hook() above?

I seem to recall a criticism of doing that in a much earlier kprobes64 patch of mine. The concern was that it would cause unnecessarily more kernel functions to be kprobes-blacklisted. Hence the hardcoded check and call.

Hi Will/Catalin,

Do you have any specific comment for this patch set?

~Pratyush

[1] https://lkml.org/lkml/2016/8/22/69

Quoted 48 lines · Pratyush Anand

ARM64 kprobe support is lying in torvalds/linux.git:master now. Therefore sending my uprobe patches which were dependent on decode-insn code of kprobe patches.

Unit tests for following have been done so far and they have been found working.
1. Step-able instructions, like sub, ldr, add etc.
2. Simulation-able like ret, cbnz, cbz etc.
3. uretprobe
4. Reject-able instructions like sev, wfe etc.
5. trapped and abort xol path
6. probe at unaligned user address.
7. longjump test cases

Currently it does not support aarch32 instruction probing.

RFC patches were sent long back, and all review comments for RFCs have been incorporated. RFCs were here: https://lwn.net/Articles/648514/

Pratyush Anand (5):
arm64: kprobe: protect/rename few definitions to be reused by uprobe
arm64: kgdb_step_brk_fn: ignore other's exception
arm64: Handle TRAP_HWBRKPT for user mode as well
arm64: Handle TRAP_BRKPT for user mode as well
arm64: Add uprobe support

 arch/arm64/Kconfig                      |   3 +
 arch/arm64/include/asm/debug-monitors.h |   3 +
 arch/arm64/include/asm/probes.h         |  23 ++--
 arch/arm64/include/asm/ptrace.h         |   8 ++
 arch/arm64/include/asm/thread_info.h    |   5 +-
 arch/arm64/include/asm/uprobes.h        |  37 ++++++
 arch/arm64/kernel/debug-monitors.c      |  40 +++---
 arch/arm64/kernel/entry.S               |   6 +-
 arch/arm64/kernel/kgdb.c                |   3 +
 arch/arm64/kernel/probes/Makefile       |   2 +
 arch/arm64/kernel/probes/decode-insn.c  |  31 ++---
 arch/arm64/kernel/probes/decode-insn.h  |   8 +-
 arch/arm64/kernel/probes/kprobes.c      |  36 ++---
 arch/arm64/kernel/probes/uprobes.c      | 227 ++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c              |   4 +-
 arch/arm64/mm/flush.c                   |   6 +
 16 files changed, 378 insertions(+), 64 deletions(-)
 create mode 100644 arch/arm64/include/asm/uprobes.h
 create mode 100644 arch/arm64/kernel/probes/uprobes.c

--
2.5.5

arch/arm64/include/asm/uprobes.h new file +9 -0

@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2014-2015 Pratyush Anand <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _ASM_UPROBES_H

arch/arm64/include/asm/uprobes.h new file +7 -0

+#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES
+
+struct arch_uprobe_task {
+	unsigned long saved_fault_code;
+};
+
+struct arch_uprobe {

arch/arm64/kernel/probes/uprobes.c new file +12 -0

@@ -0,0 +1,227 @@
+/*
+ * Copyright (C) 2014-2015 Pratyush Anand <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/highmem.h>
+#include <linux/ptrace.h>
+#include <linux/uprobes.h>
+
+#include "decode-insn.h"

arch/arm64/kernel/probes/uprobes.c new file +7 -0

+
+	preempt_enable();
+
+	kunmap_atomic(xol_page_kaddr);
+}
+
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+	if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
+		return -EINVAL;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->api)) {

arch/arm64/kernel/probes/uprobes.c new file +10 -0

+
+	/* An invalid fault code between pre/post xol event */
+	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);
+
+	user_enable_single_step(current);
+
+	return 0;

Hi Pratyush,

arch/arm64/include/asm/debug-monitors.h line 75

Quoted 9 lines · Pratyush Anand

arch/arm64/include/asm/debug-monitors.h +3 -0

@@ -70,6 +70,9 @@
 #define BRK64_ESR_MASK		0xFFFF
 #define BRK64_ESR_KPROBES	0x0004
 #define BRK64_OPCODE_KPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
+/* uprobes BRK opcodes with ESR encoding  */
+#define BRK64_ESR_UPROBES	0x0008
+#define BRK64_OPCODE_UPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))

You can use 0x0005 as immediate, we don't need individual bits here.

Quoted 7 lines

arch/arm64/include/asm/probes.h +0 -0

@@ -35,4 +35,8 @@ struct arch_specific_insn {
 };
 #endif
 

arch/arm64/include/asm/probes.h struct arch_specific_insn { · line 40

Quoted 3 lines

arch/arm64/include/asm/probes.h +3 -0

+#ifdef CONFIG_UPROBES
+typedef u32 uprobe_opcode_t;
+#endif

We don't need #ifdef around this typedef. Also, all the other architectures seem to have this typedef in asm/uprobes.h.

Quoted 7 lines

arch/arm64/include/asm/ptrace.h +0 -0

@@ -217,6 +217,14 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
 
 #include <asm-generic/ptrace.h>
 

arch/arm64/include/asm/ptrace.h int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task); · line 226

Quoted 7 lines

arch/arm64/include/asm/ptrace.h +7 -0

+#define procedure_link_pointer(regs)	((regs)->regs[30])
+
+static inline void procedure_link_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	procedure_link_pointer(regs) = val;
+}

Do you need these macro and function here? It seems that they are only used in arch_uretprobe_hijack_return_addr(), so you can just expand them in there, no need for additional definitions.

arch/arm64/include/asm/thread_info.h static inline struct thread_info *current_thread_info(void) · line 112

Quoted 8 lines

arch/arm64/include/asm/thread_info.h +1 -0

@@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
+#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */

Nitpick: you can just use 4 until we cover this gap.

Quoted 17 lines

arch/arm64/include/asm/uprobes.h new file +13 -0

@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2014-2015 Pratyush Anand <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _ASM_UPROBES_H
+#define _ASM_UPROBES_H
+
+#include <asm/debug-monitors.h>
+#include <asm/insn.h>

arch/arm64/include/asm/uprobes.h line 19

Quoted 6 lines

arch/arm64/include/asm/uprobes.h new file +6 -0

+#include <asm/probes.h>
+
+#define MAX_UINSN_BYTES		AARCH64_INSN_SIZE
+
+#define UPROBE_SWBP_INSN	BRK64_OPCODE_UPROBES
+#define UPROBE_SWBP_INSN_SIZE	4

Nitpick: for consistency, just use AARCH64_INSN_SIZE.

Quoted 14 lines

arch/arm64/include/asm/uprobes.h new file +13 -0

+#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES
+
+struct arch_uprobe_task {
+	unsigned long saved_fault_code;
+};
+
+struct arch_uprobe {
+	union {
+		u8 insn[MAX_UINSN_BYTES];
+		u8 ixol[MAX_UINSN_BYTES];
+	};
+	struct arch_probe_insn api;
+	bool simulate;

arch/arm64/include/asm/uprobes.h line 36

Quoted 4 lines

arch/arm64/include/asm/uprobes.h new file +4 -0

+};
+
+extern void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len);

I don't think we need this. It doesn't seem to be used anywhere other than the arm64 uprobes.c file. So I would rather expose sync_icache_aliases(), similarly to __sync_icache_dcache().

arch/arm64/kernel/entry.S ret_fast_syscall: · line 692

Quoted 10 lines

arch/arm64/kernel/entry.S +2 -1

@@ -688,7 +688,8 @@ ret_fast_syscall:
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
-	and	x2, x1, #_TIF_WORK_MASK
+	mov     x2, #_TIF_WORK_MASK
+	and     x2, x1, x2

Is this needed because _TIF_WORK_MASK cannot work as an immediate value to 'and'? We could reorder the TIF bits, they are not exposed to user to have ABI implications.

Quoted 5 lines

arch/arm64/kernel/entry.S +0 -0

 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
 	kernel_exit 0

arch/arm64/kernel/entry.S work_resched: · line 726

Quoted 9 lines

arch/arm64/kernel/entry.S +2 -1

@@ -718,7 +719,8 @@ work_resched:
 ret_to_user:
 	disable_irq				// disable interrupts
 	ldr	x1, [tsk, #TI_FLAGS]
-	and	x2, x1, #_TIF_WORK_MASK
+	mov     x2, #_TIF_WORK_MASK
+	and     x2, x1, x2
 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
 	kernel_exit 0

Same here.

Quoted 19 lines

arch/arm64/kernel/probes/uprobes.c new file +16 -0

@@ -0,0 +1,227 @@
+/*
+ * Copyright (C) 2014-2015 Pratyush Anand <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/highmem.h>
+#include <linux/ptrace.h>
+#include <linux/uprobes.h>
+
+#include "decode-insn.h"
+
+#define UPROBE_INV_FAULT_CODE	UINT_MAX
+
+void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,

arch/arm64/kernel/probes/uprobes.c line 22

Quoted 6 lines

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+		void *src, unsigned long len)
+{
+	void *xol_page_kaddr = kmap_atomic(page);
+	void *dst = xol_page_kaddr + (vaddr & ~PAGE_MASK);
+
+	preempt_disable();

kmap_atomic() already disabled preemption.

arch/arm64/kernel/probes/uprobes.c line 28

Quoted 7 lines

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+
+	/* Initialize the slot */
+	memcpy(dst, src, len);
+
+	/* flush caches (dcache/icache) */
+	flush_uprobe_xol_access(page, vaddr, dst, len);

Just use sync_icache_aliases() here (once exposed in an arm64 header).

Quoted 13 lines

arch/arm64/kernel/probes/uprobes.c new file +12 -0

+
+	preempt_enable();
+
+	kunmap_atomic(xol_page_kaddr);
+}
+
+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+	return instruction_pointer(regs);
+}
+
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,

arch/arm64/kernel/probes/uprobes.c line 45

Quoted 5 lines

arch/arm64/kernel/probes/uprobes.c new file +5 -0

+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

Quoted 12 lines

arch/arm64/kernel/probes/uprobes.c new file +11 -0

+	if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
+		return -EINVAL;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->api)) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;

arch/arm64/kernel/probes/uprobes.c line 62

Quoted 5 lines

arch/arm64/kernel/probes/uprobes.c new file +5 -0

+		break;
+
+	case INSN_GOOD:
+	default:
+		break;

Nitpick, we don't need case INSN_GOOD as well since default would cover it (that's unless you want to change default to BUG()).

arch/arm64/kernel/probes/uprobes.c line 73

Quoted 12 lines

arch/arm64/kernel/probes/uprobes.c new file +11 -0

+	}
+
+	return 0;
+}
+
+int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	/* saved fault code is restored in post_xol */
+	utask->autask.saved_fault_code = current->thread.fault_code;

Does the current->thread.fault_code has any meaning here? We use it in the arm64 code when delivering a signal to user but I don't think that's the case here, we are handling a breakpoint instruction and there isn't anything that set the fault_code.

Quoted 16 lines

arch/arm64/kernel/probes/uprobes.c new file +15 -0

+
+	/* An invalid fault code between pre/post xol event */
+	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);
+
+	user_enable_single_step(current);
+
+	return 0;
+}
+
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;

arch/arm64/kernel/probes/uprobes.c line 93

Quoted 5 lines

arch/arm64/kernel/probes/uprobes.c new file +5 -0

+
+	WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
+
+	/* restore fault code */
+	current->thread.fault_code = utask->autask.saved_fault_code;

Same here, I don't think this needs restoring if it wasn't meaningful in the first place.

Quoted 10 lines · Catalin Marinas

arch/arm64/include/asm/thread_info.h static inline struct thread_info *current_thread_info(void) · line 112

Quoted 7 lines · Pratyush Anand

arch/arm64/include/asm/thread_info.h +1 -0

@@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
+#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */

Nitpick: you can just use 4 until we cover this gap.

Hummm..as stated in commit log, Shi Yang suggested to define TIF_UPROBE as 5 in stead of 4, since 4 has already been used in -rt kernel. May be, I can put a comment in code as well.
Or, keep it 4 and -rt kernel will change their definitions. I am OK with both. let me know.

Quoted 5 lines

Quoted 5 lines

arch/arm64/kernel/entry.S +0 -0

@@ -688,7 +688,8 @@ ret_fast_syscall:
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK

Quoted 8 lines

arch/arm64/kernel/entry.S ret_fast_syscall: · line 692

Quoted 4 lines

arch/arm64/kernel/entry.S +2 -1

 	cbnz	x2, ret_fast_syscall_trace
-	and	x2, x1, #_TIF_WORK_MASK
+	mov     x2, #_TIF_WORK_MASK
+	and     x2, x1, x2

Is this needed because _TIF_WORK_MASK cannot work as an immediate value to 'and'? We could reorder the TIF bits, they are not exposed to user to have ABI implications.

_TIF_WORK_MASK is defined as follows:

#define _TIF_WORK_MASK          (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
                                  _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
                                  _TIF_UPROBE)

Re-ordering will not help, because 0-3 have already been used by previous definitions. Only way to use immediate value could be if, TIF_UPROBE is defined as 4.

Quoted 6 lines

Quoted 6 lines

arch/arm64/kernel/entry.S +6 -0

+unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
+{
+	return instruction_pointer(regs);
+}
+
+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,

Quoted 8 lines

arch/arm64/kernel/entry.S

Quoted 5 lines

arch/arm64/kernel/entry.S +5 -0

+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think we can not check that just by looking a single bit in an instruction. My understanding is that, we can only know about it when we are executing the instruction, by reading pstate, but that would not be useful for uprobe instruction analysis.

I hope, instruction encoding for aarch32 and aarch64 are different, and by analyzing for all types of aarch32 instructions, we will be able to decide that whether instruction is 32 bit trace-able or not. Accordingly, we can use either BRK or BKPT instruction for breakpoint generation.

Quoted 17 lines · Pratyush Anand

Quoted 9 lines · Catalin Marinas

arch/arm64/kernel/probes/uprobes.c line 45

Quoted 6 lines

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think we can not check that just by looking a single bit in an instruction. My understanding is that, we can only know about it when we are executing the instruction, by reading pstate, but that would not be useful for uprobe instruction analysis.

Quoted 4 lines

I hope, instruction encoding for aarch32 and aarch64 are different, and by analyzing for all types of aarch32 instructions, we will be able to decide that whether instruction is 32 bit trace-able or not. Accordingly, we can use either BRK or BKPT instruction for breakpoint generation.

We may have some unrelated instruction encoding overlapping but I haven't checked. I was more thinking about whether we know which task is being probed and check is_compat_task() or maybe using compat_user_mode(regs).

Quoted 23 lines · Catalin Marinas

Quoted 17 lines · Pratyush Anand

Quoted 9 lines · Catalin Marinas

arch/arm64/kernel/probes/uprobes.c line 45

Quoted 6 lines

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think we can not check that just by looking a single bit in an instruction. My understanding is that, we can only know about it when we are executing the instruction, by reading pstate, but that would not be useful for uprobe instruction analysis.

Quoted 4 lines

I hope, instruction encoding for aarch32 and aarch64 are different, and by analyzing for all types of aarch32 instructions, we will be able to decide that whether instruction is 32 bit trace-able or not. Accordingly, we can use either BRK or BKPT instruction for breakpoint generation.

Quoted 4 lines

We may have some unrelated instruction encoding overlapping but I haven't checked. I was more thinking about whether we know which task is being probed and check is_compat_task() or maybe using compat_user_mode(regs).

I had thought of this, but problem is that we might not have task in existence when we enable uprobes. For example: Lets say we are inserting a trace probe at offset 0x690 in a executable binary.

echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable

In the 'enable' step, it is decided that whether instruction is traceable or not.

(1) But at this point 'test' executable might not be running.
(2) Even if it is running, is_compat_task() or compat_user_mode() might not be usable, as they work with 'current' task.

What I was thinking that, let it go with 'TODO' as of now.

Later on, we propose some changes in core layer, so that we can read the elf headers of executable binary. ELFCLASS will be able to tell us, whether its a 32 bit or 64 bit executable. I think, moving "struct uprobe" from kernel/events/uprobes.c to a include/linux header file will do the job. "struct arch_uprobe" is part of "struct uprobe". "struct arch_uprobe" is passed in arch_uprobe_analyze_insn(). So, we can access struct uprobe's "inode" element with this change.

Quoted 33 lines · Pratyush Anand

Quoted 23 lines · Catalin Marinas

Quoted 17 lines · Pratyush Anand

Quoted 9 lines · Catalin Marinas

arch/arm64/kernel/probes/uprobes.c line 45

Quoted 6 lines

arch/arm64/kernel/probes/uprobes.c new file +6 -0

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think we can not check that just by looking a single bit in an instruction. My understanding is that, we can only know about it when we are executing the instruction, by reading pstate, but that would not be useful for uprobe instruction analysis.

Quoted 4 lines

I hope, instruction encoding for aarch32 and aarch64 are different, and by analyzing for all types of aarch32 instructions, we will be able to decide that whether instruction is 32 bit trace-able or not. Accordingly, we can use either BRK or BKPT instruction for breakpoint generation.

Quoted 4 lines

We may have some unrelated instruction encoding overlapping but I haven't checked. I was more thinking about whether we know which task is being probed and check is_compat_task() or maybe using compat_user_mode(regs).

I had thought of this, but problem is that we might not have task in existence when we enable uprobes. For example: Lets say we are inserting a trace probe at offset 0x690 in a executable binary.

Quoted 7 lines

echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable

In the 'enable' step, it is decided that whether instruction is traceable or not.

(1) But at this point 'test' executable might not be running.

Quoted 2 lines

(2) Even if it is running, is_compat_task() or compat_user_mode() might not be usable, as they work with 'current' task.

What I find strange is that uprobes allows you to insert a breakpoint instruction that's not even compatible with the task (so it would SIGILL rather than generate a debug exception).

Quoted 1 line

What I was thinking that, let it go with 'TODO' as of now.

Only that I don't have any guarantee that someone is going to fix it ;).

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

Quoted 40 lines · Catalin Marinas

Quoted 39 lines · Pratyush Anand

Quoted 23 lines · Catalin Marinas

Quoted 17 lines · Pratyush Anand

Quoted 9 lines · Catalin Marinas

Review

Quoted 6 lines

(quoted diff) +6 -0

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */

Is there a way to check (not necessarily in this file) that we don't probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think we can not check that just by looking a single bit in an instruction. My understanding is that, we can only know about it when we are executing the instruction, by reading pstate, but that would not be useful for uprobe instruction analysis.

Quoted 4 lines

I hope, instruction encoding for aarch32 and aarch64 are different, and by analyzing for all types of aarch32 instructions, we will be able to decide that whether instruction is 32 bit trace-able or not. Accordingly, we can use either BRK or BKPT instruction for breakpoint generation.

Quoted 4 lines

We may have some unrelated instruction encoding overlapping but I haven't checked. I was more thinking about whether we know which task is being probed and check is_compat_task() or maybe using compat_user_mode(regs).

I had thought of this, but problem is that we might not have task in existence when we enable uprobes. For example: Lets say we are inserting a trace probe at offset 0x690 in a executable binary.

echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable

In the 'enable' step, it is decided that whether instruction is traceable or not.

Quoted 1 line

Quoted 1 line

(1) But at this point 'test' executable might not be running.

Let me correct myself first here. When executable is not running, then, arch_uprobe_analyze_insn() is not called while uprobes enabling (ie writing '1' to 'enable'). In that case, it is called when binary is executed and task is created.

Quoted 9 lines

Quoted 2 lines

(2) Even if it is running, is_compat_task() or compat_user_mode() might not be usable, as they work with 'current' task.

What I find strange is that uprobes allows you to insert a breakpoint instruction that's not even compatible with the task (so it would SIGILL rather than generate a debug exception).

Quoted 1 line

What I was thinking that, let it go with 'TODO' as of now.

Quoted 4 lines

Only that I don't have any guarantee that someone is going to fix it ;).

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can return -EINVAL when mm->task_size < TASK_SIZE_64.

Quoted 3 lines · Pratyush Anand

Quoted 2 lines · Catalin Marinas

Quoted 1 line · Pratyush Anand

On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

Quoted 5 lines

Quoted 2 lines

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can return -EINVAL when mm->task_size < TASK_SIZE_64.

That's just a temporary workaround. If we ever merge ILP32, this test would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

Looking at prepare_uprobe(), we have a weak is_trap_insn() function. This check is meaningless without knowing which instruction set we target. A false positive here, however, is not that bad as we wouldn't end up inserting the wrong breakpoint in the executable. But it looks to me like the core uprobe code needs to pass some additional information like the type of task or ELF format to the arch code to make a useful choice of breakpoint type.

Quoted 11 lines · Catalin Marinas

Quoted 9 lines · Pratyush Anand

Quoted 5 lines · Catalin Marinas

Quoted 1 line · Pratyush Anand

On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can return -EINVAL when mm->task_size < TASK_SIZE_64.

Quoted 2 lines

That's just a temporary workaround. If we ever merge ILP32, this test would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

OK.. So what about doing something similar what x86 is doing. We can have a flag for task Type in arch specific mm_context_t. We also set this flag in COMPAT_SET_PERSONALITY() along with setting thread_info flag, and we clear them in SET_PERSONALITY().

Quoted 7 lines

Looking at prepare_uprobe(), we have a weak is_trap_insn() function. This check is meaningless without knowing which instruction set we target. A false positive here, however, is not that bad as we wouldn't end up inserting the wrong breakpoint in the executable. But it looks to me like the core uprobe code needs to pass some additional information like the type of task or ELF format to the arch code to make a useful choice of breakpoint type.

It seems that 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, may be we can use still weak is_trap_insn().

Quoted 13 lines · Pratyush Anand

Quoted 11 lines · Catalin Marinas

Quoted 9 lines · Pratyush Anand

Quoted 5 lines · Catalin Marinas

Quoted 1 line · Pratyush Anand

On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can return -EINVAL when mm->task_size < TASK_SIZE_64.

Quoted 7 lines

Quoted 2 lines

That's just a temporary workaround. If we ever merge ILP32, this test would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

OK.. So what about doing something similar what x86 is doing. We can have a flag for task Type in arch specific mm_context_t. We also set this flag in COMPAT_SET_PERSONALITY() along with setting thread_info flag, and we clear them in SET_PERSONALITY().

This looks like a better approach.

Quoted 8 lines

Quoted 7 lines

Looking at prepare_uprobe(), we have a weak is_trap_insn() function. This check is meaningless without knowing which instruction set we target. A false positive here, however, is not that bad as we wouldn't end up inserting the wrong breakpoint in the executable. But it looks to me like the core uprobe code needs to pass some additional information like the type of task or ELF format to the arch code to make a useful choice of breakpoint type.

Quoted 4 lines

It seems that 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, may be we can use still weak is_trap_insn().

Even if the is_trap_insn() check passes, we would reject the probe in arch_uprobe_analyze_insn() immediately after based on the mm type check, so not too bad.

If we add support for probing 32-bit tasks, I would rather have is_trap_insn() take the mm_struct as argument so that a non-weak implementation can check for the correct encoding.

Quoted 37 lines · Catalin Marinas

Quoted 13 lines · Pratyush Anand

Quoted 11 lines · Catalin Marinas

Quoted 9 lines · Pratyush Anand

Quoted 5 lines · Catalin Marinas

Quoted 1 line · Pratyush Anand

On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

As a quick workaround you could check mm->task_size > TASK_SIZE_32 in the arch_uprobe_analyze_insn() function.

It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can return -EINVAL when mm->task_size < TASK_SIZE_64.

Quoted 7 lines

Quoted 2 lines

That's just a temporary workaround. If we ever merge ILP32, this test would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

OK.. So what about doing something similar what x86 is doing. We can have a flag for task Type in arch specific mm_context_t. We also set this flag in COMPAT_SET_PERSONALITY() along with setting thread_info flag, and we clear them in SET_PERSONALITY().

This looks like a better approach.

Quoted 12 lines

Quoted 7 lines

Looking at prepare_uprobe(), we have a weak is_trap_insn() function. This check is meaningless without knowing which instruction set we target. A false positive here, however, is not that bad as we wouldn't end up inserting the wrong breakpoint in the executable. But it looks to me like the core uprobe code needs to pass some additional information like the type of task or ELF format to the arch code to make a useful choice of breakpoint type.

It seems that 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, may be we can use still weak is_trap_insn().

Quoted 3 lines

Even if the is_trap_insn() check passes, we would reject the probe in arch_uprobe_analyze_insn() immediately after based on the mm type check, so not too bad.

OK..I will have an always returning false from arm64 is_trap_insn() in v2.

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Looking at prepare_uprobe(), we have a weak is_trap_insn() function.

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

Quoted 6 lines

This check is meaningless without knowing which instruction set we target. A false positive here, however, is not that bad as we wouldn't end up inserting the wrong breakpoint in the executable. But it looks to me like the core uprobe code needs to pass some additional information like the type of task or ELF format to the arch code to make a useful choice of breakpoint type.

It seems that 'strtle r0, [r0], #160' would have the closest matching aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too seems a bad instruction. So, may be we can use still weak is_trap_insn().

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Even if the is_trap_insn() check passes, we would reject the probe in arch_uprobe_analyze_insn() immediately after based on the mm type check, so not too bad.

OK..I will have an always returning false from arm64 is_trap_insn() in v2.

For the time being, I think the default is_trap_insn() check is still useful on arm64.

The problem gets trickier when we add AArch32 support
as it may return 'true' on an AArch32 instruction that matches the AArch64 BRK (or vice-versa). That's when we need to either pass the mm to is_trap_insn() or simply return false and always perform the check in the arch_uprobe_analyze_insn() (which should, in addition, check for the trap instruction).

There is also the is_trap_at_addr() function which uses is_trap_insn(). I haven't checked the call paths here, are there any implications if is_trap_insn() always returns false?

Quoted 3 lines · Catalin Marinas

There is also the is_trap_at_addr() function which uses is_trap_insn(). I haven't checked the call paths here, are there any implications if is_trap_insn() always returns false?

I had looked into it and also tested that a tracepoint at an application having a same instruction as that of "uprobe break instruction" ie "BRK #0x5" is rejected. So, I think a false positive return from is_tarp_insn() is still OK.