ARM: kprobes: OPTPROBES and other improvements.

These have extra 'checker' functions associated with them so lets make sure those get covered by testing. As they may create uninitialised space on the stack we also update the test code to ensure such space is consistent between test runs. This is done by disabling interrupts in setup_test_context().

Signed-off-by Jon Medhurst <[email protected]>

---

Sorry for the extra noise, sent this new version to correct whitespace and spelling errors in previous one.

 arch/arm/probes/kprobes/test-arm.c   | 17 +++++++++++++++--
 arch/arm/probes/kprobes/test-core.c  |  9 +++++++++
 arch/arm/probes/kprobes/test-thumb.c | 12 ++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

arch/arm/probes/kprobes/test-arm.c +15 -2

index fdeb300..9b3b1b4 100644
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <asm/system_info.h>
 #include <asm/opcodes.h>
+#include <asm/probes.h>
 
 #include "test-core.h"
 

@@ -478,6 +479,7 @@ void kprobe_arm_test_cases(void)
 	TEST_RPR(  "strh	r",0, VAL1,", [r",1, 48,", -r",2, 24,"]")
 	TEST_RPR(  "streqh	r",14,VAL2,", [r",11,0, ", r",12, 48,"]")
 	TEST_UNSUPPORTED(  "streqh	r14, [r13, r12]")
+	TEST_UNSUPPORTED(  "streqh	r14, [r12, r13]")
 	TEST_RPR(  "strh	r",1, VAL1,", [r",2, 24,", r",3,  48,"]!")
 	TEST_RPR(  "strneh	r",12,VAL2,", [r",11,48,", -r",10,24,"]!")
 	TEST_RPR(  "strh	r",2, VAL1,", [r",3, 24,"], r",4, 48,"")

@@ -502,6 +504,9 @@ void kprobe_arm_test_cases(void)
 	TEST_RP(   "strplh	r",12,VAL2,", [r",11,24,", #-4]!")
 	TEST_RP(   "strh	r",2, VAL1,", [r",3, 24,"], #48")
 	TEST_RP(   "strh	r",10,VAL2,", [r",9, 64,"], #-48")
+	TEST_RP(   "strh	r",3, VAL1,", [r",13,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"]!")
+	TEST_UNSUPPORTED("strh r3, [r13, #-"__stringify(MAX_STACK_SIZE)"-8]!")
+	TEST_RP(   "strh	r",4, VAL1,", [r",14,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"-8]!")
 	TEST_UNSUPPORTED(__inst_arm(0xe1efc3b0) "	@ strh r12, [pc, #48]!")
 	TEST_UNSUPPORTED(__inst_arm(0xe0c9f3b0) "	@ strh pc, [r9], #48")
 

@@ -568,6 +573,7 @@ void kprobe_arm_test_cases(void)
 	TEST_RPR(  "strd	r",0, VAL1,", [r",1, 48,", -r",2,24,"]")
 	TEST_RPR(  "strccd	r",8, VAL2,", [r",11,0, ", r",12,48,"]")
 	TEST_UNSUPPORTED(  "strccd r8, [r13, r12]")
+	TEST_UNSUPPORTED(  "strccd r8, [r12, r13]")
 	TEST_RPR(  "strd	r",4, VAL1,", [r",2, 24,", r",3, 48,"]!")
 	TEST_RPR(  "strcsd	r",12,VAL2,", [r",11,48,", -r",10,24,"]!")
 	TEST_RPR(  "strd	r",2, VAL1,", [r",5, 24,"], r",4,48,"")

@@ -591,6 +597,9 @@ void kprobe_arm_test_cases(void)
 	TEST_RP(   "strvcd	r",12,VAL2,", [r",11,24,", #-16]!")
 	TEST_RP(   "strd	r",2, VAL1,", [r",4, 24,"], #48")
 	TEST_RP(   "strd	r",10,VAL2,", [r",9, 64,"], #-48")
+	TEST_RP(   "strd	r",6, VAL1,", [r",13,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"]!")
+	TEST_UNSUPPORTED("strd r6, [r13, #-"__stringify(MAX_STACK_SIZE)"-8]!")
+	TEST_RP(   "strd	r",4, VAL1,", [r",12,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"-8]!")
 	TEST_UNSUPPORTED(__inst_arm(0xe1efc3f0) "	@ strd r12, [pc, #48]!")
 
 	TEST_P(	   "ldrd	r0, [r",0, 24,", #-8]")

@@ -639,16 +648,20 @@ void kprobe_arm_test_cases(void)
 	TEST_RP( "str"byte"	r",12,VAL2,", [r",11,24,", #-4]!")		\
 	TEST_RP( "str"byte"	r",2, VAL1,", [r",3, 24,"], #48")		\
 	TEST_RP( "str"byte"	r",10,VAL2,", [r",9, 64,"], #-48")		\
+	TEST_RP( "str"byte"	r",3, VAL1,", [r",13,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"]!") \
+	TEST_UNSUPPORTED("str"byte" r3, [r13, #-"__stringify(MAX_STACK_SIZE)"-8]!")				\
+	TEST_RP( "str"byte"	r",4, VAL1,", [r",10,TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"-8]!") \
 	TEST_RPR("str"byte"	r",0, VAL1,", [r",1, 48,", -r",2, 24,"]")	\
 	TEST_RPR("str"byte"	r",14,VAL2,", [r",11,0, ", r",12, 48,"]")	\
-	TEST_UNSUPPORTED("str"byte" r14, [r13, r12]")	\
+	TEST_UNSUPPORTED("str"byte" r14, [r13, r12]")				\
+	TEST_UNSUPPORTED("str"byte" r14, [r12, r13]")				\
 	TEST_RPR("str"byte"	r",1, VAL1,", [r",2, 24,", r",3,  48,"]!")	\
 	TEST_RPR("str"byte"	r",12,VAL2,", [r",11,48,", -r",10,24,"]!")	\
 	TEST_RPR("str"byte"	r",2, VAL1,", [r",3, 24,"], r",4, 48,"")	\
 	TEST_RPR("str"byte"	r",10,VAL2,", [r",9, 48,"], -r",11,24,"")	\
 	TEST_RPR("str"byte"	r",0, VAL1,", [r",1, 24,", r",2,  32,", asl #1]")\
 	TEST_RPR("str"byte"	r",14,VAL2,", [r",11,0, ", r",12, 32,", lsr #2]")\
-	TEST_UNSUPPORTED("str"byte"	r14, [r13, r12, lsr #2]")\
+	TEST_UNSUPPORTED("str"byte"	r14, [r13, r12, lsr #2]")		\
 	TEST_RPR("str"byte"	r",1, VAL1,", [r",2, 24,", r",3,  32,", asr #3]!")\
 	TEST_RPR("str"byte"	r",12,VAL2,", [r",11,24,", r",10, 4,", ror #31]!")\
 	TEST_P(  "ldr"byte"	r0, [r",0,  24,", #-2]")			\

arch/arm/probes/kprobes/test-core.c +9 -0

index 7ab633d..7c5ddd5 100644
@@ -1196,6 +1196,13 @@ static void setup_test_context(struct pt_regs *regs)
 			regs->uregs[arg->reg] =
 				(unsigned long)current_stack + arg->val;
 			memory_needs_checking = true;
