[PATCH 0/5] ARM64: Uprobe support added

[PATCH 0/5] ARM64: Uprobe support added

[Pratyush Anand]

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

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

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

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

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

-- 
2.5.5

[PATCH 1/5] arm64: kprobe: protect/rename few definitions to be reused by uprobe

[Pratyush Anand]

decode-insn code has to be reused by arm64 uprobe implementation as well.
Therefore, this patch protects some portion of kprobe code and renames few
other, so that decode-insn functionality can be reused by uprobe even when
CONFIG_KPROBES is not defined.

kprobe_opcode_t and struct arch_specific_insn are also defined by
linux/kprobes.h, when CONFIG_KPROBES is not defined. So, protect these
definitions in asm/probes.h.

linux/kprobes.h already includes asm/kprobes.h. Therefore, remove inclusion
of asm/kprobes.h from decode-insn.c.

There are some definitions like kprobe_insn and kprobes_handler_t etc can
be re-used by uprobe. So, it would be better to remove 'k' from their
names.

struct arch_specific_insn is specific to kprobe. Therefore, introduce a new
struct arch_probe_insn which will be common for both kprobe and uprobe, so
that decode-insn code can be shared. Modify kprobe code accordingly.

Function arm_probe_decode_insn() will be needed by uprobe as well. So make
it global.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/include/asm/probes.h        | 19 ++++++++++--------
 arch/arm64/kernel/probes/decode-insn.c | 31 +++++++++++++++--------------
 arch/arm64/kernel/probes/decode-insn.h |  8 ++++++--
 arch/arm64/kernel/probes/kprobes.c     | 36 +++++++++++++++++-----------------
 4 files changed, 51 insertions(+), 43 deletions(-)

diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
index 5af574d632fa..e175a825b187 100644
--- a/arch/arm64/include/asm/probes.h
+++ b/arch/arm64/include/asm/probes.h
@@ -17,19 +17,22 @@
 
 #include <asm/opcodes.h>
 
-struct kprobe;
-struct arch_specific_insn;
-
-typedef u32 kprobe_opcode_t;
-typedef void (kprobes_handler_t) (u32 opcode, long addr, struct pt_regs *);
+typedef u32 probe_opcode_t;
+typedef void (probes_handler_t) (u32 opcode, long addr, struct pt_regs *);
 
 /* architecture specific copy of original instruction */
-struct arch_specific_insn {
-	kprobe_opcode_t *insn;
+struct arch_probe_insn {
+	probe_opcode_t *insn;
 	pstate_check_t *pstate_cc;
-	kprobes_handler_t *handler;
+	probes_handler_t *handler;
 	/* restore address after step xol */
 	unsigned long restore;
 };
+#ifdef CONFIG_KPROBES
+typedef u32 kprobe_opcode_t;
+struct arch_specific_insn {
+	struct arch_probe_insn api;
+};
+#endif
 
 #endif
diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c
index 37e47a9d617e..335984cd3a99 100644
--- a/arch/arm64/kernel/probes/decode-insn.c
+++ b/arch/arm64/kernel/probes/decode-insn.c
@@ -16,7 +16,6 @@
 #include <linux/kernel.h>
 #include <linux/kprobes.h>
 #include <linux/module.h>
-#include <asm/kprobes.h>
 #include <asm/insn.h>
 #include <asm/sections.h>
 
@@ -77,8 +76,8 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn)
  *   INSN_GOOD         If instruction is supported and uses instruction slot,
  *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
  */
