ARM64: Uprobe support added

Its better to keep all BRK opcodes used by kprobes and uprobes at one place. Therefore move these defines to asm/insn.h.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/include/asm/insn.h | 6 ++++++
 arch/arm64/kernel/kprobes.h   | 7 +------
 2 files changed, 7 insertions(+), 6 deletions(-)

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

index e2ff32a93b5c..87fa48746806 100644

Quoted 17 lines · Pratyush Anand

Its better to keep all BRK opcodes used by kprobes and uprobes at one place. Therefore move these defines to asm/insn.h.

Signed-off-by Pratyush Anand <[email protected]>

---
  arch/arm64/include/asm/insn.h | 6 ++++++
  arch/arm64/kernel/kprobes.h   | 7 +------
  2 files changed, 7 insertions(+), 6 deletions(-)

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

index e2ff32a93b5c..87fa48746806 100644
@@ -23,6 +23,12 @@
  /* A64 instructions are always 32 bits. */
  #define	AARCH64_INSN_SIZE		4
 

arch/arm64/include/asm/insn.h line 29

Quoted 4 lines

arch/arm64/include/asm/insn.h +4 -0

+/* BRK opcodes with ESR encoding  */
+#define BRK64_ESR_MASK		0xFFFF
+#define BRK64_ESR_KPROBES	0x0004
+#define BRK64_OPCODE_KPROBES	0xD4200080	/* "brk 0x4" */

These might be better off in debug-monitors.h, but I guess that's for the kprobes series to sort out.

arch/arm64/include/asm/insn.h line 30

Quoted 1 line

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

+#define ARCH64_NOP_OPCODE	0xD503201F

We have aarch64_insn_gen_nop for this.

Will

Most of the stuff of kprobes-arm64.c can also be used by uprobes.c. So move all those part to common code area. In the process rename kprobe to probe whereever possible.

No functional change.

Quoted 5 lines · Pratyush Anand

Most of the stuff of kprobes-arm64.c can also be used by uprobes.c. So move all those part to common code area. In the process rename kprobe to probe whereever possible.

No functional change.

