LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements.
@ 2014-12-04  5:32 Wang Nan
  2014-12-04  5:34 ` [PATCH v12 1/7] ARM: probes: move all probe code to dedicate directory Wang Nan
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:32 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

This is v12 of optprobe related patch series. In this series, code in
optprobe is improvemented; the kprobe related files names are changed
to represent their functions.

Patch 1/7 moves all ARM probe related code to arch/arm/probes according
to suggestion from Russell King and Masami Hiramatsu.

Patch 2/7 - 5/7 introduce checker framework and use it to detect stack
consumption for each probed instructions.

Patch 6/7 and 7/7 are for real optprobe work.

Previous discussion can be found from:

https://lkml.org/lkml/2014/11/22/18
https://lkml.org/lkml/2014/11/21/55
https://lkml.org/lkml/2014/11/18/26
https://lkml.org/lkml/2014/11/19/31
https://lkml.org/lkml/2014/11/18/41
https://lkml.org/lkml/2014/10/25/48
https://lkml.org/lkml/2014/10/22/254
https://lkml.org/lkml/2014/8/27/255
https://lkml.org/lkml/2014/8/12/12
https://lkml.org/lkml/2014/8/8/992
https://lkml.org/lkml/2014/8/8/5
https://lkml.org/lkml/2014/8/5/63

Masami Hiramatsu (1):
  kprobes: Pass the original kprobe for preparing optimized kprobe

Wang Nan (6):
  ARM: probes: move all probe code to dedicate directory
  ARM: kprobes: introduces checker
  ARM: kprobes: collects stack consumption for store instructions
  ARM: kprobes: disallow probing stack consuming instructions
  ARM: kprobes: Add test cases for stack consuming instructions
  ARM: kprobes: enable OPTPROBES for ARM 32

 arch/arm/Kconfig                                   |   1 +
 arch/arm/Makefile                                  |   1 +
 arch/arm/{kernel => include/asm}/insn.h            |   0
 arch/arm/include/asm/kprobes.h                     |  35 ++-
 arch/arm/{kernel => include/asm}/patch.h           |   0
 arch/arm/include/asm/probes.h                      |  13 +
 arch/arm/kernel/Makefile                           |  16 +-
 arch/arm/kernel/entry-armv.S                       |   3 +-
 arch/arm/kernel/ftrace.c                           |   3 +-
 arch/arm/kernel/jump_label.c                       |   5 +-
 arch/arm/kernel/patch.c                            |   3 +-
 arch/arm/probes/Makefile                           |   7 +
 .../{kernel/probes-arm.c => probes/decode-arm.c}   |  12 +-
 .../{kernel/probes-arm.h => probes/decode-arm.h}   |   7 +-
 .../probes-thumb.c => probes/decode-thumb.c}       |  16 +-
 .../probes-thumb.h => probes/decode-thumb.h}       |  10 +-
 arch/arm/{kernel/probes.c => probes/decode.c}      |  74 ++++-
 arch/arm/{kernel/probes.h => probes/decode.h}      |  13 +-
 arch/arm/probes/kprobes/Makefile                   |  12 +
 .../kprobes-arm.c => probes/kprobes/actions-arm.c} |   9 +-
 .../kprobes/actions-common.c}                      |   4 +-
 .../kprobes/actions-thumb.c}                       |  10 +-
 arch/arm/probes/kprobes/checkers-arm.c             |  99 +++++++
 arch/arm/probes/kprobes/checkers-common.c          | 101 +++++++
 arch/arm/probes/kprobes/checkers-thumb.c           | 110 +++++++
 arch/arm/probes/kprobes/checkers.h                 |  54 ++++
 .../{kernel/kprobes.c => probes/kprobes/core.c}    |  24 +-
 .../{kernel/kprobes.h => probes/kprobes/core.h}    |  10 +-
 arch/arm/probes/kprobes/opt-arm.c                  | 322 +++++++++++++++++++++
 .../kprobes/test-arm.c}                            |  31 +-
 .../kprobes-test.c => probes/kprobes/test-core.c}  |   8 +-
 .../kprobes-test.h => probes/kprobes/test-core.h}  |   2 +-
 .../kprobes/test-thumb.c}                          |  16 +-
 arch/arm/probes/uprobes/Makefile                   |   2 +
 .../uprobes-arm.c => probes/uprobes/actions-arm.c} |   6 +-
 .../{kernel/uprobes.c => probes/uprobes/core.c}    |   8 +-
 .../{kernel/uprobes.h => probes/uprobes/core.h}    |   0
 arch/x86/kernel/kprobes/opt.c                      |   3 +-
 include/linux/kprobes.h                            |   3 +-
 kernel/kprobes.c                                   |   4 +-
 40 files changed, 966 insertions(+), 91 deletions(-)
 rename arch/arm/{kernel => include/asm}/insn.h (100%)
 rename arch/arm/{kernel => include/asm}/patch.h (100%)
 create mode 100644 arch/arm/probes/Makefile
 rename arch/arm/{kernel/probes-arm.c => probes/decode-arm.c} (99%)
 rename arch/arm/{kernel/probes-arm.h => probes/decode-arm.h} (93%)
 rename arch/arm/{kernel/probes-thumb.c => probes/decode-thumb.c} (98%)
 rename arch/arm/{kernel/probes-thumb.h => probes/decode-thumb.h} (90%)
 rename arch/arm/{kernel/probes.c => probes/decode.c} (85%)
 rename arch/arm/{kernel/probes.h => probes/decode.h} (97%)
 create mode 100644 arch/arm/probes/kprobes/Makefile
 rename arch/arm/{kernel/kprobes-arm.c => probes/kprobes/actions-arm.c} (98%)
 rename arch/arm/{kernel/kprobes-common.c => probes/kprobes/actions-common.c} (98%)
 rename arch/arm/{kernel/kprobes-thumb.c => probes/kprobes/actions-thumb.c} (98%)
 create mode 100644 arch/arm/probes/kprobes/checkers-arm.c
 create mode 100644 arch/arm/probes/kprobes/checkers-common.c
 create mode 100644 arch/arm/probes/kprobes/checkers-thumb.c
 create mode 100644 arch/arm/probes/kprobes/checkers.h
 rename arch/arm/{kernel/kprobes.c => probes/kprobes/core.c} (96%)
 rename arch/arm/{kernel/kprobes.h => probes/kprobes/core.h} (83%)
 create mode 100644 arch/arm/probes/kprobes/opt-arm.c
 rename arch/arm/{kernel/kprobes-test-arm.c => probes/kprobes/test-arm.c} (97%)
 rename arch/arm/{kernel/kprobes-test.c => probes/kprobes/test-core.c} (99%)
 rename arch/arm/{kernel/kprobes-test.h => probes/kprobes/test-core.h} (99%)
 rename arch/arm/{kernel/kprobes-test-thumb.c => probes/kprobes/test-thumb.c} (98%)
 create mode 100644 arch/arm/probes/uprobes/Makefile
 rename arch/arm/{kernel/uprobes-arm.c => probes/uprobes/actions-arm.c} (98%)
 rename arch/arm/{kernel/uprobes.c => probes/uprobes/core.c} (97%)
 rename arch/arm/{kernel/uprobes.h => probes/uprobes/core.h} (100%)

-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 1/7] ARM: probes: move all probe code to dedicate directory
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
@ 2014-12-04  5:34 ` Wang Nan
  2014-12-04  5:35 ` [PATCH v12 2/7] ARM: kprobes: introduces checker Wang Nan
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:34 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

In discussion on LKML (https://lkml.org/lkml/2014/11/28/158), Russell
King suggests to move all probe related code to arch/arm/probes. This
patch does the work. Due to dependency on 'arch/arm/kernel/patch.h', this
patch also moves patch.h to 'arch/arm/include/asm/patch.h', and related
'#include' directives are also midified to '#include <asm/patch.h>'.

Following is an overview of this patch:

 ./arch/arm/kernel/               ./arch/arm/probes/
 |-- Makefile                     |-- Makefile
 |-- probes-arm.c          ==>    |-- decode-arm.c
 |-- probes-arm.h          ==>    |-- decode-arm.h
 |-- probes-thumb.c        ==>    |-- decode-thumb.c
 |-- probes-thumb.h        ==>    |-- decode-thumb.h
 |-- probes.c              ==>    |-- decode.c
 |-- probes.h              ==>    |-- decode.h
 |                                |-- kprobes
 |                                |   |-- Makefile
 |-- kprobes-arm.c         ==>    |   |-- actions-arm.c
 |-- kprobes-common.c      ==>    |   |-- actions-common.c
 |-- kprobes-thumb.c       ==>    |   |-- actions-thumb.c
 |-- kprobes.c             ==>    |   |-- core.c
 |-- kprobes.h             ==>    |   |-- core.h
 |-- kprobes-test-arm.c    ==>    |   |-- test-arm.c
 |-- kprobes-test.c        ==>    |   |-- test-core.c
 |-- kprobes-test.h        ==>    |   |-- test-core.h
 |-- kprobes-test-thumb.c  ==>    |   `-- test-thumb.c
 |                                `-- uprobes
 |                                    |-- Makefile
 |-- uprobes-arm.c         ==>        |-- actions-arm.c
 |-- uprobes.c             ==>        |-- core.c
 |-- uprobes.h             ==>        `-- core.h
 |
 `-- patch.h               ==>    arch/arm/include/asm/patch.h

Signed-off-by: Wang Nan <wangnan0@huawei.com>

---

v1 -> v2:
 - Rename source files to describe their functions.
 - Add Makefiles in kprobes and uprobes directories.
---
 arch/arm/Makefile                                        |  1 +
 arch/arm/{kernel => include/asm}/patch.h                 |  0
 arch/arm/kernel/Makefile                                 | 16 ++--------------
 arch/arm/kernel/jump_label.c                             |  2 +-
 arch/arm/kernel/patch.c                                  |  3 +--
 arch/arm/probes/Makefile                                 |  7 +++++++
 arch/arm/{kernel/probes-arm.c => probes/decode-arm.c}    |  7 ++++---
 arch/arm/{kernel/probes-arm.h => probes/decode-arm.h}    |  4 +++-
 .../arm/{kernel/probes-thumb.c => probes/decode-thumb.c} |  6 +++---
 .../arm/{kernel/probes-thumb.h => probes/decode-thumb.h} |  4 +++-
 arch/arm/{kernel/probes.c => probes/decode.c}            |  4 ++--
 arch/arm/{kernel/probes.h => probes/decode.h}            |  2 +-
 arch/arm/probes/kprobes/Makefile                         | 11 +++++++++++
 .../kprobes-arm.c => probes/kprobes/actions-arm.c}       |  6 +++---
 .../kprobes-common.c => probes/kprobes/actions-common.c} |  4 ++--
 .../kprobes-thumb.c => probes/kprobes/actions-thumb.c}   |  6 +++---
 arch/arm/{kernel/kprobes.c => probes/kprobes/core.c}     |  8 ++++----
 arch/arm/{kernel/kprobes.h => probes/kprobes/core.h}     |  3 ++-
 .../kprobes-test-arm.c => probes/kprobes/test-arm.c}     |  2 +-
 .../kprobes-test.c => probes/kprobes/test-core.c}        |  8 ++++----
 .../kprobes-test.h => probes/kprobes/test-core.h}        |  2 +-
 .../kprobes-test-thumb.c => probes/kprobes/test-thumb.c} |  4 ++--
 arch/arm/probes/uprobes/Makefile                         |  2 ++
 .../uprobes-arm.c => probes/uprobes/actions-arm.c}       |  6 +++---
 arch/arm/{kernel/uprobes.c => probes/uprobes/core.c}     |  6 +++---
 arch/arm/{kernel/uprobes.h => probes/uprobes/core.h}     |  0
 26 files changed, 69 insertions(+), 55 deletions(-)
 rename arch/arm/{kernel => include/asm}/patch.h (100%)
 create mode 100644 arch/arm/probes/Makefile
 rename arch/arm/{kernel/probes-arm.c => probes/decode-arm.c} (99%)
 rename arch/arm/{kernel/probes-arm.h => probes/decode-arm.h} (97%)
 rename arch/arm/{kernel/probes-thumb.c => probes/decode-thumb.c} (99%)
 rename arch/arm/{kernel/probes-thumb.h => probes/decode-thumb.h} (97%)
 rename arch/arm/{kernel/probes.c => probes/decode.c} (99%)
 rename arch/arm/{kernel/probes.h => probes/decode.h} (99%)
 create mode 100644 arch/arm/probes/kprobes/Makefile
 rename arch/arm/{kernel/kprobes-arm.c => probes/kprobes/actions-arm.c} (99%)
 rename arch/arm/{kernel/kprobes-common.c => probes/kprobes/actions-common.c} (98%)
 rename arch/arm/{kernel/kprobes-thumb.c => probes/kprobes/actions-thumb.c} (99%)
 rename arch/arm/{kernel/kprobes.c => probes/kprobes/core.c} (99%)
 rename arch/arm/{kernel/kprobes.h => probes/kprobes/core.h} (96%)
 rename arch/arm/{kernel/kprobes-test-arm.c => probes/kprobes/test-arm.c} (99%)
 rename arch/arm/{kernel/kprobes-test.c => probes/kprobes/test-core.c} (99%)
 rename arch/arm/{kernel/kprobes-test.h => probes/kprobes/test-core.h} (99%)
 rename arch/arm/{kernel/kprobes-test-thumb.c => probes/kprobes/test-thumb.c} (99%)
 create mode 100644 arch/arm/probes/uprobes/Makefile
 rename arch/arm/{kernel/uprobes-arm.c => probes/uprobes/actions-arm.c} (98%)
 rename arch/arm/{kernel/uprobes.c => probes/uprobes/core.c} (98%)
 rename arch/arm/{kernel/uprobes.h => probes/uprobes/core.h} (100%)

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 034a949..a57d9bb 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -266,6 +266,7 @@ core-$(CONFIG_KVM_ARM_HOST) 	+= arch/arm/kvm/
 
 # If we have a machine-specific directory, then include it in the build.
 core-y				+= arch/arm/kernel/ arch/arm/mm/ arch/arm/common/
+core-y				+= arch/arm/probes/
 core-y				+= arch/arm/net/
 core-y				+= arch/arm/crypto/
 core-y				+= arch/arm/firmware/
diff --git a/arch/arm/kernel/patch.h b/arch/arm/include/asm/patch.h
similarity index 100%
rename from arch/arm/kernel/patch.h
rename to arch/arm/include/asm/patch.h
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 38ddd9f..40d3e00 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -51,20 +51,8 @@ obj-$(CONFIG_DYNAMIC_FTRACE)	+= ftrace.o insn.o
 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
-obj-$(CONFIG_UPROBES)		+= probes.o probes-arm.o uprobes.o uprobes-arm.o
-obj-$(CONFIG_KPROBES)		+= probes.o kprobes.o kprobes-common.o patch.o
-ifdef CONFIG_THUMB2_KERNEL
-obj-$(CONFIG_KPROBES)		+= kprobes-thumb.o probes-thumb.o
-else
-obj-$(CONFIG_KPROBES)		+= kprobes-arm.o probes-arm.o
-endif
-obj-$(CONFIG_ARM_KPROBES_TEST)	+= test-kprobes.o
-test-kprobes-objs		:= kprobes-test.o
-ifdef CONFIG_THUMB2_KERNEL
-test-kprobes-objs		+= kprobes-test-thumb.o
-else
-test-kprobes-objs		+= kprobes-test-arm.o
-endif
+# Main staffs in KPROBES are in arch/arm/probes/ .
+obj-$(CONFIG_KPROBES)		+= patch.o
 obj-$(CONFIG_OABI_COMPAT)	+= sys_oabi-compat.o
 obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index 4ce4f78..c6c73ed 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -1,8 +1,8 @@
 #include <linux/kernel.h>
 #include <linux/jump_label.h>
+#include <asm/patch.h>
 
 #include "insn.h"
-#include "patch.h"
 
 #ifdef HAVE_JUMP_LABEL
 
diff --git a/arch/arm/kernel/patch.c b/arch/arm/kernel/patch.c
index 07314af..08bcfa1 100644
--- a/arch/arm/kernel/patch.c
+++ b/arch/arm/kernel/patch.c
@@ -5,8 +5,7 @@
 #include <asm/cacheflush.h>
 #include <asm/smp_plat.h>
 #include <asm/opcodes.h>
-
-#include "patch.h"
+#include <asm/patch.h>
 
 struct patch {
 	void *addr;
diff --git a/arch/arm/probes/Makefile b/arch/arm/probes/Makefile
new file mode 100644
index 0000000..aa1f859
--- /dev/null
+++ b/arch/arm/probes/Makefile
@@ -0,0 +1,7 @@
+obj-$(CONFIG_UPROBES)		+= decode.o decode-arm.o uprobes/
+obj-$(CONFIG_KPROBES)		+= decode.o kprobes/
+ifdef CONFIG_THUMB2_KERNEL
+obj-$(CONFIG_KPROBES)		+= decode-thumb.o
+else
+obj-$(CONFIG_KPROBES)		+= decode-arm.o
+endif
diff --git a/arch/arm/kernel/probes-arm.c b/arch/arm/probes/decode-arm.c
similarity index 99%
rename from arch/arm/kernel/probes-arm.c
rename to arch/arm/probes/decode-arm.c
index 8eaef81..e39cc75 100644
--- a/arch/arm/kernel/probes-arm.c
+++ b/arch/arm/probes/decode-arm.c
@@ -1,5 +1,6 @@
 /*
- * arch/arm/kernel/probes-arm.c
+ *
+ * arch/arm/probes/decode-arm.c
  *
  * Some code moved here from arch/arm/kernel/kprobes-arm.c
  *
@@ -20,8 +21,8 @@
 #include <linux/stddef.h>
 #include <linux/ptrace.h>
 
-#include "probes.h"
-#include "probes-arm.h"
+#include "decode.h"
+#include "decode-arm.h"
 
 #define sign_extend(x, signbit) ((x) | (0 - ((x) & (1 << (signbit)))))
 
diff --git a/arch/arm/kernel/probes-arm.h b/arch/arm/probes/decode-arm.h
similarity index 97%
rename from arch/arm/kernel/probes-arm.h
rename to arch/arm/probes/decode-arm.h
index ace6572..9c56b40 100644
--- a/arch/arm/kernel/probes-arm.h
+++ b/arch/arm/probes/decode-arm.h
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/probes-arm.h
+ * arch/arm/probes/decode-arm.h
  *
  * Copyright 2013 Linaro Ltd.
  * Written by: David A. Long
@@ -15,6 +15,8 @@
 #ifndef _ARM_KERNEL_PROBES_ARM_H
 #define  _ARM_KERNEL_PROBES_ARM_H
 
+#include "decode.h"
+
 enum probes_arm_action {
 	PROBES_EMULATE_NONE,
 	PROBES_SIMULATE_NOP,
diff --git a/arch/arm/kernel/probes-thumb.c b/arch/arm/probes/decode-thumb.c
similarity index 99%
rename from arch/arm/kernel/probes-thumb.c
rename to arch/arm/probes/decode-thumb.c
index 4131351..2f0453a 100644
--- a/arch/arm/kernel/probes-thumb.c
+++ b/arch/arm/probes/decode-thumb.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/probes-thumb.c
+ * arch/arm/probes/decode-thumb.c
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
@@ -12,8 +12,8 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 
-#include "probes.h"
-#include "probes-thumb.h"
+#include "decode.h"
+#include "decode-thumb.h"
 
 
 static const union decode_item t32_table_1110_100x_x0xx[] = {
diff --git a/arch/arm/kernel/probes-thumb.h b/arch/arm/probes/decode-thumb.h
similarity index 97%
rename from arch/arm/kernel/probes-thumb.h
rename to arch/arm/probes/decode-thumb.h
index 7c6f6eb..039013c 100644
--- a/arch/arm/kernel/probes-thumb.h
+++ b/arch/arm/probes/decode-thumb.h
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/probes-thumb.h
+ * arch/arm/probes/decode-thumb.h
  *
  * Copyright 2013 Linaro Ltd.
  * Written by: David A. Long
@@ -15,6 +15,8 @@
 #ifndef _ARM_KERNEL_PROBES_THUMB_H
 #define  _ARM_KERNEL_PROBES_THUMB_H
 
+#include "decode.h"
+
 /*
  * True if current instruction is in an IT block.
  */
diff --git a/arch/arm/kernel/probes.c b/arch/arm/probes/decode.c
similarity index 99%
rename from arch/arm/kernel/probes.c
rename to arch/arm/probes/decode.c
index a8ab540..3b05d57 100644
--- a/arch/arm/kernel/probes.c
+++ b/arch/arm/probes/decode.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/probes.c
+ * arch/arm/probes/decode.c
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
@@ -17,7 +17,7 @@
 #include <asm/ptrace.h>
 #include <linux/bug.h>
 
-#include "probes.h"
+#include "decode.h"
 
 
 #ifndef find_str_pc_offset
diff --git a/arch/arm/kernel/probes.h b/arch/arm/probes/decode.h
similarity index 99%
rename from arch/arm/kernel/probes.h
rename to arch/arm/probes/decode.h
index dba9f24..1d0b531 100644
--- a/arch/arm/kernel/probes.h
+++ b/arch/arm/probes/decode.h
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/probes.h
+ * arch/arm/probes/decode.h
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
new file mode 100644
index 0000000..eb38a42
--- /dev/null
+++ b/arch/arm/probes/kprobes/Makefile
@@ -0,0 +1,11 @@
+obj-$(CONFIG_KPROBES)		+= core.o actions-common.o
+obj-$(CONFIG_ARM_KPROBES_TEST)	+= test-kprobes.o
+test-kprobes-objs		:= test-core.o
+
+ifdef CONFIG_THUMB2_KERNEL
+obj-$(CONFIG_KPROBES)		+= actions-thumb.o
+test-kprobes-objs		+= test-thumb.o
+else
+obj-$(CONFIG_KPROBES)		+= actions-arm.o
+test-kprobes-objs		+= test-arm.o
+endif
diff --git a/arch/arm/kernel/kprobes-arm.c b/arch/arm/probes/kprobes/actions-arm.c
similarity index 99%
rename from arch/arm/kernel/kprobes-arm.c
rename to arch/arm/probes/kprobes/actions-arm.c
index ac300c6..8797879 100644
--- a/arch/arm/kernel/kprobes-arm.c
+++ b/arch/arm/probes/kprobes/actions-arm.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/kprobes-decode.c
+ * arch/arm/probes/kprobes/actions-arm.c
  *
  * Copyright (C) 2006, 2007 Motorola Inc.
  *
@@ -62,8 +62,8 @@
 #include <linux/kprobes.h>
 #include <linux/ptrace.h>
 
-#include "kprobes.h"
-#include "probes-arm.h"
+#include "../decode-arm.h"
+#include "core.h"
 
 #if  __LINUX_ARM_ARCH__ >= 6
 #define BLX(reg)	"blx	"reg"		\n\t"
diff --git a/arch/arm/kernel/kprobes-common.c b/arch/arm/probes/kprobes/actions-common.c
similarity index 98%
rename from arch/arm/kernel/kprobes-common.c
rename to arch/arm/probes/kprobes/actions-common.c
index 0bf5d64..bd20a71 100644
--- a/arch/arm/kernel/kprobes-common.c
+++ b/arch/arm/probes/kprobes/actions-common.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/kprobes-common.c
+ * arch/arm/probes/kprobes/actions-common.c
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
@@ -15,7 +15,7 @@
 #include <linux/kprobes.h>
 #include <asm/opcodes.h>
 
-#include "kprobes.h"
+#include "core.h"
 
 
 static void __kprobes simulate_ldm1stm1(probes_opcode_t insn,
diff --git a/arch/arm/kernel/kprobes-thumb.c b/arch/arm/probes/kprobes/actions-thumb.c
similarity index 99%
rename from arch/arm/kernel/kprobes-thumb.c
rename to arch/arm/probes/kprobes/actions-thumb.c
index 9495d7f..6c4e60b 100644
--- a/arch/arm/kernel/kprobes-thumb.c
+++ b/arch/arm/probes/kprobes/actions-thumb.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/kprobes-thumb.c
+ * arch/arm/probes/kprobes/actions-thumb.c
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
@@ -13,8 +13,8 @@
 #include <linux/ptrace.h>
 #include <linux/kprobes.h>
 
-#include "kprobes.h"
-#include "probes-thumb.h"
+#include "../decode-thumb.h"
+#include "core.h"
 
 /* These emulation encodings are functionally equivalent... */
 #define t32_emulate_rd8rn16rm0ra12_noflags \
diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/probes/kprobes/core.c
similarity index 99%
rename from arch/arm/kernel/kprobes.c
rename to arch/arm/probes/kprobes/core.c
index 6d64420..701f49d 100644
--- a/arch/arm/kernel/kprobes.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -30,11 +30,11 @@
 #include <asm/cacheflush.h>
 #include <linux/percpu.h>
 #include <linux/bug.h>
+#include <asm/patch.h>
 
-#include "kprobes.h"
-#include "probes-arm.h"
-#include "probes-thumb.h"
-#include "patch.h"
+#include "../decode-arm.h"
+#include "../decode-thumb.h"
+#include "core.h"
 
 #define MIN_STACK_SIZE(addr) 				\
 	min((unsigned long)MAX_STACK_SIZE,		\
diff --git a/arch/arm/kernel/kprobes.h b/arch/arm/probes/kprobes/core.h
similarity index 96%
rename from arch/arm/kernel/kprobes.h
rename to arch/arm/probes/kprobes/core.h
index 9a2712e..2e1e5a3 100644
--- a/arch/arm/kernel/kprobes.h
+++ b/arch/arm/probes/kprobes/core.h
@@ -19,7 +19,8 @@
 #ifndef _ARM_KERNEL_KPROBES_H
 #define _ARM_KERNEL_KPROBES_H
 
-#include "probes.h"
+#include <asm/kprobes.h>
+#include "../decode.h"
 
 /*
  * These undefined instructions must be unique and
diff --git a/arch/arm/kernel/kprobes-test-arm.c b/arch/arm/probes/kprobes/test-arm.c
similarity index 99%
rename from arch/arm/kernel/kprobes-test-arm.c
rename to arch/arm/probes/kprobes/test-arm.c
index cb14242..d9a1255 100644
--- a/arch/arm/kernel/kprobes-test-arm.c
+++ b/arch/arm/probes/kprobes/test-arm.c
@@ -13,7 +13,7 @@
 #include <asm/system_info.h>
 #include <asm/opcodes.h>
 
-#include "kprobes-test.h"
+#include "test-core.h"
 
 
 #define TEST_ISA "32"
diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/probes/kprobes/test-core.c
similarity index 99%
rename from arch/arm/kernel/kprobes-test.c
rename to arch/arm/probes/kprobes/test-core.c
index b206d77..7ab633d 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -209,10 +209,10 @@
 #include <linux/bug.h>
 #include <asm/opcodes.h>
 
-#include "kprobes.h"
-#include "probes-arm.h"
-#include "probes-thumb.h"
-#include "kprobes-test.h"
+#include "core.h"
+#include "test-core.h"
+#include "../decode-arm.h"
+#include "../decode-thumb.h"
 
 
 #define BENCHMARKING	1
diff --git a/arch/arm/kernel/kprobes-test.h b/arch/arm/probes/kprobes/test-core.h
similarity index 99%
rename from arch/arm/kernel/kprobes-test.h
rename to arch/arm/probes/kprobes/test-core.h
index 4430990..9991754 100644
--- a/arch/arm/kernel/kprobes-test.h
+++ b/arch/arm/probes/kprobes/test-core.h
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/kprobes-test.h
+ * arch/arm/probes/kprobes/test-core.h
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
diff --git a/arch/arm/kernel/kprobes-test-thumb.c b/arch/arm/probes/kprobes/test-thumb.c
similarity index 99%
rename from arch/arm/kernel/kprobes-test-thumb.c
rename to arch/arm/probes/kprobes/test-thumb.c
index 844dd10..6c6e9a9 100644
--- a/arch/arm/kernel/kprobes-test-thumb.c
+++ b/arch/arm/probes/kprobes/test-thumb.c
@@ -1,5 +1,5 @@
 /*
- * arch/arm/kernel/kprobes-test-thumb.c
+ * arch/arm/probes/kprobes/test-thumb.c
  *
  * Copyright (C) 2011 Jon Medhurst <tixy@yxit.co.uk>.
  *
@@ -12,7 +12,7 @@
 #include <linux/module.h>
 #include <asm/opcodes.h>
 
-#include "kprobes-test.h"
+#include "test-core.h"
 
 
 #define TEST_ISA "16"
diff --git a/arch/arm/probes/uprobes/Makefile b/arch/arm/probes/uprobes/Makefile
new file mode 100644
index 0000000..6c4c735
--- /dev/null
+++ b/arch/arm/probes/uprobes/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_UPROBES)		+= core.o actions-arm.o
+
diff --git a/arch/arm/kernel/uprobes-arm.c b/arch/arm/probes/uprobes/actions-arm.c
similarity index 98%
rename from arch/arm/kernel/uprobes-arm.c
rename to arch/arm/probes/uprobes/actions-arm.c
index d3b655f..1dd4916 100644
--- a/arch/arm/kernel/uprobes-arm.c
+++ b/arch/arm/probes/uprobes/actions-arm.c
@@ -13,9 +13,9 @@
 #include <linux/uprobes.h>
 #include <linux/module.h>
 
-#include "probes.h"
-#include "probes-arm.h"
-#include "uprobes.h"
+#include "../decode.h"
+#include "../decode-arm.h"
+#include "core.h"
 
 static int uprobes_substitute_pc(unsigned long *pinsn, u32 oregs)
 {
diff --git a/arch/arm/kernel/uprobes.c b/arch/arm/probes/uprobes/core.c
similarity index 98%
rename from arch/arm/kernel/uprobes.c
rename to arch/arm/probes/uprobes/core.c
index 56adf9c..b2954f6 100644
--- a/arch/arm/kernel/uprobes.c
+++ b/arch/arm/probes/uprobes/core.c
@@ -17,9 +17,9 @@
 #include <asm/opcodes.h>
 #include <asm/traps.h>
 
-#include "probes.h"
-#include "probes-arm.h"
-#include "uprobes.h"
+#include "../decode.h"
+#include "../decode-arm.h"
+#include "core.h"
 
 #define UPROBE_TRAP_NR	UINT_MAX
 
diff --git a/arch/arm/kernel/uprobes.h b/arch/arm/probes/uprobes/core.h
similarity index 100%
rename from arch/arm/kernel/uprobes.h
rename to arch/arm/probes/uprobes/core.h
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 2/7] ARM: kprobes: introduces checker
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
  2014-12-04  5:34 ` [PATCH v12 1/7] ARM: probes: move all probe code to dedicate directory Wang Nan