-static enum kprobe_insn __kprobes
-arm_probe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
+enum probe_insn __kprobes
+arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api)
 {
 	/*
 	 * Instructions reading or modifying the PC won't work from the XOL
@@ -88,26 +87,26 @@ arm_probe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
 		return INSN_GOOD;
 
 	if (aarch64_insn_is_bcond(insn)) {
-		asi->handler = simulate_b_cond;
+		api->handler = simulate_b_cond;
 	} else if (aarch64_insn_is_cbz(insn) ||
 	    aarch64_insn_is_cbnz(insn)) {
-		asi->handler = simulate_cbz_cbnz;
+		api->handler = simulate_cbz_cbnz;
 	} else if (aarch64_insn_is_tbz(insn) ||
 	    aarch64_insn_is_tbnz(insn)) {
-		asi->handler = simulate_tbz_tbnz;
+		api->handler = simulate_tbz_tbnz;
 	} else if (aarch64_insn_is_adr_adrp(insn)) {
-		asi->handler = simulate_adr_adrp;
+		api->handler = simulate_adr_adrp;
 	} else if (aarch64_insn_is_b(insn) ||
 	    aarch64_insn_is_bl(insn)) {
-		asi->handler = simulate_b_bl;
+		api->handler = simulate_b_bl;
 	} else if (aarch64_insn_is_br(insn) ||
 	    aarch64_insn_is_blr(insn) ||
 	    aarch64_insn_is_ret(insn)) {
-		asi->handler = simulate_br_blr_ret;
+		api->handler = simulate_br_blr_ret;
 	} else if (aarch64_insn_is_ldr_lit(insn)) {
-		asi->handler = simulate_ldr_literal;
+		api->handler = simulate_ldr_literal;
 	} else if (aarch64_insn_is_ldrsw_lit(insn)) {
-		asi->handler = simulate_ldrsw_literal;
+		api->handler = simulate_ldrsw_literal;
 	} else {
 		/*
 		 * Instruction cannot be stepped out-of-line and we don't
@@ -119,6 +118,7 @@ arm_probe_decode_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi)
 	return INSN_GOOD_NO_SLOT;
 }
 
+#ifdef CONFIG_KPROBES
 static bool __kprobes
 is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
 {
@@ -137,11 +137,11 @@ is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
 	return false;
 }
 
-enum kprobe_insn __kprobes
+enum probe_insn __kprobes
 arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi)
 {
-	enum kprobe_insn decoded;
-	kprobe_opcode_t insn = le32_to_cpu(*addr);
+	enum probe_insn decoded;
+	probe_opcode_t insn = le32_to_cpu(*addr);
 	kprobe_opcode_t *scan_start = addr - 1;
 	kprobe_opcode_t *scan_end = addr - MAX_ATOMIC_CONTEXT_SIZE;
 #if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
@@ -164,7 +164,7 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi)
 		preempt_enable();
 	}
 #endif
-	decoded = arm_probe_decode_insn(insn, asi);
+	decoded = arm_probe_decode_insn(insn, &asi->api);
 
 	if (decoded == INSN_REJECTED ||
 			is_probed_address_atomic(scan_start, scan_end))
@@ -172,3 +172,4 @@ arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi)
 
 	return decoded;
 }
+#endif
diff --git a/arch/arm64/kernel/probes/decode-insn.h b/arch/arm64/kernel/probes/decode-insn.h
index d438289646a6..76d3f315407f 100644
--- a/arch/arm64/kernel/probes/decode-insn.h
+++ b/arch/arm64/kernel/probes/decode-insn.h
@@ -23,13 +23,17 @@
  */
 #define MAX_ATOMIC_CONTEXT_SIZE	(128 / sizeof(kprobe_opcode_t))
 
-enum kprobe_insn {
+enum probe_insn {
 	INSN_REJECTED,
 	INSN_GOOD_NO_SLOT,
 	INSN_GOOD,
 };
 
-enum kprobe_insn __kprobes
+#ifdef CONFIG_KPROBES
+enum probe_insn __kprobes
 arm_kprobe_decode_insn(kprobe_opcode_t *addr, struct arch_specific_insn *asi);
+#endif
+enum probe_insn __kprobes
+arm_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *asi);
 
 #endif /* _ARM_KERNEL_KPROBES_ARM64_H */
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index bf9768588288..8e5923c04fef 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -56,31 +56,31 @@ static inline unsigned long min_stack_size(unsigned long addr)
 static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 {
 	/* prepare insn slot */
-	p->ainsn.insn[0] = cpu_to_le32(p->opcode);
+	p->ainsn.api.insn[0] = cpu_to_le32(p->opcode);
 
-	flush_icache_range((uintptr_t) (p->ainsn.insn),
-			   (uintptr_t) (p->ainsn.insn) +
+	flush_icache_range((uintptr_t) (p->ainsn.api.insn),
+			   (uintptr_t) (p->ainsn.api.insn) +
 			   MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
 
 	/*
 	 * Needs restoring of return address after stepping xol.
 	 */
-	p->ainsn.restore = (unsigned long) p->addr +
+	p->ainsn.api.restore = (unsigned long) p->addr +
 	  sizeof(kprobe_opcode_t);
 }
 
 static void __kprobes arch_prepare_simulate(struct kprobe *p)
 {
 	/* This instructions is not executed xol. No need to adjust the PC */
-	p->ainsn.restore = 0;
+	p->ainsn.api.restore = 0;
 }
 
 static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	if (p->ainsn.handler)
-		p->ainsn.handler((u32)p->opcode, (long)p->addr, regs);
+	if (p->ainsn.api.handler)
+		p->ainsn.api.handler((u32)p->opcode, (long)p->addr, regs);
 
 	/* single step simulated, now go for post processing */
 	post_kprobe_handler(kcb, regs);
@@ -110,18 +110,18 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		return -EINVAL;
 
 	case INSN_GOOD_NO_SLOT:	/* insn need simulation */
-		p->ainsn.insn = NULL;
+		p->ainsn.api.insn = NULL;
 		break;
 
 	case INSN_GOOD:	/* instruction uses slot */
-		p->ainsn.insn = get_insn_slot();
-		if (!p->ainsn.insn)
+		p->ainsn.api.insn = get_insn_slot();
+		if (!p->ainsn.api.insn)
 			return -ENOMEM;
 		break;
 	};
 
 	/* prepare the instruction */
-	if (p->ainsn.insn)
+	if (p->ainsn.api.insn)
 		arch_prepare_ss_slot(p);
 	else
 		arch_prepare_simulate(p);
@@ -154,9 +154,9 @@ void __kprobes arch_disarm_kprobe(struct kprobe *p)
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
 {
-	if (p->ainsn.insn) {
-		free_insn_slot(p->ainsn.insn, 0);
-		p->ainsn.insn = NULL;
+	if (p->ainsn.api.insn) {
+		free_insn_slot(p->ainsn.api.insn, 0);
+		p->ainsn.api.insn = NULL;
 	}
 }
 
@@ -251,9 +251,9 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 	}
 
 
-	if (p->ainsn.insn) {
+	if (p->ainsn.api.insn) {
 		/* prepare for single stepping */
-		slot = (unsigned long)p->ainsn.insn;
+		slot = (unsigned long)p->ainsn.api.insn;
 
 		set_ss_context(kcb, slot);	/* mark pending ss */
 
@@ -305,8 +305,8 @@ post_kprobe_handler(struct kprobe_ctlblk *kcb, struct pt_regs *regs)
 		return;
 
 	/* return addr restore if non-branching insn */
-	if (cur->ainsn.restore != 0)
-		instruction_pointer_set(regs, cur->ainsn.restore);
+	if (cur->ainsn.api.restore != 0)
+		instruction_pointer_set(regs, cur->ainsn.api.restore);
 
 	/* restore back original saved kprobe variables and continue */
 	if (kcb->kprobe_status == KPROBE_REENTER) {
-- 
2.5.5

[PATCH 2/5] arm64: kgdb_step_brk_fn: ignore other's exception

[Pratyush Anand]

ARM64 step exception does not have any syndrome information. So, it is
responsibility of exception handler to take care that they handle it
only if exception was raised for them.

Since kgdb_step_brk_fn() always returns 0, therefore we might have problem
when we will have other step handler registered as well.

This patch fixes kgdb_step_brk_fn() to return error in case of step handler
was not meant for kgdb.

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

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 8c57f6496e56..e126f2b15002 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -244,6 +244,9 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
 
 static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
 {
+	if (!kgdb_single_step)
+		return DBG_HOOK_ERROR;
+
 	kgdb_handle_exception(1, SIGTRAP, 0, regs);
 	return 0;
 }
-- 
2.5.5

[PATCH 3/5] 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 | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 91fff48d0f57..fae2f57a92b7 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
 static int single_step_handler(unsigned long addr, unsigned int esr,
 			       struct pt_regs *regs)
 {
+	bool handler_found = false;
+
 	/*
 	 * If we are stepping a pending breakpoint, call the hw_breakpoint
 	 * handler first.
@@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	if (!reinstall_suspended_bps(regs))
 		return 0;
 
-	if (user_mode(regs)) {
+#ifdef	CONFIG_KPROBES
+	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+#endif
+	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+
+	if (!handler_found && user_mode(regs)) {
 		send_user_sigtrap(TRAP_HWBKPT);
 
 		/*
@@ -263,15 +272,8 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 		 * to the active-not-pending state).
 		 */
 		user_rewind_single_step(current);
-	} else {
-#ifdef	CONFIG_KPROBES
-		if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
-			return 0;
-#endif
-		if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
-			return 0;
-
-		pr_warning("Unexpected kernel single-step exception at EL1\n");
+	} else if (!handler_found) {
+		pr_warn("Unexpected kernel single-step exception at EL1\n");
 		/*
 		 * Re-enable stepping since we know that we will be
 		 * returning to regs.
-- 
2.5.5

Re: [PATCH 3/5] arm64: Handle TRAP_HWBRKPT for user mode as well

[Catalin Marinas]

On Tue, Aug 02, 2016 at 11:00:07AM +0530, Pratyush Anand wrote:
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
>  static int single_step_handler(unsigned long addr, unsigned int esr,
>  			       struct pt_regs *regs)
>  {
> +	bool handler_found = false;
> +
>  	/*
>  	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>  	 * handler first.
> @@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
>  	if (!reinstall_suspended_bps(regs))
>  		return 0;
>  
> -	if (user_mode(regs)) {
> +#ifdef	CONFIG_KPROBES
> +	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
> +		handler_found = true;
> +#endif
> +	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> +		handler_found = true;
> +
> +	if (!handler_found && user_mode(regs)) {
>  		send_user_sigtrap(TRAP_HWBKPT);

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

-- 
Catalin

Re: [PATCH 3/5] arm64: Handle TRAP_HWBRKPT for user mode as well

[David Long]

On 09/06/2016 12:11 PM, Catalin Marinas wrote:
> On Tue, Aug 02, 2016 at 11:00:07AM +0530, Pratyush Anand wrote:
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
>>   static int single_step_handler(unsigned long addr, unsigned int esr,
>>   			       struct pt_regs *regs)
>>   {
>> +	bool handler_found = false;
>> +
>>   	/*
>>   	 * If we are stepping a pending breakpoint, call the hw_breakpoint
>>   	 * handler first.
>> @@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
>>   	if (!reinstall_suspended_bps(regs))
>>   		return 0;
>>
>> -	if (user_mode(regs)) {
>> +#ifdef	CONFIG_KPROBES
>> +	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
>> +		handler_found = true;
>> +#endif
>> +	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
>> +		handler_found = true;
>> +
>> +	if (!handler_found && user_mode(regs)) {
>>   		send_user_sigtrap(TRAP_HWBKPT);
>
> Could we register kprobe_single_step_handler() via register_set_hook()
> and only invoke call_step_hook() above?
>

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

-dl

Re: [PATCH 3/5] arm64: Handle TRAP_HWBRKPT for user mode as well

[Pratyush Anand]

On 06/09/2016:05:36:18 PM, David Long wrote:
> On 09/06/2016 12:11 PM, Catalin Marinas wrote:
> > On Tue, Aug 02, 2016 at 11:00:07AM +0530, Pratyush Anand wrote:
> > > --- a/arch/arm64/kernel/debug-monitors.c
> > > +++ b/arch/arm64/kernel/debug-monitors.c
> > > @@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
> > >   static int single_step_handler(unsigned long addr, unsigned int esr,
> > >   			       struct pt_regs *regs)
> > >   {
> > > +	bool handler_found = false;
> > > +
> > >   	/*
> > >   	 * If we are stepping a pending breakpoint, call the hw_breakpoint
> > >   	 * handler first.
> > > @@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
> > >   	if (!reinstall_suspended_bps(regs))
> > >   		return 0;
> > > 
> > > -	if (user_mode(regs)) {
> > > +#ifdef	CONFIG_KPROBES
> > > +	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
> > > +		handler_found = true;
> > > +#endif
> > > +	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > +		handler_found = true;
> > > +
> > > +	if (!handler_found && user_mode(regs)) {
> > >   		send_user_sigtrap(TRAP_HWBKPT);
> > 
> > Could we register kprobe_single_step_handler() via register_set_hook()
> > and only invoke call_step_hook() above?
> > 
> 
> I seem to recall a criticism of doing that in a much earlier kprobes64 patch
> of mine.  The concern was that it would cause unnecessarily more kernel
> functions to be kprobes-blacklisted.  Hence the hardcoded check and call.

Yes, all the code regions are kprobe unsafe which lie within the moment we
receive a break/single step exception to the point where it is handled for
kprobe. Therefore we must call kprobe_single_step/breakpoint_handler() before
other handlers. Otherwise, we would not be able to trace other handlers and the
functions called from those handlers.

~Pratyush

Re: [PATCH 3/5] arm64: Handle TRAP_HWBRKPT for user mode as well

[Catalin Marinas]

On Tue, Sep 06, 2016 at 05:36:18PM -0400, David Long wrote:
> On 09/06/2016 12:11 PM, Catalin Marinas wrote:
> > On Tue, Aug 02, 2016 at 11:00:07AM +0530, Pratyush Anand wrote:
> > > --- a/arch/arm64/kernel/debug-monitors.c
> > > +++ b/arch/arm64/kernel/debug-monitors.c
> > > @@ -246,6 +246,8 @@ static void send_user_sigtrap(int si_code)
> > >   static int single_step_handler(unsigned long addr, unsigned int esr,
> > >   			       struct pt_regs *regs)
> > >   {
> > > +	bool handler_found = false;
> > > +
> > >   	/*
> > >   	 * If we are stepping a pending breakpoint, call the hw_breakpoint
> > >   	 * handler first.
> > > @@ -253,7 +255,14 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
> > >   	if (!reinstall_suspended_bps(regs))
> > >   		return 0;
> > > 
> > > -	if (user_mode(regs)) {
> > > +#ifdef	CONFIG_KPROBES
> > > +	if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
> > > +		handler_found = true;
> > > +#endif
> > > +	if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> > > +		handler_found = true;
> > > +
> > > +	if (!handler_found && user_mode(regs)) {
> > >   		send_user_sigtrap(TRAP_HWBKPT);
> > 
> > Could we register kprobe_single_step_handler() via register_set_hook()
> > and only invoke call_step_hook() above?
> 
> I seem to recall a criticism of doing that in a much earlier kprobes64 patch
> of mine.  The concern was that it would cause unnecessarily more kernel
> functions to be kprobes-blacklisted.  Hence the hardcoded check and call.

Ah, ok. I missed this aspect.

-- 
Catalin

[PATCH 4/5] 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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index fae2f57a92b7..466d199498b5 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -326,16 +326,20 @@ NOKPROBE_SYMBOL(call_break_hook);
 static int brk_handler(unsigned long addr, unsigned int esr,
 		       struct pt_regs *regs)
 {
-	if (user_mode(regs)) {
-		send_user_sigtrap(TRAP_BRKPT);
-	}
+	bool handler_found = false;
+
 #ifdef	CONFIG_KPROBES
-	else if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
-		if (kprobe_breakpoint_handler(regs, esr) != DBG_HOOK_HANDLED)
-			return -EFAULT;
+	if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
+		if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
+			handler_found = true;
 	}
 #endif
-	else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
+	if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+		handler_found = true;
+
+	if (!handler_found && user_mode(regs)) {
+		send_user_sigtrap(TRAP_BRKPT);
+	} else if (!handler_found) {
 		pr_warn("Unexpected kernel BRK exception at EL1\n");
 		return -EFAULT;
 	}
-- 
2.5.5

Re: [PATCH 4/5] arm64: Handle TRAP_BRKPT for user mode as well

[Catalin Marinas]

On Tue, Aug 02, 2016 at 11:00:08AM +0530, Pratyush Anand wrote:
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -326,16 +326,20 @@ NOKPROBE_SYMBOL(call_break_hook);
>  static int brk_handler(unsigned long addr, unsigned int esr,
>  		       struct pt_regs *regs)
>  {
> -	if (user_mode(regs)) {
> -		send_user_sigtrap(TRAP_BRKPT);
> -	}
> +	bool handler_found = false;
> +
>  #ifdef	CONFIG_KPROBES
> -	else if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
> -		if (kprobe_breakpoint_handler(regs, esr) != DBG_HOOK_HANDLED)
> -			return -EFAULT;
> +	if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
> +		if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
> +			handler_found = true;
>  	}
>  #endif
> -	else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
> +	if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> +		handler_found = true;
> +
> +	if (!handler_found && user_mode(regs)) {
> +		send_user_sigtrap(TRAP_BRKPT);
> +	} else if (!handler_found) {
>  		pr_warn("Unexpected kernel BRK exception at EL1\n");
>  		return -EFAULT;
>  	}

I think we could do the same here with a single call_break_hook() and
making sure that the corresponding handlers check the esr for the
corresponding BRK encoding.

-- 
Catalin

[PATCH 5/5] 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. Step-able instructions, like sub, ldr, add etc.
    2. Simulation-able like ret, cbnz, cbz etc.
    3. uretprobe
    4. Reject-able instructions like sev, wfe etc.
    5. trapped and abort xol path
    6. probe at unaligned user address.
    7. longjump test cases

Currently it does not support aarch32 instruction probing.

Thanks to Shi Yang <yang.shi@linaro.org, for suggesting to define
TIF_UPROBE as 5 in stead of 4, since 4 has already been used in -rt kernel.

Signed-off-by: Pratyush Anand <panand@redhat.com>
Cc: Shi Yang <yang.shi@linaro.org>
---
 arch/arm64/Kconfig                      |   3 +
 arch/arm64/include/asm/debug-monitors.h |   3 +
 arch/arm64/include/asm/probes.h         |   4 +
 arch/arm64/include/asm/ptrace.h         |   8 ++
 arch/arm64/include/asm/thread_info.h    |   5 +-
 arch/arm64/include/asm/uprobes.h        |  37 ++++++
 arch/arm64/kernel/entry.S               |   6 +-
 arch/arm64/kernel/probes/Makefile       |   2 +
 arch/arm64/kernel/probes/uprobes.c      | 227 ++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c              |   4 +-
 arch/arm64/mm/flush.c                   |   6 +
 11 files changed, 301 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm64/include/asm/uprobes.h
 create mode 100644 arch/arm64/kernel/probes/uprobes.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9f8b99e20557..77be79cb123b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -232,6 +232,9 @@ config PGTABLE_LEVELS
 	default 3 if ARM64_16K_PAGES && ARM64_VA_BITS_47
 	default 4 if !ARM64_64K_PAGES && ARM64_VA_BITS_48
 
+config ARCH_SUPPORTS_UPROBES
+	def_bool y
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 4b6b3f72a215..d04248b749a8 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -70,6 +70,9 @@
 #define BRK64_ESR_MASK		0xFFFF
 #define BRK64_ESR_KPROBES	0x0004
 #define BRK64_OPCODE_KPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
+/* uprobes BRK opcodes with ESR encoding  */
+#define BRK64_ESR_UPROBES	0x0008
+#define BRK64_OPCODE_UPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))
 
 /* AArch32 */
 #define DBG_ESR_EVT_BKPT	0x4
diff --git a/arch/arm64/include/asm/probes.h b/arch/arm64/include/asm/probes.h
index e175a825b187..3d8fbca3f556 100644
--- a/arch/arm64/include/asm/probes.h
+++ b/arch/arm64/include/asm/probes.h
@@ -35,4 +35,8 @@ struct arch_specific_insn {
 };
 #endif
 
+#ifdef CONFIG_UPROBES
+typedef u32 uprobe_opcode_t;
+#endif
+
 #endif
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index ada08b5b036d..513daf050e84 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -217,6 +217,14 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
 
 #include <asm-generic/ptrace.h>
 
+#define procedure_link_pointer(regs)	((regs)->regs[30])
+
+static inline void procedure_link_pointer_set(struct pt_regs *regs,
+					   unsigned long val)
+{
+	procedure_link_pointer(regs) = val;
+}
+
 #undef profile_pc
 extern unsigned long profile_pc(struct pt_regs *regs);
 
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index abd64bd1f6d9..d5ebf2ee5024 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_NEED_RESCHED	1
 #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
 #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
+#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
 #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..434a3afebcd1
--- /dev/null
+++ b/arch/arm64/include/asm/uprobes.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2014-2015 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/debug-monitors.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
+
+struct arch_uprobe_task {
+	unsigned long saved_fault_code;
+};
+
+struct arch_uprobe {
+	union {
+		u8 insn[MAX_UINSN_BYTES];
+		u8 ixol[MAX_UINSN_BYTES];
+	};
+	struct arch_probe_insn api;
+	bool simulate;
+};
+
+extern void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len);
+#endif
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 96e4a2b64cc1..801b9064b89a 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -688,7 +688,8 @@ ret_fast_syscall:
 	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
 	and	x2, x1, #_TIF_SYSCALL_WORK
 	cbnz	x2, ret_fast_syscall_trace
-	and	x2, x1, #_TIF_WORK_MASK
+	mov     x2, #_TIF_WORK_MASK
+	and     x2, x1, x2
 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
 	kernel_exit 0
@@ -718,7 +719,8 @@ work_resched:
 ret_to_user:
 	disable_irq				// disable interrupts
 	ldr	x1, [tsk, #TI_FLAGS]
-	and	x2, x1, #_TIF_WORK_MASK
+	mov     x2, #_TIF_WORK_MASK
+	and     x2, x1, x2
 	cbnz	x2, work_pending
 	enable_step_tsk x1, x2
 	kernel_exit 0
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index ce06312e3d34..89b6df613dde 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -1,3 +1,5 @@
 obj-$(CONFIG_KPROBES)		+= kprobes.o decode-insn.o	\
 				   kprobes_trampoline.o		\
 				   simulate-insn.o
+obj-$(CONFIG_UPROBES)		+= uprobes.o decode-insn.o	\
+				   simulate-insn.o
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
new file mode 100644
index 000000000000..4d9d21f6e22e
--- /dev/null
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -0,0 +1,227 @@
+/*
+ * Copyright (C) 2014-2015 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 "decode-insn.h"
+
+#define UPROBE_INV_FAULT_CODE	UINT_MAX
+
+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;
+
+	/* TODO: Currently we do not support AARCH32 instruction probing */
+
+	if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
+		return -EINVAL;
+
+	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+
+	switch (arm_probe_decode_insn(insn, &auprobe->api)) {
+	case INSN_REJECTED:
+		return -EINVAL;
+
+	case INSN_GOOD_NO_SLOT:
+		auprobe->simulate = true;
+		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;
+
+	/* Instruction point to execute ol */
+	instruction_pointer_set(regs, utask->xol_vaddr);
+
+	user_enable_single_step(current);
+
+	return 0;
+}
+
+int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+
+	WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
+
+	/* restore fault code */
+	current->thread.fault_code = utask->autask.saved_fault_code;
+
+	/* Instruction point to execute next to breakpoint address */
+	instruction_pointer_set(regs, utask->vaddr + 4);
+
+	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->api.handler)
+		auprobe->api.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);
+}
+
+bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check ctx,
+		struct pt_regs *regs)
+{
+	/*
+	 * If a simple branch instruction (B) was called for retprobed
+	 * assembly label then return true even when regs->sp and ret->stack
+	 * are same. It will insure that cleanup and reporting of return
+	 * instances corresponding to callee label is done when
+	 * handle_trampoline for called function is executed.
+	 */
+	if (ctx == RP_CHECK_CHAIN_CALL)
+		return regs->sp <= ret->stack;
+	else
+		return regs->sp < ret->stack;
+}
+
+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_set(regs, trampoline_vaddr);
+
+	return orig_ret_vaddr;
+}
+
+int arch_uprobe_exception_notify(struct notifier_block *self,
+				 unsigned long val, void *data)
+{
+	return NOTIFY_DONE;
+}
+
+static int __kprobes uprobe_breakpoint_handler(struct pt_regs *regs,
+		unsigned int esr)
+{
+	if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
+		return DBG_HOOK_HANDLED;
+
+	return DBG_HOOK_ERROR;
+}
+
+static int __kprobes uprobe_single_step_handler(struct pt_regs *regs,
+		unsigned int esr)
+{
+	struct uprobe_task *utask = current->utask;
+
+	if (user_mode(regs)) {
+		WARN_ON(utask &&
+			(instruction_pointer(regs) != utask->xol_vaddr + 4));
+
+		if (uprobe_post_sstep_notifier(regs))
+			return DBG_HOOK_HANDLED;
+	}
+
+	return DBG_HOOK_ERROR;
+}
+
+/* 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/kernel/signal.c b/arch/arm64/kernel/signal.c
index a8eafdbc7cb8..0ff1208929c0 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -402,6 +402,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);
 
@@ -412,5 +415,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/mm/flush.c b/arch/arm64/mm/flush.c
index 43a76b07eb32..179587614756 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -54,6 +54,12 @@ static void flush_ptrace_access(struct vm_area_struct *vma, struct page *page,
 		sync_icache_aliases(kaddr, len);
 }
 
+void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
+		void *kaddr, unsigned long len)
+{
+	sync_icache_aliases(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.5.5

Re: [PATCH 5/5] arm64: Add uprobe support

[Oleg Nesterov]

On 08/02, Pratyush Anand wrote:
>
> This patch adds support for uprobe on ARM64 architecture.

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

Just one note,

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

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

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

Oleg.

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

Hi Oleg,

Thanks a lot for your review, and sorry for delayed response.

On 09/08/2016:08:49:44 PM, Oleg Nesterov wrote:
> On 08/02, Pratyush Anand wrote:
> >
> > This patch adds support for uprobe on ARM64 architecture.
> 
> I know nothing about ARM, so I can't actually review this change.
> But it looks good to me ;)
> 
> Just one note,
> 
> > +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > +{
> > +	struct uprobe_task *utask = current->utask;
> > +
> > +	/* saved fault code is restored in post_xol */
> > +	utask->autask.saved_fault_code = current->thread.fault_code;
> > +
> > +	/* An invalid fault code between pre/post xol event */
> > +	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
> > +
> > +	/* Instruction point to execute ol */
> > +	instruction_pointer_set(regs, utask->xol_vaddr);
> > +
> > +	user_enable_single_step(current);
> 
> I don't think we want user_{enable,disable{_single_step in the long term,
> please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> the same.

IIUC, then you mean that TIF_SINGLESTEP is a per task flag, while
arch_uprobe_pre/post_xol() should enable/disable single stepping using a per
uprobe_task, and we should have a flag in "struct arch_uprobe_task" to handle
this, right?

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

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

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Oleg Nesterov]

