[PATCH ftrace/core v6 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[PATCH ftrace/core v6 0/5] ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

[Masami Hiramatsu]

Hi,

Here is the 6th 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.

This version is basically an update of previous version to the
latest ftrace/core tree.

Currently, only kprobes can change the regs->ip in the handler,
but recent Kernel Live Patching also changes it.

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.

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 |  170 ++++++++++++++++++++
 .../selftests/ftrace/ipmodify/run_ipmodify.sh      |    6 +
 10 files changed, 480 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/core v6 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/core v6 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 v6:
 - Update to the latest ftrace tree (Trampoline).

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 7b2616f..93cf047 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
@@ -101,6 +106,10 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  *            The ftrace_ops trampoline can be set by the ftrace users, and
  *            in such cases the arch must not modify it. Only the arch ftrace
  *            core code should set this flag.
+ * 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,
@@ -116,6 +125,7 @@ enum {
 	FTRACE_OPS_FL_REMOVING			= 1 << 10,
 	FTRACE_OPS_FL_MODIFYING			= 1 << 11,
 	FTRACE_OPS_FL_ALLOC_TRAMP		= 1 << 12,
+	FTRACE_OPS_FL_IPMODIFY			= 1 << 13,
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -310,6 +320,7 @@ bool is_ftrace_trampoline(unsigned long addr);
  *  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
@@ -323,10 +334,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 fa0f36b..baa2c4d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1358,6 +1358,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)
@@ -1368,8 +1371,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.
@@ -1403,6 +1411,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.
@@ -1767,6 +1785,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;
@@ -2436,6 +2562,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);
@@ -2464,6 +2599,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;
@@ -3058,9 +3195,10 @@ static int t_show(struct seq_file *m, void *v)
 	if (iter->flags & FTRACE_ITER_ENABLED) {
 		struct ftrace_ops *ops = NULL;
 
-		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) {
 			ops = ftrace_find_tramp_ops_any(rec);
 			if (ops)

[PATCH ftrace/core v6 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/core v6 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 4227ec2..eb03efc 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/core v6 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 privilege to run.

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

---
Changes in v6:
 - Fix macros to avoid errors by checkpatch.pl.

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 |  170 ++++++++++++++++++++
 .../selftests/ftrace/ipmodify/run_ipmodify.sh      |    6 +
 4 files changed, 202 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..85213f8
--- /dev/null
+++ b/tools/testing/selftests/ftrace/ipmodify/ipmodify.c
@@ -0,0 +1,170 @@
+#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)
+{
+}
+
+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)							\
+	do {								\
+		if ((cond) < 0) {					\
+			pr_cont("[FAIL]\n\t" #cond " is failed\n");	\
+			goto err;					\
+		}							\
+	} while (0)
+/* For the test case which should fail */
+#define EXP_NG(cond)							\
+	do {								\
+		if ((cond) >= 0) {					\
+			pr_cont("[XPASS]\n\t" #cond " is unexpectedly passed\n");\
+			goto err;					\
+		}							\
+	} while (0)
+
+	/* 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)
+{
+}
+
+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/core v6 2/5] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Steven Rostedt]

On Fri, 21 Nov 2014 05:25:16 -0500
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

>  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 7b2616f..93cf047 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
> @@ -101,6 +106,10 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
>   *            The ftrace_ops trampoline can be set by the ftrace users, and
>   *            in such cases the arch must not modify it. Only the arch ftrace
>   *            core code should set this flag.
> + * 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.

It's blocked by any ops sharing the same function, or just another ops
with this flag set? The comment doesn't specify. The code looks like
the latter.


>   */
>  enum {
>  	FTRACE_OPS_FL_ENABLED			= 1 << 0,
> @@ -116,6 +125,7 @@ enum {
>  	FTRACE_OPS_FL_REMOVING			= 1 << 10,
>  	FTRACE_OPS_FL_MODIFYING			= 1 << 11,
>  	FTRACE_OPS_FL_ALLOC_TRAMP		= 1 << 12,
> +	FTRACE_OPS_FL_IPMODIFY			= 1 << 13,
>  };
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> @@ -310,6 +320,7 @@ bool is_ftrace_trampoline(unsigned long addr);
>   *  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
> @@ -323,10 +334,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 fa0f36b..baa2c4d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1358,6 +1358,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)
> @@ -1368,8 +1371,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.
> @@ -1403,6 +1411,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.
> @@ -1767,6 +1785,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

 is *a* very address sensitive action

The rest looks good.

-- Steve

> +	 * 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;
> @@ -2436,6 +2562,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);
> @@ -2464,6 +2599,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;
> @@ -3058,9 +3195,10 @@ static int t_show(struct seq_file *m, void *v)
>  	if (iter->flags & FTRACE_ITER_ENABLED) {
>  		struct ftrace_ops *ops = NULL;
>  
> -		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) {
>  			ops = ftrace_find_tramp_ops_any(rec);
>  			if (ops)
> 

Re: [PATCH ftrace/core v6 2/5] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Steven Rostedt]

On Fri, 21 Nov 2014 13:05:29 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 21 Nov 2014 05:25:16 -0500
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> > + * 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.
> 
> It's blocked by any ops sharing the same function, or just another ops
> with this flag set? The comment doesn't specify. The code looks like
> the latter.

I applied it and gave it the following updates to comments:

-- Steve

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 93cf0478f64e..ed501953f0b2 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -107,9 +107,9 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  *            in such cases the arch must not modify it. Only the arch ftrace
  *            core code should set this flag.
  * 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.
+ *            SAVE_REGS. If another ops with this flag set 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,
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 531e72a716c3..929a733d302e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1810,8 +1810,8 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 		return 0;
 
 	/*
-	 * Since the IPMODIFY is very address sensitive action, we do not allow
-	 * ftrace_ops to set all functions to new hash.
+	 * Since the IPMODIFY is a very address sensitive action, we do not
+	 * allow ftrace_ops to set all functions to new hash.
 	 */
 	if (!new_hash || !old_hash)
 		return -EINVAL;

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Steven Rostedt]

On Fri, 21 Nov 2014 05:25:30 -0500
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> 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 4227ec2..eb03efc 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 */
> +}

Feel free to just use ftrace_stub instead. That's what it's there for.