@ 2014-12-04  5:35 ` Wang Nan
  2014-12-04  5:35 ` [PATCH v12 3/7] ARM: kprobes: collects stack consumption for store instructions Wang Nan
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:35 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

This patch introdces 'checker' to decoding phase, and calls checkers
when instruction decoding. This allows further decoding for specific
instructions.  This patch introduces a stub call of checkers in kprobe
arch_prepare_kprobe() as an example and for further expansion.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Jon Medhurst <tixy@linaro.org>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---
v1 -> v2:
 - kprobe checker stubs are introduced in this patch.

v2 -> v3:
 - Code cleanups following Masami Hiramatsu and Tixy's advises.
 - Commit message improvements.

v3 -> v4:
 - Move to arch/arm/probes.
---
 arch/arm/probes/decode-arm.c            |  5 +--
 arch/arm/probes/decode-arm.h            |  3 +-
 arch/arm/probes/decode-thumb.c          | 10 +++---
 arch/arm/probes/decode-thumb.h          |  6 ++--
 arch/arm/probes/decode.c                | 60 +++++++++++++++++++++++++++++----
 arch/arm/probes/decode.h                | 11 +++++-
 arch/arm/probes/kprobes/actions-arm.c   |  2 ++
 arch/arm/probes/kprobes/actions-thumb.c |  3 ++
 arch/arm/probes/kprobes/core.c          |  6 +++-
 arch/arm/probes/kprobes/core.h          |  7 ++--
 arch/arm/probes/uprobes/core.c          |  2 +-
 11 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/arch/arm/probes/decode-arm.c b/arch/arm/probes/decode-arm.c