Hi Pratyush,

On 08/24, Pratyush Anand wrote:
>
> > I don't think we want user_{enable,disable{_single_step in the long term,
> > please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> > /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> > TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> > the same.
>
> IIUC, then you mean that TIF_SINGLESTEP is a per task flag,

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

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

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

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

Probably yes, because we need to record/restore X86_EFLAGS_TF in case it
was already set by ptrace or something else.

> > However, I agree we can do this later and initial version can use these
> > ptrace helpers.
>
> Yes, I would also like to do that change latter, because these set of patches
> have already been tested heavily with systemtap, so it would be better to go
> with an incremental changes latter on.

Yes, yes, I agree. Let me repeat that this patch looks good to me as initial
version, but obviously I can't really revit it and/or ack.

Oleg.

Re: [PATCH 5/5] arm64: Add uprobe support

[Will Deacon]

On Wed, Aug 24, 2016 at 05:47:11PM +0200, Oleg Nesterov wrote:
> On 08/24, Pratyush Anand wrote:
> >
> > > I don't think we want user_{enable,disable{_single_step in the long term,
> > > please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> > > /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> > > TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> > > the same.
> >
> > IIUC, then you mean that TIF_SINGLESTEP is a per task flag,
> 
> Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse
> each other. And uprobes simply doesn't need to set/clear it.

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

