Fixes for kprobes test issues

Due to a long-standing issue with Thumb symbol lookup [1] the jprobes tests fail when built into a kernel compiled as Thumb mode. (They work fine for ARM mode kernels or for Thumb when built as a loadable module.)

Rather than have this problem terminate testing prematurely lets instead emit an error message and carry on with the main kprobes tests, delaying the final failure report until the end.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/063026.html

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

---
  arch/arm/kernel/kprobes-test.c |   10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm/kernel/kprobes-test.c +8 -0

index c2fd06b..6de5e94 100644
@@ -225,6 +225,7 @@ static int pre_handler_called;
  static int post_handler_called;
  static int jprobe_func_called;
  static int kretprobe_handler_called;
+static int tests_failed;
 
  #define FUNC_ARG1 0x12345678
  #define FUNC_ARG2 0xabcdef

@@ -461,6 +462,13 @@ static int run_api_tests(long (*func)(long, long))
 
  	pr_info("    jprobe\n");
  	ret = test_jprobe(func);
+#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
+	if (ret == -EINVAL) {
+		pr_err("FAIL: Known longtime bug with jprobe on Thumb kernels\n");
+		tests_failed = ret;
+		ret = 0;
+	}
+#endif
  	if (ret < 0)
  		return ret;
 

Quoted 51 lines · Jon Medhurst

Due to a long-standing issue with Thumb symbol lookup [1] the jprobes tests fail when built into a kernel compiled as Thumb mode. (They work fine for ARM mode kernels or for Thumb when built as a loadable module.)

Rather than have this problem terminate testing prematurely lets instead emit an error message and carry on with the main kprobes tests, delaying the final failure report until the end.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/063026.html

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

---
  arch/arm/kernel/kprobes-test.c |   10 ++++++++++
  1 file changed, 10 insertions(+)

arch/arm/kernel/kprobes-test.c +10 -0

index c2fd06b..6de5e94 100644
@@ -225,6 +225,7 @@ static int pre_handler_called;
  static int post_handler_called;
  static int jprobe_func_called;
  static int kretprobe_handler_called;
+static int tests_failed;
 
  #define FUNC_ARG1 0x12345678
  #define FUNC_ARG2 0xabcdef

@@ -461,6 +462,13 @@ static int run_api_tests(long (*func)(long, long))
 
  	pr_info("    jprobe\n");
  	ret = test_jprobe(func);
+#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
+	if (ret == -EINVAL) {
+		pr_err("FAIL: Known longtime bug with jprobe on Thumb kernels\n");
+		tests_failed = ret;
+		ret = 0;
+	}
+#endif
  	if (ret < 0)
  		return ret;
 

@@ -1671,6 +1679,8 @@ static int __init run_all_tests(void)
 
  out:
  	if (ret == 0)
+		ret = tests_failed;
+	if (ret == 0)
  		pr_info("Finished kprobe tests OK\n");
  	else
  		pr_err("kprobe tests failed\n");

Because this (correctly) was marked as having no interdependencies it got marked as "applied" in the patch-tracker. That is why I omitted it from my v7 patches.

Yes, I now see this in Russell's patch tracker (with a less verbose commit message) as patch 7978, and it's marked as "Applied to git-curr (uprobes branch)". Perhaps it got dropped along with the rest of the uprobes branch when the config problems were discovered, Russell?

Quoted 4 lines · Jon Medhurst (Tixy)

Yes, I now see this in Russell's patch tracker (with a less verbose commit message) as patch 7978, and it's marked as "Applied to git-curr (uprobes branch)". Perhaps it got dropped along with the rest of the uprobes branch when the config problems were discovered, Russell?

If it was applied to the uprobes branch, and it wasn't part of David's uprobes git pull request, then yes, it doesn't exist anymore.

Quoted 6 lines · Russell King - ARM Linux

Quoted 4 lines · Jon Medhurst (Tixy)

Yes, I now see this in Russell's patch tracker (with a less verbose commit message) as patch 7978, and it's marked as "Applied to git-curr (uprobes branch)". Perhaps it got dropped along with the rest of the uprobes branch when the config problems were discovered, Russell?

Quoted 3 lines

If it was applied to the uprobes branch, and it wasn't part of David's uprobes git pull request, then yes, it doesn't exist anymore.

OK. Just to be absolutely clear: this doesn't create any new problem, but we do then need it in Tixy's new patchset in order to fix this older bug.

Quoted 11 lines · David Long

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Jon Medhurst (Tixy)

Yes, I now see this in Russell's patch tracker (with a less verbose commit message) as patch 7978, and it's marked as "Applied to git-curr (uprobes branch)". Perhaps it got dropped along with the rest of the uprobes branch when the config problems were discovered, Russell?

If it was applied to the uprobes branch, and it wasn't part of David's uprobes git pull request, then yes, it doesn't exist anymore.

Quoted 3 lines

OK. Just to be absolutely clear: this doesn't create any new problem, but we do then need it in Tixy's new patchset in order to fix this older bug.