+			/*
+			 * Test memory at an address below SP is in danger of
+			 * being altered by an interrupt occurring and pushing
+			 * data onto the stack. Disable interrupts to stop this.
+			 */
+			if (arg->reg == 13)
+				regs->ARM_cpsr |= PSR_I_BIT;
 			break;
 		}
 		case ARG_TYPE_MEM: {

@@ -1272,6 +1279,8 @@ test_after_pre_handler(struct kprobe *p, struct pt_regs *regs)
 
 	/* Undo any changes done to SP by the test case */
 	regs->ARM_sp = (unsigned long)current_stack;
+	/* Enable interrupts in case setup_test_context disabled them */
+	regs->ARM_cpsr &= ~PSR_I_BIT;
 
 	container_of(p, struct test_probe, kprobe)->hit = test_instance;
 	return 0;

arch/arm/probes/kprobes/test-thumb.c +12 -0

index 6c6e9a9..e8cf193 100644
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <asm/opcodes.h>
+#include <asm/probes.h>
 
 #include "test-core.h"
 

@@ -416,6 +417,9 @@ void kprobe_thumb32_test_cases(void)
 	TEST_RR( "strd	r",14,VAL2,", r",12,VAL1,", [sp, #16]!")
 	TEST_RRP("strd	r",1, VAL1,", r",0, VAL2,", [r",7, 24,"], #16")
 	TEST_RR( "strd	r",7, VAL2,", r",8, VAL1,", [sp], #-16")
+	TEST_RRP("strd	r",6, VAL1,", r",7, VAL2,", [r",13, TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"]!")
+	TEST_UNSUPPORTED("strd r6, r7, [r13, #-"__stringify(MAX_STACK_SIZE)"-8]!")
+	TEST_RRP("strd	r",4, VAL1,", r",5, VAL2,", [r",14, TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"-8]!")
 	TEST_UNSUPPORTED(__inst_thumb32(0xe9efec04) "	@ strd	r14, r12, [pc, #16]!")
 	TEST_UNSUPPORTED(__inst_thumb32(0xe8efec04) "	@ strd	r14, r12, [pc], #16")
 

@@ -821,14 +825,22 @@ CONDITION_INSTRUCTIONS(22,
 	TEST_RP( "str"size"	r",14,VAL2,", [r",1, 256,  ", #-128]!")		\
 	TEST_RPR("str"size".w	r",0, VAL1,", [r",1, 0,", r",2, 4,"]")		\
 	TEST_RPR("str"size"	r",14,VAL2,", [r",10,0,", r",11,4,", lsl #1]")	\
+	TEST_UNSUPPORTED("str"size"	r0, [r13, r1]")				\
 	TEST_R(  "str"size".w	r",7, VAL1,", [sp, #24]")			\
 	TEST_RP( "str"size".w	r",0, VAL2,", [r",0,0, "]")			\
+	TEST_RP( "str"size"	r",6, VAL1,", [r",13, TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"]!") \
+	TEST_UNSUPPORTED("str"size"	r6, [r13, #-"__stringify(MAX_STACK_SIZE)"-8]!")			\
+	TEST_RP( "str"size"	r",4, VAL2,", [r",12, TEST_MEMORY_SIZE,", #-"__stringify(MAX_STACK_SIZE)"-8]!") \
 	TEST_UNSUPPORTED("str"size"t	r0, [r1, #4]")
 
 	SINGLE_STORE("b")
 	SINGLE_STORE("h")
 	SINGLE_STORE("")
 
+	TEST_UNSUPPORTED(__inst_thumb32(0xf801000d) "	@ strb	r0, [r1, r13]")
+	TEST_UNSUPPORTED(__inst_thumb32(0xf821000d) "	@ strh	r0, [r1, r13]")
+	TEST_UNSUPPORTED(__inst_thumb32(0xf841000d) "	@ str	r0, [r1, r13]")
+
 	TEST("str	sp, [sp]")
 	TEST_UNSUPPORTED(__inst_thumb32(0xf8cfe000) "	@ str	r14, [pc]")
 	TEST_UNSUPPORTED(__inst_thumb32(0xf8cef000) "	@ str	pc, [r14]")

Hi Tixy,

I experienced another FAIL during test:

[11567.220477] Miscellaneous instructions
[11567.265397] ---------------------------------------------------------
[11567.342626] mrs	r0, cpsr	@ e10f0000
[11568.612656] FAIL: registers differ
[11568.653414] FAIL: Test mrs	r0, cpsr
[11568.695210] FAIL: Scenario 5
[11568.729709] initial_regs:
[11568.761083] r0  21522152 | r1  21522052 | r2  21522352 | r3  21522252
[11568.838301] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11568.915526] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11568.992748] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f590
[11569.069969] cpsr 58050013
[11569.101336] expected_regs:
[11569.133750] r0  58050013 | r1  21522052 | r2  21522352 | r3  21522252
[11569.210975] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.288197] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.365417] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.442634] cpsr 58050013
[11569.474010] result_regs:
[11569.504337] r0  58050113 | r1  21522052 | r2  21522352 | r3  21522252     <--- see R0 in this line
[11569.581556] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.658776] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.736000] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.813222] cpsr 58050013
[11569.844593] mrspl	r7, cpsr	@ 510f7000
[11571.842652] mrs	r14, cpsr	@ e10fe000

The failure is raise when testing in "mrs r0, cpsr". The added bit is PSR_A_BIT, which should be ignored.

So looks like this is also a problem in your test framework. If you don't have enough time, you can give me some hints to deal with it.

Quoted 31 lines · Wang Nan

Hi Tixy,

I experienced another FAIL during test:

[11567.220477] Miscellaneous instructions
[11567.265397] ---------------------------------------------------------
[11567.342626] mrs	r0, cpsr	@ e10f0000
[11568.612656] FAIL: registers differ
[11568.653414] FAIL: Test mrs	r0, cpsr
[11568.695210] FAIL: Scenario 5
[11568.729709] initial_regs:
[11568.761083] r0  21522152 | r1  21522052 | r2  21522352 | r3  21522252
[11568.838301] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11568.915526] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11568.992748] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f590
[11569.069969] cpsr 58050013
[11569.101336] expected_regs:
[11569.133750] r0  58050013 | r1  21522052 | r2  21522352 | r3  21522252
[11569.210975] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.288197] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.365417] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.442634] cpsr 58050013
[11569.474010] result_regs:
[11569.504337] r0  58050113 | r1  21522052 | r2  21522352 | r3  21522252     <--- see R0 in this line
[11569.581556] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.658776] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.736000] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.813222] cpsr 58050013
[11569.844593] mrspl	r7, cpsr	@ 510f7000
[11571.842652] mrs	r14, cpsr	@ e10fe000

Quoted 5 lines

The failure is raise when testing in "mrs r0, cpsr". The added bit is PSR_A_BIT, which should be ignored.

So looks like this is also a problem in your test framework. If you don't have enough time, you can give me some hints to deal with it.