In what way do things get confused?

> > while
> > arch_uprobe_pre/post_xol() should enable/disable single stepping using a per
> > uprobe_task,
> 
> I can't really answer since I know nothing about arm. x86 just needs to set
> X86_EFLAGS_TF, I guess arm needs to modify some register too?

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

Will

Re: [PATCH 5/5] arm64: Add uprobe support

[Oleg Nesterov]

On 08/24, Will Deacon wrote:
>
> On Wed, Aug 24, 2016 at 05:47:11PM +0200, Oleg Nesterov wrote:
> > On 08/24, Pratyush Anand wrote:
> > >
> > > > I don't think we want user_{enable,disable{_single_step in the long term,
> > > > please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> > > > /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> > > > TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> > > > the same.
> > >
> > > IIUC, then you mean that TIF_SINGLESTEP is a per task flag,
> >
> > Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse
> > each other. And uprobes simply doesn't need to set/clear it.
>
> We're already using it for kprobes, hw_breakpoint and kgdb as well as
> ptrace, so I'd rather uprobes either followed existing practice, or we
> converted everybody off the current code.

And perhaps this is fine for arm64, I do not know.

> In what way do things get confused?

Say, arch_uprobe_post_xol() should not blindly do user_disable_single_step(),
this can confuse ptrace if TIF_SINGLESTEP was set by debugger which wants
to step over the probed insn.

> > I can't really answer since I know nothing about arm. x86 just needs to set
> > X86_EFLAGS_TF, I guess arm needs to modify some register too?
>
> We have {user,kernel}_{enable,disable}_single_step for managing the various
> registers controlling the single-step state machine on arm64.

Yes, and perhaps uprobes can just do set_regs_spsr_ss() ? I never looked into
arch/arm64/, but it seems that we only need to ensure that call_step_hook()
will be called even if user_mode() == T, why do we need TIF_SINGLESTEP ?

Nevermind. I can be easily wrong and let me repeat that I agree with
user_{enable,disable}_single_step in the initial version in any case.

Oleg.

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

Hi Pratyush,

On Tue, Aug 02, 2016 at 11:00:09AM +0530, Pratyush Anand wrote:
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -70,6 +70,9 @@
>  #define BRK64_ESR_MASK		0xFFFF
>  #define BRK64_ESR_KPROBES	0x0004
>  #define BRK64_OPCODE_KPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
> +/* uprobes BRK opcodes with ESR encoding  */
> +#define BRK64_ESR_UPROBES	0x0008
> +#define BRK64_OPCODE_UPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))

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