In which case, can you merge this into the kprobes series (which we haven't merged yet)?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines · Will Deacon

Quoted 4 lines · Pratyush Anand

Most of the stuff of kprobes-arm64.c can also be used by uprobes.c. So move all those part to common code area. In the process rename kprobe to probe whereever possible.

Quoted 5 lines

Quoted 1 line

No functional change.

In which case, can you merge this into the kprobes series (which we haven't merged yet)?

Yes, thats why these are just RFCs. I will send next version of uprobe only after kprobe patches are accepted into maintainer's tree.

Quoted 12 lines · Pratyush Anand

Quoted 10 lines · Will Deacon

Quoted 5 lines · Pratyush Anand

Most of the stuff of kprobes-arm64.c can also be used by uprobes.c. So move all those part to common code area. In the process rename kprobe to probe whereever possible.

No functional change.

In which case, can you merge this into the kprobes series (which we haven't merged yet)?

Quoted 2 lines

Yes, thats why these are just RFCs. I will send next version of uprobe only after kprobe patches are accepted into maintainer's tree.

Ok, but it also makes sense to make kprobes refactoring changes *before* the patches are merged, as that reduces churn in mainline whilst you don't have any other dependencies.

Will

Both ARM and ARM64 handle uprobe exceptions through their own hooks.So nothing to be done in arch_uprobe_exception_notify except to return NOTIFY_DONE. Implement this as weak default function and remove definition from arm arch code.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm/kernel/uprobes.c |  6 ------
 kernel/events/uprobes.c   | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

arch/arm/kernel/uprobes.c +0 -0

index 56adf9c1fde0..0f3663ca82fc 100644

Quoted 17 lines · Pratyush Anand

Both ARM and ARM64 handle uprobe exceptions through their own hooks.So nothing to be done in arch_uprobe_exception_notify except to return NOTIFY_DONE. Implement this as weak default function and remove definition from arm arch code.

Signed-off-by Pratyush Anand <[email protected]>

---
  arch/arm/kernel/uprobes.c |  6 ------
  kernel/events/uprobes.c   | 18 ++++++++++++++++++
  2 files changed, 18 insertions(+), 6 deletions(-)

arch/arm/kernel/uprobes.c +0 -0

index 56adf9c1fde0..0f3663ca82fc 100644

arch/arm/kernel/uprobes.c void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs) · line 185

Quoted 8 lines

arch/arm/kernel/uprobes.c +0 -5

@@ -178,12 +178,6 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
  	instruction_pointer_set(regs, utask->vaddr);
  }
 
-int arch_uprobe_exception_notify(struct notifier_block *self,
-				 unsigned long val, void *data)
-{
-	return NOTIFY_DONE;
-}

I agree, this is ugly. But I disagree with this change.

I think we should simply kill uprobe_exception_nb and unexport arch_uprobe_exception_notify on x86/powerpc, and in fact I was going to do this a long ago.

I'll send the patch later. Until then please add the dummy arch_uprobe_exception_notify() like arch/arm does, to make the generic code happy.

Oleg.

instruction_pointer_set is needed for uprobe implementation. Hence define it for ARM64.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/include/asm/ptrace.h | 6 ++++++
 1 file changed, 6 insertions(+)

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

index 24cc048ac3e7..29d9bf5e3635 100644

Quoted 16 lines · Pratyush Anand

instruction_pointer_set is needed for uprobe implementation. Hence define it for ARM64.

Signed-off-by Pratyush Anand <[email protected]>

---
  arch/arm64/include/asm/ptrace.h | 6 ++++++
  1 file changed, 6 insertions(+)

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

index 24cc048ac3e7..29d9bf5e3635 100644
@@ -206,6 +206,12 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
  #define instruction_pointer(regs)	((regs)->pc)
  #define stack_pointer(regs)		((regs)->sp)
 

arch/arm64/include/asm/ptrace.h static inline int valid_user_regs(struct user_pt_regs *regs) · line 213

Quoted 5 lines

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

+static inline void instruction_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	instruction_pointer(regs) = val;
+}

Do you think we could make use of the asm-generic ptrace header to get this for free? afaict, we just need to implement GET_USP and possibly GET_FP for the compat case (although it may well be that we still handle the compat case entirely separately).

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re-factor flush_ptrace_access to reuse vma independent part.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/mm/flush.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

arch/arm64/mm/flush.c +13 -2

index b6f14e8d2121..9a4dd6f39cfb 100644
@@ -34,19 +34,25 @@ void flush_cache_range(struct vm_area_struct *vma, unsigned long start,
 		__flush_icache_all();
 }
 