> +
> +/* 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;

Why the goto out, and not just return ret?

> +		}
> +	}
> +	(*ref)++;
> +
> +out:
> +	return ret;

Probably could just return 0 here.

Rest looks fine.

-- Steve

> +}
> +
> +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;
> +}
> +

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Fri, 21 Nov 2014 05:25:37 -0500
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> 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 privilege to run.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> ---

Can you make it so that make O=/path/to/build works?

I tried running this on my test box, which has /home/rostedt mounted
read only from my build box, and I get this:

[root@bxtest ftrace]# make O=/tmp
for TARGET in ipmodify; do \
	make -C $TARGET; \
done;
make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
  CC [M]  /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o
/bin/sh: ./scripts/recordmcount: Permission denied
make[5]: *** [/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o] Error 1
make[4]: *** [ipmodify.ko] Error 2
make[3]: *** [sub-make] Error 2
make[2]: *** [__sub-make] Error 2
make[2]: Leaving directory `/home/rostedt/work/git/nobackup/bxtest/trace'
make[1]: *** [ipmodify.ko] Error 2
make[1]: Leaving directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
make: *** [all] Error 2

-- Steve

Re: [PATCH ftrace/core v6 2/5] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Masami Hiramatsu]

(2014/11/22 3:05), Steven Rostedt wrote:
> On Fri, 21 Nov 2014 05:25:16 -0500
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>>  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 7b2616f..93cf047 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
>> @@ -101,6 +106,10 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
>>   *            The ftrace_ops trampoline can be set by the ftrace users, and
>>   *            in such cases the arch must not modify it. Only the arch ftrace
>>   *            core code should set this flag.
>> + * 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.
> 
> It's blocked by any ops sharing the same function, or just another ops
> with this flag set? The comment doesn't specify. The code looks like
> the latter.

Ah, right! IPMODIFY blocks another IPMODIFY-ed ops. So the comment should
be fixed.

> 
> 
>>   */
>>  enum {
>>  	FTRACE_OPS_FL_ENABLED			= 1 << 0,
>> @@ -116,6 +125,7 @@ enum {
>>  	FTRACE_OPS_FL_REMOVING			= 1 << 10,
>>  	FTRACE_OPS_FL_MODIFYING			= 1 << 11,
>>  	FTRACE_OPS_FL_ALLOC_TRAMP		= 1 << 12,
>> +	FTRACE_OPS_FL_IPMODIFY			= 1 << 13,
>>  };
>>  
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>> @@ -310,6 +320,7 @@ bool is_ftrace_trampoline(unsigned long addr);
>>   *  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
>> @@ -323,10 +334,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 fa0f36b..baa2c4d 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -1358,6 +1358,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)
>> @@ -1368,8 +1371,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.
>> @@ -1403,6 +1411,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.
>> @@ -1767,6 +1785,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
> 
>  is *a* very address sensitive action

Right.

> 
> The rest looks good.

Thanks!

> 
> -- Steve
> 
>> +	 * 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;
>> @@ -2436,6 +2562,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);
>> @@ -2464,6 +2599,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;
>> @@ -3058,9 +3195,10 @@ static int t_show(struct seq_file *m, void *v)
>>  	if (iter->flags & FTRACE_ITER_ENABLED) {
>>  		struct ftrace_ops *ops = NULL;
>>  
>> -		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) {
>>  			ops = ftrace_find_tramp_ops_any(rec);
>>  			if (ops)
>>
> 
> 


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

Re: [PATCH ftrace/core v6 2/5] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict

[Masami Hiramatsu]

(2014/11/22 4:43), Steven Rostedt wrote:
> On Fri, 21 Nov 2014 13:05:29 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> On Fri, 21 Nov 2014 05:25:16 -0500
>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>>> + * 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.
>>
>> It's blocked by any ops sharing the same function, or just another ops
>> with this flag set? The comment doesn't specify. The code looks like
>> the latter.
> 
> I applied it and gave it the following updates to comments:

Thanks!

This looks good to me :)

> 
> -- Steve
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 93cf0478f64e..ed501953f0b2 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -107,9 +107,9 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
>   *            in such cases the arch must not modify it. Only the arch ftrace
>   *            core code should set this flag.
>   * 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.
> + *            SAVE_REGS. If another ops with this flag set 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,
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 531e72a716c3..929a733d302e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1810,8 +1810,8 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
>  		return 0;
>  
>  	/*
> -	 * Since the IPMODIFY is very address sensitive action, we do not allow
> -	 * ftrace_ops to set all functions to new hash.
> +	 * Since the IPMODIFY is a very address sensitive action, we do not
> +	 * allow ftrace_ops to set all functions to new hash.
>  	 */
>  	if (!new_hash || !old_hash)
>  		return -EINVAL;
> 


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

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Masami Hiramatsu]

(2014/11/22 5:15), Steven Rostedt wrote:
> On Fri, 21 Nov 2014 05:25:30 -0500
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> 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 4227ec2..eb03efc 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 */
>> +}
> 
> Feel free to just use ftrace_stub instead. That's what it's there for.

Ah, I didn't know that. OK :)

