[RFC 0/8] ARM64: Uprobe support added

[RFC 0/8] ARM64: Uprobe support added

[Pratyush Anand]

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

-- 
2.1.0

[RFC 0/8] ARM64: Uprobe support added

[Pratyush Anand]

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

-- 
2.1.0

[RFC 1/8] ARM64: Move BRK opcodes defines from kprobes.h to insn.h

[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 <panand@redhat.com>
---
 arch/arm64/include/asm/insn.h | 6 ++++++
 arch/arm64/kernel/kprobes.h   | 7 +------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index e2ff32a93b5c..87fa48746806 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -23,6 +23,12 @@
 /* A64 instructions are always 32 bits. */
 #define	AARCH64_INSN_SIZE		4
 
+/* BRK opcodes with ESR encoding  */
+#define BRK64_ESR_MASK		0xFFFF
+#define BRK64_ESR_KPROBES	0x0004
+#define BRK64_OPCODE_KPROBES	0xD4200080	/* "brk 0x4" */
+#define ARCH64_NOP_OPCODE	0xD503201F
+
 #ifndef __ASSEMBLY__
 /*
  * ARM Architecture Reference Manual for ARMv8 Profile-A, Issue A.a
diff --git a/arch/arm64/kernel/kprobes.h b/arch/arm64/kernel/kprobes.h
index 93c54b4972f9..a1140971b045 100644
--- a/arch/arm64/kernel/kprobes.h
+++ b/arch/arm64/kernel/kprobes.h
@@ -15,12 +15,7 @@
 
 #ifndef _ARM_KERNEL_KPROBES_H
 #define _ARM_KERNEL_KPROBES_H
-
-/* BRK opcodes with ESR encoding  */
-#define BRK64_ESR_MASK		0xFFFF
-#define BRK64_ESR_KPROBES	0x0004
-#define BRK64_OPCODE_KPROBES	0xD4200080	/* "brk 0x4" */
-#define ARCH64_NOP_OPCODE	0xD503201F
+#include <asm/insn.h>
 
 #define JPROBES_MAGIC_NUM	0xa5a5a5a5a5a5a5a5
 
-- 
2.1.0

[RFC 2/8] ARM64: Refactor kprobes-arm64

[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.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/include/asm/probes.h                    | 25 +++----
 arch/arm64/kernel/Makefile                         |  2 +-
 arch/arm64/kernel/kprobes.c                        | 11 +--
 .../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 +-
 7 files changed, 70 insertions(+), 75 deletions(-)
 rename arch/arm64/kernel/{kprobes-arm64.c => probes-arm64.c} (79%)
 rename arch/arm64/kernel/{kprobes-arm64.h => probes-arm64.h} (60%)

diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
index 9dba74d8ee86..daa2ff822a2e 100644
--- a/arch/arm64/include/asm/probes.h
+++ b/arch/arm64/include/asm/probes.h
@@ -15,36 +15,37 @@
 #ifndef _ARM_PROBES_H
 #define _ARM_PROBES_H
 
-struct kprobe;
 struct arch_specific_insn;
 
 typedef u32 kprobe_opcode_t;
-typedef unsigned long (kprobes_pstate_check_t)(unsigned long);
+typedef u32 probe_opcode_t;
+typedef unsigned long (probes_pstate_check_t)(unsigned long);
 typedef unsigned long
-(kprobes_condition_check_t)(struct kprobe *p, struct pt_regs *);
+(probes_condition_check_t)(u32 opcode, struct arch_specific_insn *,
+	struct pt_regs *);
 typedef void
-(kprobes_prepare_t)(struct kprobe *, struct arch_specific_insn *);
-typedef void (kprobes_handler_t) (u32 opcode, long addr, struct pt_regs *);
+(probes_prepare_t)(u32 opcode, struct arch_specific_insn *);
+typedef void (probes_handler_t) (u32 opcode, long addr, struct pt_regs *);
 
 enum pc_restore_type {
 	NO_RESTORE,
 	RESTORE_PC,
 };
 
-struct kprobe_pc_restore {
+struct probe_pc_restore {
 	enum pc_restore_type type;
 	unsigned long addr;
 };
 
 /* architecture specific copy of original instruction */
 struct arch_specific_insn {
-	kprobe_opcode_t *insn;
-	kprobes_pstate_check_t *pstate_cc;
-	kprobes_condition_check_t *check_condn;
-	kprobes_prepare_t *prepare;
-	kprobes_handler_t *handler;
+	probe_opcode_t *insn;
+	probes_pstate_check_t *pstate_cc;
+	probes_condition_check_t *check_condn;
+	probes_prepare_t *prepare;
+	probes_handler_t *handler;
 	/* restore address after step xol */
-	struct kprobe_pc_restore restore;
+	struct probe_pc_restore restore;
 };
 
 #endif
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 6e4dcde27742..0ed83ba3d46d 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -31,7 +31,7 @@ arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
-arm64-obj-$(CONFIG_KPROBES)		+= kprobes.o kprobes-arm64.o		\
+arm64-obj-$(CONFIG_KPROBES)		+= kprobes.o probes-arm64.o		\
 					   probes-simulate-insn.o		\
 					   probes-condn-check.o
 arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
diff --git a/arch/arm64/kernel/kprobes.c b/arch/arm64/kernel/kprobes.c
index be7c33070252..1cf5db159bc9 100644
--- a/arch/arm64/kernel/kprobes.c
+++ b/arch/arm64/kernel/kprobes.c
@@ -30,7 +30,7 @@
 #include <asm/insn.h>
 
 #include "kprobes.h"
-#include "kprobes-arm64.h"
+#include "probes-arm64.h"
 
 #define MIN_STACK_SIZE(addr)	min((unsigned long)MAX_STACK_SIZE,	\
 	(unsigned long)current_thread_info() + THREAD_START_SP - (addr))
@@ -60,7 +60,7 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 static void __kprobes arch_prepare_simulate(struct kprobe *p)
 {
 	if (p->ainsn.prepare)
-		p->ainsn.prepare(p, &p->ainsn);
+		p->ainsn.prepare(p->opcode, &p->ainsn);
 
 	/* This instructions is not executed xol. No need to adjust the PC */
 	p->ainsn.restore.addr = 0;
@@ -91,7 +91,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		return -EINVAL;
 
 	/* decode instruction */
-	switch (arm_kprobe_decode_insn(insn, &p->ainsn)) {
+	switch (arm_probe_decode_insn(insn, &p->ainsn)) {
 	case INSN_REJECTED:	/* insn not supported */
 		return -EINVAL;
 
@@ -275,7 +275,8 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
 	switch (kcb->kprobe_status) {
 	case KPROBE_HIT_SSDONE:
 	case KPROBE_HIT_ACTIVE:
-		if (!p->ainsn.check_condn || p->ainsn.check_condn(p, regs)) {
+		if (!p->ainsn.check_condn || p->ainsn.check_condn(p->opcode,
+					&p->ainsn, regs)) {
 			kprobes_inc_nmissed_count(p);
 			setup_singlestep(p, regs, kcb, 1);
 		} else	{
@@ -408,7 +409,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 			if (reenter_kprobe(p, regs, kcb))
 				return;
 		} else if (!p->ainsn.check_condn ||
-			   p->ainsn.check_condn(p, regs)) {
+			   p->ainsn.check_condn(p->opcode, &p->ainsn, regs)) {
 			/* Probe hit and conditional execution check ok. */
 			set_current_kprobe(p);
 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
diff --git a/arch/arm64/kernel/kprobes-arm64.c b/arch/arm64/kernel/probes-arm64.c
similarity index 79%
rename from arch/arm64/kernel/kprobes-arm64.c
rename to arch/arm64/kernel/probes-arm64.c
index bee7816cfd75..c258b390cbc1 100644
--- a/arch/arm64/kernel/kprobes-arm64.c
+++ b/arch/arm64/kernel/probes-arm64.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm64/kernel/kprobes-arm64.c
+ * arch/arm64/kernel/probes-arm64.c
  *
  * Copyright (C) 2013 Linaro Limited.
  *
@@ -14,81 +14,73 @@
  */
 
 #include <linux/kernel.h>
-#include <linux/kprobes.h>
 #include <linux/module.h>
-#include <asm/kprobes.h>
+#include <asm/probes.h>
 
 #include "probes-decode.h"
-#include "kprobes-arm64.h"
+#include "probes-arm64.h"
 #include "probes-simulate-insn.h"
 
 /*
- * condition check functions for kprobes simulation
+ * condition check functions for (k/u)probes simulation
  */
-static unsigned long __kprobes
-__check_pstate(struct kprobe *p, struct pt_regs *regs)
+static unsigned long
+__check_pstate(u32 opcode, struct arch_specific_insn *asi, struct pt_regs *regs)
 {
-	struct arch_specific_insn *asi = &p->ainsn;
 	unsigned long pstate = regs->pstate & 0xffffffff;
 
 	return asi->pstate_cc(pstate);
 }
 
-static unsigned long __kprobes
-__check_cbz(struct kprobe *p, struct pt_regs *regs)
+static unsigned long
+__check_cbz(u32 opcode, struct arch_specific_insn *asi, struct pt_regs *regs)
 {
-	return check_cbz((u32)p->opcode, regs);
+	return check_cbz(opcode, regs);
 }
 
-static unsigned long __kprobes
-__check_cbnz(struct kprobe *p, struct pt_regs *regs)
+static unsigned long
+__check_cbnz(u32 opcode, struct arch_specific_insn *asi, struct pt_regs *regs)
 {
-	return check_cbnz((u32)p->opcode, regs);
+	return check_cbnz(opcode, regs);
 }
 
-static unsigned long __kprobes
-__check_tbz(struct kprobe *p, struct pt_regs *regs)
+static unsigned long
+__check_tbz(u32 opcode, struct arch_specific_insn *asi, struct pt_regs *regs)
 {
-	return check_tbz((u32)p->opcode, regs);
+	return check_tbz(opcode, regs);
 }
 
-static unsigned long __kprobes
-__check_tbnz(struct kprobe *p, struct pt_regs *regs)
+static unsigned long
+__check_tbnz(u32 opcode, struct arch_specific_insn *asi, struct pt_regs *regs)
 {
-	return check_tbnz((u32)p->opcode, regs);
+	return check_tbnz(opcode, regs);
 }
 
 /*
  * prepare functions for instruction simulation
  */
-static void __kprobes
-prepare_none(struct kprobe *p, struct arch_specific_insn *asi)
+static void
+prepare_none(u32 opcode, struct arch_specific_insn *asi)
 {
 }
 
-static void __kprobes
-prepare_bcond(struct kprobe *p, struct arch_specific_insn *asi)
+static void
+prepare_bcond(u32 opcode, struct arch_specific_insn *asi)
 {
-	kprobe_opcode_t insn = p->opcode;
-
 	asi->check_condn = __check_pstate;
-	asi->pstate_cc = kprobe_condition_checks[insn & 0xf];
+	asi->pstate_cc = probe_condition_checks[opcode & 0xf];
 }
 
-static void __kprobes
-prepare_cbz_cbnz(struct kprobe *p, struct arch_specific_insn *asi)
+static void
+prepare_cbz_cbnz(u32 opcode, struct arch_specific_insn *asi)
 {
-	kprobe_opcode_t insn = p->opcode;
-
-	asi->check_condn = (insn & (1 << 24)) ? __check_cbnz : __check_cbz;
+	asi->check_condn = (opcode & (1 << 24)) ? __check_cbnz : __check_cbz;
 }
 
-static void __kprobes
-prepare_tbz_tbnz(struct kprobe *p, struct arch_specific_insn *asi)
+static void
+prepare_tbz_tbnz(u32 opcode, struct arch_specific_insn *asi)
 {
-	kprobe_opcode_t insn = p->opcode;
-
-	asi->check_condn = (insn & (1 << 24)) ? __check_tbnz : __check_tbz;
+	asi->check_condn = (opcode & (1 << 24)) ? __check_tbnz : __check_tbz;
 }
 
 
@@ -116,7 +108,7 @@ static const struct aarch64_decode_item load_literal_subtable[] = {
 	DECODE_END,
 };
 
-/* AArch64 instruction decode table for kprobes:
+/* AArch64 instruction decode table for (k/u)probes:
  * The instruction will fall into one of the 3 groups:
  *  1. Single stepped out-of-the-line slot.
  *     -Most instructions fall in this group, those does not
@@ -194,7 +186,7 @@ static const struct aarch64_decode_item aarch64_decode_table[] = {
 	 * Encoding: 1101 0101 00xx xxxx xxxx xxxx xxxx xxxx
 	 *
 	 * Note: MSR immediate (update PSTATE daif) is not safe handling
-	 * within kprobes, rejected.
+	 * within (k/u)probes, rejected.
 	 *
 	 * Don't re-arrange these decode table entries.
 	 */
@@ -265,8 +257,8 @@ static const struct aarch64_decode_item aarch64_decode_table[] = {
 	DECODE_END,
 };
 
-static int __kprobes
-kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
+static int
+probe_decode_insn(probe_opcode_t insn, struct arch_specific_insn *asi,
 		   const struct aarch64_decode_item *tbl)
 {
 	unsigned int entry, ret = INSN_REJECTED;
@@ -295,19 +287,19 @@ kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
 
 	case DECODE_TYPE_TABLE:
 		/* recurse with next level decode table */
-		ret = kprobe_decode_insn(insn, asi,
+		ret = probe_decode_insn(insn, asi,
 					 decode_sub_table(tbl[entry]));
 	};
 	return ret;
 }
 
 /* Return:
- *   INSN_REJECTED     If instruction is one not allowed to kprobe,
+ *   INSN_REJECTED     If instruction is one not allowed to (k/u)probe,
  *   INSN_GOOD         If instruction is supported and uses instruction slot,
  *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
  */
-enum kprobe_insn __kprobes
-arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+enum probe_insn
+arm_probe_decode_insn(probe_opcode_t insn, struct arch_specific_insn *asi)
 {
-	return kprobe_decode_insn(insn, asi, aarch64_decode_table);
+	return probe_decode_insn(insn, asi, aarch64_decode_table);
 }
diff --git a/arch/arm64/kernel/kprobes-arm64.h b/arch/arm64/kernel/probes-arm64.h
similarity index 60%
rename from arch/arm64/kernel/kprobes-arm64.h
rename to arch/arm64/kernel/probes-arm64.h
index ff8a55f61cda..a748d0ddfeef 100644
--- a/arch/arm64/kernel/kprobes-arm64.h
+++ b/arch/arm64/kernel/probes-arm64.h
@@ -1,5 +1,5 @@
 /*
- * arch/arm64/kernel/kprobes-arm64.h
+ * arch/arm64/kernel/probes-arm64.h
  *
  * Copyright (C) 2013 Linaro Limited.
  *
@@ -13,18 +13,19 @@
  * General Public License for more details.
  */
 
-#ifndef _ARM_KERNEL_KPROBES_ARM64_H
-#define _ARM_KERNEL_KPROBES_ARM64_H
+#ifndef _ARM_KERNEL_PROBES_ARM64_H
+#define _ARM_KERNEL_PROBES_ARM64_H
 
-enum kprobe_insn {
+enum probe_insn {
 	INSN_REJECTED,
 	INSN_GOOD_NO_SLOT,
 	INSN_GOOD,
 };
 
-extern kprobes_pstate_check_t * const kprobe_condition_checks[16];
+typedef u32 probe_opcode_t;
+extern probes_pstate_check_t * const probe_condition_checks[16];
 
-enum kprobe_insn __kprobes
-arm_kprobe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi);
+enum probe_insn
+arm_probe_decode_insn(probe_opcode_t insn, struct arch_specific_insn *asi);
 
-#endif /* _ARM_KERNEL_KPROBES_ARM64_H */
+#endif /* _ARM_KERNEL_PROBES_ARM64_H */
diff --git a/arch/arm64/kernel/probes-condn-check.c b/arch/arm64/kernel/probes-condn-check.c
index e68aa0c86b94..8da70b6d59a6 100644
--- a/arch/arm64/kernel/probes-condn-check.c
+++ b/arch/arm64/kernel/probes-condn-check.c
@@ -114,7 +114,7 @@ static unsigned long __kprobes __check_al(unsigned long pstate)
 	return true;
 }
 
-kprobes_pstate_check_t * const kprobe_condition_checks[16] = {
+probes_pstate_check_t * const probe_condition_checks[16] = {
 	&__check_eq, &__check_ne, &__check_cs, &__check_cc,
 	&__check_mi, &__check_pl, &__check_vs, &__check_vc,
 	&__check_hi, &__check_ls, &__check_ge, &__check_lt,
diff --git a/arch/arm64/kernel/probes-decode.h b/arch/arm64/kernel/probes-decode.h
index 3650ab356251..887ffb217e17 100644
--- a/arch/arm64/kernel/probes-decode.h
+++ b/arch/arm64/kernel/probes-decode.h
@@ -37,8 +37,8 @@ struct aarch64_decode_header {
 };
 
 struct aarch64_decode_actions {
-	kprobes_prepare_t *prepare;
-	kprobes_handler_t *handler;
+	probes_prepare_t *prepare;
+	probes_handler_t *handler;
 };
 
 struct aarch64_decode_table {
-- 
2.1.0

[RFC 3/8] Kernel/uprobe: Define arch_uprobe_exception_notify as __weak

[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 <panand@redhat.com>
---
 arch/arm/kernel/uprobes.c |  6 ------
 kernel/events/uprobes.c   | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
index 56adf9c1fde0..0f3663ca82fc 100644
--- a/arch/arm/kernel/uprobes.c
+++ b/arch/arm/kernel/uprobes.c
@@ -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;
-}
-
 static int uprobe_trap_handler(struct pt_regs *regs, unsigned int instr)
 {
 	unsigned long flags;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index cb346f26a22d..027e39ccb778 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1973,6 +1973,24 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
 	return 1;
 }
 
+/**
+ * arch_uprobe_exception_notify - Pass uprobe exception notigfication to
+ * architecture
+ * @self: Pointer to notifier_block
+ * @val: Type of exception
+ * @data: Exception specific data
+ * Default implementation of arch_uprobe_exception_notify
+ * Returns NOTIFY_DONE.
+ *
+ * Few architectures like arm and arm64 handle uprobe exceptions through
+ * their own hooks.So nothing to be done here.
+ */
+int __weak arch_uprobe_exception_notify(struct notifier_block *self,
+				 unsigned long val, void *data)
+{
+	return NOTIFY_DONE;
+}
+
 static struct notifier_block uprobe_exception_nb = {
 	.notifier_call		= arch_uprobe_exception_notify,
 	.priority		= INT_MAX-1,	/* notified after kprobes, kgdb */
-- 
2.1.0

[RFC 4/8] ARM64: Add instruction_pointer_set function

[Pratyush Anand]

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

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/include/asm/ptrace.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 24cc048ac3e7..29d9bf5e3635 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -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)
 
+static inline void instruction_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	instruction_pointer(regs) = val;
+}
+
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
-- 
2.1.0

[RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Pratyush Anand]

Re-factor flush_ptrace_access to reuse vma independent part.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/mm/flush.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index b6f14e8d2121..9a4dd6f39cfb 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -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);
-		}
-	}
+	if (vma->vm_flags & VM_EXEC)
+		__flush_ptrace_access(page, uaddr, kaddr, len);
 }
 
 /*
-- 
2.1.0

[RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[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 <panand@redhat.com>
---
 arch/arm64/kernel/debug-monitors.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index b056369fd47d..2676b8655241 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -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;
@@ -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;
-
 		pr_warning("Unexpected kernel single-step exception at EL1\n");
 		/*
 		 * Re-enable stepping since we know that we will be
-- 
2.1.0

[RFC 7/8] ARM64: Handle TRAP_BRKPT for user mode as well

[Pratyush Anand]

uprobe is registered at break_hook with a unique ESR code. So, when a
TRAP_BRKPT occurs, call_break_hook checks if it was for uprobe. If not,
then send a SIGTRAP to user.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/kernel/debug-monitors.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 2676b8655241..98ba5c8c5141 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -306,6 +306,9 @@ static int brk_handler(unsigned long addr, unsigned int esr,
 {
 	siginfo_t info;
 
+	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
 	if (user_mode(regs)) {
 		info = (siginfo_t) {
 			.si_signo = SIGTRAP,
@@ -315,12 +318,11 @@ static int brk_handler(unsigned long addr, unsigned int esr,
 		};
 
 		force_sig_info(SIGTRAP, &info, current);
-	} else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
+		return 0;
+	} else {
 		pr_warning("Unexpected kernel BRK exception at EL1\n");
 		return -EFAULT;
 	}
-
-	return 0;
 }
 
 int aarch32_break_handler(struct pt_regs *regs)
-- 
2.1.0

[RFC 8/8] ARM64: Add uprobe support

[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

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/Kconfig                   |   3 +
 arch/arm64/include/asm/insn.h        |   2 +
 arch/arm64/include/asm/probes.h      |   1 +
 arch/arm64/include/asm/ptrace.h      |   1 +
 arch/arm64/include/asm/thread_info.h |   5 +-
 arch/arm64/include/asm/uprobes.h     |  43 ++++++
 arch/arm64/kernel/Makefile           |   3 +
 arch/arm64/kernel/signal.c           |   4 +-
 arch/arm64/kernel/uprobes.c          | 255 +++++++++++++++++++++++++++++++++++
 arch/arm64/mm/flush.c                |   6 +
 10 files changed, 321 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/include/asm/uprobes.h
 create mode 100644 arch/arm64/kernel/uprobes.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index de4f0561cd14..51b1db7a3bc9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -145,6 +145,9 @@ config KERNEL_MODE_NEON
 config FIX_EARLYCON_MEM
 	def_bool y
 
+config ARCH_SUPPORTS_UPROBES
+	def_bool y
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 87fa48746806..412df9457260 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -27,6 +27,8 @@
 #define BRK64_ESR_MASK		0xFFFF
 #define BRK64_ESR_KPROBES	0x0004
 #define BRK64_OPCODE_KPROBES	0xD4200080	/* "brk 0x4" */
+#define BRK64_ESR_UPROBES	0x0008
+#define BRK64_OPCODE_UPROBES	0xD4200100	/* "brk 0x8" */
 #define ARCH64_NOP_OPCODE	0xD503201F
 
 #ifndef __ASSEMBLY__
diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
index daa2ff822a2e..25a69508d137 100644
--- a/arch/arm64/include/asm/probes.h
+++ b/arch/arm64/include/asm/probes.h
@@ -18,6 +18,7 @@
 struct arch_specific_insn;
 
 typedef u32 kprobe_opcode_t;
+typedef u32 uprobe_opcode_t;
 typedef u32 probe_opcode_t;
 typedef unsigned long (probes_pstate_check_t)(unsigned long);
 typedef unsigned long
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 29d9bf5e3635..79c312bb503e 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
 
 #define instruction_pointer(regs)	((regs)->pc)
 #define stack_pointer(regs)		((regs)->sp)
+#define procedure_link_pointer(regs)	((regs)->regs[30])
 
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 459bf8e53208..3d5d1ec181cb 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -108,6 +108,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		4	/* uprobe breakpoint or singlestep */
 #define TIF_NOHZ		7
 #define TIF_SYSCALL_TRACE	8
 #define TIF_SYSCALL_AUDIT	9
@@ -129,10 +130,12 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
+#define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_32BIT		(1 << TIF_32BIT)
 
 #define _TIF_WORK_MASK		(_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
-				 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE)
+				 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
+				 _TIF_UPROBE)
 
 #define _TIF_SYSCALL_WORK	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 				 _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
diff --git a/arch/arm64/include/asm/uprobes.h b/arch/arm64/include/asm/uprobes.h
new file mode 100644
index 000000000000..f575dc389b85
--- /dev/null
+++ b/arch/arm64/include/asm/uprobes.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
+ *
+ * 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/insn.h>
+#include <asm/probes.h>
+
+#define MAX_UINSN_BYTES		AARCH64_INSN_SIZE
+
+#define UPROBE_SWBP_INSN	BRK64_OPCODE_UPROBES
+#define UPROBE_SWBP_INSN_SIZE	4
+#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES
+
+/* Single step context for uprobe */
+struct uprobe_step_ctx {
+	struct list_head node;
+	unsigned long match_addr;
+};
+
+struct arch_uprobe_task {
+	unsigned long saved_fault_code;
+	u64 saved_user_pc;
+	struct uprobe_step_ctx ss_ctx;
+};
+
+struct arch_uprobe {
+	union {
+		u8 insn[MAX_UINSN_BYTES];
+		u8 ixol[MAX_UINSN_BYTES];
+	};
+	struct arch_specific_insn ainsn;
+	bool simulate;
+};
+
+extern void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len);
+#endif
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 0ed83ba3d46d..0e5f0af91540 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -31,6 +31,9 @@ arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
+arm64-obj-$(CONFIG_UPROBES)		+= uprobes.o probes-arm64.o		\
+					   probes-simulate-insn.o		\
+					   probes-condn-check.o
 arm64-obj-$(CONFIG_KPROBES)		+= kprobes.o probes-arm64.o		\
 					   probes-simulate-insn.o		\
 					   probes-condn-check.o
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 6fa792137eda..2d1e18b0cc10 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -409,6 +409,9 @@ static void do_signal(struct pt_regs *regs)
 asmlinkage void do_notify_resume(struct pt_regs *regs,
 				 unsigned int thread_flags)
 {
+	if (thread_flags & _TIF_UPROBE)
+		uprobe_notify_resume(regs);
+
 	if (thread_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
@@ -419,5 +422,4 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 
 	if (thread_flags & _TIF_FOREIGN_FPSTATE)
 		fpsimd_restore_current_state();
-
 }
diff --git a/arch/arm64/kernel/uprobes.c b/arch/arm64/kernel/uprobes.c
new file mode 100644
index 000000000000..fdc6dc3cfb0b
--- /dev/null
+++ b/arch/arm64/kernel/uprobes.c
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
+ *
+ * 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);
+	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;
+}
+
+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);
+	}
+}
+
+void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
+		void *src, unsigned long len)
+{
+	void *xol_page_kaddr = kmap_atomic(page);
+	void *dst = xol_page_kaddr + (vaddr & ~PAGE_MASK);
+
+	preempt_disable();
+
+	/* Initialize the slot */
+	memcpy(dst, src, len);
+
+	/* flush caches (dcache/icache) */
+	flush_uprobe_xol_access(page, vaddr, dst, len);
+
+	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,
+		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)
+			auprobe->ainsn.prepare(insn, &auprobe->ainsn);
+		break;
+
+	case INSN_GOOD:
+	default:
+		break;
+	}
+
+	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;
+
+	/* 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;
+
+	/* 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;
+}
+bool arch_uprobe_xol_was_trapped(struct task_struct *t)
+{
+	/*
+	 * Between arch_uprobe_pre_xol and arch_uprobe_post_xol, if an xol
+	 * insn itself is trapped, then detect the case with the help of
+	 * invalid fault code which is being set in arch_uprobe_pre_xol and
+	 * restored in arch_uprobe_post_xol.
+	 */
+	if (t->thread.fault_code != UPROBE_INV_FAULT_CODE)
+		return true;
+
+	return false;
+}
+
+bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	probe_opcode_t insn;
+	unsigned long addr;
+
+	if (!auprobe->simulate)
+		return false;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+	addr = instruction_pointer(regs);
+
+	if (auprobe->ainsn.handler)
+		auprobe->ainsn.handler(insn, addr, regs);
+
+	return true;
+}
+
+void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	current->thread.fault_code = utask->autask.saved_fault_code;
+	/*
+	 * Task has received a fatal signal, so reset back to probbed
+	 * address.
+	 */
+	instruction_pointer_set(regs, utask->vaddr);
+
+	user_disable_single_step(current);
+}
+
+unsigned long
+arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
+				  struct pt_regs *regs)
+{
+	unsigned long orig_ret_vaddr;
+
+	orig_ret_vaddr = procedure_link_pointer(regs);
+	/* Replace the return addr with trampoline addr */
+	procedure_link_pointer(regs) = trampoline_vaddr;
+	return orig_ret_vaddr;
+}
+
+static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	uprobe_pre_sstep_notifier(regs);
+	local_irq_restore(flags);
+
+	return 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);
+	local_irq_restore(flags);
+
+	return 0;
+}
+
+/* uprobe breakpoint handler hook */
+static struct break_hook uprobes_break_hook = {
+	.esr_mask = BRK64_ESR_MASK,
+	.esr_val = BRK64_ESR_UPROBES,
+	.fn = uprobe_breakpoint_handler,
+};
+
+/* uprobe single step handler hook */
+static struct step_hook uprobes_step_hook = {
+	.fn = uprobe_single_step_handler,
+};
+
+static int __init arch_init_uprobes(void)
+{
+	register_break_hook(&uprobes_break_hook);
+	register_step_hook(&uprobes_step_hook);
+
+	return 0;
+}
+
+device_initcall(arch_init_uprobes);
diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index 9a4dd6f39cfb..04fe6671907e 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -55,6 +55,12 @@ static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 		__flush_ptrace_access(page, uaddr, kaddr, len);
 }
 