+static void __flush_ptrace_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len)
+{
+	unsigned long addr = (unsigned long)kaddr;
+
+	if (icache_is_aliasing()) {
+		__flush_dcache_area(kaddr, len);
+		__flush_icache_all();
+	} else {
+		flush_icache_range(addr, addr + len);
+	}
+}
+
 static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 				unsigned long uaddr, void *kaddr,
 				unsigned long len)
 {
-	if (vma->vm_flags & VM_EXEC) {
-		unsigned long addr = (unsigned long)kaddr;

Quoted 2 lines · Pratyush Anand

Re-factor flush_ptrace_access to reuse vma independent part.

But for what? The changelog should explain this.

Quoted 37 lines

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/mm/flush.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

arch/arm64/mm/flush.c +13 -7

index b6f14e8d2121..9a4dd6f39cfb 100644
@@ -34,19 +34,25 @@ void flush_cache_range(struct vm_area_struct *vma, unsigned long start,
 		__flush_icache_all();
 }
 
+static void __flush_ptrace_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len)
+{
+	unsigned long addr = (unsigned long)kaddr;
+
+	if (icache_is_aliasing()) {
+		__flush_dcache_area(kaddr, len);
+		__flush_icache_all();
+	} else {
+		flush_icache_range(addr, addr + len);
+	}
+}
+
 static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 				unsigned long uaddr, void *kaddr,
 				unsigned long len)
 {
-	if (vma->vm_flags & VM_EXEC) {
-		unsigned long addr = (unsigned long)kaddr;
-		if (icache_is_aliasing()) {
-			__flush_dcache_area(kaddr, len);
-			__flush_icache_all();
-		} else {
-			flush_icache_range(addr, addr + len);

arch/arm64/mm/flush.c void flush_cache_range(struct vm_area_struct *vma, unsigned long start, · line 56

Quoted 5 lines

arch/arm64/mm/flush.c +2 -2

-		}
-	}
+	if (vma->vm_flags & VM_EXEC)
+		__flush_ptrace_access(page, uaddr, kaddr, len);
 }

So why uprobes can't use flush_ptrace_access() ? flush_uprobe_xol_access() is called by arch_uprobe_copy_ixol(), and xol vma has VM_EXEC bit set.

Quoted 44 lines · Oleg Nesterov

Quoted 2 lines · Pratyush Anand

Re-factor flush_ptrace_access to reuse vma independent part.

But for what? The changelog should explain this.

Quoted 37 lines

Signed-off-by Pratyush Anand <[email protected]>

---
  arch/arm64/mm/flush.c | 24 +++++++++++++++---------
  1 file changed, 15 insertions(+), 9 deletions(-)

arch/arm64/mm/flush.c +13 -7

index b6f14e8d2121..9a4dd6f39cfb 100644
@@ -34,19 +34,25 @@ void flush_cache_range(struct vm_area_struct *vma, unsigned long start,
  		__flush_icache_all();
  }
 
+static void __flush_ptrace_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len)
+{
+	unsigned long addr = (unsigned long)kaddr;
+
+	if (icache_is_aliasing()) {
+		__flush_dcache_area(kaddr, len);
+		__flush_icache_all();
+	} else {
+		flush_icache_range(addr, addr + len);
+	}
+}
+
  static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
  				unsigned long uaddr, void *kaddr,
  				unsigned long len)
  {
-	if (vma->vm_flags & VM_EXEC) {
-		unsigned long addr = (unsigned long)kaddr;
-		if (icache_is_aliasing()) {
-			__flush_dcache_area(kaddr, len);
-			__flush_icache_all();
-		} else {
-			flush_icache_range(addr, addr + len);

Quoted 8 lines

arch/arm64/mm/flush.c void flush_cache_range(struct vm_area_struct *vma, unsigned long start, · line 56

Quoted 5 lines

arch/arm64/mm/flush.c +2 -2

-		}
-	}
+	if (vma->vm_flags & VM_EXEC)
+		__flush_ptrace_access(page, uaddr, kaddr, len);
  }

So why uprobes can't use flush_ptrace_access() ? flush_uprobe_xol_access() is called by arch_uprobe_copy_ixol(), and xol vma has VM_EXEC bit set.

Ah, sorry, I forgot that arch_uprobe_copy_ixol() doesn't have "vma" so it can't use flush_ptrace_access()... perhaps you can update the changelog.

Oleg.

uprobe registers a handler at step_hook. So, single_step_handler now checks for user mode as well if there is a valid hook.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/kernel/debug-monitors.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index b056369fd47d..2676b8655241 100644
@@ -236,6 +236,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	if (!reinstall_suspended_bps(regs))
 		return 0;
 
+	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
 	if (user_mode(regs)) {
 		info.si_signo = SIGTRAP;
 		info.si_errno = 0;

Let me repeat once again that I know absolutely nothing about arm* ;)

Quoted 24 lines · Pratyush Anand

uprobe registers a handler at step_hook. So, single_step_handler now checks for user mode as well if there is a valid hook.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/kernel/debug-monitors.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index b056369fd47d..2676b8655241 100644
@@ -236,6 +236,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	if (!reinstall_suspended_bps(regs))
 		return 0;
 
+	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
 	if (user_mode(regs)) {
 		info.si_signo = SIGTRAP;
 		info.si_errno = 0;

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

Quoted 6 lines

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

@@ -251,9 +254,6 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 		 */
 		user_rewind_single_step(current);
 	} else {
-		if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-			return 0;
-

Agreed, we need something like this change...

But did you verify that it can't break other users of register_step_hook() ? The current handlers do not check user_mode() == F, they assume that they can't be called otherwise.

If this all is correct, please explain why in the changelog.

Quoted 36 lines · Oleg Nesterov

Let me repeat once again that I know absolutely nothing about arm* ;)

Quoted 24 lines · Pratyush Anand

uprobe registers a handler at step_hook. So, single_step_handler now checks for user mode as well if there is a valid hook.

Signed-off-by Pratyush Anand <[email protected]>

---
  arch/arm64/kernel/debug-monitors.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

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

index b056369fd47d..2676b8655241 100644
@@ -236,6 +236,9 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
  	if (!reinstall_suspended_bps(regs))
  		return 0;
 
+	if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
  	if (user_mode(regs)) {
  		info.si_signo = SIGTRAP;
  		info.si_errno = 0;

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

Quoted 6 lines

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

@@ -251,9 +254,6 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
  		 */
  		user_rewind_single_step(current);
  	} else {
-		if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-			return 0;
-

Agreed, we need something like this change...

Quoted 5 lines

But did you verify that it can't break other users of register_step_hook() ? The current handlers do not check user_mode() == F, they assume that they can't be called otherwise.

If this all is correct, please explain why in the changelog.

I think you're right, and kgdb will need fixing with this change.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

+++ b/arch/arm64/kernel/uprobes.c

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

@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2014 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 <asm/debug-monitors.h>
+#include <asm/probes.h>
+
+#include "probes-arm64.h"
+
+#define UPROBE_INV_FAULT_CODE	UINT_MAX
+
+static LIST_HEAD(step_ctx);
+static DEFINE_RWLOCK(step_ctx_lock);
+
+static void add_ss_context(struct uprobe_task *utask)
+{
+	struct uprobe_step_ctx *ss_ctx = &utask->autask.ss_ctx;
+
+	ss_ctx->match_addr = utask->xol_vaddr;
+	write_lock(&step_ctx_lock);
+	list_add(&ss_ctx->node, &step_ctx);
+	write_unlock(&step_ctx_lock);
+}
+
+static struct uprobe_step_ctx *find_ss_context(unsigned long vaddr)
+{
+	struct uprobe_step_ctx *ss_ctx;
+
+	read_lock(&step_ctx_lock);

arch/arm64/include/asm/ptrace.h +34 -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;
+
+	/* Save user pc */
+	utask->autask.saved_user_pc = task_pt_regs(current)->user_regs.pc;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);
+
+	add_ss_context(utask);
+
+	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;
+
+	WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
+
+	/* restore fault code */
+	current->thread.fault_code = utask->autask.saved_fault_code;
+
+	/* restore user pc */
+	task_pt_regs(current)->user_regs.pc = utask->autask.saved_user_pc;

Hi Pratyush,

I'll try to actually read this patch (and the whole series) later, just a couple of quick questions for now.

Quoted 5 lines · Pratyush Anand

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

@@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
 

arch/arm64/include/asm/ptrace.h static inline int valid_user_regs(struct user_pt_regs *regs) · line 208

Quoted 3 lines

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

  #define instruction_pointer(regs)	((regs)->pc)
  #define stack_pointer(regs)		((regs)->sp)
+#define procedure_link_pointer(regs)	((regs)->regs[30])

perhaps it makes sense to change (at least) arch_prepare_kretprobe() to use the new helper? OK, we can do this later.

arch/arm64/include/asm/ptrace.h static inline int valid_user_regs(struct user_pt_regs *regs)

Quoted 6 lines

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

+/* Single step context for uprobe */
+struct uprobe_step_ctx {
+	struct list_head node;
+	unsigned long match_addr;
+};

I don't understand this... please see below.

arch/arm64/include/asm/ptrace.h

Quoted 5 lines

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

+struct arch_uprobe_task {
+	unsigned long saved_fault_code;
+	u64 saved_user_pc;
+	struct uprobe_step_ctx ss_ctx;
+};

saved_user_pc looks unneeded, you can rely on uprobe_task->vaddr ?

Quoted 38 lines

+++ b/arch/arm64/kernel/uprobes.c

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

@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2014 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 <asm/debug-monitors.h>
+#include <asm/probes.h>
+
+#include "probes-arm64.h"
+
+#define UPROBE_INV_FAULT_CODE	UINT_MAX
+
+static LIST_HEAD(step_ctx);
+static DEFINE_RWLOCK(step_ctx_lock);
+
+static void add_ss_context(struct uprobe_task *utask)
+{
+	struct uprobe_step_ctx *ss_ctx = &utask->autask.ss_ctx;
+
+	ss_ctx->match_addr = utask->xol_vaddr;
+	write_lock(&step_ctx_lock);
+	list_add(&ss_ctx->node, &step_ctx);
+	write_unlock(&step_ctx_lock);
+}
+
+static struct uprobe_step_ctx *find_ss_context(unsigned long vaddr)
+{
+	struct uprobe_step_ctx *ss_ctx;
+
+	read_lock(&step_ctx_lock);

arch/arm64/include/asm/ptrace.h line 45

Quoted 10 lines

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

+	list_for_each_entry(ss_ctx, &step_ctx, node)	{
+		if (ss_ctx->match_addr == vaddr) {
+			read_unlock(&step_ctx_lock);
+			return ss_ctx;
+		}
+	}
+	read_unlock(&step_ctx_lock);
+
+	return NULL;
+}

This looks very wrong to me, but perhaps because I do not understand why do we need these *_ss_context() helpers.

arch/arm64/include/asm/ptrace.h line 57

Quoted 12 lines

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

+static void del_ss_context(struct uprobe_task *utask)
+{
+	struct uprobe_step_ctx *ss_ctx = find_ss_context(utask->xol_vaddr);
+
+	if (ss_ctx) {
+		write_lock(&step_ctx_lock);
+		list_del(&ss_ctx->node);
+		write_unlock(&step_ctx_lock);
+	} else {
+		WARN_ON(1);
+	}
+}

Don't we need del_ss_context() in arch_uprobe_abort_xol() ? But this is minor.

Why we can trust find_ss_context() ? What if another thread also called add_ss_context() with the same (virtual) ->xol_vaddr ?

But the main question is: why do we need add/find_ss_context ?? Please explain.

Quoted 34 lines

arch/arm64/include/asm/ptrace.h +34 -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;
+
+	/* Save user pc */
+	utask->autask.saved_user_pc = task_pt_regs(current)->user_regs.pc;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);
+
+	add_ss_context(utask);
+
+	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;
+
+	WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
+
+	/* restore fault code */
+	current->thread.fault_code = utask->autask.saved_fault_code;
+
+	/* restore user pc */
+	task_pt_regs(current)->user_regs.pc = utask->autask.saved_user_pc;

arch/arm64/include/asm/ptrace.h line 101

Quoted 10 lines

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

+
+	/* Instruction point to execute next to breakpoint address */
+	instruction_pointer_set(regs, utask->vaddr + 4);
+
+	del_ss_context(utask);
+
+	user_disable_single_step(current);
+
+	return 0;
+}