> --- a/arch/arm64/include/asm/probes.h
> +++ b/arch/arm64/include/asm/probes.h
> @@ -35,4 +35,8 @@ struct arch_specific_insn {
>  };
>  #endif
>  
> +#ifdef CONFIG_UPROBES
> +typedef u32 uprobe_opcode_t;
> +#endif

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

> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -217,6 +217,14 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
>  
>  #include <asm-generic/ptrace.h>
>  
> +#define procedure_link_pointer(regs)	((regs)->regs[30])
> +
> +static inline void procedure_link_pointer_set(struct pt_regs *regs,
> +					   unsigned long val)
> +{
> +	procedure_link_pointer(regs) = val;
> +}

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

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

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

> --- /dev/null
> +++ b/arch/arm64/include/asm/uprobes.h
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) 2014-2015 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/debug-monitors.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

Nitpick: for consistency, just use AARCH64_INSN_SIZE.

> +#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES
> +
> +struct arch_uprobe_task {
> +	unsigned long saved_fault_code;
> +};
> +
> +struct arch_uprobe {
> +	union {
> +		u8 insn[MAX_UINSN_BYTES];
> +		u8 ixol[MAX_UINSN_BYTES];
> +	};
> +	struct arch_probe_insn api;
> +	bool simulate;
> +};
> +
> +extern void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
> +		void *kaddr, unsigned long len);

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

> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -688,7 +688,8 @@ ret_fast_syscall:
>  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
>  	and	x2, x1, #_TIF_SYSCALL_WORK
>  	cbnz	x2, ret_fast_syscall_trace
> -	and	x2, x1, #_TIF_WORK_MASK
> +	mov     x2, #_TIF_WORK_MASK
> +	and     x2, x1, x2

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

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

Same here.

> --- /dev/null
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -0,0 +1,227 @@
> +/*
> + * Copyright (C) 2014-2015 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 "decode-insn.h"
> +
> +#define UPROBE_INV_FAULT_CODE	UINT_MAX
> +
> +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();

kmap_atomic() already disabled preemption.

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

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

> +
> +	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;
> +
> +	/* TODO: Currently we do not support AARCH32 instruction probing */

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

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

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

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

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

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

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


-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

Hi Catalin,

Thanks for your review.

On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> Hi Pratyush,
> 
> On Tue, Aug 02, 2016 at 11:00:09AM +0530, Pratyush Anand wrote:
> > --- a/arch/arm64/include/asm/debug-monitors.h
> > +++ b/arch/arm64/include/asm/debug-monitors.h
> > @@ -70,6 +70,9 @@
> >  #define BRK64_ESR_MASK		0xFFFF
> >  #define BRK64_ESR_KPROBES	0x0004
> >  #define BRK64_OPCODE_KPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
> > +/* uprobes BRK opcodes with ESR encoding  */
> > +#define BRK64_ESR_UPROBES	0x0008
> > +#define BRK64_OPCODE_UPROBES	(AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))
> 
> You can use 0x0005 as immediate, we don't need individual bits here.

OK. will define BRK64_ESR_UPROBES as 0x0005

> 
> > --- a/arch/arm64/include/asm/probes.h
> > +++ b/arch/arm64/include/asm/probes.h
> > @@ -35,4 +35,8 @@ struct arch_specific_insn {
> >  };
> >  #endif
> >  
> > +#ifdef CONFIG_UPROBES
> > +typedef u32 uprobe_opcode_t;
> > +#endif
> 
> We don't need #ifdef around this typedef. Also, all the other
> architectures seem to have this typedef in asm/uprobes.h.

kprobe_opcode_t was defined here, so I defined it here as well. But yes, it
would be good to move it to asm/uprobes.h to keep in sync with other arch.

> 
> > --- a/arch/arm64/include/asm/ptrace.h
> > +++ b/arch/arm64/include/asm/ptrace.h
> > @@ -217,6 +217,14 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task);
> >  
> >  #include <asm-generic/ptrace.h>
> >  
> > +#define procedure_link_pointer(regs)	((regs)->regs[30])
> > +
> > +static inline void procedure_link_pointer_set(struct pt_regs *regs,
> > +					   unsigned long val)
> > +{
> > +	procedure_link_pointer(regs) = val;
> > +}
> 
> Do you need these macro and function here? It seems that they are only
> used in arch_uretprobe_hijack_return_addr(), so you can just expand them
> in there, no need for additional definitions.

I introduced it to keep sync with other register usage like
instruction_pointer_set().
I have used it in uprobe code only, but I think, we can have a patch to modify
following code, which can use them as well.

arch/arm64/kernel/probes/kprobes.c:     ri->ret_addr = (kprobe_opcode_t *)regs->regs[30];
arch/arm64/kernel/probes/kprobes.c:     regs->regs[30] = (long)&kretprobe_trampoline;
arch/arm64/kernel/process.c:            lr = regs->regs[30];
arch/arm64/kernel/signal.c:     __put_user_error(regs->regs[30], &sf->lr, err);
arch/arm64/kernel/signal.c:     regs->regs[30] = (unsigned long)sigtramp;

> 
> > --- a/arch/arm64/include/asm/thread_info.h
> > +++ b/arch/arm64/include/asm/thread_info.h
> > @@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
> >  #define TIF_NEED_RESCHED	1
> >  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> >  #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
> > +#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
> 
> Nitpick: you can just use 4 until we cover this gap.

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

> 
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/uprobes.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Copyright (C) 2014-2015 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/debug-monitors.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
> 
> Nitpick: for consistency, just use AARCH64_INSN_SIZE.

OK

> 
> > +#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES
> > +
> > +struct arch_uprobe_task {
> > +	unsigned long saved_fault_code;
> > +};
> > +
> > +struct arch_uprobe {
> > +	union {
> > +		u8 insn[MAX_UINSN_BYTES];
> > +		u8 ixol[MAX_UINSN_BYTES];
> > +	};
> > +	struct arch_probe_insn api;
> > +	bool simulate;
> > +};
> > +
> > +extern void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
> > +		void *kaddr, unsigned long len);
> 
> I don't think we need this. It doesn't seem to be used anywhere other
> than the arm64 uprobes.c file. So I would rather expose
> sync_icache_aliases(), similarly to __sync_icache_dcache().

OK.

> 
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -688,7 +688,8 @@ ret_fast_syscall:
> >  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> >  	and	x2, x1, #_TIF_SYSCALL_WORK
> >  	cbnz	x2, ret_fast_syscall_trace
> > -	and	x2, x1, #_TIF_WORK_MASK
> > +	mov     x2, #_TIF_WORK_MASK
> > +	and     x2, x1, x2
> 
> Is this needed because _TIF_WORK_MASK cannot work as an immediate value
> to 'and'? We could reorder the TIF bits, they are not exposed to user to
> have ABI implications.

_TIF_WORK_MASK is defined as follows:

#define _TIF_WORK_MASK          (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
                                  _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
                                  _TIF_UPROBE)
Re-ordering will not help, because 0-3 have already been used by previous
definitions. Only way to use immediate value could be if, TIF_UPROBE is defined
as 4. 