>> +
>> +/* 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;
> 
> Why the goto out, and not just return ret?
> 
>> +		}
>> +	}
>> +	(*ref)++;
>> +
>> +out:
>> +	return ret;
> 
> Probably could just return 0 here.

Agreed.

> 
> Rest looks fine.

Thank you!

> 
> -- Steve
> 
>> +}
>> +
>> +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;
>> +}
>> +
> 


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

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/22 6:03), Steven Rostedt wrote:
> On Fri, 21 Nov 2014 05:25:37 -0500
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> 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 privilege to run.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> ---
> 
> Can you make it so that make O=/path/to/build works?

Hmm, I always use "O=builddir" option in my build script...

> 
> I tried running this on my test box, which has /home/rostedt mounted
> read only from my build box, and I get this:

My case is just to keep the source tree clean, so source tree is
still writable and builddir is under another directory in /home/<user>/.


> [root@bxtest ftrace]# make O=/tmp
> for TARGET in ipmodify; do \
> 	make -C $TARGET; \
> done;
> make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
> make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
>   CC [M]  /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o
> /bin/sh: ./scripts/recordmcount: Permission denied

Hmm, this error looks odd, not ENOENT but EPERM??
Could you run it again with V=1 ?

Thank you,

> make[5]: *** [/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o] Error 1
> make[4]: *** [ipmodify.ko] Error 2
> make[3]: *** [sub-make] Error 2
> make[2]: *** [__sub-make] Error 2
> make[2]: Leaving directory `/home/rostedt/work/git/nobackup/bxtest/trace'
> make[1]: *** [ipmodify.ko] Error 2
> make[1]: Leaving directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> make: *** [all] Error 2
> 
> -- Steve
> 


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

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Mon, 24 Nov 2014 11:50:06 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2014/11/22 6:03), Steven Rostedt wrote:
> > On Fri, 21 Nov 2014 05:25:37 -0500
> > Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> > 
> >> 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 privilege to run.
> >>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>
> >> ---
> > 
> > Can you make it so that make O=/path/to/build works?
> 
> Hmm, I always use "O=builddir" option in my build script...

But do you use O=builddir where builddir is where the kernel was built?
I don't.

Remember, I build on a server and install on a test box. The server
directory is read only. I can not have O= be the path to where the
kernel was built. I have to use a different directory.


> 
> > 
> > I tried running this on my test box, which has /home/rostedt mounted
> > read only from my build box, and I get this:
> 
> My case is just to keep the source tree clean, so source tree is
> still writable and builddir is under another directory in /home/<user>/.
> 
> 
> > [root@bxtest ftrace]# make O=/tmp
> > for TARGET in ipmodify; do \
> > 	make -C $TARGET; \
> > done;
> > make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> > make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
> > make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
> >   CC [M]  /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o
> > /bin/sh: ./scripts/recordmcount: Permission denied
> 
> Hmm, this error looks odd, not ENOENT but EPERM??
> Could you run it again with V=1 ?

Sure, here's the output:

[root@bxtest ftrace]# make V=1 O=/tmp
for TARGET in ipmodify; do \
	make -C $TARGET; \
done;
make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
make -C /home/rostedt/work/git/linux-trace.git O=/home/rostedt/work/git/nobackup/bxtest/trace/. ipmodify.ko
make -C /home/rostedt/work/git/nobackup/bxtest/trace KBUILD_SRC=/home/rostedt/work/git/linux-trace.git \
-f /home/rostedt/work/git/linux-trace.git/Makefile ipmodify.ko
test -e include/generated/autoconf.h -a -e include/config/auto.conf || (		\
echo >&2;							\
echo >&2 "  ERROR: Kernel configuration is invalid.";		\
echo >&2 "         include/generated/autoconf.h or include/config/auto.conf are missing.";\
echo >&2 "         Run 'make oldconfig && make prepare' on kernel src to fix it.";	\
echo >&2 ;							\
/bin/false)
mkdir -p /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.tmp_versions ; rm -f /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.tmp_versions/*
make KBUILD_MODULES=1   \
-f /home/rostedt/work/git/linux-trace.git/scripts/Makefile.build obj=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.o
  gcc -Wp,-MD,/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.ipmodify.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/4.7.2/include -I/home/rostedt/work/git/linux-trace.git/arch/x86/include -Iarch/x86/include/generated  -I/home/rostedt/work/git/linux-trace.git/include -Iinclude -I/home/rostedt/work/git/linux-trace.git/arch/x86/include/uapi -Iarch/x86/include/generated/uapi -I/home/rostedt/work/git/linux-trace.git/include/uapi -Iinclude/generated/uapi -include /home/rostedt/work/git/linux-trace.git/include/linux/kconfig.h   -I/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -m64 -mno-80387 -mno-fp-ret-in-387 -mtune=generic -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_AS_FXSAVEQ=1 -DCONFIG_AS_CRC32=1 -DCONFIG_AS_AVX=1 -DCONFIG_AS_AVX2=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fno-delete-null-pointer-checks -Os -Wno-maybe-uninitialized --param=allow-store-data-races=0 -Wframe-larger-than=2048 -fno-stack-protector -Wno-unused-but-set-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-var-tracking-assignments -g -pg -mfentry -DCC_USING_FENTRY -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes -DCC_HAVE_ASM_GOTO  -DMODULE  -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(ipmodify)"  -D"KBUILD_MODNAME=KBUILD_STR(ipmodify)" -c -o /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.c
  if [ "-pg" = "-pg" ]; then if [ /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o != "scripts/mod/empty.o" ]; then ./scripts/recordmcount  "/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o"; fi; fi;
/bin/sh: ./scripts/recordmcount: Permission denied
make[5]: *** [/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o] Error 1
make[4]: *** [ipmodify.ko] Error 2
make[3]: *** [sub-make] Error 2
make[2]: *** [__sub-make] Error 2
make[2]: Leaving directory `/home/rostedt/work/git/nobackup/bxtest/trace'
make[1]: *** [ipmodify.ko] Error 2
make[1]: Leaving directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
make: *** [all] Error 2


Thanks (on my way to bed),

-- Steve

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(CC'ed Shuah, since this is related to kselftest)

(2014/11/24 13:29), Steven Rostedt wrote:
> On Mon, 24 Nov 2014 11:50:06 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> (2014/11/22 6:03), Steven Rostedt wrote:
>>> On Fri, 21 Nov 2014 05:25:37 -0500
>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>
>>>> 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 privilege to run.
>>>>
>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>
>>>> ---
>>>
>>> Can you make it so that make O=/path/to/build works?
>>
>> Hmm, I always use "O=builddir" option in my build script...
> 
> But do you use O=builddir where builddir is where the kernel was built?
> I don't.

Hmm, yes, I always use kbin/linux-3.<arch>/ for the builddir of
ksrc/linux-3/ source tree.

> 
> Remember, I build on a server and install on a test box. The server
> directory is read only. I can not have O= be the path to where the
> kernel was built. I have to use a different directory.

Oh, I see. So your kernel is built on a directory which mounted as
readonly, and installed on the testbox.

>>> I tried running this on my test box, which has /home/rostedt mounted
>>> read only from my build box, and I get this:

Hmm, btw, would you mount it without "noexec" option?
I think only "ro" option doesn't prohibit executing scripts on it.

>>
>> My case is just to keep the source tree clean, so source tree is
>> still writable and builddir is under another directory in /home/<user>/.
>>
>>
>>> [root@bxtest ftrace]# make O=/tmp
>>> for TARGET in ipmodify; do \
>>> 	make -C $TARGET; \
>>> done;
>>> make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
>>> make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
>>> make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
>>>   CC [M]  /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o
>>> /bin/sh: ./scripts/recordmcount: Permission denied
>>
>> Hmm, this error looks odd, not ENOENT but EPERM??
>> Could you run it again with V=1 ?
> 
> Sure, here's the output:
> 
> [root@bxtest ftrace]# make V=1 O=/tmp
> for TARGET in ipmodify; do \
> 	make -C $TARGET; \
> done;
> make[1]: Entering directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> make -C /lib/modules/3.18.0-rc1-test+/build M=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.ko
> make[2]: Entering directory `/home/rostedt/work/git/nobackup/bxtest/trace'
> make -C /home/rostedt/work/git/linux-trace.git O=/home/rostedt/work/git/nobackup/bxtest/trace/. ipmodify.ko
> make -C /home/rostedt/work/git/nobackup/bxtest/trace KBUILD_SRC=/home/rostedt/work/git/linux-trace.git \
> -f /home/rostedt/work/git/linux-trace.git/Makefile ipmodify.ko
> test -e include/generated/autoconf.h -a -e include/config/auto.conf || (		\
> echo >&2;							\
> echo >&2 "  ERROR: Kernel configuration is invalid.";		\
> echo >&2 "         include/generated/autoconf.h or include/config/auto.conf are missing.";\
> echo >&2 "         Run 'make oldconfig && make prepare' on kernel src to fix it.";	\
> echo >&2 ;							\
> /bin/false)
> mkdir -p /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.tmp_versions ; rm -f /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.tmp_versions/*
> make KBUILD_MODULES=1   \
> -f /home/rostedt/work/git/linux-trace.git/scripts/Makefile.build obj=/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify ipmodify.o
>   gcc -Wp,-MD,/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/.ipmodify.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/4.7.2/include -I/home/rostedt/work/git/linux-trace.git/arch/x86/include -Iarch/x86/include/generated  -I/home/rostedt/work/git/linux-trace.git/include -Iinclude -I/home/rostedt/work/git/linux-trace.git/arch/x86/include/uapi -Iarch/x86/include/generated/uapi -I/home/rostedt/work/git/linux-trace.git/include/uapi -Iinclude/generated/uapi -include /home/rostedt/work/git/linux-trace.git/include/linux/kconfig.h   -I/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -m64 -mno-80387 -mno-fp-ret-in-387 -mtune=generic -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-
> args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_A
>  S_FXSAVEQ=1 -DCONFIG_AS_CRC32=1 -DCONFIG_AS_AVX=1 -DCONFIG_AS_AVX2=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -fno-delete-null-pointer-checks -Os -Wno-maybe-uninitialized --param=allow-store-data-races=0 -Wframe-larger-than=2048 -fno-stack-protector -Wno-unused-but-set-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-var-tracking-assignments -g -pg -mfentry -DCC_USING_FENTRY -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes -DCC_HAVE_ASM_GOTO  -DMODULE  -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(ipmodify)"  -D"KBUILD_MODNAME=KBUILD_STR(ipmodify)" -c -o /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.c
>   if [ "-pg" = "-pg" ]; then if [ /work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o != "scripts/mod/empty.o" ]; then ./scripts/recordmcount  "/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o"; fi; fi;
> /bin/sh: ./scripts/recordmcount: Permission denied
> make[5]: *** [/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify/ipmodify.o] Error 1

thanks. In my case, I didn't have same error.
---
  if [ "-pg" = "-pg" ]; then if [ /home/mhiramat/ksrc/linux-3/tools/testing/selftests/ftrace/ipmodify/ipmodify.o != "scripts/mod/empty.o" ]; then ./scripts/recordmcount
"/home/mhiramat/ksrc/linux-3/tools/testing/selftests/ftrace/ipmodify/ipmodify.o"; fi; fi;
make -f /home/mhiramat/ksrc/linux-3/scripts/Makefile.modpost
---

However, when I dropped exec bit from recordmcount, I got the same error.
---
$ chmod a-x ~/kbin/linux-3.x86_64/scripts/recordmcount
$ make V=1 O=/tmp
[...]
  if [ "-pg" = "-pg" ]; then if [ /home/mhiramat/ksrc/linux-3/tools/testing/selftests/ftrace/ipmodify/ipmodify.o != "scripts/mod/empty.o" ]; then ./scripts/recordmcount
"/home/mhiramat/ksrc/linux-3/tools/testing/selftests/ftrace/ipmodify/ipmodify.o"; fi; fi;
/bin/sh: ./scripts/recordmcount: Permission denied
make[5]: *** [/home/mhiramat/ksrc/linux-3/tools/testing/selftests/ftrace/ipmodify/ipmodify.o] Error 1
---

So, if your server directory is mounted with noexec, it's an environmental
problem. I guess you can not build any kernel drivers on that testbox, can you?

Anyway, this gives us a good question, "should test binaries be made by
server or client(testbox) environment?" This ipmodify driver is a binary
and it should be built with the kernel binary (by server) I think.
But yes, I missed the Makefile didn't allow that (this always referred
installed running kernel builddir).

I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
target to build these binaries with kernel...

Shuah, what would you think about this?

> make[4]: *** [ipmodify.ko] Error 2
> make[3]: *** [sub-make] Error 2
> make[2]: *** [__sub-make] Error 2
> make[2]: Leaving directory `/home/rostedt/work/git/nobackup/bxtest/trace'
> make[1]: *** [ipmodify.ko] Error 2
> make[1]: Leaving directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> make: *** [all] Error 2
> 
> 
> Thanks (on my way to bed),

Thank you :),


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

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Mon, 24 Nov 2014 23:11:12 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
 
> So, if your server directory is mounted with noexec, it's an environmental
> problem. I guess you can not build any kernel drivers on that testbox, can you?
> 

You're right about the noexec, even though my fstab has "exec" in it ??

I use a custom distcc build to build the kernel, and I don't waste time
setting up that environment for a testbox, which means I can't build a
module for the test kernels because it would be using a different
compiler in that case.


> Anyway, this gives us a good question, "should test binaries be made by
> server or client(testbox) environment?" This ipmodify driver is a binary
> and it should be built with the kernel binary (by server) I think.
> But yes, I missed the Makefile didn't allow that (this always referred
> installed running kernel builddir).
> 
> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
> target to build these binaries with kernel...

Is that what this would need? I would think we could add a
CONFIG_KSELFTEST_MODULES, that would build any modules needed for
selftests and have them in /lib/modules just like any other module.

-- Steve

> 
> Shuah, what would you think about this?
> 
> > make[4]: *** [ipmodify.ko] Error 2
> > make[3]: *** [sub-make] Error 2
> > make[2]: *** [__sub-make] Error 2
> > make[2]: Leaving directory `/home/rostedt/work/git/nobackup/bxtest/trace'
> > make[1]: *** [ipmodify.ko] Error 2
> > make[1]: Leaving directory `/work/git/linux-trace.git/tools/testing/selftests/ftrace/ipmodify'
> > make: *** [all] Error 2
> > 
> > 
> > Thanks (on my way to bed),
> 
> Thank you :),
> 
> 

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Mon, 24 Nov 2014 10:07:27 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 24 Nov 2014 23:11:12 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>  
> > So, if your server directory is mounted with noexec, it's an environmental
> > problem. I guess you can not build any kernel drivers on that testbox, can you?
> > 
> 
> You're right about the noexec, even though my fstab has "exec" in it ??

Ha! I hit this bug!

  https://bugzilla.redhat.com/show_bug.cgi?id=769636

If I remove "user" from the flags, it mounts exec.

This test box runs Fedora 18.

-- Steve

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Mon, 24 Nov 2014 10:18:31 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 24 Nov 2014 10:07:27 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Mon, 24 Nov 2014 23:11:12 +0900
> > Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> >  
> > > So, if your server directory is mounted with noexec, it's an environmental
> > > problem. I guess you can not build any kernel drivers on that testbox, can you?
> > > 
> > 
> > You're right about the noexec, even though my fstab has "exec" in it ??
> 
> Ha! I hit this bug!
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=769636
> 
> If I remove "user" from the flags, it mounts exec.
> 
> This test box runs Fedora 18.
> 

Oh, and "make O=/tmp" now works :-)

-- Steve

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Shuah Khan]

On 11/24/2014 07:11 AM, Masami Hiramatsu wrote:
> (CC'ed Shuah, since this is related to kselftest)
> 
> 
> So, if your server directory is mounted with noexec, it's an environmental
> problem. I guess you can not build any kernel drivers on that testbox, can you?
> 
> Anyway, this gives us a good question, "should test binaries be made by
> server or client(testbox) environment?" This ipmodify driver is a binary
> and it should be built with the kernel binary (by server) I think.
> But yes, I missed the Makefile didn't allow that (this always referred
> installed running kernel builddir).
> 
> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
> target to build these binaries with kernel...
> 
> Shuah, what would you think about this?

I am working on patch series to add an install target to the
main kernel makefile, so these tests can be built and installed
on a target just like we do with kernel and modules. I hope to
get this in 3.19 or definitely into 3.20

This probably will help address the problem you are seeing.
Install target is needed for qemu type environments as well.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/25 1:18), Shuah Khan wrote:
> On 11/24/2014 07:11 AM, Masami Hiramatsu wrote:
>> (CC'ed Shuah, since this is related to kselftest)
>>
>>
>> So, if your server directory is mounted with noexec, it's an environmental
>> problem. I guess you can not build any kernel drivers on that testbox, can you?
>>
>> Anyway, this gives us a good question, "should test binaries be made by
>> server or client(testbox) environment?" This ipmodify driver is a binary
>> and it should be built with the kernel binary (by server) I think.
>> But yes, I missed the Makefile didn't allow that (this always referred
>> installed running kernel builddir).
>>
>> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
>> target to build these binaries with kernel...
>>
>> Shuah, what would you think about this?
> 
> I am working on patch series to add an install target to the
> main kernel makefile, so these tests can be built and installed
> on a target just like we do with kernel and modules. I hope to
> get this in 3.19 or definitely into 3.20
> 
> This probably will help address the problem you are seeing.
> Install target is needed for qemu type environments as well.

Yes, that is what we need for this test case!
Please CC to me when sending the series. I'd like to try and
know how it works :)

Thank you,

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

Re: Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/25 0:19), Steven Rostedt wrote:
> On Mon, 24 Nov 2014 10:18:31 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> On Mon, 24 Nov 2014 10:07:27 -0500
>> Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>>> On Mon, 24 Nov 2014 23:11:12 +0900
>>> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
>>>  
>>>> So, if your server directory is mounted with noexec, it's an environmental
>>>> problem. I guess you can not build any kernel drivers on that testbox, can you?
>>>>
>>>
>>> You're right about the noexec, even though my fstab has "exec" in it ??
>>
>> Ha! I hit this bug!
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=769636
>>
>> If I remove "user" from the flags, it mounts exec.
>>
>> This test box runs Fedora 18.
>>
> 
> Oh, and "make O=/tmp" now works :-)

Good :)

So, I don't need to fix that at this point, do I?
It seems that current kselftest only supports this type of build
(build on installed kernel), and Shuah is working on fixing that.

Thank you,

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

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Shuah Khan]

On 11/24/2014 06:23 PM, Masami Hiramatsu wrote:
> (2014/11/25 1:18), Shuah Khan wrote:
>> On 11/24/2014 07:11 AM, Masami Hiramatsu wrote:
>>> (CC'ed Shuah, since this is related to kselftest)
>>>
>>>
>>> So, if your server directory is mounted with noexec, it's an environmental
>>> problem. I guess you can not build any kernel drivers on that testbox, can you?
>>>
>>> Anyway, this gives us a good question, "should test binaries be made by
>>> server or client(testbox) environment?" This ipmodify driver is a binary
>>> and it should be built with the kernel binary (by server) I think.
>>> But yes, I missed the Makefile didn't allow that (this always referred
>>> installed running kernel builddir).
>>>
>>> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
>>> target to build these binaries with kernel...
>>>
>>> Shuah, what would you think about this?
>>
>> I am working on patch series to add an install target to the
>> main kernel makefile, so these tests can be built and installed
>> on a target just like we do with kernel and modules. I hope to
>> get this in 3.19 or definitely into 3.20
>>
>> This probably will help address the problem you are seeing.
>> Install target is needed for qemu type environments as well.
> 
> Yes, that is what we need for this test case!
> Please CC to me when sending the series. I'd like to try and
> know how it works :)
> 

Good. Please take a look at this thread and give it a try. Please
give me feedback as well. This is the first step to get the install
feature added and then we can refine it at the selftests level as
needed.

https://lkml.org/lkml/2014/11/11/851

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Shuah Khan]

On 11/25/2014 07:42 AM, Shuah Khan wrote:
> On 11/24/2014 06:23 PM, Masami Hiramatsu wrote:
>> (2014/11/25 1:18), Shuah Khan wrote:
>>> On 11/24/2014 07:11 AM, Masami Hiramatsu wrote:
>>>> (CC'ed Shuah, since this is related to kselftest)
>>>>
>>>>
>>>> So, if your server directory is mounted with noexec, it's an environmental
>>>> problem. I guess you can not build any kernel drivers on that testbox, can you?
>>>>
>>>> Anyway, this gives us a good question, "should test binaries be made by
>>>> server or client(testbox) environment?" This ipmodify driver is a binary
>>>> and it should be built with the kernel binary (by server) I think.
>>>> But yes, I missed the Makefile didn't allow that (this always referred
>>>> installed running kernel builddir).
>>>>
>>>> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
>>>> target to build these binaries with kernel...
>>>>
>>>> Shuah, what would you think about this?
>>>
>>> I am working on patch series to add an install target to the
>>> main kernel makefile, so these tests can be built and installed
>>> on a target just like we do with kernel and modules. I hope to
>>> get this in 3.19 or definitely into 3.20
>>>
>>> This probably will help address the problem you are seeing.
>>> Install target is needed for qemu type environments as well.
>>
>> Yes, that is what we need for this test case!
>> Please CC to me when sending the series. I'd like to try and
>> know how it works :)
>>
> 
> Good. Please take a look at this thread and give it a try. Please
> give me feedback as well. This is the first step to get the install
> feature added and then we can refine it at the selftests level as
> needed.
> 
> https://lkml.org/lkml/2014/11/11/851
> 

Forgot to mention I didn't include ftrace in this first series for
install, planning to add it in my next round of patches. I have the
code ready for to do that.

-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/25 23:44), Shuah Khan wrote:
> On 11/25/2014 07:42 AM, Shuah Khan wrote:
>> On 11/24/2014 06:23 PM, Masami Hiramatsu wrote:
>>> (2014/11/25 1:18), Shuah Khan wrote:
>>>> On 11/24/2014 07:11 AM, Masami Hiramatsu wrote:
>>>>> (CC'ed Shuah, since this is related to kselftest)
>>>>>
>>>>>
>>>>> So, if your server directory is mounted with noexec, it's an environmental
>>>>> problem. I guess you can not build any kernel drivers on that testbox, can you?
>>>>>
>>>>> Anyway, this gives us a good question, "should test binaries be made by
>>>>> server or client(testbox) environment?" This ipmodify driver is a binary
>>>>> and it should be built with the kernel binary (by server) I think.
>>>>> But yes, I missed the Makefile didn't allow that (this always referred
>>>>> installed running kernel builddir).
>>>>>
>>>>> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
>>>>> target to build these binaries with kernel...
>>>>>
>>>>> Shuah, what would you think about this?
>>>>
>>>> I am working on patch series to add an install target to the
>>>> main kernel makefile, so these tests can be built and installed
>>>> on a target just like we do with kernel and modules. I hope to
>>>> get this in 3.19 or definitely into 3.20
>>>>
>>>> This probably will help address the problem you are seeing.
>>>> Install target is needed for qemu type environments as well.
>>>
>>> Yes, that is what we need for this test case!
>>> Please CC to me when sending the series. I'd like to try and
>>> know how it works :)
>>>
>>
>> Good. Please take a look at this thread and give it a try. Please
>> give me feedback as well. This is the first step to get the install
>> feature added and then we can refine it at the selftests level as
>> needed.
>>
>> https://lkml.org/lkml/2014/11/11/851

OK, I'll try that.
BTW, are those patches included in the below kernel.org tree ?

https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git/

> Forgot to mention I didn't include ftrace in this first series for
> install, planning to add it in my next round of patches. I have the
> code ready for to do that.

No problem, I'll wait for your series :)

Thank you,


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

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Steven Rostedt]

On Mon, 24 Nov 2014 23:11:12 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
> target to build these binaries with kernel...

For binaries (not modules), would that the server and client would need
the same environment? Make sure they both have the same shared
libraries. Or will the binaries all be built statically linked?

-- Steve

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Shuah Khan]

On 11/26/2014 12:20 AM, Masami Hiramatsu wrote:
> (2014/11/25 23:44), Shuah Khan wrote:

>>>
>>> Good. Please take a look at this thread and give it a try. Please
>>> give me feedback as well. This is the first step to get the install
>>> feature added and then we can refine it at the selftests level as
>>> needed.
>>>
>>> https://lkml.org/lkml/2014/11/11/851
> 
> OK, I'll try that.
> BTW, are those patches included in the below kernel.org tree ?
> 
> https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git/

I just now uploaded it to linux-kselftest.git next branch

Give it a try and let me know what you think.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/26 23:31), Steven Rostedt wrote:
> On Mon, 24 Nov 2014 23:11:12 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> I hope to have CONFIG_KSELFTEST_BINARIES for Kconfig, or make prep_kselftest
>> target to build these binaries with kernel...
> 
> For binaries (not modules), would that the server and client would need
> the same environment? Make sure they both have the same shared
> libraries. Or will the binaries all be built statically linked?

Good point! That would be optional, but I prefer latter by default :)

Thank you,

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

Re: Re: [PATCH ftrace/core v6 5/5] kselftest, ftrace: Add ftrace IPMODIFY flag test

[Masami Hiramatsu]

(2014/11/27 3:40), Shuah Khan wrote:
> On 11/26/2014 12:20 AM, Masami Hiramatsu wrote:
>> (2014/11/25 23:44), Shuah Khan wrote:
> 
>>>>
>>>> Good. Please take a look at this thread and give it a try. Please
>>>> give me feedback as well. This is the first step to get the install
>>>> feature added and then we can refine it at the selftests level as
>>>> needed.
>>>>
>>>> https://lkml.org/lkml/2014/11/11/851
>>
>> OK, I'll try that.
>> BTW, are those patches included in the below kernel.org tree ?
>>
>> https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git/
> 
> I just now uploaded it to linux-kselftest.git next branch
> 
> Give it a try and let me know what you think.

Thanks, I reviewed that and sent comments on the thread. :)


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

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Petr Mladek]

On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
> 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.

Please, what are the plans with this patch?

I have checked the interference between Kprobes and LivePatching and
here is my observation:

1. Jprobe and LivePatch must be in a hard conflict. They both need
   to change IP and continue another way after ftrace ops finishes.

   BTW: I wonder a bit why Jprobe handler could not be called directly
   from kprobe_ftrace_handler(). I guess that it is because we want
   to call the kprobe handler in a sane context: preemption and IRQs
   enabled, be able to use traced functions.


2. Normal Kprobe for the original function is ignored if the function
   is patched.

   I am working on a code that will print warning in both
   cases. First, when we add a patch and the function has
   a Kprobe registered. Second, the function is patched and
   we want to add Kprobe for the original version.

   I want to make it generic and make it dependent on the
   IPMODIFY flag. IMHO, it just could be a handshake between
   kprobe and ftrace code. I am still trying to understand
   the needed parts of the code ;-)


3. Kretprobe could live with a patch without a problem!

   The Kretprobe entry handler is called directly in
   kprobe_ftrace_handler() and does not change IP.
   On the other hand the LivePatch ftrace handler does
   not modify the return address because the return address
   is the same for the original and the patched function.

   Or did I miss something?

   This is where this patch might be useful. The other patches
   from this patch set are already in Linus' tree and I cannot
   find any information about this one.

   I could try to solve remaining problems if there are any.


Best Regards,
Petr

> 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 4227ec2..eb03efc 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);
> 
> 

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Masami Hiramatsu]

Hi Petr,

Sorry I missed this mail.

(2015/01/27 1:14), Petr Mladek wrote:> On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
>> 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.
>
> Please, what are the plans with this patch?

Well, I'll revise this for newer kernel.

>
> I have checked the interference between Kprobes and LivePatching and
> here is my observation:
>
> 1. Jprobe and LivePatch must be in a hard conflict. They both need
>    to change IP and continue another way after ftrace ops finishes.
>
>    BTW: I wonder a bit why Jprobe handler could not be called directly
>    from kprobe_ftrace_handler(). I guess that it is because we want
>    to call the kprobe handler in a sane context: preemption and IRQs
>    enabled, be able to use traced functions.

Right, Jprobe is just a different interface of kprobe handler. It must be
called from kprobes.
However, I think this is not so hard in practice, since we already have
perf-probe which allows us to find which register or stack is assigned to
which function parameter. That was the main reason why jprobe is introduced.
But now, we have perf-probe or systemtap, we don't(or less) need the hack like
jprobe anymore.


> 2. Normal Kprobe for the original function is ignored if the function
>    is patched.
>
>    I am working on a code that will print warning in both
>    cases. First, when we add a patch and the function has
>    a Kprobe registered. Second, the function is patched and
>    we want to add Kprobe for the original version.

Thanks! Maybe we can add "Ignored" flag for those kprobes so that users
can check it is working or not via debugfs.

>    I want to make it generic and make it dependent on the
>    IPMODIFY flag. IMHO, it just could be a handshake between
>    kprobe and ftrace code. I am still trying to understand
>    the needed parts of the code ;-)

BTW, the kprobes on function entry (iow, ftrace-based kprobes) should
not be ignored. Even if we patches a function-body, the entrance
address should be same.


> 3. Kretprobe could live with a patch without a problem!
>
>    The Kretprobe entry handler is called directly in
>    kprobe_ftrace_handler() and does not change IP.
>    On the other hand the LivePatch ftrace handler does
>    not modify the return address because the return address
>    is the same for the original and the patched function.

Right.

>
>    Or did I miss something?
>
>    This is where this patch might be useful. The other patches
>    from this patch set are already in Linus' tree and I cannot
>    find any information about this one.

Well, thank you for picking it up!


>
>    I could try to solve remaining problems if there are any.
>

Thank you,


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

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Petr Mladek]

On Tue 2015-02-24 16:38:18, Masami Hiramatsu wrote:
> Hi Petr,
> 
> Sorry I missed this mail.

Thanks a lot for answering it with many valuable information.
 
> (2015/01/27 1:14), Petr Mladek wrote:> On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
> >> 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.
> >
> > Please, what are the plans with this patch?
> 
> Well, I'll revise this for newer kernel.
> >
> > I have checked the interference between Kprobes and LivePatching and
> > here is my observation:
> >
> > 1. Jprobe and LivePatch must be in a hard conflict. They both need
> >    to change IP and continue another way after ftrace ops finishes.
> >
> >    BTW: I wonder a bit why Jprobe handler could not be called directly
> >    from kprobe_ftrace_handler(). I guess that it is because we want
> >    to call the kprobe handler in a sane context: preemption and IRQs
> >    enabled, be able to use traced functions.
> 
> Right, Jprobe is just a different interface of kprobe handler. It must be
> called from kprobes.
> However, I think this is not so hard in practice, since we already have
> perf-probe which allows us to find which register or stack is assigned to
> which function parameter. That was the main reason why jprobe is introduced.
> But now, we have perf-probe or systemtap, we don't(or less) need the hack like
> jprobe anymore.

I see. It is called this special way (modifies regs->ip), so that
registry and stack have the same content as if the probed function was called.


> > 2. Normal Kprobe for the original function is ignored if the function
> >    is patched.
> >
> >    I am working on a code that will print warning in both
> >    cases. First, when we add a patch and the function has
> >    a Kprobe registered. Second, the function is patched and
> >    we want to add Kprobe for the original version.
> 
> Thanks! Maybe we can add "Ignored" flag for those kprobes so that users
> can check it is working or not via debugfs.

Great idea. Well, it will solve only already existing Kprobes.


> >    I want to make it generic and make it dependent on the
> >    IPMODIFY flag. IMHO, it just could be a handshake between
> >    kprobe and ftrace code. I am still trying to understand
> >    the needed parts of the code ;-)

I have played with it and realized that only Kprobes framework has
information about all existing and newly created Kprobes. Therefore
we need to somehow inform it that there is a patch and that the code
is redirected. I have a prototype that is introducing a new fake
Kprobe, so called Patch Probe. It has new flag KPROBE_FLAG_PATCH
and no handlers. Conflicts with existing Kprobes are checked when
this special probe is added. Also conflicts with these Patch probes
are checked when new normal Kprobe is added.

I still want to clean and test it a bit before sending as RFC.

> BTW, the kprobes on function entry (iow, ftrace-based kprobes) should
> not be ignored. Even if we patches a function-body, the entrance
> address should be same.

Yup

> > 3. Kretprobe could live with a patch without a problem!
> >
> >    The Kretprobe entry handler is called directly in
> >    kprobe_ftrace_handler() and does not change IP.
> >    On the other hand the LivePatch ftrace handler does
> >    not modify the return address because the return address
> >    is the same for the original and the patched function.
> 
> Right.

Thanks for confirmation.

> >    Or did I miss something?
> >
> >    This is where this patch might be useful. The other patches
> >    from this patch set are already in Linus' tree and I cannot
> >    find any information about this one.
> 
> Well, thank you for picking it up!

I have one more patch set in the queue. It better handle errors when
kprobe_ftrace_ops could not be registered in arm_kprobe_ftrace()
and disarm_kprobe_ftrace(). This one is nearly done. Unfortunately,
I had to interupt it because my wife got sick and I had to take care
of babies. And then there is the big activity around life patching
that we need to somehow handle.

Best Regards,
Petr

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Masami Hiramatsu]

(2015/02/24 17:52), Petr Mladek wrote:
> On Tue 2015-02-24 16:38:18, Masami Hiramatsu wrote:
>> Hi Petr,
>>
>> Sorry I missed this mail.
> 
> Thanks a lot for answering it with many valuable information.
>  
>> (2015/01/27 1:14), Petr Mladek wrote:> On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
>>>> 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.
>>>
>>> Please, what are the plans with this patch?
>>
>> Well, I'll revise this for newer kernel.
>>>
>>> I have checked the interference between Kprobes and LivePatching and
>>> here is my observation:
>>>
>>> 1. Jprobe and LivePatch must be in a hard conflict. They both need
>>>    to change IP and continue another way after ftrace ops finishes.
>>>
>>>    BTW: I wonder a bit why Jprobe handler could not be called directly
>>>    from kprobe_ftrace_handler(). I guess that it is because we want
>>>    to call the kprobe handler in a sane context: preemption and IRQs
>>>    enabled, be able to use traced functions.
>>
>> Right, Jprobe is just a different interface of kprobe handler. It must be
>> called from kprobes.
>> However, I think this is not so hard in practice, since we already have
>> perf-probe which allows us to find which register or stack is assigned to
>> which function parameter. That was the main reason why jprobe is introduced.
>> But now, we have perf-probe or systemtap, we don't(or less) need the hack like
>> jprobe anymore.
> 
> I see. It is called this special way (modifies regs->ip), so that
> registry and stack have the same content as if the probed function was called.

Yes.

>>> 2. Normal Kprobe for the original function is ignored if the function
>>>    is patched.
>>>
>>>    I am working on a code that will print warning in both
>>>    cases. First, when we add a patch and the function has
>>>    a Kprobe registered. Second, the function is patched and
>>>    we want to add Kprobe for the original version.
>>
>> Thanks! Maybe we can add "Ignored" flag for those kprobes so that users
>> can check it is working or not via debugfs.
> 
> Great idea. Well, it will solve only already existing Kprobes.

Yeah, just changing the kprobe state is easy and needed.
And for newer kprobes, perhaps we need to add
bool klp_patched_function(void *func_addr); to check the
function is patched. (this will need to be done with
locking kpatch...)

>>>    I want to make it generic and make it dependent on the
>>>    IPMODIFY flag. IMHO, it just could be a handshake between
>>>    kprobe and ftrace code. I am still trying to understand
>>>    the needed parts of the code ;-)
> 
> I have played with it and realized that only Kprobes framework has
> information about all existing and newly created Kprobes. Therefore
> we need to somehow inform it that there is a patch and that the code
> is redirected. I have a prototype that is introducing a new fake
> Kprobe, so called Patch Probe. It has new flag KPROBE_FLAG_PATCH
> and no handlers. Conflicts with existing Kprobes are checked when
> this special probe is added. Also conflicts with these Patch probes
> are checked when new normal Kprobe is added.

No, you don't need that. I can make kprobes_location() or
kprobe_for_each_on(kp, start, end) {} iterator. Since the livepatch
is in-tree feature now, we can change kprobes for it...
And anyway, IPMODIFY should be only for jprobes not kprobes...

> I still want to clean and test it a bit before sending as RFC.
> 
>> BTW, the kprobes on function entry (iow, ftrace-based kprobes) should
>> not be ignored. Even if we patches a function-body, the entrance
>> address should be same.
> 
> Yup
> 
>>> 3. Kretprobe could live with a patch without a problem!
>>>
>>>    The Kretprobe entry handler is called directly in
>>>    kprobe_ftrace_handler() and does not change IP.
>>>    On the other hand the LivePatch ftrace handler does
>>>    not modify the return address because the return address
>>>    is the same for the original and the patched function.
>>
>> Right.
> 
> Thanks for confirmation.
> 
>>>    Or did I miss something?
>>>
>>>    This is where this patch might be useful. The other patches
>>>    from this patch set are already in Linus' tree and I cannot
>>>    find any information about this one.
>>
>> Well, thank you for picking it up!
> 
> I have one more patch set in the queue. It better handle errors when
> kprobe_ftrace_ops could not be registered in arm_kprobe_ftrace()
> and disarm_kprobe_ftrace(). This one is nearly done. Unfortunately,
> I had to interupt it because my wife got sick and I had to take care
> of babies. And then there is the big activity around life patching
> that we need to somehow handle.

Ah, thanks, and hope your wife to get better soon.

Thank you,

> 
> Best Regards,
> Petr
> 


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

Re: [PATCH ftrace/core v6 4/5] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Petr Mladek]

On Tue 2015-02-24 20:47:06, Masami Hiramatsu wrote:
> (2015/02/24 17:52), Petr Mladek wrote:
> > On Tue 2015-02-24 16:38:18, Masami Hiramatsu wrote:
> >> Hi Petr,
> >>
> >> Sorry I missed this mail.
> > 
> > Thanks a lot for answering it with many valuable information.
> >  
> >> (2015/01/27 1:14), Petr Mladek wrote:> On Fri 2014-11-21 05:25:30, Masami Hiramatsu wrote:
> >>>> 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.
> >>>
> >>> Please, what are the plans with this patch?
> >>
> >> Well, I'll revise this for newer kernel.
> >>>
> >>> I have checked the interference between Kprobes and LivePatching and
> >>> here is my observation:
[...]
> >>> 2. Normal Kprobe for the original function is ignored if the function
> >>>    is patched.
> >>>
> >>>    I am working on a code that will print warning in both
> >>>    cases. First, when we add a patch and the function has
> >>>    a Kprobe registered. Second, the function is patched and
> >>>    we want to add Kprobe for the original version.
> >>
> >> Thanks! Maybe we can add "Ignored" flag for those kprobes so that users
> >> can check it is working or not via debugfs.
> > 
> > Great idea. Well, it will solve only already existing Kprobes.
> 
> Yeah, just changing the kprobe state is easy and needed.
> And for newer kprobes, perhaps we need to add
> bool klp_patched_function(void *func_addr); to check the
> function is patched. (this will need to be done with
> locking kpatch...)

I like this idea and will try to use it once I get time again.

> >>>    I want to make it generic and make it dependent on the
> >>>    IPMODIFY flag. IMHO, it just could be a handshake between
> >>>    kprobe and ftrace code. I am still trying to understand
> >>>    the needed parts of the code ;-)
> > 
> > I have played with it and realized that only Kprobes framework has
> > information about all existing and newly created Kprobes. Therefore
> > we need to somehow inform it that there is a patch and that the code
> > is redirected. I have a prototype that is introducing a new fake
> > Kprobe, so called Patch Probe. It has new flag KPROBE_FLAG_PATCH
> > and no handlers. Conflicts with existing Kprobes are checked when
> > this special probe is added. Also conflicts with these Patch probes
> > are checked when new normal Kprobe is added.
> 
> No, you don't need that. I can make kprobes_location() or
> kprobe_for_each_on(kp, start, end) {} iterator. Since the livepatch
> is in-tree feature now, we can change kprobes for it...

You are right, the in-tree live patch code brings more possibilities. 

> And anyway, IPMODIFY should be only for jprobes not kprobes...

Yup.

> > I have one more patch set in the queue. It better handle errors when
> > kprobe_ftrace_ops could not be registered in arm_kprobe_ftrace()
> > and disarm_kprobe_ftrace(). This one is nearly done. Unfortunately,
> > I had to interupt it because my wife got sick and I had to take care
> > of babies. And then there is the big activity around life patching
> > that we need to somehow handle.
> 
> Ah, thanks, and hope your wife to get better soon.

Thanks a lot. Fortunately, she already is better.

Best Regards,
Petr