task_pt_regs() above looks strange. We we can't use "struct pt_regs *regs" passed as an argument?

See also the note about .saved_user_pc above. I think you can use utask->vaddr instead.

And why do you need to play with ->user_regs.pc?? instruction_pointer_set() after that modifies the same word?

Or it is possible that regs != task_pt_regs(current) ? (to remind, I do not know arm ;)

Could you also explain

	instruction_pointer_set(regs, utask->vaddr + 4);

?

I mean, I do not understand why this is always correct. What if the probed insn is "jmp" (I do not know arm64's name for jump) ?

Probably this is correct because in this case arm_probe_decode_insn() should return INSN_GOOD_NO_SLOT and this insn will be emulated? If yes, this needs a comment, imo.

Quoted 4 lines

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

+static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
+{
+	unsigned long flags;
+

arch/arm64/include/asm/ptrace.h line 111

Quoted 6 lines

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

+	local_irq_save(flags);
+	uprobe_pre_sstep_notifier(regs);
+	local_irq_restore(flags);
+
+	return 0;
+}

Again, you do not need to disable irqs around uprobe_pre_sstep_notifier().

And I am not sure I understand the logic... "return 0" actually means "return DBG_HOOK_HANDLED", right?

I do not understand this register_break_hook() interface and the usage of .esr_mask/esr_val. But given that this patch adds BRK64_ESR_UPROBES and uses BRK64_OPCODE_UPROBES, I will assume that uprobe_breakpoint_handler() will be called if this exception was triggered by UPROBE_SWBP_INSN.

In this case, why the unconditional DBG_HOOK_HANDLED is correct? For example, what if the application itself or debugger use UPROBE_SWBP_INSN for (self) debugging and this task has no uprobes? In this case uprobe_pre_sstep_notifier() will do nothing, it won't set TIF_UPROBES and handle_swbp() won't be called.

IOW, shouldn't it do

	if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
		return DBG_HOOK_HANDLED;
	return DBG_HOOK_ERROR;

?

Quoted 10 lines

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

+static int uprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
+{
+	unsigned long flags;
+
+	if (!find_ss_context(regs->pc - 4))
+		return DBG_HOOK_ERROR;
+
+	local_irq_save(flags);
+	uprobe_post_sstep_notifier(regs);

arch/arm64/include/asm/ptrace.h line 124

Quoted 4 lines

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

+	local_irq_restore(flags);
+
+	return 0;
+}

The same. No need to clear irqs, and please explain why we can't rely on user_mode() && uprobe_post_sstep_notifier(), and why do we need find_ss_context().

arch/arm64/include/asm/ptrace.h line 129

Quoted 5 lines

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

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

I have some concerns... I'll reply to 5/8 which adds __flush_ptrace_access.

Oleg.

Quoted 3 lines · Oleg Nesterov

But the main question is: why do we need add/find_ss_context ?? Please explain.

See arch/arm64/kernel/debug-monitors.c: call_step_hook

Unlike breakpoint exception, there is no ESR info check for step exception. So, it is the responsibility of step handler (uprobe_single_step_handler) to make sure that exception was generated for it.

Quoted 6 lines · Pratyush Anand

Quoted 3 lines · Oleg Nesterov

But the main question is: why do we need add/find_ss_context ?? Please explain.

Quoted 6 lines

See arch/arm64/kernel/debug-monitors.c: call_step_hook

Unlike breakpoint exception, there is no ESR info check for step exception. So, it is the responsibility of step handler (uprobe_single_step_handler) to make sure that exception was generated for it.

Yes, yes, this is clear. My point was, we can (I think) rely on uprobe_post_sstep_notifier() which checks ->active_uprobe != NULL.

And I guess you understood what I meant, but since I wasn't clear let me repeat to ensure we really understand each other.

Can't

	uprobe_single_step_handler(regs, esr)
	{
		if (user_mode(regs) && uprobe_post_sstep_notifier(regs))
			return HANDLED;
		return ERROR;
	}

work without this step_ctx logic?

If everything is correct, the probed task can execute a single (xol) insn in user-mode before the trap. If ->active_uprobe is set we know that we expect the ss trap in user-mode, and nothing else except this xol insn can generate it?

Perhaps arm64 needs additional checks, I dunno... If you think that the ->active_uprobe check is not enough you can probably also verify that "utask->state = UTASK_SSTEP" and/or "regs->pc - 4 == utask->xol_vaddr", but so far it seems to me that these additional checks can only make sense under WARN_ON().

This patch adds support for uprobe on ARM64 architecture.

Unit test for following has been done so far and they have been found working
1. Normal instruction, which can be probed like sub, ldr, add etc.
2. Instructions which can be simulated like ret.
3. uretprobe

Quoted 7 lines · Pratyush Anand

This patch adds support for uprobe on ARM64 architecture.

Unit test for following has been done so far and they have been found working
1. Normal instruction, which can be probed like sub, ldr, add etc.
2. Instructions which can be simulated like ret.
3. uretprobe

I'm assuming that means you don't support compat (AArch32) tasks with this?

How do you enforce that?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 2 lines · Will Deacon

I'm assuming that means you don't support compat (AArch32) tasks with this?

Hummm.. Yes, compat not yet supported.

I will come back on this.

Quoted 3 lines · Pratyush Anand

Quoted 1 line · Will Deacon

Quoted 5 lines

Quoted 1 line

I'm assuming that means you don't support compat (AArch32) tasks with this?

Hummm.. Yes, compat not yet supported.

I will come back on this.

I obviously do not know how difficult is to support the compat tasks, and in any case I leave this to you and Will.

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

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->ainsn)) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		if (auprobe->ainsn.prepare)