> 
> >  	cbnz	x2, work_pending
> >  	enable_step_tsk x1, x2
> >  	kernel_exit 0
> > @@ -718,7 +719,8 @@ work_resched:
> >  ret_to_user:
> >  	disable_irq				// disable interrupts
> >  	ldr	x1, [tsk, #TI_FLAGS]
> > -	and	x2, x1, #_TIF_WORK_MASK
> > +	mov     x2, #_TIF_WORK_MASK
> > +	and     x2, x1, x2
> >  	cbnz	x2, work_pending
> >  	enable_step_tsk x1, x2
> >  	kernel_exit 0
> 
> Same here.
> 
> > --- /dev/null
> > +++ b/arch/arm64/kernel/probes/uprobes.c
> > @@ -0,0 +1,227 @@
> > +/*
> > + * Copyright (C) 2014-2015 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 "decode-insn.h"
> > +
> > +#define UPROBE_INV_FAULT_CODE	UINT_MAX
> > +
> > +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();
> 
> kmap_atomic() already disabled preemption.

Yes..will remove.

> 
> > +
> > +	/* Initialize the slot */
> > +	memcpy(dst, src, len);
> > +
> > +	/* flush caches (dcache/icache) */
> > +	flush_uprobe_xol_access(page, vaddr, dst, len);
> 
> Just use sync_icache_aliases() here (once exposed in an arm64 header).

OK.

> 
> > +
> > +	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;
> > +
> > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> 
> Is there a way to check (not necessarily in this file) that we don't
> probe 32-bit tasks?

- Well, I do not have complete idea about it that, how it can be done. I think
  we can not check that just by looking a single bit in an instruction.
  My understanding is that, we can only know about it when we are executing the
  instruction, by reading pstate, but that would not be useful for uprobe
  instruction analysis.
  
  I hope, instruction encoding for aarch32 and aarch64 are different, and by
  analyzing for all types of aarch32 instructions, we will be able to decide
  that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
  either BRK or BKPT instruction for breakpoint generation.

> 
> > +	if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
> > +		return -EINVAL;
> > +
> > +	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
> > +
> > +	switch (arm_probe_decode_insn(insn, &auprobe->api)) {
> > +	case INSN_REJECTED:
> > +		return -EINVAL;
> > +
> > +	case INSN_GOOD_NO_SLOT:
> > +		auprobe->simulate = true;
> > +		break;
> > +
> > +	case INSN_GOOD:
> > +	default:
> > +		break;
> 
> Nitpick, we don't need case INSN_GOOD as well since default would cover
> it (that's unless you want to change default to BUG()).

we will not have other than these 3, so need to introduce BUG(). I will remove
INSN_GOOD.

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > +{
> > +	struct uprobe_task *utask = current->utask;
> > +
> > +	/* saved fault code is restored in post_xol */
> > +	utask->autask.saved_fault_code = current->thread.fault_code;
> 
> Does the current->thread.fault_code has any meaning here? We use it in
> the arm64 code when delivering a signal to user but I don't think that's
> the case here, we are handling a breakpoint instruction and there isn't
> anything that set the fault_code.

Correct, they will just having garbage values. will remove.

> 
> > +
> > +	/* An invalid fault code between pre/post xol event */
> > +	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
> > +
> > +	/* Instruction point to execute ol */
> > +	instruction_pointer_set(regs, utask->xol_vaddr);
> > +
> > +	user_enable_single_step(current);
> > +
> > +	return 0;
> > +}
> > +
> > +int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > +{
> > +	struct uprobe_task *utask = current->utask;
> > +
> > +	WARN_ON_ONCE(current->thread.fault_code != UPROBE_INV_FAULT_CODE);
> > +
> > +	/* restore fault code */
> > +	current->thread.fault_code = utask->autask.saved_fault_code;
> 
> Same here, I don't think this needs restoring if it wasn't meaningful in
> the first place.

OK.

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > On Tue, Aug 02, 2016 at 11:00:09AM +0530, Pratyush Anand wrote:
> > > --- a/arch/arm64/include/asm/thread_info.h
> > > +++ b/arch/arm64/include/asm/thread_info.h
> > > @@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
> > >  #define TIF_NEED_RESCHED	1
> > >  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> > >  #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
> > > +#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
> > 
> > Nitpick: you can just use 4 until we cover this gap.
> 
> Hummm..as stated in commit log, Shi Yang suggested to define TIF_UPROBE as 5 in
> stead of 4, since 4 has already been used in -rt kernel. May be, I can put a
> comment in code as well.
> Or, keep it 4 and -rt kernel will change their definitions. I am OK with both.
> let me know.

I forgot about the -rt kernel. I guess the -rt guys need to rebase the
patches anyway on top of mainline, so it's just a matter of sorting out
a minor conflict (as I already said, these bits are internal to the
kernel, so no ABI affected). For now, just use 4 so that we avoid
additional asm changes.

> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -688,7 +688,8 @@ ret_fast_syscall:
> > >  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> > >  	and	x2, x1, #_TIF_SYSCALL_WORK
> > >  	cbnz	x2, ret_fast_syscall_trace
> > > -	and	x2, x1, #_TIF_WORK_MASK
> > > +	mov     x2, #_TIF_WORK_MASK
> > > +	and     x2, x1, x2
> > 
> > Is this needed because _TIF_WORK_MASK cannot work as an immediate value
> > to 'and'? We could reorder the TIF bits, they are not exposed to user to
> > have ABI implications.
> 
> _TIF_WORK_MASK is defined as follows:
> 
> #define _TIF_WORK_MASK          (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>                                   _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
>                                   _TIF_UPROBE)
> Re-ordering will not help, because 0-3 have already been used by previous
> definitions. Only way to use immediate value could be if, TIF_UPROBE is defined
> as 4. 

Yes, see above.

> > > +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;
> > > +
> > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > 
> > Is there a way to check (not necessarily in this file) that we don't
> > probe 32-bit tasks?
> 
> - Well, I do not have complete idea about it that, how it can be done. I think
>   we can not check that just by looking a single bit in an instruction.
>   My understanding is that, we can only know about it when we are executing the
>   instruction, by reading pstate, but that would not be useful for uprobe
>   instruction analysis.
>   
>   I hope, instruction encoding for aarch32 and aarch64 are different, and by
>   analyzing for all types of aarch32 instructions, we will be able to decide
>   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
>   either BRK or BKPT instruction for breakpoint generation.

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

-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> > On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > +		unsigned long addr)
> > > > +{
> > > > +	probe_opcode_t insn;
> > > > +
> > > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > > 
> > > Is there a way to check (not necessarily in this file) that we don't
> > > probe 32-bit tasks?
> > 
> > - Well, I do not have complete idea about it that, how it can be done. I think
> >   we can not check that just by looking a single bit in an instruction.
> >   My understanding is that, we can only know about it when we are executing the
> >   instruction, by reading pstate, but that would not be useful for uprobe
> >   instruction analysis.
> >   
> >   I hope, instruction encoding for aarch32 and aarch64 are different, and by
> >   analyzing for all types of aarch32 instructions, we will be able to decide
> >   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
> >   either BRK or BKPT instruction for breakpoint generation.
> 
> We may have some unrelated instruction encoding overlapping but I
> haven't checked. I was more thinking about whether we know which task is
> being probed and check is_compat_task() or maybe using
> compat_user_mode(regs).

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

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

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

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

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

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> > > On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > > +		unsigned long addr)
> > > > > +{
> > > > > +	probe_opcode_t insn;
> > > > > +
> > > > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > > > 
> > > > Is there a way to check (not necessarily in this file) that we don't
> > > > probe 32-bit tasks?
> > > 
> > > - Well, I do not have complete idea about it that, how it can be done. I think
> > >   we can not check that just by looking a single bit in an instruction.
> > >   My understanding is that, we can only know about it when we are executing the
> > >   instruction, by reading pstate, but that would not be useful for uprobe
> > >   instruction analysis.
> > >   
> > >   I hope, instruction encoding for aarch32 and aarch64 are different, and by
> > >   analyzing for all types of aarch32 instructions, we will be able to decide
> > >   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
> > >   either BRK or BKPT instruction for breakpoint generation.
> > 
> > We may have some unrelated instruction encoding overlapping but I
> > haven't checked. I was more thinking about whether we know which task is
> > being probed and check is_compat_task() or maybe using
> > compat_user_mode(regs).
> 
> I had thought of this, but problem is that we might not have task in existence
> when we enable uprobes.  For example: Lets say we are inserting a trace probe at
> offset 0x690 in a executable binary.
> 
> echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events
> echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
> 
> In the 'enable' step, it is decided that whether instruction is traceable or
> not. 
> 
> (1) But at this point 'test' executable might not be running.
> (2) Even if it is running, is_compat_task() or compat_user_mode() might not be
> usable, as they work with 'current' task.

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

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

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

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

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

You can get access to struct linux_binfmt via mm_struct but it doesn't
currently help much since all the members of this structure point to
static functions. Maybe an enum in struct linux_binfmt with format types
exposed to the rest of the kernel?

-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > > On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> > > > On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > > > +		unsigned long addr)
> > > > > > +{
> > > > > > +	probe_opcode_t insn;
> > > > > > +
> > > > > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > > > > 
> > > > > Is there a way to check (not necessarily in this file) that we don't
> > > > > probe 32-bit tasks?
> > > > 
> > > > - Well, I do not have complete idea about it that, how it can be done. I think
> > > >   we can not check that just by looking a single bit in an instruction.
> > > >   My understanding is that, we can only know about it when we are executing the
> > > >   instruction, by reading pstate, but that would not be useful for uprobe
> > > >   instruction analysis.
> > > >   
> > > >   I hope, instruction encoding for aarch32 and aarch64 are different, and by
> > > >   analyzing for all types of aarch32 instructions, we will be able to decide
> > > >   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
> > > >   either BRK or BKPT instruction for breakpoint generation.
> > > 
> > > We may have some unrelated instruction encoding overlapping but I
> > > haven't checked. I was more thinking about whether we know which task is
> > > being probed and check is_compat_task() or maybe using
> > > compat_user_mode(regs).
> > 
> > I had thought of this, but problem is that we might not have task in existence
> > when we enable uprobes.  For example: Lets say we are inserting a trace probe at
> > offset 0x690 in a executable binary.
> > 
> > echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events
> > echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
> > 
> > In the 'enable' step, it is decided that whether instruction is traceable or
> > not. 
> > 
> > (1) But at this point 'test' executable might not be running.

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

> > (2) Even if it is running, is_compat_task() or compat_user_mode() might not be
> > usable, as they work with 'current' task.
> 
> What I find strange is that uprobes allows you to insert a breakpoint
> instruction that's not even compatible with the task (so it would
> SIGILL rather than generate a debug exception).
> 
> > What I was thinking that, let it go with 'TODO' as of now. 
> 
> Only that I don't have any guarantee that someone is going to fix it ;).
> 
> As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> the arch_uprobe_analyze_insn() function.

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

Thanks for your input. 

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > > > On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> > > > > On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > > > > > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > > > > > +		unsigned long addr)
> > > > > > > +{
> > > > > > > +	probe_opcode_t insn;
> > > > > > > +
> > > > > > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > > > > > 
> > > > > > Is there a way to check (not necessarily in this file) that we don't
> > > > > > probe 32-bit tasks?
> > > > > 
> > > > > - Well, I do not have complete idea about it that, how it can be done. I think
> > > > >   we can not check that just by looking a single bit in an instruction.
> > > > >   My understanding is that, we can only know about it when we are executing the
> > > > >   instruction, by reading pstate, but that would not be useful for uprobe
> > > > >   instruction analysis.
> > > > >   
> > > > >   I hope, instruction encoding for aarch32 and aarch64 are different, and by
> > > > >   analyzing for all types of aarch32 instructions, we will be able to decide
> > > > >   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
> > > > >   either BRK or BKPT instruction for breakpoint generation.
> > > > 
> > > > We may have some unrelated instruction encoding overlapping but I
> > > > haven't checked. I was more thinking about whether we know which task is
> > > > being probed and check is_compat_task() or maybe using
> > > > compat_user_mode(regs).
> > > 
> > > I had thought of this, but problem is that we might not have task in existence
> > > when we enable uprobes.  For example: Lets say we are inserting a trace probe at
> > > offset 0x690 in a executable binary.
> > > 
> > > echo "p test:0x690" > /sys/kernel/debug/tracing/uprobe_events
> > > echo 1 > /sys/kernel/debug/tracing/events/uprobes/enable
> > > 
> > > In the 'enable' step, it is decided that whether instruction is traceable or
> > > not. 
> > > 
> > > (1) But at this point 'test' executable might not be running.
> 
> Let me correct myself first here. When executable is not running, then,
> arch_uprobe_analyze_insn() is not called while uprobes enabling (ie writing '1'
> to 'enable'). In that case, it is called when binary is executed and task is
> created.

I kind of figured this out. This function needs to read the actual
instruction from memory, so that won't be possible until at least the
executable file has an address_space in the kernel. What's not clear to
me is how early this is done, do we have the mm_struct fully populated?

> > > (2) Even if it is running, is_compat_task() or compat_user_mode() might not be
> > > usable, as they work with 'current' task.
> > 
> > What I find strange is that uprobes allows you to insert a breakpoint
> > instruction that's not even compatible with the task (so it would
> > SIGILL rather than generate a debug exception).
> > 
> > > What I was thinking that, let it go with 'TODO' as of now. 
> > 
> > Only that I don't have any guarantee that someone is going to fix it ;).
> > 
> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > the arch_uprobe_analyze_insn() function.
> 
> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> return -EINVAL when mm->task_size < TASK_SIZE_64.

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

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

-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
>> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
>> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
>> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:

>> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
>> > the arch_uprobe_analyze_insn() function.
>>
>> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
>> return -EINVAL when mm->task_size < TASK_SIZE_64.
>
> That's just a temporary workaround. If we ever merge ILP32, this test
> would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).

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

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

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

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> 
> >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> >> > the arch_uprobe_analyze_insn() function.
> >>
> >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> >
> > That's just a temporary workaround. If we ever merge ILP32, this test
> > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> 
> OK.. So what about doing something similar what x86 is doing.
> We can have a flag for task Type in arch specific mm_context_t. We
> also set this flag in COMPAT_SET_PERSONALITY() along with setting
> thread_info flag, and we clear them in SET_PERSONALITY().