index e39cc75..f46d8fc 100644
--- a/arch/arm/probes/decode-arm.c
+++ b/arch/arm/probes/decode-arm.c
@@ -726,10 +726,11 @@ static void __kprobes arm_singlestep(probes_opcode_t insn,
  */
 enum probes_insn __kprobes
 arm_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
-		       bool emulate, const union decode_action *actions)
+		       bool emulate, const union decode_action *actions,
+		       const struct decode_checker *checkers[])
 {
 	asi->insn_singlestep = arm_singlestep;
 	asi->insn_check_cc = probes_condition_checks[insn>>28];
 	return probes_decode_insn(insn, asi, probes_decode_arm_table, false,
-				  emulate, actions);
+				  emulate, actions, checkers);
 }
diff --git a/arch/arm/probes/decode-arm.h b/arch/arm/probes/decode-arm.h
index 9c56b40..a7b0398 100644
--- a/arch/arm/probes/decode-arm.h
+++ b/arch/arm/probes/decode-arm.h
@@ -70,6 +70,7 @@ extern const union decode_item probes_decode_arm_table[];
 
 enum probes_insn arm_probes_decode_insn(probes_opcode_t,
 		struct arch_probes_insn *, bool emulate,
-		const union decode_action *actions);
+		const union decode_action *actions,
+		const struct decode_checker *checkers[]);
 
 #endif
diff --git a/arch/arm/probes/decode-thumb.c b/arch/arm/probes/decode-thumb.c
index 2f0453a..985e7dd 100644
--- a/arch/arm/probes/decode-thumb.c
+++ b/arch/arm/probes/decode-thumb.c
@@ -863,20 +863,22 @@ static void __kprobes thumb32_singlestep(probes_opcode_t opcode,
 
 enum probes_insn __kprobes
 thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
-			   bool emulate, const union decode_action *actions)
+			   bool emulate, const union decode_action *actions,
+			   const struct decode_checker *checkers[])
 {
 	asi->insn_singlestep = thumb16_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
 	return probes_decode_insn(insn, asi, probes_decode_thumb16_table, true,
-				  emulate, actions);
+				  emulate, actions, checkers);
 }
 
 enum probes_insn __kprobes
 thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
-			   bool emulate, const union decode_action *actions)
+			   bool emulate, const union decode_action *actions,
+			   const struct decode_checker *checkers[])
 {
 	asi->insn_singlestep = thumb32_singlestep;
 	asi->insn_check_cc = thumb_check_cc;
 	return probes_decode_insn(insn, asi, probes_decode_thumb32_table, true,
-				  emulate, actions);
+				  emulate, actions, checkers);
 }
diff --git a/arch/arm/probes/decode-thumb.h b/arch/arm/probes/decode-thumb.h
index 039013c..8457add 100644
--- a/arch/arm/probes/decode-thumb.h
+++ b/arch/arm/probes/decode-thumb.h
@@ -91,9 +91,11 @@ extern const union decode_item probes_decode_thumb16_table[];
 
 enum probes_insn __kprobes
 thumb16_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
-		bool emulate, const union decode_action *actions);
+		bool emulate, const union decode_action *actions,
+		const struct decode_checker *checkers[]);
 enum probes_insn __kprobes
 thumb32_probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
-		bool emulate, const union decode_action *actions);
+		bool emulate, const union decode_action *actions,
+		const struct decode_checker *checkers[]);
 
 #endif
diff --git a/arch/arm/probes/decode.c b/arch/arm/probes/decode.c
index 3b05d57..c7d4420 100644
--- a/arch/arm/probes/decode.c
+++ b/arch/arm/probes/decode.c
@@ -342,6 +342,31 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 	[DECODE_TYPE_REJECT]	= sizeof(struct decode_reject)
 };
 
+static int run_checkers(const struct decode_checker *checkers[],
+		int action, probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	const struct decode_checker **p;
+
+	if (!checkers)
+		return INSN_GOOD;
+
+	p = checkers;
+	while (*p != NULL) {
+		int retval;
+		probes_check_t *checker_func = (*p)[action].checker;
+
+		retval = INSN_GOOD;
+		if (checker_func)
+			retval = checker_func(insn, asi, h);
+		if (retval == INSN_REJECTED)
+			return retval;
+		p++;
+	}
+	return INSN_GOOD;
+}
+
 /*
  * probes_decode_insn operates on data tables in order to decode an ARM
  * architecture instruction onto which a kprobe has been placed.
@@ -388,11 +413,17 @@ static const int decode_struct_sizes[NUM_DECODE_TYPES] = {
 int __kprobes
 probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
 		   const union decode_item *table, bool thumb,
-		   bool emulate, const union decode_action *actions)
+		   bool emulate, const union decode_action *actions,
+		   const struct decode_checker *checkers[])
 {
 	const struct decode_header *h = (struct decode_header *)table;
 	const struct decode_header *next;
 	bool matched = false;
+	/*
+	 * @insn can be modified by decode_regs. Save its original
+	 * value for checkers.
+	 */
+	probes_opcode_t origin_insn = insn;
 
 	if (emulate)
 		insn = prepare_emulated_insn(insn, asi, thumb);
@@ -422,24 +453,41 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
 		}
 
 		case DECODE_TYPE_CUSTOM: {
+			int err;
 			struct decode_custom *d = (struct decode_custom *)h;
-			return actions[d->decoder.action].decoder(insn, asi, h);
+			int action = d->decoder.action;
+
+			err = run_checkers(checkers, action, origin_insn, asi, h);
+			if (err == INSN_REJECTED)
+				return INSN_REJECTED;
+			return actions[action].decoder(insn, asi, h);
 		}
 
 		case DECODE_TYPE_SIMULATE: {
+			int err;
 			struct decode_simulate *d = (struct decode_simulate *)h;
-			asi->insn_handler = actions[d->handler.action].handler;
+			int action = d->handler.action;
+
+			err = run_checkers(checkers, action, origin_insn, asi, h);
+			if (err == INSN_REJECTED)
+				return INSN_REJECTED;
+			asi->insn_handler = actions[action].handler;
 			return INSN_GOOD_NO_SLOT;
 		}
 
 		case DECODE_TYPE_EMULATE: {
+			int err;
 			struct decode_emulate *d = (struct decode_emulate *)h;
+			int action = d->handler.action;
+
+			err = run_checkers(checkers, action, origin_insn, asi, h);
+			if (err == INSN_REJECTED)
+				return INSN_REJECTED;
 
 			if (!emulate)
-				return actions[d->handler.action].decoder(insn,
-					asi, h);
+				return actions[action].decoder(insn, asi, h);
 
-			asi->insn_handler = actions[d->handler.action].handler;
+			asi->insn_handler = actions[action].handler;
 			set_emulated_insn(insn, asi, thumb);
 			return INSN_GOOD;
 		}
diff --git a/arch/arm/probes/decode.h b/arch/arm/probes/decode.h
index 1d0b531..f9b08ba 100644
--- a/arch/arm/probes/decode.h
+++ b/arch/arm/probes/decode.h
@@ -314,6 +314,14 @@ union decode_action {
 	probes_custom_decode_t	*decoder;
 };
 
+typedef enum probes_insn (probes_check_t)(probes_opcode_t,
+					   struct arch_probes_insn *,
+					   const struct decode_header *);
+
+struct decode_checker {
+	probes_check_t	*checker;
+};
+
 #define DECODE_END			\
 	{.bits = DECODE_TYPE_END}
 
@@ -402,6 +410,7 @@ probes_insn_handler_t probes_emulate_none;
 int __kprobes
 probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
 		const union decode_item *table, bool thumb, bool emulate,
-		const union decode_action *actions);
+		const union decode_action *actions,
+		const struct decode_checker **checkers);
 
 #endif
diff --git a/arch/arm/probes/kprobes/actions-arm.c b/arch/arm/probes/kprobes/actions-arm.c
index 8797879..3c3afa4 100644
--- a/arch/arm/probes/kprobes/actions-arm.c
+++ b/arch/arm/probes/kprobes/actions-arm.c
@@ -341,3 +341,5 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
 	[PROBES_BRANCH] = {.handler = simulate_bbl},
 	[PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
 };
+
+const struct decode_checker *kprobes_arm_checkers[] = {NULL};
diff --git a/arch/arm/probes/kprobes/actions-thumb.c b/arch/arm/probes/kprobes/actions-thumb.c
index 6c4e60b..2796121 100644
--- a/arch/arm/probes/kprobes/actions-thumb.c
+++ b/arch/arm/probes/kprobes/actions-thumb.c
@@ -664,3 +664,6 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
 	[PROBES_T32_MUL_ADD_LONG] = {
 		.handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
 };
+
+const struct decode_checker *kprobes_t32_checkers[] = {NULL};
+const struct decode_checker *kprobes_t16_checkers[] = {NULL};
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 701f49d..74f3dc3 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -61,6 +61,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	kprobe_decode_insn_t *decode_insn;
 	const union decode_action *actions;
 	int is;
+	const struct decode_checker **checkers;
 
 	if (in_exception_text(addr))
 		return -EINVAL;
@@ -74,9 +75,11 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		insn = __opcode_thumb32_compose(insn, inst2);
 		decode_insn = thumb32_probes_decode_insn;
 		actions = kprobes_t32_actions;
+		checkers = kprobes_t32_checkers;
 	} else {
 		decode_insn = thumb16_probes_decode_insn;
 		actions = kprobes_t16_actions;
+		checkers = kprobes_t16_checkers;
 	}
 #else /* !CONFIG_THUMB2_KERNEL */
 	thumb = false;
@@ -85,12 +88,13 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	insn = __mem_to_opcode_arm(*p->addr);
 	decode_insn = arm_probes_decode_insn;
 	actions = kprobes_arm_actions;
+	checkers = kprobes_arm_checkers;
 #endif
 
 	p->opcode = insn;
 	p->ainsn.insn = tmp_insn;
 
-	switch ((*decode_insn)(insn, &p->ainsn, true, actions)) {
+	switch ((*decode_insn)(insn, &p->ainsn, true, actions, checkers)) {
 	case INSN_REJECTED:	/* not supported */
 		return -EINVAL;
 
diff --git a/arch/arm/probes/kprobes/core.h b/arch/arm/probes/kprobes/core.h
index 2e1e5a3..f88c79f 100644
--- a/arch/arm/probes/kprobes/core.h
+++ b/arch/arm/probes/kprobes/core.h
@@ -37,16 +37,19 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_probes_insn *asi,
 typedef enum probes_insn (kprobe_decode_insn_t)(probes_opcode_t,
 						struct arch_probes_insn *,
 						bool,
-						const union decode_action *);
+						const union decode_action *,
+						const struct decode_checker *[*]);
 
 #ifdef CONFIG_THUMB2_KERNEL
 
 extern const union decode_action kprobes_t32_actions[];
 extern const union decode_action kprobes_t16_actions[];
-
+extern const struct decode_checker *kprobes_t32_checkers[];
+extern const struct decode_checker *kprobes_t16_checkers[];
 #else /* !CONFIG_THUMB2_KERNEL */
 
 extern const union decode_action kprobes_arm_actions[];
+extern const struct decode_checker *kprobes_arm_checkers[];
 
 #endif
 
diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c
index b2954f6..d1329f1 100644
--- a/arch/arm/probes/uprobes/core.c
+++ b/arch/arm/probes/uprobes/core.c
@@ -88,7 +88,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	auprobe->ixol[1] = __opcode_to_mem_arm(UPROBE_SS_ARM_INSN);
 
 	ret = arm_probes_decode_insn(insn, &auprobe->asi, false,
-				     uprobes_probes_actions);
+				     uprobes_probes_actions, NULL);
 	switch (ret) {
 	case INSN_REJECTED:
 		return -EINVAL;
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 3/7] ARM: kprobes: collects stack consumption for store instructions
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
  2014-12-04  5:34 ` [PATCH v12 1/7] ARM: probes: move all probe code to dedicate directory Wang Nan
  2014-12-04  5:35 ` [PATCH v12 2/7] ARM: kprobes: introduces checker Wang Nan
@ 2014-12-04  5:35 ` Wang Nan
  2014-12-04  5:35 ` [PATCH v12 4/7] ARM: kprobes: disallow probing stack consuming instructions Wang Nan
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:35 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

This patch uses the previously introduced checker functionality on
store instructions to record their stack consumption information to
arch_probes_insn.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Jon Medhurst <tixy@linaro.org>
Reviewed-by: Jon Medhurst <tixy@linaro.org>

---
v1 -> v2:
 - Bugfix and code improvements following Tixy's suggestion. See:
   http://www.spinics.net/lists/arm-kernel/msg372912.html

v2 -> v3:
 - Totaly reconstructed following Tixy' instruction. See:
   https://lkml.org/lkml/2014/10/27/662 .
   Add his SOB.

v3 -> v4:
 - Commit message improvements.
 - Comments improvements and code cleanup.
 - A bug is found and fixed in decode table in arm_check_stack().

v4 -> v5:
 - Bugs in thumb2 decoding table is found by Tixy and fixed.

v5 -> v6:
 - Move to arch/arm/probes/ .
---
 arch/arm/include/asm/probes.h             |   1 +
 arch/arm/probes/decode.c                  |  10 +++
 arch/arm/probes/kprobes/Makefile          |   6 +-
 arch/arm/probes/kprobes/actions-arm.c     |   3 +-
 arch/arm/probes/kprobes/actions-thumb.c   |   5 +-
 arch/arm/probes/kprobes/checkers-arm.c    |  99 +++++++++++++++++++++++++++
 arch/arm/probes/kprobes/checkers-common.c | 101 +++++++++++++++++++++++++++
 arch/arm/probes/kprobes/checkers-thumb.c  | 110 ++++++++++++++++++++++++++++++
 arch/arm/probes/kprobes/checkers.h        |  54 +++++++++++++++
 9 files changed, 383 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/probes/kprobes/checkers-arm.c
 create mode 100644 arch/arm/probes/kprobes/checkers-common.c
 create mode 100644 arch/arm/probes/kprobes/checkers-thumb.c
 create mode 100644 arch/arm/probes/kprobes/checkers.h

diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
index 806cfe6..ccf9af3 100644
--- a/arch/arm/include/asm/probes.h
+++ b/arch/arm/include/asm/probes.h
@@ -38,6 +38,7 @@ struct arch_probes_insn {
 	probes_check_cc			*insn_check_cc;
 	probes_insn_singlestep_t	*insn_singlestep;
 	probes_insn_fn_t		*insn_fn;
+	int stack_space;
 };
 
 #endif