Quoted 15 lines · Pratyush Anand

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

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->ainsn)) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		if (auprobe->ainsn.prepare)

arch/arm64/kernel/uprobes.c line 107

Quoted 10 lines

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

+			auprobe->ainsn.prepare(insn, &auprobe->ainsn);
+		break;
+
+	case INSN_GOOD:
+	default:
+		break;
+	}
+
+	return 0;
+}

forgot to mention... shouldn't it also check IS_ALIGNED(addr, AARCH64_INSN_SIZE) ?

I do not know if unaligned insn address is valid on arm64 or not, but please

note that at least it should not cross the page boundary, set_swbp() needs to write AARCH64_INSN_SIZE == UPROBE_SWBP_INSN bytes and it assumes that this should fit the single page.

Quoted 31 lines · Oleg Nesterov

Quoted 16 lines · Pratyush Anand

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,

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

+        unsigned long addr)
+{
+    probe_opcode_t insn;
+
+    insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+    switch (arm_probe_decode_insn(insn, &auprobe->ainsn)) {
+    case INSN_REJECTED:
+        return -EINVAL;
+
+    case INSN_GOOD_NO_SLOT:
+        auprobe->simulate = true;
+        if (auprobe->ainsn.prepare)

arch/arm64/kernel/uprobes.c line 107

Quoted 10 lines

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

+            auprobe->ainsn.prepare(insn, &auprobe->ainsn);
+        break;
+
+    case INSN_GOOD:
+    default:
+        break;
+    }
+
+    return 0;
+}