This looks like a better approach.

> > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > This check is meaningless without knowing which instruction set we
> > target. A false positive here, however, is not that bad as we wouldn't
> > end up inserting the wrong breakpoint in the executable. But it looks to
> > me like the core uprobe code needs to pass some additional information
> > like the type of task or ELF format to the arch code to make a useful
> > choice of breakpoint type.
> 
> It seems that 'strtle r0, [r0], #160' would have the closest matching
> aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> seems a bad instruction. So, may be we can use still weak
> is_trap_insn().

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

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

-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

On 26/09/2016:12:01:59 PM, Catalin Marinas wrote:
> On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> > On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> > >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > 
> > >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > >> > the arch_uprobe_analyze_insn() function.
> > >>
> > >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> > >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> > >
> > > That's just a temporary workaround. If we ever merge ILP32, this test
> > > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> > 
> > OK.. So what about doing something similar what x86 is doing.
> > We can have a flag for task Type in arch specific mm_context_t. We
> > also set this flag in COMPAT_SET_PERSONALITY() along with setting
> > thread_info flag, and we clear them in SET_PERSONALITY().
> 
> This looks like a better approach.
> 
> > > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > > This check is meaningless without knowing which instruction set we
> > > target. A false positive here, however, is not that bad as we wouldn't
> > > end up inserting the wrong breakpoint in the executable. But it looks to
> > > me like the core uprobe code needs to pass some additional information
> > > like the type of task or ELF format to the arch code to make a useful
> > > choice of breakpoint type.
> > 
> > It seems that 'strtle r0, [r0], #160' would have the closest matching
> > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> > seems a bad instruction. So, may be we can use still weak
> > is_trap_insn().
> 
> Even if the is_trap_insn() check passes, we would reject the probe in
> arch_uprobe_analyze_insn() immediately after based on the mm type check,
> so not too bad.

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

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

Yes, for 32 bit task we would need mm_struct as arg in is_trap_insn() as well as
in is_swbp_insn(). We would also need to have arm64 specific set_swbp().

Thanks for all your input. It was helpful. I will send V2 soon.

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Mon, Sep 26, 2016 at 06:33:59PM +0530, Pratyush Anand wrote:
> On 26/09/2016:12:01:59 PM, Catalin Marinas wrote:
> > On Sun, Sep 25, 2016 at 10:32:28PM +0530, Pratyush Anand wrote:
> > > On Fri, Sep 23, 2016 at 6:35 PM, Catalin Marinas
> > > <catalin.marinas@arm.com> wrote:
> > > > On Fri, Sep 23, 2016 at 09:42:30AM +0530, Pratyush Anand wrote:
> > > >> On 22/09/2016:05:50:30 PM, Catalin Marinas wrote:
> > > >> > On Thu, Sep 22, 2016 at 08:53:28AM +0530, Pratyush Anand wrote:
> > > >> > > On 21/09/2016:06:04:04 PM, Catalin Marinas wrote:
> > > 
> > > >> > As a quick workaround you could check mm->task_size > TASK_SIZE_32 in
> > > >> > the arch_uprobe_analyze_insn() function.
> > > >>
> > > >> It would be doable. TASK_SIZE_32 is defined only for COMPAT. So, may be I can
> > > >> return -EINVAL when mm->task_size < TASK_SIZE_64.
> > > >
> > > > That's just a temporary workaround. If we ever merge ILP32, this test
> > > > would no longer be enough (as the ISA is AArch64 but with TASK_SIZE_32).
> > > 
> > > OK.. So what about doing something similar what x86 is doing.
> > > We can have a flag for task Type in arch specific mm_context_t. We
> > > also set this flag in COMPAT_SET_PERSONALITY() along with setting
> > > thread_info flag, and we clear them in SET_PERSONALITY().
> > 
> > This looks like a better approach.
> > 
> > > > Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
> > > > This check is meaningless without knowing which instruction set we
> > > > target. A false positive here, however, is not that bad as we wouldn't
> > > > end up inserting the wrong breakpoint in the executable. But it looks to
> > > > me like the core uprobe code needs to pass some additional information
> > > > like the type of task or ELF format to the arch code to make a useful
> > > > choice of breakpoint type.
> > > 
> > > It seems that 'strtle r0, [r0], #160' would have the closest matching
> > > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
> > > seems a bad instruction. So, may be we can use still weak
> > > is_trap_insn().
> > 
> > Even if the is_trap_insn() check passes, we would reject the probe in
> > arch_uprobe_analyze_insn() immediately after based on the mm type check,
> > so not too bad.
> 
> OK..I will have an always returning false from arm64 is_trap_insn() in v2.

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

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