diff --git a/arch/arm/probes/decode.c b/arch/arm/probes/decode.c
index c7d4420..f9d7c42 100644
--- a/arch/arm/probes/decode.c
+++ b/arch/arm/probes/decode.c
@@ -425,6 +425,16 @@ probes_decode_insn(probes_opcode_t insn, struct arch_probes_insn *asi,
 	 */
 	probes_opcode_t origin_insn = insn;
 
+	/*
+	 * stack_space is initialized to 0 here. Checker functions
+	 * should update is value if they find this is a stack store
+	 * instruction: positive value means bytes of stack usage,
+	 * negitive value means unable to determine stack usage
+	 * statically. For instruction doesn't store to stack, checker
+	 * do nothing with it.
+	 */
+	asi->stack_space = 0;
+
 	if (emulate)
 		insn = prepare_emulated_insn(insn, asi, thumb);
 
diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
index eb38a42..bc8d504 100644
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -1,11 +1,11 @@
-obj-$(CONFIG_KPROBES)		+= core.o actions-common.o
+obj-$(CONFIG_KPROBES)		+= core.o actions-common.o checkers-common.o
 obj-$(CONFIG_ARM_KPROBES_TEST)	+= test-kprobes.o
 test-kprobes-objs		:= test-core.o
 
 ifdef CONFIG_THUMB2_KERNEL
-obj-$(CONFIG_KPROBES)		+= actions-thumb.o
+obj-$(CONFIG_KPROBES)		+= actions-thumb.o checkers-thumb.o
 test-kprobes-objs		+= test-thumb.o
 else
-obj-$(CONFIG_KPROBES)		+= actions-arm.o
+obj-$(CONFIG_KPROBES)		+= actions-arm.o checkers-arm.o
 test-kprobes-objs		+= test-arm.o
 endif
diff --git a/arch/arm/probes/kprobes/actions-arm.c b/arch/arm/probes/kprobes/actions-arm.c
index 3c3afa4..1e67839 100644
--- a/arch/arm/probes/kprobes/actions-arm.c
+++ b/arch/arm/probes/kprobes/actions-arm.c
@@ -64,6 +64,7 @@
 
 #include "../decode-arm.h"
 #include "core.h"
+#include "checkers.h"
 
 #if  __LINUX_ARM_ARCH__ >= 6
 #define BLX(reg)	"blx	"reg"		\n\t"
@@ -342,4 +343,4 @@ const union decode_action kprobes_arm_actions[NUM_PROBES_ARM_ACTIONS] = {
 	[PROBES_LDMSTM] = {.decoder = kprobe_decode_ldmstm}
 };
 
-const struct decode_checker *kprobes_arm_checkers[] = {NULL};
+const struct decode_checker *kprobes_arm_checkers[] = {arm_stack_checker, NULL};
diff --git a/arch/arm/probes/kprobes/actions-thumb.c b/arch/arm/probes/kprobes/actions-thumb.c
index 2796121..07cfd9b 100644
--- a/arch/arm/probes/kprobes/actions-thumb.c
+++ b/arch/arm/probes/kprobes/actions-thumb.c
@@ -15,6 +15,7 @@
 
 #include "../decode-thumb.h"
 #include "core.h"
+#include "checkers.h"
 
 /* These emulation encodings are functionally equivalent... */
 #define t32_emulate_rd8rn16rm0ra12_noflags \
@@ -665,5 +666,5 @@ const union decode_action kprobes_t32_actions[NUM_PROBES_T32_ACTIONS] = {
 		.handler = t32_emulate_rdlo12rdhi8rn16rm0_noflags},
 };
 
-const struct decode_checker *kprobes_t32_checkers[] = {NULL};
-const struct decode_checker *kprobes_t16_checkers[] = {NULL};
+const struct decode_checker *kprobes_t32_checkers[] = {t32_stack_checker, NULL};
+const struct decode_checker *kprobes_t16_checkers[] = {t16_stack_checker, NULL};
diff --git a/arch/arm/probes/kprobes/checkers-arm.c b/arch/arm/probes/kprobes/checkers-arm.c
new file mode 100644
index 0000000..f817663
--- /dev/null
+++ b/arch/arm/probes/kprobes/checkers-arm.c
@@ -0,0 +1,99 @@
+/*
+ * arch/arm/probes/kprobes/checkers-arm.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "../decode.h"
+#include "../decode-arm.h"
+#include "checkers.h"
+
+static enum probes_insn __kprobes arm_check_stack(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	/*
+	 * PROBES_LDRSTRD, PROBES_LDMSTM, PROBES_STORE,
+	 * PROBES_STORE_EXTRA may get here. Simply mark all normal
+	 * insns as STACK_USE_NONE.
+	 */
+	static const union decode_item table[] = {
+		/*
+		 * 'STR{,D,B,H}, Rt, [Rn, Rm]' should be marked as UNKNOWN
+		 * if Rn or Rm is SP.
+		 *                                 x
+		 * STR (register)	cccc 011x x0x0 xxxx xxxx xxxx xxxx xxxx
+		 * STRB (register)	cccc 011x x1x0 xxxx xxxx xxxx xxxx xxxx
+		 */
+		DECODE_OR	(0x0e10000f, 0x0600000d),
+		DECODE_OR	(0x0e1f0000, 0x060d0000),
+
+		/*
+		 *                                                     x
+		 * STRD (register)	cccc 000x x0x0 xxxx xxxx xxxx 1111 xxxx
+		 * STRH (register)	cccc 000x x0x0 xxxx xxxx xxxx 1011 xxxx
+		 */
+		DECODE_OR	(0x0e5000bf, 0x000000bd),
+		DECODE_CUSTOM	(0x0e5f00b0, 0x000d00b0, STACK_USE_UNKNOWN),
+
+		/*
+		 * For PROBES_LDMSTM, only stmdx sp, [...] need to examine
+		 *
+		 * Bit B/A (bit 24) encodes arithmetic operation order. 1 means
+		 * before, 0 means after.
+		 * Bit I/D (bit 23) encodes arithmetic operation. 1 means
+		 * increment, 0 means decrement.
+		 *
+		 * So:
+		 *                              B I
+		 *                              / /
+		 *                              A D   | Rn |
+		 * STMDX SP, [...]	cccc 100x 00x0 xxxx xxxx xxxx xxxx xxxx
+		 */
+		DECODE_CUSTOM	(0x0edf0000, 0x080d0000, STACK_USE_STMDX),
+
+		/*                              P U W | Rn | Rt |     imm12    |*/
+		/* STR (immediate)	cccc 010x x0x0 1101 xxxx xxxx xxxx xxxx */
+		/* STRB (immediate)	cccc 010x x1x0 1101 xxxx xxxx xxxx xxxx */
+		/*                              P U W | Rn | Rt |imm4|    |imm4|*/
+		/* STRD (immediate)	cccc 000x x1x0 1101 xxxx xxxx 1111 xxxx */
+		/* STRH (immediate)	cccc 000x x1x0 1101 xxxx xxxx 1011 xxxx */
+		/*
+		 * index = (P == '1'); add = (U == '1').
+		 * Above insns with:
+		 *    index == 0 (str{,d,h} rx, [sp], #+/-imm) or
+		 *    add == 1 (str{,d,h} rx, [sp, #+<imm>])
+		 * should be STACK_USE_NONE.
+		 * Only str{,b,d,h} rx,[sp,#-n] (P == 1 and U == 0) are
+		 * required to be examined.
+		 */
+		/* STR{,B} Rt,[SP,#-n]	cccc 0101 0xx0 1101 xxxx xxxx xxxx xxxx */
+		DECODE_CUSTOM	(0x0f9f0000, 0x050d0000, STACK_USE_FIXED_XXX),
+
+		/* STR{D,H} Rt,[SP,#-n]	cccc 0001 01x0 1101 xxxx xxxx 1x11 xxxx */
+		DECODE_CUSTOM	(0x0fdf00b0, 0x014d00b0, STACK_USE_FIXED_X0X),
+
+		/* fall through */
+		DECODE_CUSTOM	(0, 0, STACK_USE_NONE),
+		DECODE_END
+	};
+
+	return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
+}
+
+const struct decode_checker arm_stack_checker[NUM_PROBES_ARM_ACTIONS] = {
+	[PROBES_LDRSTRD] = {.checker = arm_check_stack},
+	[PROBES_STORE_EXTRA] = {.checker = arm_check_stack},
+	[PROBES_STORE] = {.checker = arm_check_stack},
+	[PROBES_LDMSTM] = {.checker = arm_check_stack},
+};
diff --git a/arch/arm/probes/kprobes/checkers-common.c b/arch/arm/probes/kprobes/checkers-common.c
new file mode 100644
index 0000000..971119c
--- /dev/null
+++ b/arch/arm/probes/kprobes/checkers-common.c
@@ -0,0 +1,101 @@
+/*
+ * arch/arm/probes/kprobes/checkers-common.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "../decode.h"
+#include "../decode-arm.h"
+#include "checkers.h"
+
+enum probes_insn checker_stack_use_none(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	asi->stack_space = 0;
+	return INSN_GOOD_NO_SLOT;
+}
+
+enum probes_insn checker_stack_use_unknown(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	asi->stack_space = -1;
+	return INSN_GOOD_NO_SLOT;
+}
+
+#ifdef CONFIG_THUMB2_KERNEL
+enum probes_insn checker_stack_use_imm_0xx(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	int imm = insn & 0xff;
+	asi->stack_space = imm;
+	return INSN_GOOD_NO_SLOT;
+}
+
+/*
+ * Different from other insn uses imm8, the real addressing offset of
+ * STRD in T32 encoding should be imm8 * 4. See ARMARM description.
+ */
+enum probes_insn checker_stack_use_t32strd(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	int imm = insn & 0xff;
+	asi->stack_space = imm << 2;
+	return INSN_GOOD_NO_SLOT;
+}
+#else
+enum probes_insn checker_stack_use_imm_x0x(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	int imm = ((insn & 0xf00) >> 4) + (insn & 0xf);
+	asi->stack_space = imm;
+	return INSN_GOOD_NO_SLOT;
+}
+#endif
+
+enum probes_insn checker_stack_use_imm_xxx(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	int imm = insn & 0xfff;
+	asi->stack_space = imm;
+	return INSN_GOOD_NO_SLOT;
+}
+
+enum probes_insn checker_stack_use_stmdx(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	unsigned int reglist = insn & 0xffff;
+	int pbit = insn & (1 << 24);
+	asi->stack_space = (hweight32(reglist) - (!pbit ? 1 : 0)) * 4;
+
+	return INSN_GOOD_NO_SLOT;
+}
+
+const union decode_action stack_check_actions[] = {
+	[STACK_USE_NONE] = {.decoder = checker_stack_use_none},
+	[STACK_USE_UNKNOWN] = {.decoder = checker_stack_use_unknown},
+#ifdef CONFIG_THUMB2_KERNEL
+	[STACK_USE_FIXED_0XX] = {.decoder = checker_stack_use_imm_0xx},
+	[STACK_USE_T32STRD] = {.decoder = checker_stack_use_t32strd},
+#else
+	[STACK_USE_FIXED_X0X] = {.decoder = checker_stack_use_imm_x0x},
+#endif
+	[STACK_USE_FIXED_XXX] = {.decoder = checker_stack_use_imm_xxx},
+	[STACK_USE_STMDX] = {.decoder = checker_stack_use_stmdx},
+};
diff --git a/arch/arm/probes/kprobes/checkers-thumb.c b/arch/arm/probes/kprobes/checkers-thumb.c
new file mode 100644
index 0000000..d608e3b
--- /dev/null
+++ b/arch/arm/probes/kprobes/checkers-thumb.c
@@ -0,0 +1,110 @@
+/*
+ * arch/arm/probes/kprobes/checkers-thumb.c
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include "../decode.h"
+#include "../decode-thumb.h"
+#include "checkers.h"
+
+static enum probes_insn __kprobes t32_check_stack(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	/*
+	 * PROBES_T32_LDMSTM, PROBES_T32_LDRDSTRD and PROBES_T32_LDRSTR
+	 * may get here. Simply mark all normal insns as STACK_USE_NONE.
+	 */
+	static const union decode_item table[] = {
+
+		/*
+		 * First, filter out all ldr insns to make our life easier.
+		 * Following load insns may come here:
+		 * LDM, LDRD, LDR.
+		 * In T32 encoding, bit 20 is enough for distinguishing
+		 * load and store. All load insns have this bit set, when
+		 * all store insns have this bit clear.
+		 */
+		DECODE_CUSTOM	(0x00100000, 0x00100000, STACK_USE_NONE),
+
+		/*
+		 * Mark all 'STR{,B,H}, Rt, [Rn, Rm]' as STACK_USE_UNKNOWN
+		 * if Rn or Rm is SP. T32 doesn't encode STRD.
+		 */
+		/*                                 xx | Rn | Rt |         | Rm |*/
+		/* STR (register)	1111 1000 0100 xxxx xxxx 0000 00xx xxxx */
+		/* STRB (register)	1111 1000 0000 xxxx xxxx 0000 00xx xxxx */
+		/* STRH (register)	1111 1000 0010 xxxx xxxx 0000 00xx xxxx */
+		/* INVALID INSN		1111 1000 0110 xxxx xxxx 0000 00xx xxxx */
+		/* By Introducing INVALID INSN, bit 21 and 22 can be ignored. */
+		DECODE_OR	(0xff9f0fc0, 0xf80d0000),
+		DECODE_CUSTOM	(0xff900fcf, 0xf800000d, STACK_USE_UNKNOWN),
+
+
+		/*                                 xx | Rn | Rt | PUW|   imm8  |*/
+		/* STR (imm 8)		1111 1000 0100 1101 xxxx 110x xxxx xxxx */
+		/* STRB (imm 8)		1111 1000 0000 1101 xxxx 110x xxxx xxxx */
+		/* STRH (imm 8)		1111 1000 0010 1101 xxxx 110x xxxx xxxx */
+		/* INVALID INSN		1111 1000 0110 1101 xxxx 110x xxxx xxxx */
+		/* Only consider U == 0 and P == 1: strx rx, [sp, #-<imm>] */
+		DECODE_CUSTOM	(0xff9f0e00, 0xf80d0c00, STACK_USE_FIXED_0XX),
+
+		/* For STR{,B,H} (imm 12), offset is always positive, so ignore them. */
+
+		/*                              P U W | Rn | Rt | Rt2|   imm8  |*/
+		/* STRD (immediate)	1110 1001 01x0 1101 xxxx xxxx xxxx xxxx */
+		/*
+		 * Only consider U == 0 and P == 1.
+		 * Also note that STRD in T32 encoding is special:
+		 * imm = ZeroExtend(imm8:'00', 32)
+		 */
+		DECODE_CUSTOM	(0xffdf0000, 0xe94d0000, STACK_USE_T32STRD),
+
+		/*                                    | Rn | */
+		/* STMDB		1110 1001 00x0 1101 xxxx xxxx xxxx xxxx */
+		DECODE_CUSTOM	(0xffdf0000, 0xe90d0000, STACK_USE_STMDX),
+
+		/* fall through */
+		DECODE_CUSTOM	(0, 0, STACK_USE_NONE),
+		DECODE_END
+	};
+
+	return probes_decode_insn(insn, asi, table, false, false, stack_check_actions, NULL);
+}
+
+const struct decode_checker t32_stack_checker[NUM_PROBES_T32_ACTIONS] = {
+	[PROBES_T32_LDMSTM] = {.checker = t32_check_stack},
+	[PROBES_T32_LDRDSTRD] = {.checker = t32_check_stack},
+	[PROBES_T32_LDRSTR] = {.checker = t32_check_stack},
+};
+
+/*
+ * See following comments. This insn must be 'push'.
+ */
+static enum probes_insn __kprobes t16_check_stack(probes_opcode_t insn,
+		struct arch_probes_insn *asi,
+		const struct decode_header *h)
+{
+	unsigned int reglist = insn & 0x1ff;
+	asi->stack_space = hweight32(reglist) * 4;
+	return INSN_GOOD;
+}
+
+/*
+ * T16 encoding is simple: only the 'push' insn can need extra stack space.
+ * Other insns, like str, can only use r0-r7 as Rn.
+ */
+const struct decode_checker t16_stack_checker[NUM_PROBES_T16_ACTIONS] = {
+	[PROBES_T16_PUSH] = {.checker = t16_check_stack},
+};
diff --git a/arch/arm/probes/kprobes/checkers.h b/arch/arm/probes/kprobes/checkers.h
new file mode 100644
index 0000000..bddfa0e
--- /dev/null
+++ b/arch/arm/probes/kprobes/checkers.h
@@ -0,0 +1,54 @@
+/*
+ * arch/arm/probes/kprobes/checkers.h
+ *
+ * Copyright (C) 2014 Huawei Inc.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#ifndef _ARM_KERNEL_PROBES_CHECKERS_H
+#define _ARM_KERNEL_PROBES_CHECKERS_H
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include "../decode.h"
+
+extern probes_check_t checker_stack_use_none;
+extern probes_check_t checker_stack_use_unknown;
+#ifdef CONFIG_THUMB2_KERNEL
+extern probes_check_t checker_stack_use_imm_0xx;
+#else
+extern probes_check_t checker_stack_use_imm_x0x;
+#endif
+extern probes_check_t checker_stack_use_imm_xxx;
+extern probes_check_t checker_stack_use_stmdx;
+
+enum {
+	STACK_USE_NONE,
+	STACK_USE_UNKNOWN,
+#ifdef CONFIG_THUMB2_KERNEL
+	STACK_USE_FIXED_0XX,
+	STACK_USE_T32STRD,
+#else
+	STACK_USE_FIXED_X0X,
+#endif
+	STACK_USE_FIXED_XXX,
+	STACK_USE_STMDX,
+	NUM_STACK_USE_TYPES
+};
+
+extern const union decode_action stack_check_actions[];
+
+#ifndef CONFIG_THUMB2_KERNEL
+extern const struct decode_checker arm_stack_checker[];
+#else
+#endif
+extern const struct decode_checker t32_stack_checker[];
+extern const struct decode_checker t16_stack_checker[];
+#endif
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 4/7] ARM: kprobes: disallow probing stack consuming instructions
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
                   ` (2 preceding siblings ...)
  2014-12-04  5:35 ` [PATCH v12 3/7] ARM: kprobes: collects stack consumption for store instructions Wang Nan
@ 2014-12-04  5:35 ` Wang Nan
  2014-12-04  5:35 ` [PATCH v12 5/7] ARM: kprobes: Add test cases for " Wang Nan
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:35 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

This patch prohibits probing instructions for which the stack
requirements are unable to be determined statically. Some test cases
are found not work again after the modification, this patch also
removes them.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Reviewed-by: Jon Medhurst <tixy@linaro.org>

---
v1 -> v2:
 - Use MAX_STACK_SIZE macro instead of hard coded stack size.

v2 -> v3:
 - Commit message improvements.

v3 -> v4:
 - Commit message improvements.

v4 -> v5:
 - Move to arch/arm/probes.
---
 arch/arm/include/asm/kprobes.h     |  1 -
 arch/arm/include/asm/probes.h      | 12 ++++++++++++
 arch/arm/kernel/entry-armv.S       |  3 ++-
 arch/arm/probes/kprobes/core.c     |  9 +++++++++
 arch/arm/probes/kprobes/test-arm.c | 16 ++++++++++------
 5 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
index 49fa0df..56f9ac6 100644
--- a/arch/arm/include/asm/kprobes.h
+++ b/arch/arm/include/asm/kprobes.h
@@ -22,7 +22,6 @@
 
 #define __ARCH_WANT_KPROBES_INSN_SLOT
 #define MAX_INSN_SIZE			2
-#define MAX_STACK_SIZE			64	/* 32 would probably be OK */
 
 #define flush_insn_slot(p)		do { } while (0)
 #define kretprobe_blacklist_size	0