forgot to mention... shouldn't it also check IS_ALIGNED(addr, AARCH64_INSN_SIZE) ?

Quoted 2 lines

I do not know if unaligned insn address is valid on arm64 or not, but please

AARCH64 instructions are always of fixed lenght ie 4 bytes. I do not see possibility of addr being unaligned. Please let me know, if I am missing something.

Quoted 5 lines

note that at least it should not cross the page boundary, set_swbp() needs to
write AARCH64_INSN_SIZE == UPROBE_SWBP_INSN bytes and it assumes that this
should fit the single page.

So, again I do not see the possibility of crossing of page boundary for any instruction address.

~Pratyush

Hi Oleg,

How can I generate a scenario to test:

a) arch_uprobe_xol_was_trapped
b) arch_uprobe_abort_xol

Quoted 29 lines · Oleg Nesterov

Quoted 15 lines · Pratyush Anand

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

+int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
+		unsigned long addr)
+{
+	probe_opcode_t insn;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->ainsn)) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		if (auprobe->ainsn.prepare)

arch/arm64/kernel/uprobes.c line 107

Quoted 10 lines

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

+			auprobe->ainsn.prepare(insn, &auprobe->ainsn);
+		break;
+
+	case INSN_GOOD:
+	default:
+		break;
+	}
+
+	return 0;
+}