+void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len)
+{
+	__flush_ptrace_access(page, uaddr, kaddr, len);
+}
+
 /*
  * Copy user data from/to a page which is mapped into a different processes
  * address space.  Really, we want to allow our "user space" model to handle
-- 
2.1.0

Re: [RFC 0/8] ARM64: Uprobe support added

[Pratyush Anand]

+Dave

Sorry, I took all cc list from your kprobe patches and forgot to add you. :(

Please review.

On Wednesday 31 December 2014 08:51 PM, Pratyush Anand wrote:
> 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
>

[RFC 0/8] ARM64: Uprobe support added

[Pratyush Anand]

+Dave

Sorry, I took all cc list from your kprobe patches and forgot to add you. :(

Please review.

On Wednesday 31 December 2014 08:51 PM, Pratyush Anand wrote:
> 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
>

Re: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

Hi Pratyush,

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

On 12/31, Pratyush Anand wrote:
>
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
>
>  #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.

> +/* 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.

> +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 ?

> +++ b/arch/arm64/kernel/uprobes.c
> @@ -0,0 +1,255 @@
> +/*
> + * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
> + *
> + * 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);
> +	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.

> +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.

> +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;
> +
> +	/* 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.

> +static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> +{
> +	unsigned long flags;
> +
> +	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;

?

> +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);
> +	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().

> +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.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

Hi Pratyush,

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

On 12/31, Pratyush Anand wrote:
>
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
>
>  #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.

> +/* 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.

> +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 ?

> +++ b/arch/arm64/kernel/uprobes.c
> @@ -0,0 +1,255 @@
> +/*
> + * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
> + *
> + * 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);
> +	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.

> +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.

> +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;
> +
> +	/* 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.

> +static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> +{
> +	unsigned long flags;
> +
> +	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;

?

> +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);
> +	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().

> +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.

Re: [RFC 3/8] Kernel/uprobe: Define arch_uprobe_exception_notify as __weak

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> 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 <panand@redhat.com>
> ---
>  arch/arm/kernel/uprobes.c |  6 ------
>  kernel/events/uprobes.c   | 18 ++++++++++++++++++
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> index 56adf9c1fde0..0f3663ca82fc 100644
> --- a/arch/arm/kernel/uprobes.c
> +++ b/arch/arm/kernel/uprobes.c
> @@ -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.

[RFC 3/8] Kernel/uprobe: Define arch_uprobe_exception_notify as __weak

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> 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 <panand@redhat.com>
> ---
>  arch/arm/kernel/uprobes.c |  6 ------
>  kernel/events/uprobes.c   | 18 ++++++++++++++++++
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
> index 56adf9c1fde0..0f3663ca82fc 100644
> --- a/arch/arm/kernel/uprobes.c
> +++ b/arch/arm/kernel/uprobes.c
> @@ -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.

Re: [RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> Re-factor flush_ptrace_access to reuse vma independent part.

But for what? The changelog should explain this.

> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  arch/arm64/mm/flush.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> index b6f14e8d2121..9a4dd6f39cfb 100644
> --- a/arch/arm64/mm/flush.c
> +++ b/arch/arm64/mm/flush.c
> @@ -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);
> -		}
> -	}
> +	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.

Oleg.

[RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> Re-factor flush_ptrace_access to reuse vma independent part.

But for what? The changelog should explain this.

> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  arch/arm64/mm/flush.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> index b6f14e8d2121..9a4dd6f39cfb 100644
> --- a/arch/arm64/mm/flush.c
> +++ b/arch/arm64/mm/flush.c
> @@ -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);
> -		}
> -	}
> +	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.

Oleg.

Re: [RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Oleg Nesterov]

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

On 12/31, Pratyush Anand wrote:
>
> 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 <panand@redhat.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index b056369fd47d..2676b8655241 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -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;
> @@ -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.

The same for the next patch.

Oleg.

[RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Oleg Nesterov]

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

On 12/31, Pratyush Anand wrote:
>
> 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 <panand@redhat.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index b056369fd47d..2676b8655241 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -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;
> @@ -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.

The same for the next patch.

Oleg.

Re: [RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Oleg Nesterov]

On 01/02, Oleg Nesterov wrote:
>
> On 12/31, Pratyush Anand wrote:
> >
> > Re-factor flush_ptrace_access to reuse vma independent part.
>
> But for what? The changelog should explain this.
>
> > Signed-off-by: Pratyush Anand <panand@redhat.com>
> > ---
> >  arch/arm64/mm/flush.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> > index b6f14e8d2121..9a4dd6f39cfb 100644
> > --- a/arch/arm64/mm/flush.c
> > +++ b/arch/arm64/mm/flush.c
> > @@ -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);
> > -		}
> > -	}
> > +	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.

[RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Oleg Nesterov]

On 01/02, Oleg Nesterov wrote:
>
> On 12/31, Pratyush Anand wrote:
> >
> > Re-factor flush_ptrace_access to reuse vma independent part.
>
> But for what? The changelog should explain this.
>
> > Signed-off-by: Pratyush Anand <panand@redhat.com>
> > ---
> >  arch/arm64/mm/flush.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> > index b6f14e8d2121..9a4dd6f39cfb 100644
> > --- a/arch/arm64/mm/flush.c
> > +++ b/arch/arm64/mm/flush.c
> > @@ -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);
> > -		}
> > -	}
> > +	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.

Re: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
> Hi Pratyush,
>
> I'll try to actually read this patch (and the whole series) later, just
> a couple of quick questions for now.
>
> On 12/31, Pratyush Anand wrote:
>>
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
>>
>>   #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.

Yes.

>
>> +/* 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.
>
>> +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 ?

Probably yes. Will change.

>
>> +++ b/arch/arm64/kernel/uprobes.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
>> + *
>> + * 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);
>> +	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.
>
>> +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.

Thanks. Yes, we need del_ss_context() in arch_uprobe_abort_xol().

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

OK..So xol_vaddr is not unique, then need to look for something else 
which could be unique for each probe.

>
> 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.

>> +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;
>> +
>> +	/* 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);
>
> ?
>

Correct, saved_user_pc is not needed and also no need to play with 
->user_regs.pc. instruction_pointer_set should be sufficient.


> 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.
>

Yes, a branch instruction like b or bl or ret will be simulated.

>> +static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
>> +{
>> +	unsigned long flags;
>> +
>> +	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().

OK. (Actually took it from arch/arm/kernel/uprobe.c)

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

Yes. So better I will put return DBG_HOOK_HANDLED;

>
> 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.

Correct.

>
> 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;
>
> ?

Thanks for pointing out this bug with the code.
Your point seems pretty valid.

>
>> +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);
>> +	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().
>

I had not looked into uprobe_post_sstep_notifier implementtaion.I think, 
you are right.


Thanks for all these valuable comments.

~Pratyush


>> +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.
>

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
> Hi Pratyush,
>
> I'll try to actually read this patch (and the whole series) later, just
> a couple of quick questions for now.
>
> On 12/31, Pratyush Anand wrote:
>>
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -205,6 +205,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
>>
>>   #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.

Yes.

>
>> +/* 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.
>
>> +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 ?

Probably yes. Will change.

>
>> +++ b/arch/arm64/kernel/uprobes.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * Copyright (C) 2014 Pratyush Anand <panand@redhat.com>
>> + *
>> + * 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);
>> +	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.
>
>> +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.

Thanks. Yes, we need del_ss_context() in arch_uprobe_abort_xol().

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

OK..So xol_vaddr is not unique, then need to look for something else 
which could be unique for each probe.

>
> 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.

>> +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;
>> +
>> +	/* 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);
>
> ?
>

Correct, saved_user_pc is not needed and also no need to play with 
->user_regs.pc. instruction_pointer_set should be sufficient.


> 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.
>

Yes, a branch instruction like b or bl or ret will be simulated.

>> +static int uprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
>> +{
>> +	unsigned long flags;
>> +
>> +	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().

OK. (Actually took it from arch/arm/kernel/uprobe.c)

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

Yes. So better I will put return DBG_HOOK_HANDLED;

>
> 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.

Correct.

>
> 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;
>
> ?

Thanks for pointing out this bug with the code.
Your point seems pretty valid.

>
>> +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);
>> +	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().
>

I had not looked into uprobe_post_sstep_notifier implementtaion.I think, 
you are right.


Thanks for all these valuable comments.

~Pratyush


>> +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.
>

Re: [RFC 3/8] Kernel/uprobe: Define arch_uprobe_exception_notify as __weak

[Pratyush Anand]

On Friday 02 January 2015 11:13 PM, Oleg Nesterov wrote:
> On 12/31, Pratyush Anand wrote:
>>
>> 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 <panand@redhat.com>
>> ---
>>   arch/arm/kernel/uprobes.c |  6 ------
>>   kernel/events/uprobes.c   | 18 ++++++++++++++++++
>>   2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
>> index 56adf9c1fde0..0f3663ca82fc 100644
>> --- a/arch/arm/kernel/uprobes.c
>> +++ b/arch/arm/kernel/uprobes.c
>> @@ -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.

OK.

~Pratyush
>
> Oleg.
>

[RFC 3/8] Kernel/uprobe: Define arch_uprobe_exception_notify as __weak

[Pratyush Anand]

On Friday 02 January 2015 11:13 PM, Oleg Nesterov wrote:
> On 12/31, Pratyush Anand wrote:
>>
>> 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 <panand@redhat.com>
>> ---
>>   arch/arm/kernel/uprobes.c |  6 ------
>>   kernel/events/uprobes.c   | 18 ++++++++++++++++++
>>   2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/kernel/uprobes.c
>> index 56adf9c1fde0..0f3663ca82fc 100644
>> --- a/arch/arm/kernel/uprobes.c
>> +++ b/arch/arm/kernel/uprobes.c
>> @@ -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.

OK.

~Pratyush
>
> Oleg.
>

Re: [RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Pratyush Anand]

On Friday 02 January 2015 11:49 PM, Oleg Nesterov wrote:
> On 01/02, Oleg Nesterov wrote:
>>
>> On 12/31, Pratyush Anand wrote:
>>>
>>> Re-factor flush_ptrace_access to reuse vma independent part.
>>
>> But for what? The changelog should explain this.
>>
>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>> ---
>>>   arch/arm64/mm/flush.c | 24 +++++++++++++++---------
>>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
>>> index b6f14e8d2121..9a4dd6f39cfb 100644
>>> --- a/arch/arm64/mm/flush.c
>>> +++ b/arch/arm64/mm/flush.c
>>> @@ -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);
>>> -		}
>>> -	}
>>> +	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.
>

OK.. Will improve commit log.

~Pratyush
> Oleg.
>

[RFC 5/8] ARM64: Re-factor flush_ptrace_access

[Pratyush Anand]

On Friday 02 January 2015 11:49 PM, Oleg Nesterov wrote:
> On 01/02, Oleg Nesterov wrote:
>>
>> On 12/31, Pratyush Anand wrote:
>>>
>>> Re-factor flush_ptrace_access to reuse vma independent part.
>>
>> But for what? The changelog should explain this.
>>
>>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>>> ---
>>>   arch/arm64/mm/flush.c | 24 +++++++++++++++---------
>>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
>>> index b6f14e8d2121..9a4dd6f39cfb 100644
>>> --- a/arch/arm64/mm/flush.c
>>> +++ b/arch/arm64/mm/flush.c
>>> @@ -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);
>>> -		}
>>> -	}
>>> +	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.
>

OK.. Will improve commit log.

~Pratyush
> Oleg.
>

Re: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/04, Pratyush Anand wrote:
>
> On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
>> 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.

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().

Oleg.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/04, Pratyush Anand wrote:
>
> On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
>> 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.

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().

Oleg.

Re: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Monday 05 January 2015 12:10 AM, Oleg Nesterov wrote:
> On 01/04, Pratyush Anand wrote:
>>
>> On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
>>> 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.
>
> 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?
>

Yes,yes, no need of 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?

Yes, I do see any value addition in saving xol_vaddr in ss_ctx->match_addr.

>
> 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().

Yes.

~Pratyush

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Monday 05 January 2015 12:10 AM, Oleg Nesterov wrote:
> On 01/04, Pratyush Anand wrote:
>>
>> On Friday 02 January 2015 10:53 PM, Oleg Nesterov wrote:
>>> 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.
>
> 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?
>

Yes,yes, no need of 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?

Yes, I do see any value addition in saving xol_vaddr in ss_ctx->match_addr.

>
> 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().

Yes.

~Pratyush

Re: [RFC 1/8] ARM64: Move BRK opcodes defines from kprobes.h to insn.h

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:17PM +0000, Pratyush Anand wrote:
> 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 <panand@redhat.com>
> ---
>  arch/arm64/include/asm/insn.h | 6 ++++++
>  arch/arm64/kernel/kprobes.h   | 7 +------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index e2ff32a93b5c..87fa48746806 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -23,6 +23,12 @@
>  /* A64 instructions are always 32 bits. */
>  #define	AARCH64_INSN_SIZE		4
>  
> +/* 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.

> +#define ARCH64_NOP_OPCODE	0xD503201F

We have aarch64_insn_gen_nop for this.

Will

[RFC 1/8] ARM64: Move BRK opcodes defines from kprobes.h to insn.h

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:17PM +0000, Pratyush Anand wrote:
> 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 <panand@redhat.com>
> ---
>  arch/arm64/include/asm/insn.h | 6 ++++++
>  arch/arm64/kernel/kprobes.h   | 7 +------
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index e2ff32a93b5c..87fa48746806 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -23,6 +23,12 @@
>  /* A64 instructions are always 32 bits. */
>  #define	AARCH64_INSN_SIZE		4
>  
> +/* 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.

> +#define ARCH64_NOP_OPCODE	0xD503201F

We have aarch64_insn_gen_nop for this.

Will

Re: [RFC 2/8] ARM64: Refactor kprobes-arm64

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
> 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

[RFC 2/8] ARM64: Refactor kprobes-arm64

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
> 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

Re: [RFC 4/8] ARM64: Add instruction_pointer_set function

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:20PM +0000, Pratyush Anand wrote:
> instruction_pointer_set is needed for uprobe implementation. Hence
> define it for ARM64.
> 
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  arch/arm64/include/asm/ptrace.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 24cc048ac3e7..29d9bf5e3635 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -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)
>  
> +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

[RFC 4/8] ARM64: Add instruction_pointer_set function

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:20PM +0000, Pratyush Anand wrote:
> instruction_pointer_set is needed for uprobe implementation. Hence
> define it for ARM64.
> 
> Signed-off-by: Pratyush Anand <panand@redhat.com>
> ---
>  arch/arm64/include/asm/ptrace.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 24cc048ac3e7..29d9bf5e3635 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -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)
>  
> +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

Re: [RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Will Deacon]

On Fri, Jan 02, 2015 at 06:05:23PM +0000, Oleg Nesterov wrote:
> Let me repeat once again that I know absolutely nothing about arm* ;)
> 
> On 12/31, Pratyush Anand wrote:
> >
> > 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 <panand@redhat.com>
> > ---
> >  arch/arm64/kernel/debug-monitors.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > index b056369fd47d..2676b8655241 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -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;
> > @@ -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.

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

Will

[RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Will Deacon]

On Fri, Jan 02, 2015 at 06:05:23PM +0000, Oleg Nesterov wrote:
> Let me repeat once again that I know absolutely nothing about arm* ;)
> 
> On 12/31, Pratyush Anand wrote:
> >
> > 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 <panand@redhat.com>
> > ---
> >  arch/arm64/kernel/debug-monitors.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > index b056369fd47d..2676b8655241 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -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;
> > @@ -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.

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

Will

Re: [RFC 8/8] ARM64: Add uprobe support

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:24PM +0000, Pratyush Anand wrote:
> 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

[RFC 8/8] ARM64: Add uprobe support

[Will Deacon]

On Wed, Dec 31, 2014 at 03:21:24PM +0000, Pratyush Anand wrote:
> 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

Re: [RFC 1/8] ARM64: Move BRK opcodes defines from kprobes.h to insn.h

[Pratyush Anand]

On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:17PM +0000, Pratyush Anand wrote:
>> 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 <panand@redhat.com>
>> ---
>>   arch/arm64/include/asm/insn.h | 6 ++++++
>>   arch/arm64/kernel/kprobes.h   | 7 +------
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index e2ff32a93b5c..87fa48746806 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -23,6 +23,12 @@
>>   /* A64 instructions are always 32 bits. */
>>   #define	AARCH64_INSN_SIZE		4
>>
>> +/* 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.
>

Agreed.

~Pratyush
>> +#define ARCH64_NOP_OPCODE	0xD503201F
>
> We have aarch64_insn_gen_nop for this.
>
> Will
>

[RFC 1/8] ARM64: Move BRK opcodes defines from kprobes.h to insn.h

[Pratyush Anand]

On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:17PM +0000, Pratyush Anand wrote:
>> 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 <panand@redhat.com>
>> ---
>>   arch/arm64/include/asm/insn.h | 6 ++++++
>>   arch/arm64/kernel/kprobes.h   | 7 +------
>>   2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index e2ff32a93b5c..87fa48746806 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -23,6 +23,12 @@
>>   /* A64 instructions are always 32 bits. */
>>   #define	AARCH64_INSN_SIZE		4
>>
>> +/* 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.
>

Agreed.

~Pratyush
>> +#define ARCH64_NOP_OPCODE	0xD503201F
>
> We have aarch64_insn_gen_nop for this.
>
> Will
>

Re: [RFC 2/8] ARM64: Refactor kprobes-arm64

[Pratyush Anand]

On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
>> 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)?
>

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

~Pratyush

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

[RFC 2/8] ARM64: Refactor kprobes-arm64

[Pratyush Anand]

On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
>> 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)?
>

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

~Pratyush

> 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: [RFC 2/8] ARM64: Refactor kprobes-arm64

[Will Deacon]

On Thu, Jan 08, 2015 at 05:33:08PM +0000, Pratyush Anand wrote:
> On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> > On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
> >> 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)?
> >
> 
> 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

[RFC 2/8] ARM64: Refactor kprobes-arm64

[Will Deacon]

On Thu, Jan 08, 2015 at 05:33:08PM +0000, Pratyush Anand wrote:
> On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
> > On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
> >> 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)?
> >
> 
> 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

Re: [RFC 2/8] ARM64: Refactor kprobes-arm64

[Pratyush Anand]

On Thursday 08 January 2015 11:06 PM, Will Deacon wrote:
> On Thu, Jan 08, 2015 at 05:33:08PM +0000, Pratyush Anand wrote:
>> On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
>>> On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
>>>> 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)?
>>>
>>
>> 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.
>

Sure, Sure.. I too expect first two patches to be merged with kprobe 
series. I just did that to develop my uprobe code.

~Pratyush

> Will
>

[RFC 2/8] ARM64: Refactor kprobes-arm64

[Pratyush Anand]

On Thursday 08 January 2015 11:06 PM, Will Deacon wrote:
> On Thu, Jan 08, 2015 at 05:33:08PM +0000, Pratyush Anand wrote:
>> On Thursday 08 January 2015 10:25 PM, Will Deacon wrote:
>>> On Wed, Dec 31, 2014 at 03:21:18PM +0000, Pratyush Anand wrote:
>>>> 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)?
>>>
>>
>> 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.
>

Sure, Sure.. I too expect first two patches to be merged with kprobe 
series. I just did that to develop my uprobe code.

~Pratyush

> Will
>

Re: [RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Pratyush Anand]

On Thursday 08 January 2015 10:31 PM, Will Deacon wrote:
> On Fri, Jan 02, 2015 at 06:05:23PM +0000, Oleg Nesterov wrote:
>> Let me repeat once again that I know absolutely nothing about arm* ;)
>>
>> On 12/31, Pratyush Anand wrote:
>>>
>>> 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 <panand@redhat.com>
>>> ---
>>>   arch/arm64/kernel/debug-monitors.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>>> index b056369fd47d..2676b8655241 100644
>>> --- a/arch/arm64/kernel/debug-monitors.c
>>> +++ b/arch/arm64/kernel/debug-monitors.c
>>> @@ -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;
>>> @@ -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.
>
> I think you're right, and kgdb will need fixing with this change.

OK, I will fix kgdb  too when I will send next revision.

~Pratyush

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

[RFC 6/8] ARM64: Handle TRAP_HWBRKPT for user mode as well

[Pratyush Anand]

On Thursday 08 January 2015 10:31 PM, Will Deacon wrote:
> On Fri, Jan 02, 2015 at 06:05:23PM +0000, Oleg Nesterov wrote:
>> Let me repeat once again that I know absolutely nothing about arm* ;)
>>
>> On 12/31, Pratyush Anand wrote:
>>>
>>> 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 <panand@redhat.com>
>>> ---
>>>   arch/arm64/kernel/debug-monitors.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>>> index b056369fd47d..2676b8655241 100644
>>> --- a/arch/arm64/kernel/debug-monitors.c
>>> +++ b/arch/arm64/kernel/debug-monitors.c
>>> @@ -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;
>>> @@ -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.
>
> I think you're right, and kgdb will need fixing with this change.

OK, I will fix kgdb  too when I will send next revision.

~Pratyush

>
> 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: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:24PM +0000, Pratyush Anand wrote:
>> 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?

Hummm.. Yes, compat not yet supported.

I will come back on this.

~Pratyush

> How do you enforce that?
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:24PM +0000, Pratyush Anand wrote:
>> 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?

Hummm.. Yes, compat not yet supported.

I will come back on this.

~Pratyush

> 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
>

Re: [RFC 4/8] ARM64: Add instruction_pointer_set function

[Pratyush Anand]

On Thursday 08 January 2015 10:29 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:20PM +0000, Pratyush Anand wrote:
>> instruction_pointer_set is needed for uprobe implementation. Hence
>> define it for ARM64.
>>
>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>> ---
>>   arch/arm64/include/asm/ptrace.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index 24cc048ac3e7..29d9bf5e3635 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -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)
>>
>> +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).
>

Yes, We can use asm-generic for it.

I think , we can also add link_pointer and link_pointer_set in 
asm-generic/ptrace.h to help with all retprobes.

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

[RFC 4/8] ARM64: Add instruction_pointer_set function

[Pratyush Anand]

On Thursday 08 January 2015 10:29 PM, Will Deacon wrote:
> On Wed, Dec 31, 2014 at 03:21:20PM +0000, Pratyush Anand wrote:
>> instruction_pointer_set is needed for uprobe implementation. Hence
>> define it for ARM64.
>>
>> Signed-off-by: Pratyush Anand <panand@redhat.com>
>> ---
>>   arch/arm64/include/asm/ptrace.h | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index 24cc048ac3e7..29d9bf5e3635 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -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)
>>
>> +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).
>

Yes, We can use asm-generic for it.

I think , we can also add link_pointer and link_pointer_set in 
asm-generic/ptrace.h to help with all retprobes.

~Pratyush
> 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: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/08, Pratyush Anand wrote:
>
> On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
>>
>> 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.

But if this is not simple, perhaps the initial implementation could simply
nack the probe in arch_uprobe_analyze_insn() if this mm is not 64-bit.
Although I do not see something like is_64bit_mm() in arch/arm64... So may be
the first version can even ignore this problem and just document the fact
that AArch32 tasks are not supported.

Oleg.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/08, Pratyush Anand wrote:
>
> On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
>>
>> 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.

But if this is not simple, perhaps the initial implementation could simply
nack the probe in arch_uprobe_analyze_insn() if this mm is not 64-bit.
Although I do not see something like is_64bit_mm() in arch/arm64... So may be
the first version can even ignore this problem and just document the fact
that AArch32 tasks are not supported.

Oleg.

Re: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> +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)
> +			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.

Oleg.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 12/31, Pratyush Anand wrote:
>
> +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)
> +			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.

Oleg.

Re: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 09 January 2015 11:15 PM, Oleg Nesterov wrote:
> On 01/08, Pratyush Anand wrote:
>>
>> On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
>>>
>>> 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.
>

I think most of the stuff can be picked from arch/arm. But I do not have 
a platform to test this feature.So, I will not be taking up as of now.

~Pratyush

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 09 January 2015 11:15 PM, Oleg Nesterov wrote:
> On 01/08, Pratyush Anand wrote:
>>
>> On Thursday 08 January 2015 10:33 PM, Will Deacon wrote:
>>>
>>> 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.
>

I think most of the stuff can be picked from arch/arm. But I do not have 
a platform to test this feature.So, I will not be taking up as of now.

~Pratyush

Re: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
> On 12/31, Pratyush Anand wrote:
>>
>> +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)
>> +			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

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.

> 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

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
> On 12/31, Pratyush Anand wrote:
>>
>> +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)
>> +			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

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.

> 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

Re: [RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

Hi Oleg,

How can I generate a scenario to test:

a) arch_uprobe_xol_was_trapped
b) arch_uprobe_abort_xol

~Pratyush

On Monday 12 January 2015 10:34 AM, Pratyush Anand wrote:
>
>
> On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
>> On 12/31, Pratyush Anand wrote:
>>>
>>> +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)
>>> +            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
>
> 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.
>
>> 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

[RFC 8/8] ARM64: Add uprobe support

[Pratyush Anand]

Hi Oleg,

How can I generate a scenario to test:

a) arch_uprobe_xol_was_trapped
b) arch_uprobe_abort_xol

~Pratyush

On Monday 12 January 2015 10:34 AM, Pratyush Anand wrote:
>
>
> On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
>> On 12/31, Pratyush Anand wrote:
>>>
>>> +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)
>>> +            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
>
> 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.
>
>> 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

Re: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/12, Pratyush Anand wrote:
>
>
> On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
>> On 12/31, Pratyush Anand wrote:
>>>
>>> +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)
>>> +			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
>
> 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.

A user can write any offset into uprobe_events, and the generic code doesn't
check it is aligned.

>> 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.

See above. So yes, it should really check IS_ALIGNED(AARCH64_INSN_SIZE).

Oleg.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/12, Pratyush Anand wrote:
>
>
> On Friday 09 January 2015 11:29 PM, Oleg Nesterov wrote:
>> On 12/31, Pratyush Anand wrote:
>>>
>>> +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)
>>> +			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
>
> 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.

A user can write any offset into uprobe_events, and the generic code doesn't
check it is aligned.

>> 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.

See above. So yes, it should really check IS_ALIGNED(AARCH64_INSN_SIZE).

Oleg.

Re: [RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/12, Pratyush Anand wrote:
>
> Hi Oleg,
>
> How can I generate a scenario to test:
>
> a) arch_uprobe_xol_was_trapped
> b) arch_uprobe_abort_xol

For example, you can probe something like

	*(int *)NULL = 0;

See also the x86 test-case in https://lkml.org/lkml/2011/10/21/148
and the whole thread.

Oleg.

[RFC 8/8] ARM64: Add uprobe support

[Oleg Nesterov]

On 01/12, Pratyush Anand wrote:
>
> Hi Oleg,
>
> How can I generate a scenario to test:
>
> a) arch_uprobe_xol_was_trapped
> b) arch_uprobe_abort_xol

For example, you can probe something like

	*(int *)NULL = 0;

See also the x86 test-case in https://lkml.org/lkml/2011/10/21/148
and the whole thread.

Oleg.