The problem is that the A bit can change randomly when interrupts or reschedules happen (I'm not sure it should, and it may be a bug). For the purposes of this test code, we can't disable interrupts and scheduling around all the iterations of a test case, because inserting and removing probes requires them to be enabled. So we need a method of ignoring bits in cpsr, how about the following patch...

----------------------------------------------------------------------------

From: Jon Medhurst <[email protected]>
Subject: [PATCH] ARM: kprobes: Fix unreliable MRS instruction tests

For the instruction 'mrs Rn, cpsr' the resulting value of Rn can vary due to external factors we can't control. So get the test code to mask out these indeterminate bits.

Signed-off-by Jon Medhurst <[email protected]>

---
 arch/arm/probes/kprobes/test-arm.c   |  6 +++---
 arch/arm/probes/kprobes/test-core.c  | 19 ++++++++++---------
 arch/arm/probes/kprobes/test-core.h  | 33 ++++++++++++++++++++++++++++-----
 arch/arm/probes/kprobes/test-thumb.c |  4 ++--
 4 files changed, 43 insertions(+), 19 deletions(-)

arch/arm/probes/kprobes/test-arm.c +3 -3

index 9b3b1b4..e72b07e 100644
@@ -204,9 +204,9 @@ void kprobe_arm_test_cases(void)
 #endif
 	TEST_GROUP("Miscellaneous instructions")
 
-	TEST("mrs	r0, cpsr")
-	TEST("mrspl	r7, cpsr")
-	TEST("mrs	r14, cpsr")
+	TEST_RMASKED("mrs	r",0,~PSR_IGNORE_BITS,", cpsr")
+	TEST_RMASKED("mrspl	r",7,~PSR_IGNORE_BITS,", cpsr")
+	TEST_RMASKED("mrs	r",14,~PSR_IGNORE_BITS,", cpsr")
 	TEST_UNSUPPORTED(__inst_arm(0xe10ff000) "	@ mrs r15, cpsr")
 	TEST_UNSUPPORTED("mrs	r0, spsr")
 	TEST_UNSUPPORTED("mrs	lr, spsr")

arch/arm/probes/kprobes/test-core.c +10 -9

index 7c5ddd5..e495127 100644
@@ -1056,15 +1056,6 @@ static int test_case_run_count;
 static bool test_case_is_thumb;
 static int test_instance;
 
-/*
- * We ignore the state of the imprecise abort disable flag (CPSR.A) because this
- * can change randomly as the kernel doesn't take care to preserve or initialise
- * this across context switches. Also, with Security Extentions, the flag may
- * not be under control of the kernel; for this reason we ignore the state of
- * the FIQ disable flag CPSR.F as well.
- */
-#define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
-
 static unsigned long test_check_cc(int cc, unsigned long cpsr)
 {
 	int ret = arm_check_condition(cc << 28, cpsr);

@@ -1271,11 +1262,21 @@ test_case_pre_handler(struct kprobe *p, struct pt_regs *regs)
 static int __kprobes
 test_after_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	struct test_arg *args;
+
 	if (container_of(p, struct test_probe, kprobe)->hit == test_instance)
 		return 0; /* Already run for this test instance */
 
 	result_regs = *regs;
+
+	/* Mask out results which are indeterminate */
 	result_regs.ARM_cpsr &= ~PSR_IGNORE_BITS;
+	for (args = current_args; args[0].type != ARG_TYPE_END; ++args)
+		if (args[0].type == ARG_TYPE_REG_MASKED) {
+			struct test_arg_regptr *arg =
+				(struct test_arg_regptr *)args;
+			result_regs.uregs[arg->reg] &= arg->val;
+		}
 
 	/* Undo any changes done to SP by the test case */
 	regs->ARM_sp = (unsigned long)current_stack;

arch/arm/probes/kprobes/test-core.h +28 -5

index 9991754..32f7aa7 100644
@@ -45,10 +45,11 @@ extern int kprobe_test_cc_position;
  *
  */
 
-#define	ARG_TYPE_END	0
-#define	ARG_TYPE_REG	1
-#define	ARG_TYPE_PTR	2
-#define	ARG_TYPE_MEM	3
+#define	ARG_TYPE_END		0
+#define	ARG_TYPE_REG		1
+#define	ARG_TYPE_PTR		2
+#define	ARG_TYPE_MEM		3
+#define	ARG_TYPE_REG_MASKED	4
 
 #define ARG_FLAG_UNSUPPORTED	0x01
 #define ARG_FLAG_SUPPORTED	0x02

@@ -61,7 +62,7 @@ struct test_arg {
 };
 
 struct test_arg_regptr {
-	u8	type;		/* ARG_TYPE_REG or ARG_TYPE_PTR */
+	u8	type;		/* ARG_TYPE_REG or ARG_TYPE_PTR or ARG_TYPE_REG_MASKED */
 	u8	reg;
 	u8	_padding[2];
 	u32	val;

@@ -138,6 +139,12 @@ struct test_arg_end {
 	".short	0					\n\t"	\
 	".word	"#val"					\n\t"
 
+#define	TEST_ARG_REG_MASKED(reg, val)				\
+	".byte	"__stringify(ARG_TYPE_REG_MASKED)"	\n\t"	\
+	".byte	"#reg"					\n\t"	\
+	".short	0					\n\t"	\
+	".word	"#val"					\n\t"
+
 #define	TEST_ARG_END(flags)					\
 	".byte	"__stringify(ARG_TYPE_END)"		\n\t"	\
 	".byte	"TEST_ISA flags"			\n\t"	\

@@ -395,6 +402,22 @@ struct test_arg_end {
 	"	"codex"			\n\t"					\
 	TESTCASE_END
 
+#define TEST_RMASKED(code1, reg, mask, code2)		\
+	TESTCASE_START(code1 #reg code2)		\
+	TEST_ARG_REG_MASKED(reg, mask)			\
+	TEST_ARG_END("")				\
+	TEST_INSTRUCTION(code1 #reg code2)		\
+	TESTCASE_END
+
+/*
+ * We ignore the state of the imprecise abort disable flag (CPSR.A) because this
+ * can change randomly as the kernel doesn't take care to preserve or initialise
+ * this across context switches. Also, with Security Extensions, the flag may
+ * not be under control of the kernel; for this reason we ignore the state of
+ * the FIQ disable flag CPSR.F as well.
+ */
+#define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
+
 
 /*
  * Macros for defining space directives spread over multiple lines.

arch/arm/probes/kprobes/test-thumb.c +0 -0

index e8cf193..b683b45 100644

Well, I'm just working on a patch which introducing a TEST_INTOFF and disable interrupts for "MRS" testcase. Do you think it is inappropriate?

Quoted 192 lines · Jon Medhurst (Tixy)

Quoted 31 lines · Wang Nan

Hi Tixy,

I experienced another FAIL during test:

[11567.220477] Miscellaneous instructions
[11567.265397] ---------------------------------------------------------
[11567.342626] mrs	r0, cpsr	@ e10f0000
[11568.612656] FAIL: registers differ
[11568.653414] FAIL: Test mrs	r0, cpsr
[11568.695210] FAIL: Scenario 5
[11568.729709] initial_regs:
[11568.761083] r0  21522152 | r1  21522052 | r2  21522352 | r3  21522252
[11568.838301] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11568.915526] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11568.992748] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f590
[11569.069969] cpsr 58050013
[11569.101336] expected_regs:
[11569.133750] r0  58050013 | r1  21522052 | r2  21522352 | r3  21522252
[11569.210975] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.288197] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.365417] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.442634] cpsr 58050013
[11569.474010] result_regs:
[11569.504337] r0  58050113 | r1  21522052 | r2  21522352 | r3  21522252     <--- see R0 in this line
[11569.581556] r4  21522552 | r5  21522452 | r6  21522752 | r7  21522652
[11569.658776] r8  21522952 | r9  21522852 | r10 21522b52 | r11 21522a52
[11569.736000] r12 21522d52 | sp  ed343cf0 | lr  21522f52 | pc  bf11f594
[11569.813222] cpsr 58050013
[11569.844593] mrspl	r7, cpsr	@ 510f7000
[11571.842652] mrs	r14, cpsr	@ e10fe000

Quoted 5 lines

The failure is raise when testing in "mrs r0, cpsr". The added bit is PSR_A_BIT, which should be ignored.

So looks like this is also a problem in your test framework. If you don't have enough time, you can give me some hints to deal with it.

The problem is that the A bit can change randomly when interrupts or reschedules happen (I'm not sure it should, and it may be a bug). For the purposes of this test code, we can't disable interrupts and scheduling around all the iterations of a test case, because inserting and removing probes requires them to be enabled. So we need a method of ignoring bits in cpsr, how about the following patch...

----------------------------------------------------------------------------

From: Jon Medhurst <[email protected]>
Subject: [PATCH] ARM: kprobes: Fix unreliable MRS instruction tests

For the instruction 'mrs Rn, cpsr' the resulting value of Rn can vary due to external factors we can't control. So get the test code to mask out these indeterminate bits.

Signed-off-by Jon Medhurst <[email protected]>

---
 arch/arm/probes/kprobes/test-arm.c   |  6 +++---
 arch/arm/probes/kprobes/test-core.c  | 19 ++++++++++---------
 arch/arm/probes/kprobes/test-core.h  | 33 ++++++++++++++++++++++++++++-----
 arch/arm/probes/kprobes/test-thumb.c |  4 ++--
 4 files changed, 43 insertions(+), 19 deletions(-)

arch/arm/probes/kprobes/test-arm.c +3 -3

index 9b3b1b4..e72b07e 100644
@@ -204,9 +204,9 @@ void kprobe_arm_test_cases(void)
 #endif
 	TEST_GROUP("Miscellaneous instructions")
 
-	TEST("mrs	r0, cpsr")
-	TEST("mrspl	r7, cpsr")
-	TEST("mrs	r14, cpsr")
+	TEST_RMASKED("mrs	r",0,~PSR_IGNORE_BITS,", cpsr")
+	TEST_RMASKED("mrspl	r",7,~PSR_IGNORE_BITS,", cpsr")
+	TEST_RMASKED("mrs	r",14,~PSR_IGNORE_BITS,", cpsr")
 	TEST_UNSUPPORTED(__inst_arm(0xe10ff000) "	@ mrs r15, cpsr")
 	TEST_UNSUPPORTED("mrs	r0, spsr")
 	TEST_UNSUPPORTED("mrs	lr, spsr")

arch/arm/probes/kprobes/test-core.c +10 -9

index 7c5ddd5..e495127 100644
@@ -1056,15 +1056,6 @@ static int test_case_run_count;
 static bool test_case_is_thumb;
 static int test_instance;
 
-/*
- * We ignore the state of the imprecise abort disable flag (CPSR.A) because this
- * can change randomly as the kernel doesn't take care to preserve or initialise
- * this across context switches. Also, with Security Extentions, the flag may
- * not be under control of the kernel; for this reason we ignore the state of
- * the FIQ disable flag CPSR.F as well.
- */
-#define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
-
 static unsigned long test_check_cc(int cc, unsigned long cpsr)
 {
 	int ret = arm_check_condition(cc << 28, cpsr);

@@ -1271,11 +1262,21 @@ test_case_pre_handler(struct kprobe *p, struct pt_regs *regs)
 static int __kprobes
 test_after_pre_handler(struct kprobe *p, struct pt_regs *regs)
 {
+	struct test_arg *args;
+
 	if (container_of(p, struct test_probe, kprobe)->hit == test_instance)
 		return 0; /* Already run for this test instance */
 
 	result_regs = *regs;
+
+	/* Mask out results which are indeterminate */
 	result_regs.ARM_cpsr &= ~PSR_IGNORE_BITS;
+	for (args = current_args; args[0].type != ARG_TYPE_END; ++args)
+		if (args[0].type == ARG_TYPE_REG_MASKED) {
+			struct test_arg_regptr *arg =
+				(struct test_arg_regptr *)args;
+			result_regs.uregs[arg->reg] &= arg->val;
+		}
 
 	/* Undo any changes done to SP by the test case */
 	regs->ARM_sp = (unsigned long)current_stack;

arch/arm/probes/kprobes/test-core.h +28 -5

index 9991754..32f7aa7 100644
@@ -45,10 +45,11 @@ extern int kprobe_test_cc_position;
  *
  */
 
-#define	ARG_TYPE_END	0
-#define	ARG_TYPE_REG	1
-#define	ARG_TYPE_PTR	2
-#define	ARG_TYPE_MEM	3
+#define	ARG_TYPE_END		0
+#define	ARG_TYPE_REG		1
+#define	ARG_TYPE_PTR		2
+#define	ARG_TYPE_MEM		3
+#define	ARG_TYPE_REG_MASKED	4
 
 #define ARG_FLAG_UNSUPPORTED	0x01
 #define ARG_FLAG_SUPPORTED	0x02

@@ -61,7 +62,7 @@ struct test_arg {
 };
 
 struct test_arg_regptr {
-	u8	type;		/* ARG_TYPE_REG or ARG_TYPE_PTR */
+	u8	type;		/* ARG_TYPE_REG or ARG_TYPE_PTR or ARG_TYPE_REG_MASKED */
 	u8	reg;
 	u8	_padding[2];
 	u32	val;

@@ -138,6 +139,12 @@ struct test_arg_end {
 	".short	0					\n\t"	\
 	".word	"#val"					\n\t"
 
+#define	TEST_ARG_REG_MASKED(reg, val)				\
+	".byte	"__stringify(ARG_TYPE_REG_MASKED)"	\n\t"	\
+	".byte	"#reg"					\n\t"	\
+	".short	0					\n\t"	\
+	".word	"#val"					\n\t"
+
 #define	TEST_ARG_END(flags)					\
 	".byte	"__stringify(ARG_TYPE_END)"		\n\t"	\
 	".byte	"TEST_ISA flags"			\n\t"	\

@@ -395,6 +402,22 @@ struct test_arg_end {
 	"	"codex"			\n\t"					\
 	TESTCASE_END
 
+#define TEST_RMASKED(code1, reg, mask, code2)		\
+	TESTCASE_START(code1 #reg code2)		\
+	TEST_ARG_REG_MASKED(reg, mask)			\
+	TEST_ARG_END("")				\
+	TEST_INSTRUCTION(code1 #reg code2)		\
+	TESTCASE_END
+
+/*
+ * We ignore the state of the imprecise abort disable flag (CPSR.A) because this
+ * can change randomly as the kernel doesn't take care to preserve or initialise
+ * this across context switches. Also, with Security Extensions, the flag may
+ * not be under control of the kernel; for this reason we ignore the state of
+ * the FIQ disable flag CPSR.F as well.
+ */
+#define PSR_IGNORE_BITS (PSR_A_BIT | PSR_F_BIT)
+
 
 /*
  * Macros for defining space directives spread over multiple lines.

arch/arm/probes/kprobes/test-thumb.c +0 -0

index e8cf193..b683b45 100644

arch/arm/probes/kprobes/test-thumb.c CONDITION_INSTRUCTIONS(22, · line 785

Quoted 10 lines

arch/arm/probes/kprobes/test-thumb.c +2 -2

@@ -778,8 +778,8 @@ CONDITION_INSTRUCTIONS(22,
 
 	TEST_UNSUPPORTED("subs	pc, lr, #4")
 
-	TEST("mrs	r0, cpsr")
-	TEST("mrs	r14, cpsr")
+	TEST_RMASKED("mrs	r",0,~PSR_IGNORE_BITS,", cpsr")
+	TEST_RMASKED("mrs	r",14,~PSR_IGNORE_BITS,", cpsr")
 	TEST_UNSUPPORTED(__inst_thumb32(0xf3ef8d00) "	@ mrs	sp, spsr")
 	TEST_UNSUPPORTED(__inst_thumb32(0xf3ef8f00) "	@ mrs	pc, spsr")
 	TEST_UNSUPPORTED("mrs	r0, spsr")

Well, I'm just working on a patch which introducing a TEST_INTOFF and disable interrupts for "MRS" testcase. Do you think it is inappropriate? I'll use your patch test for a night in my hardware, and will let you know the result tomorrow.

Thank you!

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

From: Masami Hiramatsu <[email protected]>

Pass the original kprobe for preparing an optimized kprobe arch-dep part, since for some architecture (e.g. ARM32) requires the information in original kprobe.

Signed-off-by Masami Hiramatsu <[email protected]>

Signed-off-by Wang Nan <[email protected]>

---
 arch/x86/kernel/kprobes/opt.c | 3 ++-
 include/linux/kprobes.h       | 3 ++-
 kernel/kprobes.c              | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

arch/x86/kernel/kprobes/opt.c +2 -1

index f1314d0..7028296 100644
@@ -320,7 +320,8 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
  * Target instructions MUST be relocatable (checked inside)
  * This is called when new aggr(opt)probe is allocated or reused.
  */
-int arch_prepare_optimized_kprobe(struct optimized_kprobe *op)
+int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
+				  struct kprobe *__unused)
 {
 	u8 *buf;
 	int ret;

include/linux/kprobes.h +2 -1

index f7296e5..7ab2c93 100644
@@ -308,7 +308,8 @@ struct optimized_kprobe {
 /* Architecture dependent functions for direct jump optimization */
 extern int arch_prepared_optinsn(struct arch_optimized_insn *optinsn);
 extern int arch_check_optimized_kprobe(struct optimized_kprobe *op);
-extern int arch_prepare_optimized_kprobe(struct optimized_kprobe *op);
+extern int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
+					 struct kprobe *orig);
 extern void arch_remove_optimized_kprobe(struct optimized_kprobe *op);
 extern void arch_optimize_kprobes(struct list_head *oplist);
 extern void arch_unoptimize_kprobes(struct list_head *oplist,

kernel/kprobes.c +2 -2

index 3995f54..9f28aa7 100644
@@ -717,7 +717,7 @@ static void prepare_optimized_kprobe(struct kprobe *p)
 	struct optimized_kprobe *op;
 
 	op = container_of(p, struct optimized_kprobe, kp);
-	arch_prepare_optimized_kprobe(op);
+	arch_prepare_optimized_kprobe(op, p);
 }
 
 /* Allocate new optimized_kprobe and try to prepare optimized instructions */

@@ -731,7 +731,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 
 	INIT_LIST_HEAD(&op->list);
 	op->kp.addr = p->addr;
-	arch_prepare_optimized_kprobe(op);
+	arch_prepare_optimized_kprobe(op, p);
 
 	return &op->kp;
 }

Hi all,

Looks like v15 patch solves the problems found in patch v13 and v14. I run Tixy's test cases in a loop for a whole night on two real unpublished Hisilicon ARM A15 SoCs (I backported kprobes to 3.10 kernel), no problem arise, they are still running now.

This patch introduce kprobeopt for ARM 32.

Limitations:
- Currently only kernel compiled with ARM ISA is supported.

- Offset between probe point and optinsn slot must not larger than 32MiB. Masami Hiramatsu suggests replacing 2 words, it will make things complex. Futher patch can make such optimization.

Kprobe opt on ARM is relatively simpler than kprobe opt on x86 because ARM instruction is always 4 bytes aligned and 4 bytes long. This patch replace probed instruction by a 'b', branch to trampoline code and then calls optimized_callback(). optimized_callback() calls opt_pre_handler() to execute kprobe handler. It also emulate/simulate replaced instruction.

When unregistering kprobe, the deferred manner of unoptimizer may leave branch instruction before optimizer is called. Different from x86_64, which only copy the probed insn after optprobe_template_end and reexecute them, this patch call singlestep to emulate/simulate the insn directly. Futher patch can optimize this behavior.

Signed-off-by Wang Nan <[email protected]>

Acked-by Masami Hiramatsu <[email protected]>

Cc Jon Medhurst (Tixy) <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Cc Will Deacon <[email protected]>

---
v1 -> v2:
- Improvement: if replaced instruction is conditional, generate a conditional branch instruction for it;
- Introduces RELATIVEJUMP_OPCODES due to ARM kprobe_opcode_t is 4 bytes;
- Removes size field in struct arch_optimized_insn;
- Use arm_gen_branch() to generate branch instruction;
- Remove all recover logic: ARM doesn't use tail buffer, no need recover replaced instructions like x86;
- Remove incorrect CONFIG_THUMB checking;
- can_optimize() always returns true if address is well aligned;
- Improve optimized_callback: using opt_pre_handler();
- Bugfix: correct range checking code and improve comments;
- Fix commit message.

v2 -> v3:
- Rename RELATIVEJUMP_OPCODES to MAX_COPIED_INSNS;
- Remove unneeded checking:

      arch_check_optimized_kprobe(), can_optimize();

- Add missing flush_icache_range() in arch_prepare_optimized_kprobe();
- Remove unneeded 'return;'.

v3 -> v4:
- Use __mem_to_opcode_arm() to translate copied_insn to ensure it works in big endian kernel;
- Replace 'nop' placeholder in trampoline code template with '.long 0' to avoid confusion: reader may regard 'nop' as an instruction, but it is value in fact.

v4 -> v5:
- Don't optimize stack store operations.
- Introduce prepared field to arch_optimized_insn to indicate whether it is prepared. Similar to size field with x86. See v1 -> v2.

v5 -> v6:
- Dynamically reserve stack according to instruction.
- Rename: kprobes-opt.c -> kprobes-opt-arm.c.
- Set op->optinsn.insn after all works are done.

v6 -> v7:
- Using checker to check stack consumption.

v7 -> v8:
- Small code adjustments.

v8 -> v9:
- Utilize original kprobe passed to arch_prepare_optimized_kprobe() to avoid copy ainsn twice.
- A bug in arch_prepare_optimized_kprobe() is found and fixed.

v9 -> v10:
- Commit message improvements.

v10 -> v11:
- Move to arch/arm/probes/, insn.h is moved to arch/arm/include/asm.
- Code cleanup.
- Bugfix based on Tixy's test result:
- Trampoline deal with ARM -> Thumb transision instructions and AEABI stack alignment requirement correctly.
- Trampoline code buffer should start at 4 byte aligned address. We enforces it in this series by using macro to wrap 'code' var.

v11 -> v12:
- Remove trampoline code stack trick and use r4 to save original stack.
- Remove trampoline code buffer alignment trick.
- Names of files are changed.

v12 -> v13:
- Assume stack always aligned by 4-bytes in any case.
- Comments update.

v13 -> v14:
- Use stop_machine to wrap arch_optimize_kprobes to avoid a racing.

v14 -> v15:
- In v14, stop_machine() wraps a coarse-grained piece of code which makes things complex than it should be. In this version, kprobes_remove_breakpoint() is extracted, core.c and opt-arm.c use it to replace breakpoint to valid instruction. stop_machine() used inside in kprobes_remove_breakpoint().

---
 arch/arm/Kconfig                        |   1 +
 arch/arm/{kernel => include/asm}/insn.h |   0
 arch/arm/include/asm/kprobes.h          |  29 +++
 arch/arm/kernel/Makefile                |   2 +-
 arch/arm/kernel/ftrace.c                |   3 +-
 arch/arm/kernel/jump_label.c            |   3 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/core.c          |  26 ++-
 arch/arm/probes/kprobes/core.h          |   2 +
 arch/arm/probes/kprobes/opt-arm.c       | 317 ++++++++++++++++++++++++++++++++
 10 files changed, 372 insertions(+), 12 deletions(-)
 rename arch/arm/{kernel => include/asm}/insn.h (100%)
 create mode 100644 arch/arm/probes/kprobes/opt-arm.c

arch/arm/Kconfig +1 -0

index 89c4b5c..2471240 100644
@@ -59,6 +59,7 @@ config ARM
 	select HAVE_MEMBLOCK
 	select HAVE_MOD_ARCH_SPECIFIC if ARM_UNWIND
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
+	select HAVE_OPTPROBES if !THUMB2_KERNEL
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP

arch/arm/include/asm/insn.h renamed from arch/arm/kernel/insn.h +0 -0

similarity index 100%
rename from arch/arm/kernel/insn.h
rename to arch/arm/include/asm/insn.h

arch/arm/include/asm/kprobes.h +29 -0

index 56f9ac6..50ff3bc 100644
@@ -50,5 +50,34 @@ int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 int kprobe_exceptions_notify(struct notifier_block *self,
 			     unsigned long val, void *data);
 
+/* optinsn template addresses */
+extern __visible kprobe_opcode_t optprobe_template_entry;
+extern __visible kprobe_opcode_t optprobe_template_val;
+extern __visible kprobe_opcode_t optprobe_template_call;
+extern __visible kprobe_opcode_t optprobe_template_end;
+extern __visible kprobe_opcode_t optprobe_template_sub_sp;
+extern __visible kprobe_opcode_t optprobe_template_add_sp;
+
+#define MAX_OPTIMIZED_LENGTH	4
+#define MAX_OPTINSN_SIZE				\
+	((unsigned long)&optprobe_template_end -	\
+	 (unsigned long)&optprobe_template_entry)
+#define RELATIVEJUMP_SIZE	4
+
+struct arch_optimized_insn {
+	/*
+	 * copy of the original instructions.
+	 * Different from x86, ARM kprobe_opcode_t is u32.
+	 */
+#define MAX_COPIED_INSN	DIV_ROUND_UP(RELATIVEJUMP_SIZE, sizeof(kprobe_opcode_t))
+	kprobe_opcode_t copied_insn[MAX_COPIED_INSN];
+	/* detour code buffer */
+	kprobe_opcode_t *insn;
+	/*
+	 * We always copy one instruction on ARM,
+	 * so size will always be 4, and unlike x86, there is no
+	 * need for a size field.
+	 */
+};
 
 #endif /* _ARM_KPROBES_H */

arch/arm/kernel/Makefile +1 -1

index 40d3e00..1d0f4e7 100644
@@ -52,7 +52,7 @@ obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
 # Main staffs in KPROBES are in arch/arm/probes/ .
-obj-$(CONFIG_KPROBES)		+= patch.o
+obj-$(CONFIG_KPROBES)		+= patch.o insn.o
 obj-$(CONFIG_OABI_COMPAT)	+= sys_oabi-compat.o
 obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o

arch/arm/kernel/ftrace.c +1 -2

index af9a8a9..ec7e332 100644
@@ -19,8 +19,7 @@
 #include <asm/cacheflush.h>
 #include <asm/opcodes.h>
 #include <asm/ftrace.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef CONFIG_THUMB2_KERNEL
 #define	NOP		0xf85deb04	/* pop.w {lr} */

arch/arm/kernel/jump_label.c +1 -2

index c6c73ed..35a8fbb 100644
@@ -1,8 +1,7 @@
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
 #include <asm/patch.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef HAVE_JUMP_LABEL
 

arch/arm/probes/kprobes/Makefile +1 -0

index bc8d504..76a36bf 100644
@@ -7,5 +7,6 @@ obj-$(CONFIG_KPROBES)		+= actions-thumb.o checkers-thumb.o
 test-kprobes-objs		+= test-thumb.o
 else
 obj-$(CONFIG_KPROBES)		+= actions-arm.o checkers-arm.o
+obj-$(CONFIG_OPTPROBES)		+= opt-arm.o
 test-kprobes-objs		+= test-arm.o
 endif

arch/arm/probes/kprobes/core.c +8 -6

index 3a58db4..a4ec240 100644
@@ -163,19 +163,31 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
  * memory. It is also needed to atomically set the two half-words of a 32-bit
  * Thumb breakpoint.
  */
-int __kprobes __arch_disarm_kprobe(void *p)
-{
-	struct kprobe *kp = p;
-	void *addr = (void *)((uintptr_t)kp->addr & ~1);
-
-	__patch_text(addr, kp->opcode);
+struct patch {
+	void *addr;
+	unsigned int insn;
+};
 
+static int __kprobes_remove_breakpoint(void *data)
+{
+	struct patch *p = data;
+	__patch_text(p->addr, p->insn);
 	return 0;

Quoted 256 lines · Wang Nan

This patch introduce kprobeopt for ARM 32.

Limitations:
- Currently only kernel compiled with ARM ISA is supported.

- Offset between probe point and optinsn slot must not larger than 32MiB. Masami Hiramatsu suggests replacing 2 words, it will make things complex. Futher patch can make such optimization.

Kprobe opt on ARM is relatively simpler than kprobe opt on x86 because ARM instruction is always 4 bytes aligned and 4 bytes long. This patch replace probed instruction by a 'b', branch to trampoline code and then calls optimized_callback(). optimized_callback() calls opt_pre_handler() to execute kprobe handler. It also emulate/simulate replaced instruction.

When unregistering kprobe, the deferred manner of unoptimizer may leave branch instruction before optimizer is called. Different from x86_64, which only copy the probed insn after optprobe_template_end and reexecute them, this patch call singlestep to emulate/simulate the insn directly. Futher patch can optimize this behavior.

Signed-off-by Wang Nan <[email protected]>

Acked-by Masami Hiramatsu <[email protected]>

Cc Jon Medhurst (Tixy) <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Cc Will Deacon <[email protected]>

---
v1 -> v2:
- Improvement: if replaced instruction is conditional, generate a conditional branch instruction for it;
- Introduces RELATIVEJUMP_OPCODES due to ARM kprobe_opcode_t is 4 bytes;
- Removes size field in struct arch_optimized_insn;
- Use arm_gen_branch() to generate branch instruction;
- Remove all recover logic: ARM doesn't use tail buffer, no need recover replaced instructions like x86;
- Remove incorrect CONFIG_THUMB checking;
- can_optimize() always returns true if address is well aligned;
- Improve optimized_callback: using opt_pre_handler();
- Bugfix: correct range checking code and improve comments;
- Fix commit message.

v2 -> v3:
- Rename RELATIVEJUMP_OPCODES to MAX_COPIED_INSNS;
- Remove unneeded checking:

      arch_check_optimized_kprobe(), can_optimize();

- Add missing flush_icache_range() in arch_prepare_optimized_kprobe();
- Remove unneeded 'return;'.

v3 -> v4:
- Use __mem_to_opcode_arm() to translate copied_insn to ensure it works in big endian kernel;
- Replace 'nop' placeholder in trampoline code template with '.long 0' to avoid confusion: reader may regard 'nop' as an instruction, but it is value in fact.

v4 -> v5:
- Don't optimize stack store operations.
- Introduce prepared field to arch_optimized_insn to indicate whether it is prepared. Similar to size field with x86. See v1 -> v2.

v5 -> v6:
- Dynamically reserve stack according to instruction.
- Rename: kprobes-opt.c -> kprobes-opt-arm.c.
- Set op->optinsn.insn after all works are done.

v6 -> v7:
- Using checker to check stack consumption.

v7 -> v8:
- Small code adjustments.

v8 -> v9:
- Utilize original kprobe passed to arch_prepare_optimized_kprobe() to avoid copy ainsn twice.
- A bug in arch_prepare_optimized_kprobe() is found and fixed.

v9 -> v10:
- Commit message improvements.

v10 -> v11:
- Move to arch/arm/probes/, insn.h is moved to arch/arm/include/asm.
- Code cleanup.
- Bugfix based on Tixy's test result:
- Trampoline deal with ARM -> Thumb transision instructions and AEABI stack alignment requirement correctly.
- Trampoline code buffer should start at 4 byte aligned address. We enforces it in this series by using macro to wrap 'code' var.

v11 -> v12:
- Remove trampoline code stack trick and use r4 to save original stack.
- Remove trampoline code buffer alignment trick.
- Names of files are changed.

v12 -> v13:
- Assume stack always aligned by 4-bytes in any case.
- Comments update.

v13 -> v14:
- Use stop_machine to wrap arch_optimize_kprobes to avoid a racing.

v14 -> v15:
- In v14, stop_machine() wraps a coarse-grained piece of code which makes things complex than it should be. In this version, kprobes_remove_breakpoint() is extracted, core.c and opt-arm.c use it to replace breakpoint to valid instruction. stop_machine() used inside in kprobes_remove_breakpoint().

---
 arch/arm/Kconfig                        |   1 +
 arch/arm/{kernel => include/asm}/insn.h |   0
 arch/arm/include/asm/kprobes.h          |  29 +++
 arch/arm/kernel/Makefile                |   2 +-
 arch/arm/kernel/ftrace.c                |   3 +-
 arch/arm/kernel/jump_label.c            |   3 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/core.c          |  26 ++-
 arch/arm/probes/kprobes/core.h          |   2 +
 arch/arm/probes/kprobes/opt-arm.c       | 317 ++++++++++++++++++++++++++++++++
 10 files changed, 372 insertions(+), 12 deletions(-)
 rename arch/arm/{kernel => include/asm}/insn.h (100%)
 create mode 100644 arch/arm/probes/kprobes/opt-arm.c

arch/arm/Kconfig +1 -0

index 89c4b5c..2471240 100644
@@ -59,6 +59,7 @@ config ARM
 	select HAVE_MEMBLOCK
 	select HAVE_MOD_ARCH_SPECIFIC if ARM_UNWIND
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
+	select HAVE_OPTPROBES if !THUMB2_KERNEL
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP

arch/arm/include/asm/insn.h renamed from arch/arm/kernel/insn.h +0 -0

similarity index 100%
rename from arch/arm/kernel/insn.h
rename to arch/arm/include/asm/insn.h

arch/arm/include/asm/kprobes.h +29 -0

index 56f9ac6..50ff3bc 100644
@@ -50,5 +50,34 @@ int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 int kprobe_exceptions_notify(struct notifier_block *self,
 			     unsigned long val, void *data);
 
+/* optinsn template addresses */
+extern __visible kprobe_opcode_t optprobe_template_entry;
+extern __visible kprobe_opcode_t optprobe_template_val;
+extern __visible kprobe_opcode_t optprobe_template_call;
+extern __visible kprobe_opcode_t optprobe_template_end;
+extern __visible kprobe_opcode_t optprobe_template_sub_sp;
+extern __visible kprobe_opcode_t optprobe_template_add_sp;
+
+#define MAX_OPTIMIZED_LENGTH	4
+#define MAX_OPTINSN_SIZE				\
+	((unsigned long)&optprobe_template_end -	\
+	 (unsigned long)&optprobe_template_entry)
+#define RELATIVEJUMP_SIZE	4
+
+struct arch_optimized_insn {
+	/*
+	 * copy of the original instructions.
+	 * Different from x86, ARM kprobe_opcode_t is u32.
+	 */
+#define MAX_COPIED_INSN	DIV_ROUND_UP(RELATIVEJUMP_SIZE, sizeof(kprobe_opcode_t))
+	kprobe_opcode_t copied_insn[MAX_COPIED_INSN];
+	/* detour code buffer */
+	kprobe_opcode_t *insn;
+	/*
+	 * We always copy one instruction on ARM,
+	 * so size will always be 4, and unlike x86, there is no
+	 * need for a size field.
+	 */
+};
 
 #endif /* _ARM_KPROBES_H */

arch/arm/kernel/Makefile +1 -1

index 40d3e00..1d0f4e7 100644
@@ -52,7 +52,7 @@ obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
 # Main staffs in KPROBES are in arch/arm/probes/ .
-obj-$(CONFIG_KPROBES)		+= patch.o
+obj-$(CONFIG_KPROBES)		+= patch.o insn.o
 obj-$(CONFIG_OABI_COMPAT)	+= sys_oabi-compat.o
 obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o

arch/arm/kernel/ftrace.c +1 -2

index af9a8a9..ec7e332 100644
@@ -19,8 +19,7 @@
 #include <asm/cacheflush.h>
 #include <asm/opcodes.h>
 #include <asm/ftrace.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef CONFIG_THUMB2_KERNEL
 #define	NOP		0xf85deb04	/* pop.w {lr} */

arch/arm/kernel/jump_label.c +1 -2

index c6c73ed..35a8fbb 100644
@@ -1,8 +1,7 @@
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
 #include <asm/patch.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef HAVE_JUMP_LABEL
 

arch/arm/probes/kprobes/Makefile +1 -0

index bc8d504..76a36bf 100644
@@ -7,5 +7,6 @@ obj-$(CONFIG_KPROBES)		+= actions-thumb.o checkers-thumb.o
 test-kprobes-objs		+= test-thumb.o
 else
 obj-$(CONFIG_KPROBES)		+= actions-arm.o checkers-arm.o
+obj-$(CONFIG_OPTPROBES)		+= opt-arm.o
 test-kprobes-objs		+= test-arm.o
 endif

arch/arm/probes/kprobes/core.c +8 -6

index 3a58db4..a4ec240 100644
@@ -163,19 +163,31 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
  * memory. It is also needed to atomically set the two half-words of a 32-bit
  * Thumb breakpoint.
  */
-int __kprobes __arch_disarm_kprobe(void *p)
-{
-	struct kprobe *kp = p;
-	void *addr = (void *)((uintptr_t)kp->addr & ~1);
-
-	__patch_text(addr, kp->opcode);
+struct patch {
+	void *addr;
+	unsigned int insn;
+};
 
+static int __kprobes_remove_breakpoint(void *data)
+{
+	struct patch *p = data;
+	__patch_text(p->addr, p->insn);
 	return 0;
 }
 

arch/arm/probes/kprobes/core.c void __kprobes arch_arm_kprobe(struct kprobe *p) · line 185

Quoted 8 lines

arch/arm/probes/kprobes/core.c +8 -0

+void __kprobes kprobes_remove_breakpoint(void *addr, unsigned int insn)
+{
+	struct patch p = {
+		.addr = addr,
+		.insn = insn,
+	};
+	stop_machine(__kprobes_remove_breakpoint, &p, cpu_online_mask);
+}

Hmm, I think finally we should fix patch_text() in patch.c to forcibly use stop_machine by adding "bool stop" parameter, instead of introducing new another patch_text() implementation, because we'd better avoid two private "patch" data structures.

Quoted 26 lines

Quoted 26 lines

arch/arm/probes/kprobes/core.c +8 -6

index 3a58db4..a4ec240 100644
@@ -163,19 +163,31 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
  * memory. It is also needed to atomically set the two half-words of a 32-bit
  * Thumb breakpoint.
  */
-int __kprobes __arch_disarm_kprobe(void *p)
-{
-	struct kprobe *kp = p;
-	void *addr = (void *)((uintptr_t)kp->addr & ~1);
-
-	__patch_text(addr, kp->opcode);
+struct patch {
+	void *addr;
+	unsigned int insn;
+};
 
+static int __kprobes_remove_breakpoint(void *data)
+{
+	struct patch *p = data;
+	__patch_text(p->addr, p->insn);
 	return 0;
 }
 

Quoted 12 lines

arch/arm/probes/kprobes/core.c void __kprobes arch_arm_kprobe(struct kprobe *p) · line 185

Quoted 8 lines

arch/arm/probes/kprobes/core.c +8 -0

+void __kprobes kprobes_remove_breakpoint(void *addr, unsigned int insn)
+{
+	struct patch p = {
+		.addr = addr,
+		.insn = insn,
+	};
+	stop_machine(__kprobes_remove_breakpoint, &p, cpu_online_mask);
+}

Hmm, I think finally we should fix patch_text() in patch.c to forcibly use stop_machine by adding "bool stop" parameter, instead of introducing new another patch_text() implementation, because we'd better avoid two private "patch" data structures.

That was my first thought too, then I realised that breaks encapsulation of the patch_text implementation, because its use of stop_machine is an implementation detail and it could be rewritten to not use stop machine. (That is sort of on my long term todo list
https://lkml.org/lkml/2014/9/4/188)

Whereas stop machine is used by kprobes to avoid race conditions with the undefined instruction exception handler and something like that would be needed even if patch_text didn't use stop_machine.

Quoted 254 lines · Wang Nan

This patch introduce kprobeopt for ARM 32.

Limitations:
- Currently only kernel compiled with ARM ISA is supported.

- Offset between probe point and optinsn slot must not larger than 32MiB. Masami Hiramatsu suggests replacing 2 words, it will make things complex. Futher patch can make such optimization.

Kprobe opt on ARM is relatively simpler than kprobe opt on x86 because ARM instruction is always 4 bytes aligned and 4 bytes long. This patch replace probed instruction by a 'b', branch to trampoline code and then calls optimized_callback(). optimized_callback() calls opt_pre_handler() to execute kprobe handler. It also emulate/simulate replaced instruction.

When unregistering kprobe, the deferred manner of unoptimizer may leave branch instruction before optimizer is called. Different from x86_64, which only copy the probed insn after optprobe_template_end and reexecute them, this patch call singlestep to emulate/simulate the insn directly. Futher patch can optimize this behavior.

Signed-off-by Wang Nan <[email protected]>

Acked-by Masami Hiramatsu <[email protected]>

Cc Jon Medhurst (Tixy) <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Cc Will Deacon <[email protected]>

---
v1 -> v2:
- Improvement: if replaced instruction is conditional, generate a conditional branch instruction for it;
- Introduces RELATIVEJUMP_OPCODES due to ARM kprobe_opcode_t is 4 bytes;
- Removes size field in struct arch_optimized_insn;
- Use arm_gen_branch() to generate branch instruction;
- Remove all recover logic: ARM doesn't use tail buffer, no need recover replaced instructions like x86;
- Remove incorrect CONFIG_THUMB checking;
- can_optimize() always returns true if address is well aligned;
- Improve optimized_callback: using opt_pre_handler();
- Bugfix: correct range checking code and improve comments;
- Fix commit message.

v2 -> v3:
- Rename RELATIVEJUMP_OPCODES to MAX_COPIED_INSNS;
- Remove unneeded checking:

      arch_check_optimized_kprobe(), can_optimize();

- Add missing flush_icache_range() in arch_prepare_optimized_kprobe();
- Remove unneeded 'return;'.

v3 -> v4:
- Use __mem_to_opcode_arm() to translate copied_insn to ensure it works in big endian kernel;
- Replace 'nop' placeholder in trampoline code template with '.long 0' to avoid confusion: reader may regard 'nop' as an instruction, but it is value in fact.

v4 -> v5:
- Don't optimize stack store operations.
- Introduce prepared field to arch_optimized_insn to indicate whether it is prepared. Similar to size field with x86. See v1 -> v2.

v5 -> v6:
- Dynamically reserve stack according to instruction.
- Rename: kprobes-opt.c -> kprobes-opt-arm.c.
- Set op->optinsn.insn after all works are done.

v6 -> v7:
- Using checker to check stack consumption.

v7 -> v8:
- Small code adjustments.

v8 -> v9:
- Utilize original kprobe passed to arch_prepare_optimized_kprobe() to avoid copy ainsn twice.
- A bug in arch_prepare_optimized_kprobe() is found and fixed.

v9 -> v10:
- Commit message improvements.

v10 -> v11:
- Move to arch/arm/probes/, insn.h is moved to arch/arm/include/asm.
- Code cleanup.
- Bugfix based on Tixy's test result:
- Trampoline deal with ARM -> Thumb transision instructions and AEABI stack alignment requirement correctly.
- Trampoline code buffer should start at 4 byte aligned address. We enforces it in this series by using macro to wrap 'code' var.

v11 -> v12:
- Remove trampoline code stack trick and use r4 to save original stack.
- Remove trampoline code buffer alignment trick.
- Names of files are changed.

v12 -> v13:
- Assume stack always aligned by 4-bytes in any case.
- Comments update.

v13 -> v14:
- Use stop_machine to wrap arch_optimize_kprobes to avoid a racing.

v14 -> v15:
- In v14, stop_machine() wraps a coarse-grained piece of code which makes things complex than it should be. In this version, kprobes_remove_breakpoint() is extracted, core.c and opt-arm.c use it to replace breakpoint to valid instruction. stop_machine() used inside in kprobes_remove_breakpoint().

---
 arch/arm/Kconfig                        |   1 +
 arch/arm/{kernel => include/asm}/insn.h |   0
 arch/arm/include/asm/kprobes.h          |  29 +++
 arch/arm/kernel/Makefile                |   2 +-
 arch/arm/kernel/ftrace.c                |   3 +-
 arch/arm/kernel/jump_label.c            |   3 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/core.c          |  26 ++-
 arch/arm/probes/kprobes/core.h          |   2 +
 arch/arm/probes/kprobes/opt-arm.c       | 317 ++++++++++++++++++++++++++++++++
 10 files changed, 372 insertions(+), 12 deletions(-)
 rename arch/arm/{kernel => include/asm}/insn.h (100%)
 create mode 100644 arch/arm/probes/kprobes/opt-arm.c

arch/arm/Kconfig +1 -0

index 89c4b5c..2471240 100644
@@ -59,6 +59,7 @@ config ARM
 	select HAVE_MEMBLOCK
 	select HAVE_MOD_ARCH_SPECIFIC if ARM_UNWIND
 	select HAVE_OPROFILE if (HAVE_PERF_EVENTS)
+	select HAVE_OPTPROBES if !THUMB2_KERNEL
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP

arch/arm/include/asm/insn.h renamed from arch/arm/kernel/insn.h +0 -0

similarity index 100%
rename from arch/arm/kernel/insn.h
rename to arch/arm/include/asm/insn.h

arch/arm/include/asm/kprobes.h +29 -0

index 56f9ac6..50ff3bc 100644
@@ -50,5 +50,34 @@ int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
 int kprobe_exceptions_notify(struct notifier_block *self,
 			     unsigned long val, void *data);
 
+/* optinsn template addresses */
+extern __visible kprobe_opcode_t optprobe_template_entry;
+extern __visible kprobe_opcode_t optprobe_template_val;
+extern __visible kprobe_opcode_t optprobe_template_call;
+extern __visible kprobe_opcode_t optprobe_template_end;
+extern __visible kprobe_opcode_t optprobe_template_sub_sp;
+extern __visible kprobe_opcode_t optprobe_template_add_sp;
+
+#define MAX_OPTIMIZED_LENGTH	4
+#define MAX_OPTINSN_SIZE				\
+	((unsigned long)&optprobe_template_end -	\
+	 (unsigned long)&optprobe_template_entry)
+#define RELATIVEJUMP_SIZE	4
+
+struct arch_optimized_insn {
+	/*
+	 * copy of the original instructions.
+	 * Different from x86, ARM kprobe_opcode_t is u32.
+	 */
+#define MAX_COPIED_INSN	DIV_ROUND_UP(RELATIVEJUMP_SIZE, sizeof(kprobe_opcode_t))
+	kprobe_opcode_t copied_insn[MAX_COPIED_INSN];
+	/* detour code buffer */
+	kprobe_opcode_t *insn;
+	/*
+	 * We always copy one instruction on ARM,
+	 * so size will always be 4, and unlike x86, there is no
+	 * need for a size field.
+	 */
+};
 
 #endif /* _ARM_KPROBES_H */

arch/arm/kernel/Makefile +1 -1

index 40d3e00..1d0f4e7 100644
@@ -52,7 +52,7 @@ obj-$(CONFIG_FUNCTION_GRAPH_TRACER)	+= ftrace.o insn.o
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o insn.o patch.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o
 # Main staffs in KPROBES are in arch/arm/probes/ .
-obj-$(CONFIG_KPROBES)		+= patch.o
+obj-$(CONFIG_KPROBES)		+= patch.o insn.o
 obj-$(CONFIG_OABI_COMPAT)	+= sys_oabi-compat.o
 obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o

arch/arm/kernel/ftrace.c +1 -2

index af9a8a9..ec7e332 100644
@@ -19,8 +19,7 @@
 #include <asm/cacheflush.h>
 #include <asm/opcodes.h>
 #include <asm/ftrace.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef CONFIG_THUMB2_KERNEL
 #define	NOP		0xf85deb04	/* pop.w {lr} */

arch/arm/kernel/jump_label.c +1 -2

index c6c73ed..35a8fbb 100644
@@ -1,8 +1,7 @@
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
 #include <asm/patch.h>
-
-#include "insn.h"
+#include <asm/insn.h>
 
 #ifdef HAVE_JUMP_LABEL
 

arch/arm/probes/kprobes/Makefile +1 -0

index bc8d504..76a36bf 100644
@@ -7,5 +7,6 @@ obj-$(CONFIG_KPROBES)		+= actions-thumb.o checkers-thumb.o
 test-kprobes-objs		+= test-thumb.o
 else
 obj-$(CONFIG_KPROBES)		+= actions-arm.o checkers-arm.o
+obj-$(CONFIG_OPTPROBES)		+= opt-arm.o
 test-kprobes-objs		+= test-arm.o
 endif

arch/arm/probes/kprobes/core.c +8 -6

index 3a58db4..a4ec240 100644
@@ -163,19 +163,31 @@ void __kprobes arch_arm_kprobe(struct kprobe *p)
  * memory. It is also needed to atomically set the two half-words of a 32-bit
  * Thumb breakpoint.
  */
-int __kprobes __arch_disarm_kprobe(void *p)
-{
-	struct kprobe *kp = p;
-	void *addr = (void *)((uintptr_t)kp->addr & ~1);
-
-	__patch_text(addr, kp->opcode);
+struct patch {
+	void *addr;
+	unsigned int insn;
+};
 
+static int __kprobes_remove_breakpoint(void *data)
+{
+	struct patch *p = data;
+	__patch_text(p->addr, p->insn);
 	return 0;

arch/arm/probes/kprobes/core.c void __kprobes arch_arm_kprobe(struct kprobe *p) · line 185

Quoted 10 lines

arch/arm/probes/kprobes/core.c +8 -0

 }
 
+void __kprobes kprobes_remove_breakpoint(void *addr, unsigned int insn)
+{
+	struct patch p = {
+		.addr = addr,
+		.insn = insn,
+	};
+	stop_machine(__kprobes_remove_breakpoint, &p, cpu_online_mask);
+}

Hmm, I think finally we should fix patch_text() in patch.c to forcibly use stop_machine by adding "bool stop" parameter, instead of introducing new another patch_text() implementation, because we'd better avoid two private "patch" data structures. Maybe someday we can find another better solution for self modifying.
(I actually doesn't like stop_machine() which disturbs real-time processing)

The other parts look good to me :)

Thank you!