forgot to mention... shouldn't it also check IS_ALIGNED(addr, AARCH64_INSN_SIZE) ?

Quoted 1 line

I do not know if unaligned insn address is valid on arm64 or not, but please

AARCH64 instructions are always of fixed lenght ie 4 bytes. I do not see possibility of addr being unaligned. Please let me know, if I am missing something.

Quoted 3 lines

note that at least it should not cross the page boundary, set_swbp() needs to write AARCH64_INSN_SIZE == UPROBE_SWBP_INSN bytes and it assumes that this should fit the single page.

So, again I do not see the possibility of crossing of page boundary for any instruction address.

These patches have been prepared on top of ARM64 kprobe v3 patches [1] under review.

Unit test for following has been done so far and they have been found working
1. Normal instruction, which can be probed like sub, ldr, add etc.
2. Instructions which can be simulated like ret.
3. uretprobe

[1]https://lkml.org/lkml/2014/11/18/33

Pratyush Anand (8):
ARM64: Move BRK opcodes defines from kprobes.h to insn.h
ARM64: Refactor kprobes-arm64
Kernel/uprobe: Define arch_uprobe_exception_notify as __weak
ARM64: Add instruction_pointer_set function
ARM64: Re-factor flush_ptrace_access
ARM64: Handle TRAP_HWBRKPT for user mode as well
ARM64: Handle TRAP_BRKPT for user mode as well
ARM64: Add uprobe support

  arch/arm/kernel/uprobes.c                          |   6 -
  arch/arm64/Kconfig                                 |   3 +
  arch/arm64/include/asm/insn.h                      |   8 +
  arch/arm64/include/asm/probes.h                    |  26 ++-
  arch/arm64/include/asm/ptrace.h                    |   7 +
  arch/arm64/include/asm/thread_info.h               |   5 +-
  arch/arm64/include/asm/uprobes.h                   |  43 ++++
  arch/arm64/kernel/Makefile                         |   5 +-
  arch/arm64/kernel/debug-monitors.c                 |  14 +-
  arch/arm64/kernel/kprobes.c                        |  11 +-
  arch/arm64/kernel/kprobes.h                        |   7 +-
  .../kernel/{kprobes-arm64.c => probes-arm64.c}     |  84 +++----
  .../kernel/{kprobes-arm64.h => probes-arm64.h}     |  17 +-
  arch/arm64/kernel/probes-condn-check.c             |   2 +-
  arch/arm64/kernel/probes-decode.h                  |   4 +-
  arch/arm64/kernel/signal.c                         |   4 +-
  arch/arm64/kernel/uprobes.c                        | 255 +++++++++++++++++++++
  arch/arm64/mm/flush.c                              |  30 ++-
  kernel/events/uprobes.c                            |  18 ++
  19 files changed, 445 insertions(+), 104 deletions(-)

create mode 100644 arch/arm64/include/asm/uprobes.h rename arch/arm64/kernel/{kprobes-arm64.c => probes-arm64.c} (79%) rename arch/arm64/kernel/{kprobes-arm64.h => probes-arm64.h} (60%) create mode 100644 arch/arm64/kernel/uprobes.c