diff --git a/arch/arm/include/asm/probes.h b/arch/arm/include/asm/probes.h
index ccf9af3..f0a1ee8 100644
--- a/arch/arm/include/asm/probes.h
+++ b/arch/arm/include/asm/probes.h
@@ -19,6 +19,8 @@
 #ifndef _ASM_PROBES_H
 #define _ASM_PROBES_H
 
+#ifndef __ASSEMBLY__
+
 typedef u32 probes_opcode_t;
 
 struct arch_probes_insn;
@@ -41,4 +43,14 @@ struct arch_probes_insn {
 	int stack_space;
 };
 
+#endif /* __ASSEMBLY__ */
+
+/*
+ * We assume one instruction can consume at most 64 bytes stack, which is
+ * 'push {r0-r15}'. Instructions consume more or unknown stack space like
+ * 'str r0, [sp, #-80]' and 'str r0, [sp, r1]' should be prohibit to probe.
+ * Both kprobe and jprobe use this macro.
+ */
+#define MAX_STACK_SIZE			64
+
 #endif
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 2f5555d..672b219 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -31,6 +31,7 @@
 
 #include "entry-header.S"
 #include <asm/entry-macro-multi.S>
+#include <asm/probes.h>
 
 /*
  * Interrupt handling.
@@ -249,7 +250,7 @@ __und_svc:
 	@ If a kprobe is about to simulate a "stmdb sp..." instruction,
 	@ it obviously needs free stack space which then will belong to
 	@ the saved context.
-	svc_entry 64
+	svc_entry MAX_STACK_SIZE
 #else
 	svc_entry
 #endif
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 74f3dc3..3a58db4 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -115,6 +115,15 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		break;
 	}
 
+	/*
+	 * Never instrument insn like 'str r0, [sp, +/-r1]'. Also, insn likes
+	 * 'str r0, [sp, #-68]' should also be prohibited.
+	 * See __und_svc.
+	 */
+	if ((p->ainsn.stack_space < 0) ||
+			(p->ainsn.stack_space > MAX_STACK_SIZE))
+		return -EINVAL;
+
 	return 0;
 }
 
diff --git a/arch/arm/probes/kprobes/test-arm.c b/arch/arm/probes/kprobes/test-arm.c
index d9a1255..fdeb300 100644
--- a/arch/arm/probes/kprobes/test-arm.c
+++ b/arch/arm/probes/kprobes/test-arm.c
@@ -476,7 +476,8 @@ void kprobe_arm_test_cases(void)
 	TEST_GROUP("Extra load/store instructions")
 
 	TEST_RPR(  "strh	r",0, VAL1,", [r",1, 48,", -r",2, 24,"]")
-	TEST_RPR(  "streqh	r",14,VAL2,", [r",13,0, ", r",12, 48,"]")
+	TEST_RPR(  "streqh	r",14,VAL2,", [r",11,0, ", r",12, 48,"]")
+	TEST_UNSUPPORTED(  "streqh	r14, [r13, r12]")
 	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,"")
@@ -565,7 +566,8 @@ void kprobe_arm_test_cases(void)
 
 #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(  "strccd	r",8, VAL2,", [r",11,0, ", r",12,48,"]")
+	TEST_UNSUPPORTED(  "strccd r8, [r13, r12]")
 	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,"")
@@ -638,13 +640,15 @@ void kprobe_arm_test_cases(void)
 	TEST_RP( "str"byte"	r",2, VAL1,", [r",3, 24,"], #48")		\
 	TEST_RP( "str"byte"	r",10,VAL2,", [r",9, 64,"], #-48")		\
 	TEST_RPR("str"byte"	r",0, VAL1,", [r",1, 48,", -r",2, 24,"]")	\
-	TEST_RPR("str"byte"	r",14,VAL2,", [r",13,0, ", r",12, 48,"]")	\
+	TEST_RPR("str"byte"	r",14,VAL2,", [r",11,0, ", r",12, 48,"]")	\
+	TEST_UNSUPPORTED("str"byte" r14, [r13, r12]")	\
 	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",13,0, ", r",12, 32,", lsr #2]")\
+	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_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]")			\
@@ -668,12 +672,12 @@ void kprobe_arm_test_cases(void)
 
 	LOAD_STORE("")
 	TEST_P(   "str	pc, [r",0,0,", #15*4]")
-	TEST_R(   "str	pc, [sp, r",2,15*4,"]")
+	TEST_UNSUPPORTED(   "str	pc, [sp, r2]")
 	TEST_BF(  "ldr	pc, [sp, #15*4]")
 	TEST_BF_R("ldr	pc, [sp, r",2,15*4,"]")
 
 	TEST_P(   "str	sp, [r",0,0,", #13*4]")
-	TEST_R(   "str	sp, [sp, r",2,13*4,"]")
+	TEST_UNSUPPORTED(   "str	sp, [sp, r2]")
 	TEST_BF(  "ldr	sp, [sp, #13*4]")
 	TEST_BF_R("ldr	sp, [sp, r",2,13*4,"]")
 
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 5/7] ARM: kprobes: Add test cases for stack consuming instructions
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
                   ` (3 preceding siblings ...)
  2014-12-04  5:35 ` [PATCH v12 4/7] ARM: kprobes: disallow probing stack consuming instructions Wang Nan
@ 2014-12-04  5:35 ` Wang Nan
  2014-12-04 16:22   ` Jon Medhurst (Tixy)
  2014-12-04  5:35 ` [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe Wang Nan
  2014-12-04  5:36 ` [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32 Wang Nan
  6 siblings, 1 reply; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:35 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

These have extra 'checker' functions associated with them so
lets make sure those get covered by testing.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
Signed-off-by: Wang Nan <wangnan0@huawei.com>

---
v1 -> v2:
 - Move to arch/arm/probes/ .
---
 arch/arm/probes/kprobes/test-arm.c   | 17 +++++++++++++++--
 arch/arm/probes/kprobes/test-thumb.c | 12 ++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/arm/probes/kprobes/test-arm.c b/arch/arm/probes/kprobes/test-arm.c
index fdeb300..9b3b1b4 100644
--- a/arch/arm/probes/kprobes/test-arm.c
+++ b/arch/arm/probes/kprobes/test-arm.c
@@ -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]")			\
diff --git a/arch/arm/probes/kprobes/test-thumb.c b/arch/arm/probes/kprobes/test-thumb.c
index 6c6e9a9..e8cf193 100644
--- a/arch/arm/probes/kprobes/test-thumb.c
+++ b/arch/arm/probes/kprobes/test-thumb.c
@@ -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]")
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
                   ` (4 preceding siblings ...)
  2014-12-04  5:35 ` [PATCH v12 5/7] ARM: kprobes: Add test cases for " Wang Nan
@ 2014-12-04  5:35 ` Wang Nan
  2014-12-04 16:28   ` Jon Medhurst (Tixy)
  2014-12-04  5:36 ` [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32 Wang Nan
  6 siblings, 1 reply; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:35 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

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 <masami.hiramatsu.pt@hitachi.com>
Cc: Wang Nan <wangnan0@huawei.com>
---
 arch/x86/kernel/kprobes/opt.c | 3 ++-
 include/linux/kprobes.h       | 3 ++-
 kernel/kprobes.c              | 4 ++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index f1314d0..7028296 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -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;
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index f7296e5..7ab2c93 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -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,
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3995f54..9f28aa7 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -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;
 }
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
                   ` (5 preceding siblings ...)
  2014-12-04  5:35 ` [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe Wang Nan
@ 2014-12-04  5:36 ` Wang Nan
  2014-12-04 16:21   ` Jon Medhurst (Tixy)
  2014-12-04 18:29   ` Russell King - ARM Linux
  6 siblings, 2 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-04  5:36 UTC (permalink / raw)
  To: masami.hiramatsu.pt, tixy, linux; +Cc: lizefan, linux-kernel, linux-arm-kernel

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 <wangnan0@huawei.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Will Deacon <will.deacon@arm.com>

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.
---
 arch/arm/Kconfig                        |   1 +
 arch/arm/{kernel => include/asm}/insn.h |   0
 arch/arm/include/asm/kprobes.h          |  34 ++++
 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          |   1 +
 arch/arm/probes/kprobes/opt-arm.c       | 322 ++++++++++++++++++++++++++++++++
 9 files changed, 362 insertions(+), 5 deletions(-)
 rename arch/arm/{kernel => include/asm}/insn.h (100%)
 create mode 100644 arch/arm/probes/kprobes/opt-arm.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 89c4b5c..8281cea 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -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
diff --git a/arch/arm/kernel/insn.h b/arch/arm/include/asm/insn.h
similarity index 100%
rename from arch/arm/kernel/insn.h
rename to arch/arm/include/asm/insn.h
diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
index 56f9ac6..5574008 100644
--- a/arch/arm/include/asm/kprobes.h
+++ b/arch/arm/include/asm/kprobes.h
@@ -50,5 +50,39 @@ 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;
+
+/*
+ * Plus 4 for potential alignment adjustment. See comments
+ * in arch_prepare_optimized_kprobe() in
+ * arch/arm/probes/kprobes-opt-arm.c .
+ */
+#define MAX_OPTIMIZED_LENGTH	4
+#define MAX_OPTINSN_SIZE				\
+	(((unsigned long)&optprobe_template_end -	\
+	  (unsigned long)&optprobe_template_entry) + 4)
+#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 arm32,
+	 * size always be 4, so didn't like x86, there is no
+	 * size field.
+	 */
+};
 
 #endif /* _ARM_KPROBES_H */
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 40d3e00..1d0f4e7 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -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
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index af9a8a9..ec7e332 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -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} */
diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index c6c73ed..35a8fbb 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -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
 
diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
index bc8d504..76a36bf 100644
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -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
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 3a58db4..4a2cf40 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -630,6 +630,7 @@ static struct undef_hook kprobes_arm_break_hook = {
 
 int __init arch_init_kprobes()
 {
+
 	arm_probes_decode_init();
 #ifdef CONFIG_THUMB2_KERNEL
 	register_undef_hook(&kprobes_thumb16_break_hook);
diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c
new file mode 100644
index 0000000..46e4474
--- /dev/null
+++ b/arch/arm/probes/kprobes/opt-arm.c
@@ -0,0 +1,322 @@
+/*
+ *  Kernel Probes Jump Optimization (Optprobes)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) IBM Corporation, 2002, 2004
+ * Copyright (C) Hitachi Ltd., 2012
+ * Copyright (C) Huawei Inc., 2014
+ */
+
+#include <linux/kprobes.h>
+#include <linux/jump_label.h>
+#include <asm/kprobes.h>
+#include <asm/cacheflush.h>
+/* for arm_gen_branch */
+#include <asm/insn.h>
+/* for patch_text */
+#include <asm/patch.h>
+
+/*
+ * NOTE: the first sub and add instruction will be modified according
+ * to the stack cost of the instruction.
+ */
+asm (
+			".global optprobe_template_entry\n"
+			"optprobe_template_entry:\n"
+			".global optprobe_template_sub_sp\n"
+			"optprobe_template_sub_sp:"
+			"	sub	sp, sp, #0xff\n"
+			"	stmia	sp, {r0 - r14} \n"
+			".global optprobe_template_add_sp\n"
+			"optprobe_template_add_sp:"
+			"	add	r3, sp, #0xff\n"
+			"	str	r3, [sp, #52]\n"
+			"	mrs	r4, cpsr\n"
+			"	str	r4, [sp, #64]\n"
+			"	mov	r1, sp\n"
+			"	ldr	r0, 1f\n"
+			"	ldr	r2, 2f\n"
+			/*
+			 * AEABI requires a 8-bytes alignment stack. If
+			 * SP % 8 != 0, alloc more bytes here.
+			 */
+			"	and	r4, sp, #7\n"
+			"	sub	sp, sp, r4\n"
+			"	blx	r2\n"
+			"	add	sp, sp, r4\n"
+			"	ldr	r1, [sp, #64]\n"
+			"	tst	r1, #"__stringify(PSR_T_BIT)"\n"
+			"	ldrne	r2, [sp, #60]\n"
+			"	orrne	r2, #1\n"
+			"	strne	r2, [sp, #60] @ set bit0 of PC for thumb\n"
+			"	msr	cpsr_cxsf, r1\n"
+			"	ldmia	sp, {r0 - r15}\n"
+			".global optprobe_template_val\n"
+			"optprobe_template_val:\n"
+			"1:	.long 0\n"
+			".global optprobe_template_call\n"
+			"optprobe_template_call:\n"
+			"2:	.long 0\n"
+			".global optprobe_template_end\n"
+			"optprobe_template_end:\n");
+
+#define TMPL_VAL_IDX \
+	((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry)
+#define TMPL_CALL_IDX \
+	((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry)
+#define TMPL_END_IDX \
+	((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry)
+#define TMPL_ADD_SP \
+	((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry)
+#define TMPL_SUB_SP \
+	((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry)
+
+/*
+ * ARM can always optimize an instruction when using ARM ISA, except
+ * instructions like 'str r0, [sp, r1]' which store to stack and unable
+ * to determine stack space consumption statically.
+ */
+int arch_prepared_optinsn(struct arch_optimized_insn *optinsn)
+{
+	return optinsn->insn != NULL;
+}
+
+/*
+ * In ARM ISA, kprobe opt always replace one instruction (4 bytes
+ * aligned and 4 bytes long). It is impossible to encounter another
+ * kprobe in the address range. So always return 0.
+ */
+int arch_check_optimized_kprobe(struct optimized_kprobe *op)
+{
+	return 0;
+}
+
+/* Caller must ensure addr & 3 == 0 */
+static int can_optimize(struct kprobe *kp)
+{
+	if (kp->ainsn.stack_space < 0)
+		return 0;
+	/*
+	 * 255 is the biggest imm can be used in 'sub r0, r0, #<imm>'.
+	 * Number larger than 255 needs special encoding.
+	 */
+	if (kp->ainsn.stack_space > 255 - sizeof(struct pt_regs))
+		return 0;
+	return 1;
+}
+
+/* Free optimized instruction slot */
+static void
+__arch_remove_optimized_kprobe(struct optimized_kprobe *op, int dirty)
+{
+	if (op->optinsn.insn) {
+		free_optinsn_slot(op->optinsn.insn, dirty);
+		op->optinsn.insn = NULL;
+	}
+}
+
+extern void kprobe_handler(struct pt_regs *regs);
+
+static void
+optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
+{
+	unsigned long flags;
+	struct kprobe *p = &op->kp;
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	/* Save skipped registers */
+	regs->ARM_pc = (unsigned long)op->kp.addr;
+	regs->ARM_ORIG_r0 = ~0UL;
+
+	local_irq_save(flags);
+
+	if (kprobe_running()) {
+		kprobes_inc_nmissed_count(&op->kp);
+	} else {
+		__this_cpu_write(current_kprobe, &op->kp);
+		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+		opt_pre_handler(&op->kp, regs);
+		__this_cpu_write(current_kprobe, NULL);
+	}
+
+	/* In each case, we must singlestep the replaced instruction. */
+	op->kp.ainsn.insn_singlestep(p->opcode, &p->ainsn, regs);
+
+	local_irq_restore(flags);
+}
+
+int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *orig)
+{
+	kprobe_opcode_t *code;
+	unsigned long rel_chk;
+	unsigned long val;
+	unsigned long stack_protect = sizeof(struct pt_regs);
+
+	if (!can_optimize(orig))
+		return -EILSEQ;
+
+	/*
+	 * 'code' must be 4-bytes aligned on arm, so we can use
+	 * 'code[x] = y' without triggering alignment exception.
+	 * Unfortunately get_optinsn_slot() uses module_alloc and
+	 * doesn't ensure any alignment.
+	 */
+	code = get_optinsn_slot();
+	if (!code)
+		return -ENOMEM;
+
+	/*
+	 * Verify if the address gap is in 32MiB range, because this uses
+	 * a relative jump.
+	 *
+	 * kprobe opt use a 'b' instruction to branch to optinsn.insn.
+	 * According to ARM manual, branch instruction is:
+	 *
+	 *   31  28 27           24 23             0
+	 *  +------+---+---+---+---+----------------+
+	 *  | cond | 1 | 0 | 1 | 0 |      imm24     |
+	 *  +------+---+---+---+---+----------------+
+	 *
+	 * imm24 is a signed 24 bits integer. The real branch offset is computed
+	 * by: imm32 = SignExtend(imm24:'00', 32);
+	 *
+	 * So the maximum forward branch should be:
+	 *   (0x007fffff << 2) = 0x01fffffc =  0x1fffffc
+	 * The maximum backword branch should be:
+	 *   (0xff800000 << 2) = 0xfe000000 = -0x2000000
+	 *
+	 * We can simply check (rel & 0xfe000003):
+	 *  if rel is positive, (rel & 0xfe000000) shoule be 0
+	 *  if rel is negitive, (rel & 0xfe000000) should be 0xfe000000
+	 *  the last '3' is used for alignment checking.
+	 */
+	rel_chk = (unsigned long)((long)code -
+			(long)orig->addr + 8) & 0xfe000003;
+
+	if ((rel_chk != 0) && (rel_chk != 0xfe000000)) {
+		/*
+		 * Different from x86, we free code buf directly instead of
+		 * calling __arch_remove_optimized_kprobe() because
+		 * we have not fill any field in op.
+		 */
+		free_optinsn_slot(code, 0);
+		return -ERANGE;
+	}
+
+	/* Copy arch-dep-instance from template. */
+	memcpy(code, &optprobe_template_entry,
+			TMPL_END_IDX * sizeof(kprobe_opcode_t));
+
+	/* Adjust buffer according to instruction. */
+	BUG_ON(orig->ainsn.stack_space < 0);
+
+	/*
+	 * Add more 4 byte for potential AEABI requirement. If probing is triggered
+	 * when SP % 8 == 4, we sub SP by another 4 bytes.
+	 */
+	stack_protect += orig->ainsn.stack_space + 4;
+
+	/* Should have been filtered by can_optimize(). */
+	BUG_ON(stack_protect > 255);
+
+	/* Create a 'sub sp, sp, #<stack_protect>' */
+	code[TMPL_SUB_SP] = __opcode_to_mem_arm(0xe24dd000 | stack_protect);
+	/* Create a 'add r3, sp, #<stack_protect>' */
+	code[TMPL_ADD_SP] = __opcode_to_mem_arm(0xe28d3000 | stack_protect);
+
+	/* Set probe information */
+	val = (unsigned long)op;
+	code[TMPL_VAL_IDX] = val;
+
+	/* Set probe function call */
+	val = (unsigned long)optimized_callback;
+	code[TMPL_CALL_IDX] = val;
+
+	flush_icache_range((unsigned long)code,
+			   (unsigned long)(&code[TMPL_END_IDX]));
+
+	/*
+	 * Set op->optinsn.insn means prepared.
+	 * NOTE: what we saved here is potentially unaligned.
+	 */
+	op->optinsn.insn = code;
+	return 0;
+}
+
+void arch_optimize_kprobes(struct list_head *oplist)
+{
+	struct optimized_kprobe *op, *tmp;
+
+	list_for_each_entry_safe(op, tmp, oplist, list) {
+		unsigned long insn;
+		WARN_ON(kprobe_disabled(&op->kp));
+
+		/*
+		 * Backup instructions which will be replaced
+		 * by jump address
+		 */
+		memcpy(op->optinsn.copied_insn, op->kp.addr,
+				RELATIVEJUMP_SIZE);
+
+		insn = arm_gen_branch((unsigned long)op->kp.addr,
+				(unsigned long)op->optinsn.insn);
+		BUG_ON(insn == 0);
+
+		/*
+		 * Make it a conditional branch if replaced insn
+		 * is consitional
+		 */
+		insn = (__mem_to_opcode_arm(
+			  op->optinsn.copied_insn[0]) & 0xf0000000) |
+			(insn & 0x0fffffff);
+
+		patch_text(op->kp.addr, insn);
+
+		list_del_init(&op->list);
+	}
+}
+
+void arch_unoptimize_kprobe(struct optimized_kprobe *op)
+{
+	arch_arm_kprobe(&op->kp);
+}
+
+/*
+ * Recover original instructions and breakpoints from relative jumps.
+ * Caller must call with locking kprobe_mutex.
+ */
+void arch_unoptimize_kprobes(struct list_head *oplist,
+			    struct list_head *done_list)
+{
+	struct optimized_kprobe *op, *tmp;
+
+	list_for_each_entry_safe(op, tmp, oplist, list) {
+		arch_unoptimize_kprobe(op);
+		list_move(&op->list, done_list);
+	}
+}
+
+int arch_within_optimized_kprobe(struct optimized_kprobe *op,
+				unsigned long addr)
+{
+	return ((unsigned long)op->kp.addr <= addr &&
+		(unsigned long)op->kp.addr + RELATIVEJUMP_SIZE > addr);
+}
+
+void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
+{
+	__arch_remove_optimized_kprobe(op, 1);
+}
-- 
1.8.4


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-04  5:36 ` [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32 Wang Nan
@ 2014-12-04 16:21   ` Jon Medhurst (Tixy)
  2014-12-05  3:38     ` Wang Nan
  2014-12-04 18:29   ` Russell King - ARM Linux
  1 sibling, 1 reply; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-04 16:21 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On Thu, 2014-12-04 at 13:36 +0800, Wang Nan wrote:
> 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 <wangnan0@huawei.com>
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Will Deacon <will.deacon@arm.com>


I have retested this patch and on one of the arm test cases I get an
undefined instruction exception in kprobe_arm_test_cases. When this
happens PC points to the second nop below. 


80028a38:	e320f000 	nop	{0}
80028a3c:	e11000b2 	ldrh	r0, [r0, -r2]
80028a40:	e320f000 	nop	{0}

As all three instructions will have probes on them during testing, and
un-optimised probes are implemented by using an undefined instruction to
act as a breakpoint, my first thought was that we have a race condition
somewhere with adding, removing or optimizing probes. Though a reboot a
retest failed in the same way on the same instruction, so I'm not 100%
convinced about strictly timing related bugs.
 
Meanwhile, I have some review comments of the code below...


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


Looks like you accidentally have the '---' break in the wrong place,
should be before the version changes description.


>  arch/arm/Kconfig                        |   1 +
>  arch/arm/{kernel => include/asm}/insn.h |   0
>  arch/arm/include/asm/kprobes.h          |  34 ++++
>  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          |   1 +
>  arch/arm/probes/kprobes/opt-arm.c       | 322 ++++++++++++++++++++++++++++++++
>  9 files changed, 362 insertions(+), 5 deletions(-)
>  rename arch/arm/{kernel => include/asm}/insn.h (100%)
>  create mode 100644 arch/arm/probes/kprobes/opt-arm.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..8281cea 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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
> diff --git a/arch/arm/kernel/insn.h b/arch/arm/include/asm/insn.h
> similarity index 100%
> rename from arch/arm/kernel/insn.h
> rename to arch/arm/include/asm/insn.h
> diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
> index 56f9ac6..5574008 100644
> --- a/arch/arm/include/asm/kprobes.h
> +++ b/arch/arm/include/asm/kprobes.h
> @@ -50,5 +50,39 @@ 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;
> +
> +/*
> + * Plus 4 for potential alignment adjustment. See comments
> + * in arch_prepare_optimized_kprobe() in
> + * arch/arm/probes/kprobes-opt-arm.c .
> + */
> +#define MAX_OPTIMIZED_LENGTH	4
> +#define MAX_OPTINSN_SIZE				\
> +	(((unsigned long)&optprobe_template_end -	\
> +	  (unsigned long)&optprobe_template_entry) + 4)

Is this "+ 4" needed now? I think it might be left over from the
previous version where you were aligning code in the slot to a 4 byte
boundary.



> +#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 arm32,
> +	 * size always be 4, so didn't like x86, there is no
> +	 * size field.

The above comment doesn't parse very well, how about...

	 * 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 */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 40d3e00..1d0f4e7 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -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
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index af9a8a9..ec7e332 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -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} */
> diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
> index c6c73ed..35a8fbb 100644
> --- a/arch/arm/kernel/jump_label.c
> +++ b/arch/arm/kernel/jump_label.c
> @@ -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
>  
> diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
> index bc8d504..76a36bf 100644
> --- a/arch/arm/probes/kprobes/Makefile
> +++ b/arch/arm/probes/kprobes/Makefile
> @@ -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
> diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
> index 3a58db4..4a2cf40 100644
> --- a/arch/arm/probes/kprobes/core.c
> +++ b/arch/arm/probes/kprobes/core.c
> @@ -630,6 +630,7 @@ static struct undef_hook kprobes_arm_break_hook = {
>  
>  int __init arch_init_kprobes()
>  {
> +

Looks like an accidental blank line got added here.


>  	arm_probes_decode_init();
>  #ifdef CONFIG_THUMB2_KERNEL
>  	register_undef_hook(&kprobes_thumb16_break_hook);
> diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c
> new file mode 100644
> index 0000000..46e4474
> --- /dev/null
> +++ b/arch/arm/probes/kprobes/opt-arm.c
> @@ -0,0 +1,322 @@
> +/*
> + *  Kernel Probes Jump Optimization (Optprobes)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) IBM Corporation, 2002, 2004
> + * Copyright (C) Hitachi Ltd., 2012
> + * Copyright (C) Huawei Inc., 2014
> + */
> +
> +#include <linux/kprobes.h>
> +#include <linux/jump_label.h>
> +#include <asm/kprobes.h>
> +#include <asm/cacheflush.h>
> +/* for arm_gen_branch */
> +#include <asm/insn.h>
> +/* for patch_text */
> +#include <asm/patch.h>
> +
> +/*
> + * NOTE: the first sub and add instruction will be modified according
> + * to the stack cost of the instruction.
> + */
> +asm (
> +			".global optprobe_template_entry\n"
> +			"optprobe_template_entry:\n"
> +			".global optprobe_template_sub_sp\n"
> +			"optprobe_template_sub_sp:"
> +			"	sub	sp, sp, #0xff\n"
> +			"	stmia	sp, {r0 - r14} \n"
> +			".global optprobe_template_add_sp\n"
> +			"optprobe_template_add_sp:"
> +			"	add	r3, sp, #0xff\n"
> +			"	str	r3, [sp, #52]\n"
> +			"	mrs	r4, cpsr\n"
> +			"	str	r4, [sp, #64]\n"
> +			"	mov	r1, sp\n"
> +			"	ldr	r0, 1f\n"
> +			"	ldr	r2, 2f\n"
> +			/*
> +			 * AEABI requires a 8-bytes alignment stack. If
> +			 * SP % 8 != 0, alloc more bytes here.
> +			 */
> +			"	and	r4, sp, #7\n"

We already know that the stack must be aligned to 4 bytes here, because
if it weren't, pushing the registers to the stack which we did earlier
would cause an exception. We are also assuming that alignment because
otherwise the pt_regs* we pass to optimized_callback would be
misaligned. So whilst ANDing with 7 is functionally correct I think it's
a bit misleading as it implies we aren't sure of the current alignment.

Therefore I suggest sticking with '4', which matches the method used by
the exception handling code in svc_entry to align the stack. It also
matches arch_prepare_optimized_kprobe which you have allocating only 4
extra bytes for alignment rather than 7.


> +			"	sub	sp, sp, r4\n"
> +			"	blx	r2\n"
> +			"	add	sp, sp, r4\n"
> +			"	ldr	r1, [sp, #64]\n"
> +			"	tst	r1, #"__stringify(PSR_T_BIT)"\n"
> +			"	ldrne	r2, [sp, #60]\n"
> +			"	orrne	r2, #1\n"
> +			"	strne	r2, [sp, #60] @ set bit0 of PC for thumb\n"
> +			"	msr	cpsr_cxsf, r1\n"
> +			"	ldmia	sp, {r0 - r15}\n"
> +			".global optprobe_template_val\n"
> +			"optprobe_template_val:\n"
> +			"1:	.long 0\n"
> +			".global optprobe_template_call\n"
> +			"optprobe_template_call:\n"
> +			"2:	.long 0\n"
> +			".global optprobe_template_end\n"
> +			"optprobe_template_end:\n");
> +
> +#define TMPL_VAL_IDX \
> +	((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry)
> +#define TMPL_CALL_IDX \
> +	((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry)
> +#define TMPL_END_IDX \
> +	((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry)
> +#define TMPL_ADD_SP \
> +	((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry)
> +#define TMPL_SUB_SP \
> +	((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry)
> +
> +/*
> + * ARM can always optimize an instruction when using ARM ISA, except
> + * instructions like 'str r0, [sp, r1]' which store to stack and unable
> + * to determine stack space consumption statically.
> + */
> +int arch_prepared_optinsn(struct arch_optimized_insn *optinsn)
> +{
> +	return optinsn->insn != NULL;
> +}
> +
> +/*
> + * In ARM ISA, kprobe opt always replace one instruction (4 bytes
> + * aligned and 4 bytes long). It is impossible to encounter another
> + * kprobe in the address range. So always return 0.
> + */
> +int arch_check_optimized_kprobe(struct optimized_kprobe *op)
> +{
> +	return 0;
> +}
> +
> +/* Caller must ensure addr & 3 == 0 */
> +static int can_optimize(struct kprobe *kp)
> +{
> +	if (kp->ainsn.stack_space < 0)
> +		return 0;
> +	/*
> +	 * 255 is the biggest imm can be used in 'sub r0, r0, #<imm>'.
> +	 * Number larger than 255 needs special encoding.
> +	 */
> +	if (kp->ainsn.stack_space > 255 - sizeof(struct pt_regs))
> +		return 0;
> +	return 1;
> +}
> +
> +/* Free optimized instruction slot */
> +static void
> +__arch_remove_optimized_kprobe(struct optimized_kprobe *op, int dirty)
> +{
> +	if (op->optinsn.insn) {
> +		free_optinsn_slot(op->optinsn.insn, dirty);
> +		op->optinsn.insn = NULL;
> +	}
> +}
> +
> +extern void kprobe_handler(struct pt_regs *regs);
> +
> +static void
> +optimized_callback(struct optimized_kprobe *op, struct pt_regs *regs)
> +{
> +	unsigned long flags;
> +	struct kprobe *p = &op->kp;
> +	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +
> +	/* Save skipped registers */
> +	regs->ARM_pc = (unsigned long)op->kp.addr;
> +	regs->ARM_ORIG_r0 = ~0UL;
> +
> +	local_irq_save(flags);
> +
> +	if (kprobe_running()) {
> +		kprobes_inc_nmissed_count(&op->kp);
> +	} else {
> +		__this_cpu_write(current_kprobe, &op->kp);
> +		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> +		opt_pre_handler(&op->kp, regs);
> +		__this_cpu_write(current_kprobe, NULL);
> +	}
> +
> +	/* In each case, we must singlestep the replaced instruction. */
> +	op->kp.ainsn.insn_singlestep(p->opcode, &p->ainsn, regs);
> +
> +	local_irq_restore(flags);
> +}
> +
> +int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *orig)
> +{
> +	kprobe_opcode_t *code;
> +	unsigned long rel_chk;
> +	unsigned long val;
> +	unsigned long stack_protect = sizeof(struct pt_regs);
> +
> +	if (!can_optimize(orig))
> +		return -EILSEQ;
> +
> +	/*
> +	 * 'code' must be 4-bytes aligned on arm, so we can use
> +	 * 'code[x] = y' without triggering alignment exception.
> +	 * Unfortunately get_optinsn_slot() uses module_alloc and
> +	 * doesn't ensure any alignment.
> +	 */

Don't think we need the above comment now, cerntainly not the last two
lines, because we've decided that slots are aligned after all. 


> +	code = get_optinsn_slot();
> +	if (!code)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Verify if the address gap is in 32MiB range, because this uses
> +	 * a relative jump.
> +	 *
> +	 * kprobe opt use a 'b' instruction to branch to optinsn.insn.
> +	 * According to ARM manual, branch instruction is:
> +	 *
> +	 *   31  28 27           24 23             0
> +	 *  +------+---+---+---+---+----------------+
> +	 *  | cond | 1 | 0 | 1 | 0 |      imm24     |
> +	 *  +------+---+---+---+---+----------------+
> +	 *
> +	 * imm24 is a signed 24 bits integer. The real branch offset is computed
> +	 * by: imm32 = SignExtend(imm24:'00', 32);
> +	 *
> +	 * So the maximum forward branch should be:
> +	 *   (0x007fffff << 2) = 0x01fffffc =  0x1fffffc
> +	 * The maximum backword branch should be:
> +	 *   (0xff800000 << 2) = 0xfe000000 = -0x2000000
> +	 *
> +	 * We can simply check (rel & 0xfe000003):
> +	 *  if rel is positive, (rel & 0xfe000000) shoule be 0
> +	 *  if rel is negitive, (rel & 0xfe000000) should be 0xfe000000
> +	 *  the last '3' is used for alignment checking.
> +	 */
> +	rel_chk = (unsigned long)((long)code -
> +			(long)orig->addr + 8) & 0xfe000003;
> +
> +	if ((rel_chk != 0) && (rel_chk != 0xfe000000)) {
> +		/*
> +		 * Different from x86, we free code buf directly instead of
> +		 * calling __arch_remove_optimized_kprobe() because
> +		 * we have not fill any field in op.
> +		 */
> +		free_optinsn_slot(code, 0);
> +		return -ERANGE;
> +	}
> +
> +	/* Copy arch-dep-instance from template. */
> +	memcpy(code, &optprobe_template_entry,
> +			TMPL_END_IDX * sizeof(kprobe_opcode_t));
> +
> +	/* Adjust buffer according to instruction. */
> +	BUG_ON(orig->ainsn.stack_space < 0);
> +
> +	/*
> +	 * Add more 4 byte for potential AEABI requirement. If probing is triggered
> +	 * when SP % 8 == 4, we sub SP by another 4 bytes.
> +	 */
> +	stack_protect += orig->ainsn.stack_space + 4;

The above comment and code don't match up any more with the code in
optprobe_template_entry, it should be '+ 7' here. Alternatively, change
the code in optprobe_template_entry back to use 4 as I suggested.


> +
> +	/* Should have been filtered by can_optimize(). */
> +	BUG_ON(stack_protect > 255);
> +
> +	/* Create a 'sub sp, sp, #<stack_protect>' */
> +	code[TMPL_SUB_SP] = __opcode_to_mem_arm(0xe24dd000 | stack_protect);
> +	/* Create a 'add r3, sp, #<stack_protect>' */
> +	code[TMPL_ADD_SP] = __opcode_to_mem_arm(0xe28d3000 | stack_protect);
> +
> +	/* Set probe information */
> +	val = (unsigned long)op;
> +	code[TMPL_VAL_IDX] = val;
> +
> +	/* Set probe function call */
> +	val = (unsigned long)optimized_callback;
> +	code[TMPL_CALL_IDX] = val;
> +
> +	flush_icache_range((unsigned long)code,
> +			   (unsigned long)(&code[TMPL_END_IDX]));
> +
> +	/*
> +	 * Set op->optinsn.insn means prepared.
> +	 * NOTE: what we saved here is potentially unaligned.
> +	 */
> +	op->optinsn.insn = code;
> +	return 0;
> +}
> +
> +void arch_optimize_kprobes(struct list_head *oplist)
> +{
> +	struct optimized_kprobe *op, *tmp;
> +
> +	list_for_each_entry_safe(op, tmp, oplist, list) {
> +		unsigned long insn;
> +		WARN_ON(kprobe_disabled(&op->kp));
> +
> +		/*
> +		 * Backup instructions which will be replaced
> +		 * by jump address
> +		 */
> +		memcpy(op->optinsn.copied_insn, op->kp.addr,
> +				RELATIVEJUMP_SIZE);
> +
> +		insn = arm_gen_branch((unsigned long)op->kp.addr,
> +				(unsigned long)op->optinsn.insn);
> +		BUG_ON(insn == 0);
> +
> +		/*
> +		 * Make it a conditional branch if replaced insn
> +		 * is consitional
> +		 */
> +		insn = (__mem_to_opcode_arm(
> +			  op->optinsn.copied_insn[0]) & 0xf0000000) |
> +			(insn & 0x0fffffff);
> +
> +		patch_text(op->kp.addr, insn);
> +
> +		list_del_init(&op->list);
> +	}
> +}
> +
> +void arch_unoptimize_kprobe(struct optimized_kprobe *op)
> +{
> +	arch_arm_kprobe(&op->kp);
> +}
> +
> +/*
> + * Recover original instructions and breakpoints from relative jumps.
> + * Caller must call with locking kprobe_mutex.
> + */
> +void arch_unoptimize_kprobes(struct list_head *oplist,
> +			    struct list_head *done_list)
> +{
> +	struct optimized_kprobe *op, *tmp;
> +
> +	list_for_each_entry_safe(op, tmp, oplist, list) {
> +		arch_unoptimize_kprobe(op);
> +		list_move(&op->list, done_list);
> +	}
> +}
> +
> +int arch_within_optimized_kprobe(struct optimized_kprobe *op,
> +				unsigned long addr)
> +{
> +	return ((unsigned long)op->kp.addr <= addr &&
> +		(unsigned long)op->kp.addr + RELATIVEJUMP_SIZE > addr);
> +}
> +
> +void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
> +{
> +	__arch_remove_optimized_kprobe(op, 1);
> +}

-- 
Tixy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 5/7] ARM: kprobes: Add test cases for stack consuming instructions
  2014-12-04  5:35 ` [PATCH v12 5/7] ARM: kprobes: Add test cases for " Wang Nan
@ 2014-12-04 16:22   ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-04 16:22 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

Looks like the source file reorg caused the author of this patch to
change. It should be...

From: Jon Medhurst <tixy@linaro.org>

On Thu, 2014-12-04 at 13:35 +0800, Wang Nan wrote:
> These have extra 'checker' functions associated with them so
> lets make sure those get covered by testing.
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> 
>

[rest of patch trimmed]


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe
  2014-12-04  5:35 ` [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe Wang Nan
@ 2014-12-04 16:28   ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-04 16:28 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On Thu, 2014-12-04 at 13:35 +0800, Wang Nan wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> 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 <masami.hiramatsu.pt@hitachi.com>
> Cc: Wang Nan <wangnan0@huawei.com>

Seems strange cc'ing yourself :-) That should be Signed-off-by instead,
as you're the person sending out the patch.

[rest of patch trimmed]

-- 
Tixy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-04  5:36 ` [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32 Wang Nan
  2014-12-04 16:21   ` Jon Medhurst (Tixy)
@ 2014-12-04 18:29   ` Russell King - ARM Linux
  1 sibling, 0 replies; 19+ messages in thread
From: Russell King - ARM Linux @ 2014-12-04 18:29 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, tixy, lizefan, linux-kernel, linux-arm-kernel

On Thu, Dec 04, 2014 at 01:36:00PM +0800, Wang Nan wrote:
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 89c4b5c..8281cea 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -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)

Please don't add extra parens where they're not required.

> +#define MAX_COPIED_INSN	(DIV_ROUND_UP(RELATIVEJUMP_SIZE, sizeof(kprobe_opcode_t)))

ditto.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-04 16:21   ` Jon Medhurst (Tixy)
@ 2014-12-05  3:38     ` Wang Nan
  2014-12-05 10:10       ` Jon Medhurst (Tixy)
  0 siblings, 1 reply; 19+ messages in thread
From: Wang Nan @ 2014-12-05  3:38 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On 2014/12/5 0:21, Jon Medhurst (Tixy) wrote:
> On Thu, 2014-12-04 at 13:36 +0800, Wang Nan wrote:
> 

[trim some text]

> 
> I have retested this patch and on one of the arm test cases I get an
> undefined instruction exception in kprobe_arm_test_cases. When this
> happens PC points to the second nop below. 
> 
> 
> 80028a38:	e320f000 	nop	{0}
> 80028a3c:	e11000b2 	ldrh	r0, [r0, -r2]
> 80028a40:	e320f000 	nop	{0}
> 
> As all three instructions will have probes on them during testing, and
> un-optimised probes are implemented by using an undefined instruction to
> act as a breakpoint, my first thought was that we have a race condition
> somewhere with adding, removing or optimizing probes. Though a reboot a
> retest failed in the same way on the same instruction, so I'm not 100%
> convinced about strictly timing related bugs.
>  

Does the problem appear in your platform in each time? Currently I have only
QEMU machine for testing and haven't seen problem like this before. Could
you please provide a detail steps for me to reproduce it? Or do you just
enable kprobe test code when booting and this exception simply appear twice?

> Meanwhile, I have some review comments of the code below...
> 

[trim some code]

>> +	/*
>> +	 * Add more 4 byte for potential AEABI requirement. If probing is triggered
>> +	 * when SP % 8 == 4, we sub SP by another 4 bytes.
>> +	 */
>> +	stack_protect += orig->ainsn.stack_space + 4;
> 
> The above comment and code don't match up any more with the code in
> optprobe_template_entry, it should be '+ 7' here. Alternatively, change
> the code in optprobe_template_entry back to use 4 as I suggested.
> 
> 

Looks like we don't really need this 4 bytes. ASM code should operate SP correctly in
each case.




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05  3:38     ` Wang Nan
@ 2014-12-05 10:10       ` Jon Medhurst (Tixy)
  2014-12-05 10:32         ` Wang Nan
                           ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-05 10:10 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On Fri, 2014-12-05 at 11:38 +0800, Wang Nan wrote:
> On 2014/12/5 0:21, Jon Medhurst (Tixy) wrote:
> > On Thu, 2014-12-04 at 13:36 +0800, Wang Nan wrote:
> > 
> 
> [trim some text]
> 
> > 
> > I have retested this patch and on one of the arm test cases I get an
> > undefined instruction exception in kprobe_arm_test_cases. When this
> > happens PC points to the second nop below. 
> > 
> > 
> > 80028a38:	e320f000 	nop	{0}
> > 80028a3c:	e11000b2 	ldrh	r0, [r0, -r2]
> > 80028a40:	e320f000 	nop	{0}
> > 
> > As all three instructions will have probes on them during testing, and
> > un-optimised probes are implemented by using an undefined instruction to
> > act as a breakpoint, my first thought was that we have a race condition
> > somewhere with adding, removing or optimizing probes. Though a reboot a
> > retest failed in the same way on the same instruction, so I'm not 100%
> > convinced about strictly timing related bugs.
> >  
> 
> Does the problem appear in your platform in each time?

Three times out of three tries yes. Though the third try was built
differently and the problem occurred on a different test case.


>  Currently I have only
> QEMU machine for testing and haven't seen problem like this before.

I don't know much about QEMU and have never used it, but I'm assuming
QEMU doesn't make any attempt to simulate caches like the data cache,
instruction cache, TLBs, branch predictor? Does it even emulate multiple
CPUs with multiple host CPU threads? Basically, I very much doubt QEMU
is a very good test of kernel code in general, and especially code that
modifies code and has multiple cpus running in parallel.

Do you not have access to any kind of ARM board to try some testing on?


>  Could
> you please provide a detail steps for me to reproduce it? Or do you just
> enable kprobe test code when booting and this exception simply appear twice?

I applied the patches on top of Linux 3.18-rc5 and set VERBOSE in
arm/probes/kprobes/test-core.h to 1. Then built a kernel configured
using vexpress_defconfig and enabled

CONFIG_KPROBES=y
CONFIG_ARM_KPROBES_TEST=y
CONFIG_DEBUG_INFO=y

then booted on a Versatile Express board with a TC2 CoreTile (A15/A7
big.LITTLE CPU).

The Oops I described happened on two consecutive boots of the board. I
then tried again setting VERBOSE to 0 and I got a similar OOPs but on a
different test case.

I'm worried because this whole optimised kprobes has some rather
complicated interactions, e.g. can the background thread that changes
breakpoints to jumps (or back again?) could occur at the same time
another CPU is processing a kprobe that's been hit, or is in the process
of removing a probe.

-- 
Tixy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05 10:10       ` Jon Medhurst (Tixy)
@ 2014-12-05 10:32         ` Wang Nan
  2014-12-05 10:48           ` Jon Medhurst (Tixy)
  2014-12-05 14:59         ` Jon Medhurst (Tixy)
  2014-12-05 19:57         ` Peter Maydell
  2 siblings, 1 reply; 19+ messages in thread
From: Wang Nan @ 2014-12-05 10:32 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On 2014/12/5 18:10, Jon Medhurst (Tixy) wrote:
> On Fri, 2014-12-05 at 11:38 +0800, Wang Nan wrote:
>> On 2014/12/5 0:21, Jon Medhurst (Tixy) wrote:
>>> On Thu, 2014-12-04 at 13:36 +0800, Wang Nan wrote:
>>>
>>
>> [trim some text]
>>
>>>
>>> I have retested this patch and on one of the arm test cases I get an
>>> undefined instruction exception in kprobe_arm_test_cases. When this
>>> happens PC points to the second nop below. 
>>>
>>>
>>> 80028a38:	e320f000 	nop	{0}
>>> 80028a3c:	e11000b2 	ldrh	r0, [r0, -r2]
>>> 80028a40:	e320f000 	nop	{0}
>>>
>>> As all three instructions will have probes on them during testing, and
>>> un-optimised probes are implemented by using an undefined instruction to
>>> act as a breakpoint, my first thought was that we have a race condition
>>> somewhere with adding, removing or optimizing probes. Though a reboot a
>>> retest failed in the same way on the same instruction, so I'm not 100%
>>> convinced about strictly timing related bugs.
>>>  
>>
>> Does the problem appear in your platform in each time?
> 
> Three times out of three tries yes. Though the third try was built
> differently and the problem occurred on a different test case.
> 
> 
>>  Currently I have only
>> QEMU machine for testing and haven't seen problem like this before.
> 
> I don't know much about QEMU and have never used it, but I'm assuming
> QEMU doesn't make any attempt to simulate caches like the data cache,
> instruction cache, TLBs, branch predictor? Does it even emulate multiple
> CPUs with multiple host CPU threads? Basically, I very much doubt QEMU
> is a very good test of kernel code in general, and especially code that
> modifies code and has multiple cpus running in parallel.
> 
> Do you not have access to any kind of ARM board to try some testing on?
> 
> 
>>  Could
>> you please provide a detail steps for me to reproduce it? Or do you just
>> enable kprobe test code when booting and this exception simply appear twice?
> 
> I applied the patches on top of Linux 3.18-rc5 and set VERBOSE in
> arm/probes/kprobes/test-core.h to 1. Then built a kernel configured
> using vexpress_defconfig and enabled
> 
> CONFIG_KPROBES=y
> CONFIG_ARM_KPROBES_TEST=y
> CONFIG_DEBUG_INFO=y
> 
> then booted on a Versatile Express board with a TC2 CoreTile (A15/A7
> big.LITTLE CPU).
> 
> The Oops I described happened on two consecutive boots of the board. I
> then tried again setting VERBOSE to 0 and I got a similar OOPs but on a
> different test case.
> 


Before your reply I also did my testing on real hardware platform. I tried 3
times and hit one similar failure. dmesg is pasted at the end of this mail.
After the Oops arises, I wrap the failed testcase with a loop and run it
100 times, they all passed.

I use your test code for testing, with following modification:

--- ../temp/arch/arm/probes/kprobes/test-core.c	2014-12-05 15:42:28.000000000 +0800
+++ ./arch/arm/probes/kprobes/test-core.c	2014-12-05 16:06:18.000000000 +0800
@@ -311,6 +311,7 @@
 	pre_handler_called = test_func_instance;
 	if (regs->ARM_r0 == FUNC_ARG1 && regs->ARM_r1 == FUNC_ARG2)
 		test_regs_ok = true;
+	post_handler_called = test_func_instance + 1;
 	return 0;
 }

@@ -325,7 +326,7 @@
 static struct kprobe the_kprobe = {
 	.addr		= 0,
 	.pre_handler	= pre_handler,
-	.post_handler	= post_handler
+	.post_handler	= NULL
 };

 static int test_kprobe(long (*func)(long, long))
@@ -346,6 +347,7 @@

 	if (!ret)
 		return -EINVAL;
+#if 0
 	if (pre_handler_called != test_func_instance) {
 		pr_err("FAIL: kprobe pre_handler not called\n");
 		return -EINVAL;
@@ -361,7 +363,7 @@
 		pr_err("FAIL: probe called after unregistering\n");
 		return -EINVAL;
 	}
-
+#endif
 	return 0;
 }

and with the kernel config options you mentioned before selected.

The hardware platform I use doesn't have stable BSP for 3.18, so
I have to backport all kprobe related code to 3.10.61.

Both your and mine failure are related to ldrd/h instruction. What
about your second failed testcase?


> I'm worried because this whole optimised kprobes has some rather
> complicated interactions, e.g. can the background thread that changes
> breakpoints to jumps (or back again?) could occur at the same time
> another CPU is processing a kprobe that's been hit, or is in the process
> of removing a probe.
> 

I think x86 should also has to deal with these problems, so if the fault
is caused by these race, they may not ARM specific.

Thank you!

------------ dmesg --------------
...
[ 1398.496592] .long ((0xe089c0df) & 0xFFFFFFFF)
[ 1398.496592] 		@ ldrd r12, [r9], pc
[ 1398.504338] strd	r0, [r1, #-8]	@ e14100f8
[ 1400.241108] strvsd	r8, [r13, #8]	@ 61cd80f8
[ 1402.031108] strd	r4, [r2, #16]!	@ e1e241f0
[ 1403.821107] strvcd	r12, [r11, #-16]!	@ 716bc1f0
[ 1405.641104] strd	r2, [r4], #48	@ e0c423f0
[ 1407.441108] strd	r10, [r9], #-48	@ e049a3f0
[ 1409.261105] strd	r6, [r13, #-64]!	@ e16d64f0
[ 1411.051105] strd r6, [r13, #-64-8]!	@ e16d64f8
[ 1411.101114] strd	r4, [r12, #-64-8]!	@ e16c44f8
[ 1412.871105] .long ((0xe1efc3f0) & 0xFFFFFFFF)
[ 1412.871105] 		@ strd r12, [pc, #48]!
[ 1412.921113] ldrd	r0, [r0, #-8]	@ e14000d8
[ 1413.031116] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
[ 1413.037809] Modules linked in:
[ 1413.040876] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.53-HULK2+ #31
[ 1413.047569] task: ef238000 ti: ef226000 task.ti: ef226000
[ 1413.052971] PC is at kprobe_arm_test_cases+0xa1ec/0xfeec
[ 1413.058276] LR is at 0x21522f52
[ 1413.061416] pc : [<c00286d4>]    lr : [<21522f52>]    psr: 18010113
[ 1413.061416] sp : ef227dd8  ip : 21522d52  fp : 21522a52
[ 1413.072876] r10: 21522b52  r9 : 21522852  r8 : 21522952
[ 1413.078095] r7 : 21522652  r6 : 21522752  r5 : 21522452  r4 : 21522552
[ 1413.084613] r3 : 21522252  r2 : 21522352  r1 : 45678eab  r0 : 45678dab
[ 1413.091132] Flags: nzcV  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[ 1413.098430] Control: 18c53c7d  Table: 8000404a  DAC: 00000015
[ 1413.104167] Process swapper/0 (pid: 1, stack limit = 0xef226238)
[ 1413.110166] Stack: (0xef227dd8 to 0xef228000)
[ 1413.114518] 7dc0:                                                       456789ab 45678aab
[ 1413.122686] 7de0: 45678bab 45678cab 45678dab 45678eab 45678fab 456790ab 456791ab 456792ab
[ 1413.130855] 7e00: 456793ab 456794ab 456795ab ef227e50 456797ab 456798ab 456799ab 45679aab
[ 1413.139022] 7e20: 45679bab 45679cab 45679dab 45679eab 45679fab 4567a0ab 4567a1ab 4567a2ab
[ 1413.147191] 7e40: 4567a3ab 4567a4ab 4567a5ab 4567a6ab 4567a7ab 4567a8ab 4567a9ab 4567aaab
[ 1413.155360] 7e60: 4567abab 4567acab 4567adab 4567aeab 4567afab 4567b0ab 4567b1ab 4567b2ab
[ 1413.163527] 7e80: 4567b3ab 4567b4ab 4567b5ab 4567b6ab 4567b7ab 4567b8ab 4567b9ab 4567baab
[ 1413.171694] 7ea0: 4567bbab 4567bcab 4567bdab 4567beab 4567bfab 4567c0ab 4567c1ab 4567c2ab
[ 1413.179863] 7ec0: 4567c3ab 4567c4ab 4567c5ab 4567c6ab 4567c7ab 4567c8ab c03bd920 c03bdf5e
[ 1413.188031] 7ee0: c02f7708 00000000 c001dd58 00000000 c046526c 00000000 0000000c 00000000
[ 1413.196199] 7f00: c04c1a4c c04427f0 00000001 c02f7708 00000000 00000000 c04c1440 00000007
[ 1413.204367] 7f20: c0465264 c0476520 c04c1440 c044266c c046526c c000868c ef227f64 c0418fac
[ 1413.212534] 7f40: c0418a2c 00000058 00000007 00000007 00000001 00000007 c0465264 c0476520
[ 1413.220702] 7f60: c04c1440 00000058 c043a478 c046526c 00000000 c043ac50 00000007 00000007
[ 1413.228869] 7f80: c043a478 ef226000 c04c1440 c02d8e9c 00000000 00000000 00000000 00000000
[ 1413.237036] 7fa0: 00000000 c02d8ea8 00000000 c000e138 00000000 00000000 00000000 00000000
[ 1413.245203] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1413.253371] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 1413.261545] [<c00286d4>] (kprobe_arm_test_cases+0xa1ec/0xfeec) from [<45678cab>] (0x45678cab)
[ 1413.270060] Code: 00002000 000c000c e7f001f8 e14000d8 (eabf69b9)
[ 1413.276164] ---[ end trace 81706e7e45d860af ]---
[ 1413.280776] Kernel panic - not syncing: Fatal exception
[ 1413.285996] CPU1: stopping
[ 1413.288704] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D      3.10.53-HULK2+ #31
[ 1413.296372] [<c0014b84>] (unwind_backtrace+0x0/0x120) from [<c00118a8>] (show_stack+0x10/0x14)
[ 1413.304977] [<c00118a8>] (show_stack+0x10/0x14) from [<c00137b4>] (handle_IPI+0xc0/0x124)
[ 1413.313148] [<c00137b4>] (handle_IPI+0xc0/0x124) from [<c0008530>] (gic_handle_irq+0x58/0x60)
[ 1413.321675] [<c0008530>] (gic_handle_irq+0x58/0x60) from [<c02e6900>] (__irq_svc+0x40/0x50)
[ 1413.330013] Exception stack(0xef259fa0 to 0xef259fe8)




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05 10:32         ` Wang Nan
@ 2014-12-05 10:48           ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-05 10:48 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On Fri, 2014-12-05 at 18:32 +0800, Wang Nan wrote:
> Both your and mine failure are related to ldrd/h instruction. What
> about your second failed testcase?

Can't remember exactly, but it was some instruction that didn't access
memory I remember that much.

I've also just run the tests on an A9 CPU and get the Oops on the nop
after the 'wfi' instruction test. So looks like the problem isn't
related to particular test cases.

-- 
Tixy


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05 10:10       ` Jon Medhurst (Tixy)
  2014-12-05 10:32         ` Wang Nan
@ 2014-12-05 14:59         ` Jon Medhurst (Tixy)
  2014-12-08  6:34           ` Wang Nan
  2014-12-05 19:57         ` Peter Maydell
  2 siblings, 1 reply; 19+ messages in thread
From: Jon Medhurst (Tixy) @ 2014-12-05 14:59 UTC (permalink / raw)
  To: Wang Nan
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On Fri, 2014-12-05 at 10:10 +0000, Jon Medhurst (Tixy) wrote:
[...]
> I'm worried because this whole optimised kprobes has some rather
> complicated interactions, e.g. can the background thread that changes
> breakpoints to jumps (or back again?) could occur at the same time
> another CPU is processing a kprobe that's been hit, or is in the process
> of removing a probe.

I think that is a plausible theory. We can have this situation...

1. CPU A executes a probe's 'breakpoint' instruction and the undefined
instruction exception handler is triggered.

2. CPU B is executing the kprobes optimisation thread and replaces the
'breakpoint' with a branch instruction.

3. CPU A reads the invalid instruction from memory and because this is
now the branch instruction it doesn't match
KPROBE_ARM_BREAKPOINT_INSTRUCTION which kprobes registered to handle.
This means the undefined instruction exception is treated as just that,
execution of an undefined instruction.

The above scenario is the exact reason why arch_disarm_kprobe is
implemented to always use stop_machine to modify the code and we need to
ensure the same happens with arch_optimize_kprobes.

-- 
Tixy



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05 10:10       ` Jon Medhurst (Tixy)
  2014-12-05 10:32         ` Wang Nan
  2014-12-05 14:59         ` Jon Medhurst (Tixy)
@ 2014-12-05 19:57         ` Peter Maydell
  2 siblings, 0 replies; 19+ messages in thread
From: Peter Maydell @ 2014-12-05 19:57 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: Wang Nan, masami.hiramatsu.pt, lizefan, Russell King,
	lkml - Kernel Mailing List, arm-mail-list

On 5 December 2014 at 10:10, Jon Medhurst (Tixy) <tixy@linaro.org> wrote:
> I don't know much about QEMU and have never used it, but I'm assuming
> QEMU doesn't make any attempt to simulate caches like the data cache,
> instruction cache, TLBs, branch predictor? Does it even emulate multiple
> CPUs with multiple host CPU threads? Basically, I very much doubt QEMU
> is a very good test of kernel code in general, and especially code that
> modifies code and has multiple cpus running in parallel.

You're generally correct here, yes. QEMU doesn't emulate caches
or TLBs or branch predictors, and we currently emulate SMP by
doing round-robin execution on a single host thread (though
we're working on that for performance reasons). There are also
a range of buggy-guest-code conditions (alignment faults, for
instance) which we don't emulate. I tend to think of QEMU's
overall philosophy as "run known-good code quickly" rather
than "diagnose problems in buggy code". So it's definitely wise
to test complicated kernel code like this on real hardware
(though of course QEMU may be very helpful in speeding up the
development cycle compared to h/w).

thanks
-- PMM

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32
  2014-12-05 14:59         ` Jon Medhurst (Tixy)
@ 2014-12-08  6:34           ` Wang Nan
  0 siblings, 0 replies; 19+ messages in thread
From: Wang Nan @ 2014-12-08  6:34 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: masami.hiramatsu.pt, linux, lizefan, linux-kernel, linux-arm-kernel

On 2014/12/5 22:59, Jon Medhurst (Tixy) wrote:
> On Fri, 2014-12-05 at 10:10 +0000, Jon Medhurst (Tixy) wrote:
> [...]
>> I'm worried because this whole optimised kprobes has some rather
>> complicated interactions, e.g. can the background thread that changes
>> breakpoints to jumps (or back again?) could occur at the same time
>> another CPU is processing a kprobe that's been hit, or is in the process
>> of removing a probe.
> 
> I think that is a plausible theory. We can have this situation...
> 
> 1. CPU A executes a probe's 'breakpoint' instruction and the undefined
> instruction exception handler is triggered.
> 
> 2. CPU B is executing the kprobes optimisation thread and replaces the
> 'breakpoint' with a branch instruction.
> 
> 3. CPU A reads the invalid instruction from memory and because this is
> now the branch instruction it doesn't match
> KPROBE_ARM_BREAKPOINT_INSTRUCTION which kprobes registered to handle.
> This means the undefined instruction exception is treated as just that,
> execution of an undefined instruction.
> 

I confirmed your theory by printing the buggy instruction:

...
[  474.824206] subls	r9,  r9, r14, lsr r7	@ 9049973e
[  476.954206] subge	r10, r11, r14, asr r7	@ a04ba75e
[  479.014206] sublt	r11, r11, r14, asr r7	@ b04bb75e
[  479.194212] undefined instruction: pc=bf001bbc, instruction=ea01187f
[  479.290190] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
[  479.370533] Modules linked in: test_kprobes(+)
[  479.423990] CPU: 10 PID: 1410 Comm: insmod Not tainted 3.10.53-HULK2+ #31
[  479.505377] task: c42b72c0 ti: ed4f8000 task.ti: ed4f8000
[  479.570189] PC is at kprobe_arm_test_cases+0x122c/0xfeed [test_kprobes]
...

ea01187f is a branch instruction.

Please help me to review my v14 patch series:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/309236.html

In which I fix it by wrapping __arch_optimize_kprobes() using stop_machine().


> The above scenario is the exact reason why arch_disarm_kprobe is
> implemented to always use stop_machine to modify the code and we need to
> ensure the same happens with arch_optimize_kprobes.
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2014-12-08  6:35 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-04  5:32 [PATCH v12 0/7] ARM: kprobes: OPTPROBES and other improvements Wang Nan
2014-12-04  5:34 ` [PATCH v12 1/7] ARM: probes: move all probe code to dedicate directory Wang Nan
2014-12-04  5:35 ` [PATCH v12 2/7] ARM: kprobes: introduces checker Wang Nan
2014-12-04  5:35 ` [PATCH v12 3/7] ARM: kprobes: collects stack consumption for store instructions Wang Nan
2014-12-04  5:35 ` [PATCH v12 4/7] ARM: kprobes: disallow probing stack consuming instructions Wang Nan
2014-12-04  5:35 ` [PATCH v12 5/7] ARM: kprobes: Add test cases for " Wang Nan
2014-12-04 16:22   ` Jon Medhurst (Tixy)
2014-12-04  5:35 ` [PATCH v12 6/7] kprobes: Pass the original kprobe for preparing optimized kprobe Wang Nan
2014-12-04 16:28   ` Jon Medhurst (Tixy)
2014-12-04  5:36 ` [PATCH v12 7/7] ARM: kprobes: enable OPTPROBES for ARM 32 Wang Nan
2014-12-04 16:21   ` Jon Medhurst (Tixy)
2014-12-05  3:38     ` Wang Nan
2014-12-05 10:10       ` Jon Medhurst (Tixy)
2014-12-05 10:32         ` Wang Nan
2014-12-05 10:48           ` Jon Medhurst (Tixy)
2014-12-05 14:59         ` Jon Medhurst (Tixy)
2014-12-08  6:34           ` Wang Nan
2014-12-05 19:57         ` Peter Maydell
2014-12-04 18:29   ` Russell King - ARM Linux

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).