Okay, so I'll just wait for Tixy's patch set then?

Quoted 16 lines · Russell King - ARM Linux

Quoted 14 lines · David Long

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Jon Medhurst (Tixy)

Yes, I now see this in Russell's patch tracker (with a less verbose commit message) as patch 7978, and it's marked as "Applied to git-curr (uprobes branch)". Perhaps it got dropped along with the rest of the uprobes branch when the config problems were discovered, Russell?

If it was applied to the uprobes branch, and it wasn't part of David's uprobes git pull request, then yes, it doesn't exist anymore.

OK. Just to be absolutely clear: this doesn't create any new problem, but we do then need it in Tixy's new patchset in order to fix this older bug.

Quoted 2 lines

Okay, so I'll just wait for Tixy's patch set then?

You don't have to delay anything, just eventually (e.g.: v3.16) take Tixy's patch as a separate patch fixing this old bug. The uprobes stuff is fine just as it is. There are no interdependencies.

ARM data processing instructions which have a register specified shift are defined as UNPREDICTABLE if PC is used for any register, not just the shift value as the code was previous assuming. This issue manifests on A15 devices as either test case failures or undefined instructions aborts.

Reported-by David Long <[email protected]>

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

---
  arch/arm/kernel/kprobes-test-arm.c |   25 +++++++++++++------------
  arch/arm/kernel/probes-arm.c       |    6 +++---
  2 files changed, 16 insertions(+), 15 deletions(-)

arch/arm/kernel/kprobes-test-arm.c +0 -0

index 87839de..8a7428b 100644

Quoted 18 lines · Jon Medhurst

ARM data processing instructions which have a register specified shift are defined as UNPREDICTABLE if PC is used for any register, not just the shift value as the code was previous assuming. This issue manifests on A15 devices as either test case failures or undefined instructions aborts.

Reported-by David Long <[email protected]>

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

---
  arch/arm/kernel/kprobes-test-arm.c |   25 +++++++++++++------------
  arch/arm/kernel/probes-arm.c       |    6 +++---
  2 files changed, 16 insertions(+), 15 deletions(-)

arch/arm/kernel/kprobes-test-arm.c +0 -0

index 87839de..8a7428b 100644

arch/arm/kernel/kprobes-test-arm.c void kprobe_arm_test_cases(void) · line 81

Quoted 9 lines

arch/arm/kernel/kprobes-test-arm.c +0 -4

@@ -73,12 +73,9 @@ void kprobe_arm_test_cases(void)
  	TEST_RRR( op "lt" s "	r11, r",11,VAL1,", r",14,N(val),", asr r",7, 6,"")\
  	TEST_RR(  op "gt" s "	r12, r13"       ", r",14,val, ", ror r",14,7,"")\
  	TEST_RR(  op "le" s "	r14, r",0, val, ", r13"       ", lsl r",14,8,"")\
-	TEST_RR(  op s "	r12, pc"        ", r",14,val, ", ror r",14,7,"")\
-	TEST_RR(  op s "	r14, r",0, val, ", pc"        ", lsl r",14,8,"")\
  	TEST_R(   op "eq" s "	r0,  r",11,VAL1,", #0xf5")			\
  	TEST_R(   op "ne" s "	r11, r",0, VAL1,", #0xf5000000")		\
-	TEST_R(   op s "	r7,  r",8, VAL2,", #0x000af000")		\
-	TEST(     op s "	r4,  pc"        ", #0x00005a00")

The last two lines above confuse me. Can you explain why those needed to be removed? Is there somehow a shift involved with those instructions?

The rest looked OK to me. I'm omitting it for the sake of brevity.

Conditionally compile kprobes test cases for ARMv5 instructions to avoid compilation errors with ARMv4 targets like:

/tmp/cc7Tx8ST.s:16740: Error: selected processor does not support ARM mode `clz r0,r0'

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

---
  arch/arm/kernel/kprobes-test-arm.c |    8 ++++++++
  1 file changed, 8 insertions(+)

arch/arm/kernel/kprobes-test-arm.c +6 -0

index 8a7428b..f9ed7cf 100644
@@ -216,6 +216,7 @@ void kprobe_arm_test_cases(void)
  	TEST_BB_R("bx	r",7,2f,"")
  	TEST_BF_R("bxeq	r",14,2f,"")
 
+#if __LINUX_ARM_ARCH__ >= 5
  	TEST_R("clz	r0, r",0, 0x0,"")
  	TEST_R("clzeq	r7, r",14,0x1,"")
  	TEST_R("clz	lr, r",7, 0xffffffff,"")

@@ -337,6 +338,7 @@ void kprobe_arm_test_cases(void)
  	TEST_UNSUPPORTED(".word 0xe16f02e1 @ smultt pc, r1, r2")
  	TEST_UNSUPPORTED(".word 0xe16002ef @ smultt r0, pc, r2")
  	TEST_UNSUPPORTED(".word 0xe1600fe1 @ smultt r0, r1, pc")
+#endif
 
  	TEST_GROUP("Multiply and multiply-accumulate")
 

@@ -559,6 +561,7 @@ void kprobe_arm_test_cases(void)
  	TEST_UNSUPPORTED("ldrsht	r1, [r2], #48")
  #endif
 
+#if __LINUX_ARM_ARCH__ >= 5
  	TEST_RPR(  "strd	r",0, VAL1,", [r",1, 48,", -r",2,24,"]")
  	TEST_RPR(  "strccd	r",8, VAL2,", [r",13,0, ", r",12,48,"]")
  	TEST_RPR(  "strd	r",4, VAL1,", [r",2, 24,", r",3, 48,"]!")

@@ -595,6 +598,7 @@ void kprobe_arm_test_cases(void)
  	TEST_UNSUPPORTED(".word 0xe1efc3d0	@ ldrd r12, [pc, #48]!")
  	TEST_UNSUPPORTED(".word 0xe0c9f3d0	@ ldrd pc, [r9], #48")
  	TEST_UNSUPPORTED(".word 0xe0c9e3d0	@ ldrd lr, [r9], #48")
+#endif
 
  	TEST_GROUP("Miscellaneous")
 

@@ -1227,7 +1231,9 @@ void kprobe_arm_test_cases(void)
  	TEST_COPROCESSOR( "mrc"two"	0, 0, r0, cr0, cr0, 0")
 
  	COPROCESSOR_INSTRUCTIONS_ST_LD("","e")
+#if __LINUX_ARM_ARCH__ >= 5
  	COPROCESSOR_INSTRUCTIONS_MC_MR("","e")
+#endif
  	TEST_UNSUPPORTED("svc	0")
  	TEST_UNSUPPORTED("svc	0xffffff")
 

Quoted 6 lines · Jon Medhurst

Conditionally compile kprobes test cases for ARMv5 instructions to avoid compilation errors with ARMv4 targets like:

/tmp/cc7Tx8ST.s:16740: Error: selected processor does not support ARM mode `clz r0,r0'

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

This looks OK to me. Feel free to add my ack.

Quoted 4 lines · On Tuesday 25 March 2014 09:27:29 David Long wrote:

Quoted 3 lines · Jon Medhurst

Conditionally compile kprobes test cases for ARMv5 instructions to avoid compilation errors with ARMv4 targets like:

Quoted 5 lines

Quoted 3 lines

/tmp/cc7Tx8ST.s:16740: Error: selected processor does not support ARM mode `clz r0,r0'

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

This looks OK to me. Feel free to add my ack.

Ah, I had a similar patch in my 'randconfig-fixes' series.

I noticed three
other configurations that are broken with kprobes-test:

- ARMv3 (enabled by ARCH_RPC)

- ARMv7-M (enabled by ARCH_EFM32)

- CPU_ENDIAN_BE32 (enabled by building a big-endian kernel on ARMv5 or older)

Should we treat those the same way, or just disable Kprobes for this case if nobody cares?

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · On Tuesday 25 March 2014 09:27:29 David Long wrote:

Quoted 5 lines · Jon Medhurst

Conditionally compile kprobes test cases for ARMv5 instructions to avoid compilation errors with ARMv4 targets like:

/tmp/cc7Tx8ST.s:16740: Error: selected processor does not support ARM mode `clz r0,r0'

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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

This looks OK to me. Feel free to add my ack.

Ah, I had a similar patch in my 'randconfig-fixes' series.

Where's that?

Quoted 4 lines

I noticed three
other configurations that are broken with kprobes-test:

- ARMv3 (enabled by ARCH_RPC)

Didn't know we support < ARMv4!

Quoted 1 line

- ARMv7-M (enabled by ARCH_EFM32)

I guess that problem is because randconfig is enabling HAVE_KPROBES, even though it wouldn't normally be selected, because we have

config ARCH_ARM
select HAVE_KPROBES if !XIP_KERNEL

and ARCH_EFM32 is XIP_KERNEL.

I think this problem goes all the way back to the commit which added HAVE_KPROBES to all arches (3f550096dede4430f83b16457da83bf429155ac2) That replaced

   config KPROBES
	depends on ... (ARM && !XIP_KERNEL)

with the current select if !XIP_KERNEL, which made it possible to enable kprobes for XIP when before we couldn't. And presumably that was for a good reason like it doesn't work on XIP?

Not sure at the moment how best to fix that. (Making ARM_KPROBES_TEST depend on !XIP_KERNEL doesn't solve the underlying problem of KPROBES feature still being enabled for XIP kernels.)

Quoted 4 lines

- CPU_ENDIAN_BE32 (enabled by building a big-endian kernel on ARMv5 or older)

Should we treat those the same way, or just disable Kprobes for this case if nobody cares?

For CPU_ENDIAN_BE32 I have a feeling that the kprobes code wouldn't work, would have to think more about why I have that feeling. If it doesn't, we come back to the problem that arch code can't add dependencies to CONFIG_KPROBES as things stand.