-- 
Catalin

Re: [PATCH 5/5] arm64: Add uprobe support

[Pratyush Anand]

On Tuesday 27 September 2016 07:21 PM, Catalin Marinas wrote:
>>>>> Looking at prepare_uprobe(), we have a weak is_trap_insn() function.
>>>>> > > > > This check is meaningless without knowing which instruction set we
>>>>> > > > > target. A false positive here, however, is not that bad as we wouldn't
>>>>> > > > > end up inserting the wrong breakpoint in the executable. But it looks to
>>>>> > > > > me like the core uprobe code needs to pass some additional information
>>>>> > > > > like the type of task or ELF format to the arch code to make a useful
>>>>> > > > > choice of breakpoint type.
>>>> > > >
>>>> > > > It seems that 'strtle r0, [r0], #160' would have the closest matching
>>>> > > > aarch32 instruction wrt BRK64_OPCODE_UPROBES(0xd42000A0). But that too
>>>> > > > seems a bad instruction. So, may be we can use still weak
>>>> > > > is_trap_insn().
>>> > >
>>> > > Even if the is_trap_insn() check passes, we would reject the probe in
>>> > > arch_uprobe_analyze_insn() immediately after based on the mm type check,
>>> > > so not too bad.
>> >
>> > OK..I will have an always returning false from arm64 is_trap_insn() in v2.
> For the time being, I think the default is_trap_insn() check is still
> useful on arm64.

I have already sent V2 with arm64 is_trap_insn() :(

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

Yes, I agree that we will have to modify is_trap_insn() for supporting 
aarch32 task tracing.

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

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

~Pratyush

Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas]

On Tue, Sep 27, 2016 at 08:33:25PM +0530, Pratyush Anand wrote:
> On Tuesday 27 September 2016 07:21 PM, Catalin Marinas wrote:
> >There is also the is_trap_at_addr() function which uses is_trap_insn().
> >I haven't checked the call paths here, are there any implications if
> >is_trap_insn() always returns false?
> 
> I had looked into it and also tested that a tracepoint at an application
> having a same instruction as that of "uprobe break instruction" ie "BRK
> #0x5" is rejected. So, I think a false positive return from is_tarp_insn()
> is still OK.

Looking at handle_swbp(), if we hit a breakpoint for which we don't have
a valid uprobe, this function currently sends a SIGTRAP. But if
is_trap_insn() returns false always, is_trap_at_addr() would return 0 in
this case so the SIGTRAP is never issued.

-- 
Catalin

Re: [PATCH 0/5] ARM64: Uprobe support added

[Pratyush Anand]

Hi Will/Catalin,

Do you have any specific comment for this patch set?

~Pratyush

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

On 02/08/2016:11:00:04 AM, Pratyush Anand wrote:
> ARM64 kprobe support is lying in torvalds/linux.git:master now. Therefore
> sending my uprobe patches which were dependent on decode-insn code of kprobe
> patches.
> 
> Unit tests for following have been done so far and they have been found
> working.
>      1. Step-able instructions, like sub, ldr, add etc.
>      2. Simulation-able like ret, cbnz, cbz etc.
>      3. uretprobe
>      4. Reject-able instructions like sev, wfe etc.
>      5. trapped and abort xol path
>      6. probe at unaligned user address.
>      7. longjump test cases
>  
> Currently it does not support aarch32 instruction probing.
> 
> RFC patches were sent long back, and all review comments for RFCs have been
> incorporated. RFCs were here: https://lwn.net/Articles/648514/
> 
> Pratyush Anand (5):
>   arm64: kprobe: protect/rename few definitions to be reused by uprobe
>   arm64: kgdb_step_brk_fn: ignore other's exception
>   arm64: Handle TRAP_HWBRKPT for user mode as well
>   arm64: Handle TRAP_BRKPT for user mode as well
>   arm64: Add uprobe support
> 
>  arch/arm64/Kconfig                      |   3 +
>  arch/arm64/include/asm/debug-monitors.h |   3 +
>  arch/arm64/include/asm/probes.h         |  23 ++--
>  arch/arm64/include/asm/ptrace.h         |   8 ++
>  arch/arm64/include/asm/thread_info.h    |   5 +-
>  arch/arm64/include/asm/uprobes.h        |  37 ++++++
>  arch/arm64/kernel/debug-monitors.c      |  40 +++---
>  arch/arm64/kernel/entry.S               |   6 +-
>  arch/arm64/kernel/kgdb.c                |   3 +
>  arch/arm64/kernel/probes/Makefile       |   2 +
>  arch/arm64/kernel/probes/decode-insn.c  |  31 ++---
>  arch/arm64/kernel/probes/decode-insn.h  |   8 +-
>  arch/arm64/kernel/probes/kprobes.c      |  36 ++---
>  arch/arm64/kernel/probes/uprobes.c      | 227 ++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/signal.c              |   4 +-
>  arch/arm64/mm/flush.c                   |   6 +
>  16 files changed, 378 insertions(+), 64 deletions(-)
>  create mode 100644 arch/arm64/include/asm/uprobes.h
>  create mode 100644 arch/arm64/kernel/probes/uprobes.c
> 
> -- 
> 2.5.5

Re: [PATCH 0/5] ARM64: Uprobe support added

[Pratyush Anand]

Hi Will/Catalin,

So far there is no comment which would require any modification in
this patch set. However, a rebase would be required, because we have a
fixup "arm64: Improve kprobes test for atomic sequence" in the next
now. Also, since we have "arm64: kprobe: Always clear pstate.D in
breakpoint exception handler" in next, so I can remove __kprobes tag
from uprobe_breakpoint_handler() and uprobe_single_step_handler().

Do you think that these patches are good to be taken for 4.9? If yes,
then I will send a quick V2 with above modifications.

~Pratyush


On Wed, Aug 24, 2016 at 12:56 PM, Pratyush Anand <panand@redhat.com> wrote:
> Hi Will/Catalin,
>
> Do you have any specific comment for this patch set?
>
> ~Pratyush
>
> [1] https://lkml.org/lkml/2016/8/22/69
>
> On 02/08/2016:11:00:04 AM, Pratyush Anand wrote:
>> ARM64 kprobe support is lying in torvalds/linux.git:master now. Therefore
>> sending my uprobe patches which were dependent on decode-insn code of kprobe
>> patches.
>>
>> Unit tests for following have been done so far and they have been found
>> working.
>>      1. Step-able instructions, like sub, ldr, add etc.
>>      2. Simulation-able like ret, cbnz, cbz etc.
>>      3. uretprobe
>>      4. Reject-able instructions like sev, wfe etc.
>>      5. trapped and abort xol path
>>      6. probe at unaligned user address.
>>      7. longjump test cases
>>
>> Currently it does not support aarch32 instruction probing.
>>
>> RFC patches were sent long back, and all review comments for RFCs have been
>> incorporated. RFCs were here: https://lwn.net/Articles/648514/
>>
>> Pratyush Anand (5):
>>   arm64: kprobe: protect/rename few definitions to be reused by uprobe
>>   arm64: kgdb_step_brk_fn: ignore other's exception
>>   arm64: Handle TRAP_HWBRKPT for user mode as well
>>   arm64: Handle TRAP_BRKPT for user mode as well
>>   arm64: Add uprobe support
>>
>>  arch/arm64/Kconfig                      |   3 +
>>  arch/arm64/include/asm/debug-monitors.h |   3 +
>>  arch/arm64/include/asm/probes.h         |  23 ++--
>>  arch/arm64/include/asm/ptrace.h         |   8 ++
>>  arch/arm64/include/asm/thread_info.h    |   5 +-
>>  arch/arm64/include/asm/uprobes.h        |  37 ++++++
>>  arch/arm64/kernel/debug-monitors.c      |  40 +++---
>>  arch/arm64/kernel/entry.S               |   6 +-
>>  arch/arm64/kernel/kgdb.c                |   3 +
>>  arch/arm64/kernel/probes/Makefile       |   2 +
>>  arch/arm64/kernel/probes/decode-insn.c  |  31 ++---
>>  arch/arm64/kernel/probes/decode-insn.h  |   8 +-
>>  arch/arm64/kernel/probes/kprobes.c      |  36 ++---
>>  arch/arm64/kernel/probes/uprobes.c      | 227 ++++++++++++++++++++++++++++++++
>>  arch/arm64/kernel/signal.c              |   4 +-
>>  arch/arm64/mm/flush.c                   |   6 +
>>  16 files changed, 378 insertions(+), 64 deletions(-)
>>  create mode 100644 arch/arm64/include/asm/uprobes.h
>>  create mode 100644 arch/arm64/kernel/probes/uprobes.c
>>
>> --
>> 2.5.5