[PATCH ftrace/for-next v5 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[PATCH ftrace/for-next v5 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[Masami Hiramatsu]

Hi,

Here is the 5th version of the series of patches which introduces
IPMODIFY flag for ftrace_ops to detect conflicts of ftrace users
who can modify regs->ip in their handler.

The previous version is here;
 https://lkml.org/lkml/2014/7/28/13

This version is basically an update of previous version to the
latest ftrace tree, and adding a test code to selftest.

Currently, only kprobes can change the regs->ip in the handler,
but recently kpatch is also want to change it. Moreover, since
the ftrace itself exported to modules, it might be considerable
senario.

Here we talked on github.
 https://github.com/dynup/kpatch/issues/47

To protect modified regs-ip from each other, this series
introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
the flag can be set on each function entry location. If there
is someone who already reserve regs->ip on target function
entry, ftrace_set_filter_ip or register_ftrace_function will
return -EBUSY. Users must handle that.
The ftrace_ops with IPMODIFY flag requires at least one
entry for filter hash, and its notrace_hash must be empty,
because the IPMODIFY action is very address sensitve and
user must consider the ip address.

The 3rd patch adds a special reservation of IPMODIFY on the
jprobed address, since it is the only user who will change
the regs->ip. Other kprobes do not change it anymore. 

Thank you,

---

Masami Hiramatsu (5):
      kprobes/ftrace: Recover original IP if pre_handler doesn't change it
      ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict
      kprobes: Add IPMODIFY flag to kprobe_ftrace_ops
      kprobes: Set IPMODIFY flag only if the probe can change regs->ip
      kselftest,ftrace: Add ftrace IPMODIFY flag test


 Documentation/kprobes.txt                          |   12 +
 Documentation/trace/ftrace.txt                     |    5 +
 arch/x86/kernel/kprobes/ftrace.c                   |    9 +
 include/linux/ftrace.h                             |   16 ++
 kernel/kprobes.c                                   |  123 +++++++++++++--
 kernel/trace/ftrace.c                              |  142 +++++++++++++++++
 tools/testing/selftests/ftrace/Makefile            |   11 +
 tools/testing/selftests/ftrace/ipmodify/Makefile   |   15 ++
 tools/testing/selftests/ftrace/ipmodify/ipmodify.c |  168 ++++++++++++++++++++
 .../selftests/ftrace/ipmodify/run_ipmodify.sh      |    6 +
 10 files changed, 478 insertions(+), 29 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/Makefile
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/ipmodify.c
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/run_ipmodify.sh

--

[PATCH ftrace/for-next v5 1/5] kprobes/ftrace: Recover original IP if pre_handler doesn't change it

[Masami Hiramatsu]

Recover original IP register if the pre_handler doesn't change it.
Since current kprobes doesn't expect that another ftrace handler
may change regs->ip, it sets kprobe.addr + MCOUNT_INSN_SIZE to
regs->ip and returns to ftrace.
This seems wrong behavior since kprobes can recover regs->ip
and safely pass it to another handler.

This adds a code which recovers original regs->ip passed from
ftrace right before returning to ftrace, so that another ftrace
user can change regs->ip.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---
Changes in v4:
 - Fix description and remove bugfix tag.
---
 arch/x86/kernel/kprobes/ftrace.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 717b02a..5f8f0b3 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -27,7 +27,7 @@
 
 static nokprobe_inline
 int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
-		      struct kprobe_ctlblk *kcb)
+		      struct kprobe_ctlblk *kcb, unsigned long orig_ip)
 {
 	/*
 	 * Emulate singlestep (and also recover regs->ip)
@@ -39,6 +39,8 @@ int __skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 		p->post_handler(p, regs, 0);
 	}
 	__this_cpu_write(current_kprobe, NULL);
+	if (orig_ip)
+		regs->ip = orig_ip;
 	return 1;
 }
 
@@ -46,7 +48,7 @@ int skip_singlestep(struct kprobe *p, struct pt_regs *regs,
 		    struct kprobe_ctlblk *kcb)
 {
 	if (kprobe_ftrace(p))
-		return __skip_singlestep(p, regs, kcb);
+		return __skip_singlestep(p, regs, kcb, 0);
 	else
 		return 0;
 }
@@ -71,13 +73,14 @@ void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 	if (kprobe_running()) {
 		kprobes_inc_nmissed_count(p);
 	} else {
+		unsigned long orig_ip = regs->ip;
 		/* Kprobe handler expects regs->ip = ip + 1 as breakpoint hit */
 		regs->ip = ip + sizeof(kprobe_opcode_t);
 
 		__this_cpu_write(current_kprobe, p);
 		kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 		if (!p->pre_handler || !p->pre_handler(p, regs))
-			__skip_singlestep(p, regs, kcb);
+			__skip_singlestep(p, regs, kcb, orig_ip);
 		/*
 		 * If pre_handler returns !0, it sets regs->ip and
 		 * resets current kprobe.

[PATCH ftrace/for-next v5 2/5] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Masami Hiramatsu]

Introduce FTRACE_OPS_FL_IPMODIFY to avoid conflict among
ftrace users who may modify regs->ip to change the execution
path. If two or more users modify the regs->ip on the same
function entry, one of them will be broken. So they must add
IPMODIFY flag and make sure that ftrace_set_filter_ip() succeeds.

Note that ftrace doesn't allow ftrace_ops which has IPMODIFY
flag to have notrace hash, and the ftrace_ops must have a
filter hash (so that the ftrace_ops can hook only specific
entries), because it strongly depends on the address and
must be allowed for only few selected functions.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>

---
Changes in v5:
 - Update to the latest ftrace tree.

Changes in v4:
 - Split kprobe's part from this patch.
 - Add no-notrace hash check.
 - Reject if the filter_hash is empty. Since IPMODIFY action
   is very address sensitive, empty hash (means hooking all
   functions) must not be allowed.

Changes in v3:
 - Update for the latest ftrace/core.
 - Add a comment about FTRACE_OPS_FL_* attribute flags.
 - Don't check FTRACE_OPS_FL_SAVE_REGS in
   __ftrace_hash_update_ipmodify().
 - Fix comments.

Changes in v2:
 - Add a description how __ftrace_hash_update_ipmodify() will
   handle the given hashes (NULL and EMPTY_HASH cases).
 - Clear FTRACE_OPS_FL_ENABLED after calling
   __unregister_ftrace_function() in error path.
---
 Documentation/trace/ftrace.txt |    5 +
 include/linux/ftrace.h         |   16 ++++-
 kernel/trace/ftrace.c          |  142 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 159 insertions(+), 4 deletions(-)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index 4da4261..f10f5f5 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -234,6 +234,11 @@ of ftrace. Here is a list of some of the key files:
 	will be displayed on the same line as the function that
 	is returning registers.
 
+	If the callback registered to be traced by a function with
+	the "ip modify" attribute (thus the regs->ip can be changed),
+	an 'I' will be displayed on the same line as the function that
+	can be overridden.
+
   function_profile_enabled:
 
 	When set it will enable all functions with either the function
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 662697b..4f52023 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -61,6 +61,11 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
 /*
  * FTRACE_OPS_FL_* bits denote the state of ftrace_ops struct and are
  * set in the flags member.
+ * CONTROL, SAVE_REGS, SAVE_REGS_IF_SUPPORTED, RECURSION_SAFE, STUB and
+ * IPMODIFY are a kind of attribute flags which can be set only before
+ * registering the ftrace_ops, and can not be modified while registered.
+ * Changing those attribute flags after regsitering ftrace_ops will
+ * cause unexpected results.
  *
  * ENABLED - set/unset when ftrace_ops is registered/unregistered
  * DYNAMIC - set when ftrace_ops is registered to denote dynamically
@@ -94,6 +99,10 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  * ADDING  - The ops is in the process of being added.
  * REMOVING - The ops is in the process of being removed.
  * MODIFYING - The ops is in the process of changing its filter functions.
+ * IPMODIFY - The ops can modify the IP register. This can only be set with
+ *            SAVE_REGS. If another ops is already registered for any of the
+ *            functions that this ops will be registered for, then this ops
+ *            will fail to register or set_filter_ip.
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= 1 << 0,
@@ -108,6 +117,7 @@ enum {
 	FTRACE_OPS_FL_ADDING			= 1 << 9,
 	FTRACE_OPS_FL_REMOVING			= 1 << 10,
 	FTRACE_OPS_FL_MODIFYING			= 1 << 11,
+	FTRACE_OPS_FL_IPMODIFY			= 1 << 12,
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -297,6 +307,7 @@ extern int ftrace_nr_registered_ops(void);
  *  ENABLED - the function is being traced
  *  REGS    - the record wants the function to save regs
  *  REGS_EN - the function is set up to save regs.
+ *  IPMODIFY - the record allows for the IP address to be changed.
  *
  * When a new ftrace_ops is registered and wants a function to save
  * pt_regs, the rec->flag REGS is set. When the function has been
@@ -310,10 +321,11 @@ enum {
 	FTRACE_FL_REGS_EN	= (1UL << 29),
 	FTRACE_FL_TRAMP		= (1UL << 28),
 	FTRACE_FL_TRAMP_EN	= (1UL << 27),
+	FTRACE_FL_IPMODIFY	= (1UL << 26),
 };
 
-#define FTRACE_REF_MAX_SHIFT	27
-#define FTRACE_FL_BITS		5
+#define FTRACE_REF_MAX_SHIFT	26
+#define FTRACE_FL_BITS		6
 #define FTRACE_FL_MASKED_BITS	((1UL << FTRACE_FL_BITS) - 1)
 #define FTRACE_FL_MASK		(FTRACE_FL_MASKED_BITS << FTRACE_REF_MAX_SHIFT)
 #define FTRACE_REF_MAX		((1UL << FTRACE_REF_MAX_SHIFT) - 1)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index fb186b9..a80597c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1315,6 +1315,9 @@ ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
 static void
 ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
 
+static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
+				       struct ftrace_hash *new_hash);
+
 static int
 ftrace_hash_move(struct ftrace_ops *ops, int enable,
 		 struct ftrace_hash **dst, struct ftrace_hash *src)
@@ -1325,8 +1328,13 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
 	struct ftrace_hash *new_hash;
 	int size = src->count;
 	int bits = 0;
+	int ret;
 	int i;
 
+	/* Reject setting notrace hash on IPMODIFY ftrace_ops */
+	if (ops->flags & FTRACE_OPS_FL_IPMODIFY && !enable)
+		return -EINVAL;
+
 	/*
 	 * If the new source is empty, just free dst and assign it
 	 * the empty_hash.
@@ -1360,6 +1368,16 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
 	}
 
 update:
+	/* Make sure this can be applied if it is IPMODIFY ftrace_ops */
+	if (enable) {
+		/* IPMODIFY should be updated only when filter_hash updating */
+		ret = ftrace_hash_ipmodify_update(ops, new_hash);
+		if (ret < 0) {
+			free_ftrace_hash(new_hash);
+			return ret;
+		}
+	}
+
 	/*
 	 * Remove the current set, update the hash and add
 	 * them back.
@@ -1724,6 +1742,114 @@ static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
 	ftrace_hash_rec_update_modify(ops, filter_hash, 1);
 }
 
+/*
+ * Try to update IPMODIFY flag on each ftrace_rec. Return 0 if it is OK
+ * or no-needed to update, -EBUSY if it detects a conflict of the flag
+ * on a ftrace_rec, and -EINVAL if the new_hash tries to trace all recs.
+ * Note that old_hash and new_hash has below meanings
+ *  - If the hash is NULL, it hits all recs (if IPMODIFY is set, this is rejected)
+ *  - If the hash is EMPTY_HASH, it hits nothing
+ *  - Anything else hits the recs which match the hash entries.
+ */
+static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
+					 struct ftrace_hash *old_hash,
+					 struct ftrace_hash *new_hash)
+{
+	struct ftrace_page *pg;
+	struct dyn_ftrace *rec, *end = NULL;
+	int in_old, in_new;
+
+	/* Only update if the ops has been registered */
+	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
+		return 0;
+
+	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
+		return 0;
+
+	/*
+	 * Since the IPMODIFY is very address sensitive action, we do not allow
+	 * ftrace_ops to set all functions to new hash.
+	 */
+	if (!new_hash || !old_hash)
+		return -EINVAL;
+
+	/* Update rec->flags */
+	do_for_each_ftrace_rec(pg, rec) {
+		/* We need to update only differences of filter_hash */
+		in_old = !!ftrace_lookup_ip(old_hash, rec->ip);
+		in_new = !!ftrace_lookup_ip(new_hash, rec->ip);
+		if (in_old == in_new)
+			continue;
+
+		if (in_new) {
+			/* New entries must ensure no others are using it */
+			if (rec->flags & FTRACE_FL_IPMODIFY)
+				goto rollback;
+			rec->flags |= FTRACE_FL_IPMODIFY;
+		} else /* Removed entry */
+			rec->flags &= ~FTRACE_FL_IPMODIFY;
+	} while_for_each_ftrace_rec();
+
+	return 0;
+
+rollback:
+	end = rec;
+
+	/* Roll back what we did above */
+	do_for_each_ftrace_rec(pg, rec) {
+		if (rec == end)
+			goto err_out;
+
+		in_old = !!ftrace_lookup_ip(old_hash, rec->ip);
+		in_new = !!ftrace_lookup_ip(new_hash, rec->ip);
+		if (in_old == in_new)
+			continue;
+
+		if (in_new)
+			rec->flags &= ~FTRACE_FL_IPMODIFY;
+		else
+			rec->flags |= FTRACE_FL_IPMODIFY;
+	} while_for_each_ftrace_rec();
+
+err_out:
+	return -EBUSY;
+}
+
+static int ftrace_hash_ipmodify_enable(struct ftrace_ops *ops)
+{
+	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+
+	if (ftrace_hash_empty(hash))
+		hash = NULL;
+
+	return __ftrace_hash_update_ipmodify(ops, EMPTY_HASH, hash);
+}
+
+/* Disabling always succeeds */
+static void ftrace_hash_ipmodify_disable(struct ftrace_ops *ops)
+{
+	struct ftrace_hash *hash = ops->func_hash->filter_hash;
+
+	if (ftrace_hash_empty(hash))
+		hash = NULL;
+
+	__ftrace_hash_update_ipmodify(ops, hash, EMPTY_HASH);
+}
+
+static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
+				       struct ftrace_hash *new_hash)
+{
+	struct ftrace_hash *old_hash = ops->func_hash->filter_hash;
+
+	if (ftrace_hash_empty(old_hash))
+		old_hash = NULL;
+
+	if (ftrace_hash_empty(new_hash))
+		new_hash = NULL;
+
+	return __ftrace_hash_update_ipmodify(ops, old_hash, new_hash);
+}
+
 static void print_ip_ins(const char *fmt, unsigned char *p)
 {
 	int i;
@@ -2352,6 +2478,15 @@ static int ftrace_startup(struct ftrace_ops *ops, int command)
 	 */
 	ops->flags |= FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_ADDING;
 
+	ret = ftrace_hash_ipmodify_enable(ops);
+	if (ret < 0) {
+		/* Rollback registration process */
+		__unregister_ftrace_function(ops);
+		ftrace_start_up--;
+		ops->flags &= ~FTRACE_OPS_FL_ENABLED;
+		return ret;
+	}
+
 	ftrace_hash_rec_enable(ops, 1);
 
 	ftrace_startup_enable(command);
@@ -2380,6 +2515,8 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 	 */
 	WARN_ON_ONCE(ftrace_start_up < 0);
 
+	/* Disabling ipmodify never fails */
+	ftrace_hash_ipmodify_disable(ops);
 	ftrace_hash_rec_disable(ops, 1);
 
 	ops->flags &= ~FTRACE_OPS_FL_ENABLED;
@@ -2954,9 +3091,10 @@ static int t_show(struct seq_file *m, void *v)
 
 	seq_printf(m, "%ps", (void *)rec->ip);
 	if (iter->flags & FTRACE_ITER_ENABLED) {
-		seq_printf(m, " (%ld)%s",
+		seq_printf(m, " (%ld)%s%s",
 			   ftrace_rec_count(rec),
-			   rec->flags & FTRACE_FL_REGS ? " R" : "  ");
+			   rec->flags & FTRACE_FL_REGS ? " R" : "  ",
+			   rec->flags & FTRACE_FL_IPMODIFY ? " I" : "  ");
 		if (rec->flags & FTRACE_FL_TRAMP_EN) {
 			struct ftrace_ops *ops;
 

[PATCH ftrace/for-next v5 3/5] kprobes: Add IPMODIFY flag to kprobe_ftrace_ops

[Masami Hiramatsu]

Add FTRACE_OPS_FL_IPMODIFY flag to kprobe_ftrace_ops
since kprobes can changes regs->ip.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 3995f54..831978c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -915,7 +915,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 #ifdef CONFIG_KPROBES_ON_FTRACE
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
-	.flags = FTRACE_OPS_FL_SAVE_REGS,
+	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
 };
 static int kprobe_ftrace_enabled;
 

[PATCH ftrace/for-next v5 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Masami Hiramatsu]

Set FTRACE_OPS_FL_IPMODIFY flag only for the probes which can change
regs->ip, which has kprobe->break_handler.
Currently we can not put jprobe and another ftrace handler which
changes regs->ip on the same function because all kprobes have
FTRACE_OPS_FL_IPMODIFY flag. This removes FTRACE_OPS_FL_IPMODIFY
flag from kprobes and only when the user uses jprobe (or the
kprobe.break_handler != NULL) we add additinal ftrace_ops with
FTRACE_OPS_FL_IPMODIFY on target function.

Note about the implementation: This uses a dummy ftrace_ops to
reserve IPMODIFY flag on the given ftrace address, for the case
that we have a enabled kprobe on a function entry and a jprobe
is added on the same point. In that case, we already have a
ftrace_ops without IPMODIFY flag on the entry, and we have to
add another ftrace_ops with IPMODIFY on the same address.
If we put a same handler on both ftrace_ops, the handler can
be called twice on that entry until the first one is removed.
This means that the kprobe and the jprobe are called twice too,
and that will not what kprobes expected.
Thus I added a dummy ftrace_ops just for reserving IPMODIFY flag.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
Changes in v4:
 - Increment refcounter after succeeded to register ftrace_ops.

Changes in v3:
 - Update __ftrace_add/remove_filter_ip() according to
   Namhyng's comments (thanks!)
 - Split out regs->ip recovering code from this patch.
---
 Documentation/kprobes.txt |   12 ++--
 kernel/kprobes.c          |  125 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 114 insertions(+), 23 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 4bbeca8..177f051 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -264,15 +264,13 @@ stop-machine method that ksplice uses for supporting a CONFIG_PREEMPT=y
 kernel.
 
 NOTE for geeks:
-The jump optimization changes the kprobe's pre_handler behavior.
-Without optimization, the pre_handler can change the kernel's execution
+The jump optimization (and ftrace-based kprobes) changes the kprobe's
+pre_handler behavior.
+Without optimizations, the pre_handler can change the kernel's execution
 path by changing regs->ip and returning 1.  However, when the probe
 is optimized, that modification is ignored.  Thus, if you want to
-tweak the kernel's execution path, you need to suppress optimization,
-using one of the following techniques:
-- Specify an empty function for the kprobe's post_handler or break_handler.
- or
-- Execute 'sysctl -w debug.kprobes_optimization=n'
+tweak the kernel's execution path, you need to suppress optimization or
+notify your handler will modify regs->ip by setting p->break_handler.
 
 1.5 Blacklist
 
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 831978c..4b4b7c5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -915,10 +915,93 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 #ifdef CONFIG_KPROBES_ON_FTRACE
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
-	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
+	.flags = FTRACE_OPS_FL_SAVE_REGS,
 };
 static int kprobe_ftrace_enabled;
 
+static void kprobe_ftrace_stub(unsigned long a0, unsigned long a1,
+			struct ftrace_ops *op, struct pt_regs *regs)
+{
+	/* Do nothing. This is just a dummy handler */
+}
+
+/* This is only for checking conflict with other ftrace users */
+static struct ftrace_ops kprobe_ipmod_ftrace_ops __read_mostly = {
+	.func = kprobe_ftrace_stub,
+	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
+};
+static int kprobe_ipmod_ftrace_enabled;
+
+static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip,
+				  int *ref)
+{
+	int ret;
+
+	/* Try to set given ip to filter */
+	ret = ftrace_set_filter_ip(ops, ip, 0, 0);
+	if (ret < 0)
+		return ret;
+
+	if (*ref == 0) {
+		ret = register_ftrace_function(ops);
+		if (ret < 0) {
+			/* Rollback the filter */
+			ftrace_set_filter_ip(ops, ip, 1, 0);
+			goto out;
+		}
+	}
+	(*ref)++;
+
+out:
+	return ret;
+}
+
+static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long ip,
+				     int *ref)
+{
+	int ret;
+
+	if (*ref == 1) {
+		ret = unregister_ftrace_function(ops);
+		if (ret < 0)
+			return ret;
+		/*Ignore failure, because it is already unregistered */
+		ftrace_set_filter_ip(ops, ip, 1, 0);
+	} else {
+		/* Try to remove given ip to filter */
+		ret = ftrace_set_filter_ip(ops, ip, 1, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	(*ref)--;
+
+	return 0;
+}
+
+static int try_reserve_ftrace_ipmodify(struct kprobe *p)
+{
+	if (!p->break_handler)
+		return 0;
+
+	return __ftrace_add_filter_ip(&kprobe_ipmod_ftrace_ops,
+				      (unsigned long)p->addr,
+				      &kprobe_ipmod_ftrace_enabled);
+}
+
+static void release_ftrace_ipmodify(struct kprobe *p)
+{
+	int ret;
+
+	if (p->break_handler) {
+		ret = __ftrace_remove_filter_ip(&kprobe_ipmod_ftrace_ops,
+						(unsigned long)p->addr,
+						&kprobe_ipmod_ftrace_enabled);
+		WARN(ret < 0, "Failed to release ftrace at %p (%d)\n",
+		     p->addr, ret);
+	}
+}
+
 /* Must ensure p->addr is really on ftrace */
 static int prepare_kprobe(struct kprobe *p)
 {
@@ -933,14 +1016,10 @@ static void arm_kprobe_ftrace(struct kprobe *p)
 {
 	int ret;
 
-	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
-				   (unsigned long)p->addr, 0, 0);
+	ret = __ftrace_add_filter_ip(&kprobe_ftrace_ops,
+				     (unsigned long)p->addr,
+				     &kprobe_ftrace_enabled);
 	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
-	kprobe_ftrace_enabled++;
-	if (kprobe_ftrace_enabled == 1) {
-		ret = register_ftrace_function(&kprobe_ftrace_ops);
-		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
-	}
 }
 
 /* Caller must lock kprobe_mutex */
@@ -948,17 +1027,16 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 {
 	int ret;
 
-	kprobe_ftrace_enabled--;
-	if (kprobe_ftrace_enabled == 0) {
-		ret = unregister_ftrace_function(&kprobe_ftrace_ops);
-		WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
-	}
-	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
-			   (unsigned long)p->addr, 1, 0);
-	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+	ret = __ftrace_remove_filter_ip(&kprobe_ftrace_ops,
+					(unsigned long)p->addr,
+					&kprobe_ftrace_enabled);
+	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n",
+	     p->addr, ret);
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
+#define try_reserve_ftrace_ipmodify(p)	(0)
+#define release_ftrace_ipmodify(p)	do {} while (0)
 #define arm_kprobe_ftrace(p)	do {} while (0)
 #define disarm_kprobe_ftrace(p)	do {} while (0)
 #endif
@@ -1502,6 +1580,14 @@ int register_kprobe(struct kprobe *p)
 	mutex_lock(&kprobe_mutex);
 
 	old_p = get_kprobe(p->addr);
+
+	/* Try to reserve ftrace ipmodify if needed */
+	if (kprobe_ftrace(p) && (!old_p || !old_p->break_handler)) {
+		ret = try_reserve_ftrace_ipmodify(p);
+		if (ret < 0)
+			goto out_noreserve;
+	}
+
 	if (old_p) {
 		/* Since this may unoptimize old_p, locking text_mutex. */
 		ret = register_aggr_kprobe(old_p, p);
@@ -1525,6 +1611,9 @@ int register_kprobe(struct kprobe *p)
 	try_to_optimize_kprobe(p);
 
 out:
+	if (ret < 0 && kprobe_ftrace(p))
+		release_ftrace_ipmodify(p);
+out_noreserve:
 	mutex_unlock(&kprobe_mutex);
 
 	if (probed_mod)
@@ -1639,6 +1728,10 @@ static void __unregister_kprobe_bottom(struct kprobe *p)
 {
 	struct kprobe *ap;
 
+	/* Release reserved ftrace ipmodify if needed */
+	if (kprobe_ftrace(p))
+		release_ftrace_ipmodify(p);
+
 	if (list_empty(&p->list))
 		/* This is an independent kprobe */
 		arch_remove_kprobe(p);

[PATCH ftrace/for-next v5 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

Add ftrace IPMODIFY flag test to selftest/ftrace. The
test checks simple ftrace handler insertion and
combinations of ftrace, kprobe, and jprobe.
This test requires kernel build tree to build a test
kernel module and root privilage to run.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

---
Changes in v5:
 - Add this test to kselftest.
---
 tools/testing/selftests/ftrace/Makefile            |   11 +
 tools/testing/selftests/ftrace/ipmodify/Makefile   |   15 ++
 tools/testing/selftests/ftrace/ipmodify/ipmodify.c |  168 ++++++++++++++++++++
 .../selftests/ftrace/ipmodify/run_ipmodify.sh      |    6 +
 4 files changed, 200 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/Makefile
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/ipmodify.c
 create mode 100644 tools/testing/selftests/ftrace/ipmodify/run_ipmodify.sh

diff --git a/tools/testing/selftests/ftrace/Makefile b/tools/testing/selftests/ftrace/Makefile
index 76cc9f1..69f0b1a 100644
--- a/tools/testing/selftests/ftrace/Makefile
+++ b/tools/testing/selftests/ftrace/Makefile
@@ -1,7 +1,18 @@
+TARGETS += ipmodify
+
 all:
+	for TARGET in $(TARGETS); do \
+		make -C $$TARGET; \
+	done;
 
 run_tests:
 	@/bin/sh ./ftracetest || echo "ftrace selftests: [FAIL]"
+	for TARGET in $(TARGETS); do \
+		make -C $$TARGET run_tests; \
+	done;
 
 clean:
 	rm -rf logs/*
+	for TARGET in $(TARGETS); do \
+		make -C $$TARGET clean; \
+	done;
diff --git a/tools/testing/selftests/ftrace/ipmodify/Makefile b/tools/testing/selftests/ftrace/ipmodify/Makefile
new file mode 100644
index 0000000..416e9a8
--- /dev/null
+++ b/tools/testing/selftests/ftrace/ipmodify/Makefile
@@ -0,0 +1,15 @@
+BUILDDIR = /lib/modules/$(shell uname -r)/build
+HERE = $(abspath ./)
+
+obj-m := ipmodify.o
+
+ipmodify.ko: ipmodify.c
+	$(MAKE) -C $(BUILDDIR) M=$(HERE) $@
+
+all: ipmodify.ko
+
+run_tests:
+	@/bin/sh ./run_ipmodify.sh || echo "ftrace ipmodify test: [FAIL]"
+
+clean:
+	$(RM) -Rf .*.o.cmd .*.ko.cmd .tmp_versions *.o *.ko *.mod.c modules.order Module.symvers
diff --git a/tools/testing/selftests/ftrace/ipmodify/ipmodify.c b/tools/testing/selftests/ftrace/ipmodify/ipmodify.c
new file mode 100644
index 0000000..88677f4
--- /dev/null
+++ b/tools/testing/selftests/ftrace/ipmodify/ipmodify.c
@@ -0,0 +1,168 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/ftrace.h>
+#include <linux/kprobes.h>
+
+/* Testing the IPMODIFY flag by using kprobe, jprobe, and ftrace handler */
+
+static __used int ipmodify_target_function(int a1, int a2, int a3)
+{
+	return a1 + a2 + a3;
+}
+
+/* Kprobe pre handler (IPMODIFY bit is NOT set) */
+static int kprobe_test_handler(struct kprobe *kp, struct pt_regs *regs)
+{
+	return 0;
+}
+
+static struct kprobe test_kp, __test_kp = {
+	.pre_handler = kprobe_test_handler,
+	.addr = (void *)ipmodify_target_function,
+};
+
+/* Jprobe entry handler (IPMODIFY should be set) */
+static void jprobe_test_handler(int a1, int a2, int a3)
+{
+	jprobe_return();
+}
+
+static struct jprobe test_jp, __test_jp = {
+	.entry = jprobe_test_handler,
+	.kp = {
+		.addr = (void *)ipmodify_target_function,
+	},
+};
+
+/* Ftrace handler (with IPMODIFY flag) */
+static void ftrace_ipmodify_handler(unsigned long a0, unsigned long a1,
+				struct ftrace_ops *op, struct pt_regs *regs)
+{
+	return;
+}
+
+static struct ftrace_ops test_ops __read_mostly, __test_ops = {
+	.func = ftrace_ipmodify_handler,
+	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
+};
+
+static void cleanup_probes(void)
+{
+	unregister_kprobe(&test_kp);
+	unregister_jprobe(&test_jp);
+	if (test_ops.flags & FTRACE_OPS_FL_ENABLED)
+		unregister_ftrace_function(&test_ops);
+	test_kp = __test_kp;
+	test_jp = __test_jp;
+}
+
+
+
+/* We'll test various probes on a target function. */
+static int __init ipmodify_init(void)
+{
+	int ret;
+
+	test_kp = __test_kp;
+	test_jp = __test_jp;
+	test_ops = __test_ops;
+
+	/* Setup ftrace filter */
+	ret = ftrace_set_filter_ip(&test_ops,
+				 (unsigned long)ipmodify_target_function, 0, 0);
+	if (ret < 0) {
+		ret = pr_err("IPMODIFY test: ftrace_set_filter_ip\t[FAILED]\n");
+		goto err;
+	}
+
+	ret = -EINVAL;
+
+/* For the test case which should pass */
+#define EXP_OK(cond)		\
+	if ((cond) < 0) {		\
+		pr_cont("[FAIL]\n\t" #cond " is failed\n");	\
+		goto err;	\
+	}
+/* For the test case which should fail */
+#define EXP_NG(cond)		\
+	if ((cond) >= 0) {	\
+		pr_cont("[XPASS]\n\t" #cond " is unexpectedly passed\n");\
+		goto err;	\
+	}
+
+	/* Case 1 */
+	pr_info("IPMODIFY test: ipmodify only\t");
+	EXP_OK(register_ftrace_function(&test_ops))
+	ipmodify_target_function(1, 2, 3);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 2 */
+	pr_info("IPMODIFY test: kprobe->ipmodify\t");
+	EXP_OK(register_kprobe(&test_kp))
+	EXP_OK(register_ftrace_function(&test_ops))
+	ipmodify_target_function(2, 3, 4);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 3 */
+	pr_info("IPMODIFY test: jprobe->ipmodify(NG)\t");
+	EXP_OK(register_jprobe(&test_jp))
+	EXP_NG(register_ftrace_function(&test_ops))
+	ipmodify_target_function(3, 4, 5);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 4 */
+	pr_info("IPMODIFY test: ipmodify->kprobe\t");
+	EXP_OK(register_ftrace_function(&test_ops))
+	EXP_OK(register_kprobe(&test_kp))
+	ipmodify_target_function(4, 5, 6);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 5 */
+	pr_info("IPMODIFY test: ipmodify->jprobe(NG)\t");
+	EXP_OK(register_ftrace_function(&test_ops))
+	EXP_NG(register_jprobe(&test_jp))
+	ipmodify_target_function(5, 6, 7);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 6 */
+	pr_info("IPMODIFY test: kprobe->jprobe->ipmodify(NG)\t");
+	EXP_OK(register_kprobe(&test_kp))
+	EXP_OK(register_jprobe(&test_jp))
+	EXP_NG(register_ftrace_function(&test_ops))
+	ipmodify_target_function(6, 7, 8);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 7 */
+	pr_info("IPMODIFY test: kprobe->ipmodify->jprobe(NG)\t");
+	EXP_OK(register_kprobe(&test_kp))
+	EXP_OK(register_ftrace_function(&test_ops))
+	EXP_NG(register_jprobe(&test_jp))
+	ipmodify_target_function(7, 8, 9);
+	pr_cont("[PASS]\n");
+	cleanup_probes();
+
+	/* Case 8 */
+	pr_info("IPMODIFY test: setting notrace filter with ipmodify(NG)\t");
+	EXP_NG(ftrace_set_notrace(&test_ops, "do_fork", 0, 0))
+	pr_cont("[PASS]\n");
+
+	return 0;
+err:
+	cleanup_probes();
+	return ret;
+}
+
+void ipmodify_exit(void)
+{
+	return;
+}
+
+module_init(ipmodify_init)
+module_exit(ipmodify_exit)
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/ftrace/ipmodify/run_ipmodify.sh b/tools/testing/selftests/ftrace/ipmodify/run_ipmodify.sh
new file mode 100644
index 0000000..4878d9d
--- /dev/null
+++ b/tools/testing/selftests/ftrace/ipmodify/run_ipmodify.sh
@@ -0,0 +1,6 @@
+#!/bin/sh
+set -e
+insmod ipmodify.ko
+rmmod ipmodify.ko
+dmesg | tail -n 8
+

Re: [PATCH ftrace/for-next v5 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[Steven Rostedt]

On Thu, 09 Oct 2014 13:00:59 +0000
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Hi,
> 
> Here is the 5th version of the series of patches which introduces
> IPMODIFY flag for ftrace_ops to detect conflicts of ftrace users
> who can modify regs->ip in their handler.
> 
> The previous version is here;
>  https://lkml.org/lkml/2014/7/28/13
> 
> This version is basically an update of previous version to the
> latest ftrace tree, and adding a test code to selftest.
> 
> Currently, only kprobes can change the regs->ip in the handler,
> but recently kpatch is also want to change it. Moreover, since
> the ftrace itself exported to modules, it might be considerable
> senario.
> 
> Here we talked on github.
>  https://github.com/dynup/kpatch/issues/47
> 
> To protect modified regs-ip from each other, this series
> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
> the flag can be set on each function entry location. If there
> is someone who already reserve regs->ip on target function
> entry, ftrace_set_filter_ip or register_ftrace_function will
> return -EBUSY. Users must handle that.
> The ftrace_ops with IPMODIFY flag requires at least one
> entry for filter hash, and its notrace_hash must be empty,
> because the IPMODIFY action is very address sensitve and
> user must consider the ip address.
> 
> The 3rd patch adds a special reservation of IPMODIFY on the
> jprobed address, since it is the only user who will change
> the regs->ip. Other kprobes do not change it anymore. 
> 
> Thank you,
> 

Masami,

Thanks for sending this. I'm going to look at it after Dusseldorf. It's
too late to get it into 3.18, but it looks like a good fit for the work
I have for 3.19.

Just don't let me forget you sent this :-)  Even though I tagged it as
important, I'm sure I'll be tagging a lot of other emails as important
in the next week.

Also, my main test box has finally died. I ordered a new motherboard
(thanks Linus for the suggestion!) and unfortunately it is due to
arrive tomorrow. That's the same day I leave and I don't trust my wife
to install it for me ;-)

This means I can not do my tests that I like to run before adding to
linux-next.

-- Steve

Re: Re: [PATCH ftrace/for-next v5 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[Masami Hiramatsu]

(2014/10/10 0:21), Steven Rostedt wrote:
> On Thu, 09 Oct 2014 13:00:59 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Hi,
>>
>> Here is the 5th version of the series of patches which introduces
>> IPMODIFY flag for ftrace_ops to detect conflicts of ftrace users
>> who can modify regs->ip in their handler.
>>
>> The previous version is here;
>>  https://lkml.org/lkml/2014/7/28/13
>>
>> This version is basically an update of previous version to the
>> latest ftrace tree, and adding a test code to selftest.
>>
>> Currently, only kprobes can change the regs->ip in the handler,
>> but recently kpatch is also want to change it. Moreover, since
>> the ftrace itself exported to modules, it might be considerable
>> senario.
>>
>> Here we talked on github.
>>  https://github.com/dynup/kpatch/issues/47
>>
>> To protect modified regs-ip from each other, this series
>> introduces FTRACE_OPS_FL_IPMODIFY flag and ftrace now ensures
>> the flag can be set on each function entry location. If there
>> is someone who already reserve regs->ip on target function
>> entry, ftrace_set_filter_ip or register_ftrace_function will
>> return -EBUSY. Users must handle that.
>> The ftrace_ops with IPMODIFY flag requires at least one
>> entry for filter hash, and its notrace_hash must be empty,
>> because the IPMODIFY action is very address sensitve and
>> user must consider the ip address.
>>
>> The 3rd patch adds a special reservation of IPMODIFY on the
>> jprobed address, since it is the only user who will change
>> the regs->ip. Other kprobes do not change it anymore. 
>>
>> Thank you,
>>
> 
> Masami,
> 
> Thanks for sending this. I'm going to look at it after Dusseldorf. It's
> too late to get it into 3.18, but it looks like a good fit for the work
> I have for 3.19.

Yeah, I think there is no problem until someone tries to use
both ftrace and jprobe on the same target.

> Just don't let me forget you sent this :-)  Even though I tagged it as
> important, I'm sure I'll be tagging a lot of other emails as important
> in the next week.

OK, I'll ping after the event.

> Also, my main test box has finally died. I ordered a new motherboard
> (thanks Linus for the suggestion!) and unfortunately it is due to
> arrive tomorrow. That's the same day I leave and I don't trust my wife
> to install it for me ;-)

Oh, that is a bad timing...

> This means I can not do my tests that I like to run before adding to
> linux-next.

OK, so see you in next week :)

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com