ftrace, kprobes: Introduce IPMODIFY flag for ftrace_ops to detect conflicts

 Documentation/trace/ftrace.txt |    5 +
 include/linux/ftrace.h         |   16 ++++-
 kernel/trace/ftrace.c          |  142 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 159 insertions(+), 4 deletions(-)

Documentation/trace/ftrace.txt +5 -0

index 4da4261..f10f5f5 100644
@@ -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

include/linux/ftrace.h +5 -0

index 7b2616f..93cf047 100644
@@ -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

include/linux/ftrace.h +5 -2

  */
 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)

kernel/trace/ftrace.c +34 -0

index fa0f36b..baa2c4d 100644
@@ -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;

kernel/trace/ftrace.c +96 -2

+	 * 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)

Quoted 4 lines · On Fri, 21 Nov 2014 05:25:16 -0500 Masami Hiramatsu…

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

Quoted 8 lines · On Fri, 21 Nov 2014 13:05:29 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 4 lines · On Fri, 21 Nov 2014 05:25:16 -0500 Masami Hiramatsu…

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

Quoted 3 lines

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

include/linux/ftrace.h +3 -3

index 93cf0478f64e..ed501953f0b2 100644
@@ -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,

kernel/trace/ftrace.c +2 -2

index 531e72a716c3..929a733d302e 100644
@@ -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;

Quoted 41 lines · On Fri, 21 Nov 2014 05:25:16 -0500 Masami Hiramatsu…

 Documentation/trace/ftrace.txt |    5 +
 include/linux/ftrace.h         |   16 ++++-
 kernel/trace/ftrace.c          |  142 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 159 insertions(+), 4 deletions(-)

Documentation/trace/ftrace.txt +5 -0

index 4da4261..f10f5f5 100644
@@ -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

include/linux/ftrace.h +5 -0

index 7b2616f..93cf047 100644
@@ -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.

include/linux/ftrace.h ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops); · line 112

Quoted 4 lines

include/linux/ftrace.h +4 -0

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

Quoted 104 lines

include/linux/ftrace.h +5 -2

  */
 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)

kernel/trace/ftrace.c +38 -0

index fa0f36b..baa2c4d 100644
@@ -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;

kernel/trace/ftrace.c static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, · line 1813

Quoted 6 lines

kernel/trace/ftrace.c +6 -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

Quoted 124 lines

kernel/trace/ftrace.c +96 -2

+	 * 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)

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 <[email protected]>

Cc Ananth N Mavinakayanahalli <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Namhyung Kim <[email protected]>

---
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(-)

Documentation/kprobes.txt +5 -7

index 4227ec2..eb03efc 100644
@@ -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
 

kernel/kprobes.c +0 -0

index 831978c..4b4b7c5 100644
@@ -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,

kernel/kprobes.c +14 -0

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

kernel/kprobes.c +25 -0

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

Quoted 77 lines · On Fri, 21 Nov 2014 05:25:30 -0500 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 <[email protected]>

Cc Ananth N Mavinakayanahalli <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Namhyung Kim <[email protected]>

---
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(-)

Documentation/kprobes.txt +5 -7

index 4227ec2..eb03efc 100644
@@ -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
 

kernel/kprobes.c +1 -1

index 831978c..4b4b7c5 100644
@@ -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;
 

kernel/kprobes.c static struct kprobe *alloc_aggr_kprobe(struct kprobe *p) · line 926

Quoted 5 lines

kernel/kprobes.c +5 -0

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

Quoted 14 lines

kernel/kprobes.c +14 -0

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

kernel/kprobes.c static struct kprobe *alloc_aggr_kprobe(struct kprobe *p) · line 950

Quoted 10 lines

kernel/kprobes.c +10 -0

+	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?

kernel/kprobes.c static struct kprobe *alloc_aggr_kprobe(struct kprobe *p) · line 956

Quoted 6 lines

kernel/kprobes.c +6 -0

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

Probably could just return 0 here.

Rest looks fine.

-- Steve

Quoted 25 lines

kernel/kprobes.c +25 -0

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

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 <[email protected]>

Cc Ananth N Mavinakayanahalli <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Namhyung Kim <[email protected]>

---
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(-)

Documentation/kprobes.txt +5 -7

index 4227ec2..eb03efc 100644
@@ -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
 

kernel/kprobes.c +109 -16

index 831978c..4b4b7c5 100644
@@ -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);

Quoted 8 lines

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.

Hi Petr,

Sorry I missed this mail.

Quoted 10 lines · Masami Hiramatsu

Quoted 8 lines

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.

Quoted 11 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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!

Quoted 3 lines · Masami Hiramatsu

Hi Petr,

Sorry I missed this mail.

Thanks a lot for answering it with many valuable information.

Quoted 25 lines

Quoted 10 lines · Masami Hiramatsu

Quoted 8 lines

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.

Quoted 11 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 9 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 1 line

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

Quoted 23 lines · Petr Mladek

Quoted 3 lines · Masami Hiramatsu

Hi Petr,

Sorry I missed this mail.

Thanks a lot for answering it with many valuable information.

Quoted 16 lines

Quoted 10 lines · Masami Hiramatsu

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

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 ;-)

Quoted 8 lines

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

Quoted 6 lines

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.

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 <[email protected]>

---

Quoted 6 lines · On Fri, 21 Nov 2014 05:25:37 -0500 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.

Quoted 3 lines

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

---

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

Quoted 9 lines · Steven Rostedt

Quoted 6 lines · On Fri, 21 Nov 2014 05:25:37 -0500 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.

Quoted 5 lines

Quoted 3 lines

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

---

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

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

Quoted 4 lines

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

Quoted 9 lines

[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 ?

Quoted 12 lines · On Mon, 24 Nov 2014 11:50:06 +0900 Masami Hiramatsu…

Quoted 11 lines · Steven Rostedt

Quoted 8 lines · On Fri, 21 Nov 2014 05:25:37 -0500 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 <[email protected]>

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

---

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 15 lines

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

Quoted 9 lines

[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

Quoted 2 lines

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

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),

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?

Quoted 10 lines

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 :),

Quoted 3 lines · On Mon, 24 Nov 2014 23:11:12 +0900 Masami Hiramatsu…

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

Quoted 8 lines · On Mon, 24 Nov 2014 10:07:27 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 3 lines · On Mon, 24 Nov 2014 23:11:12 +0900 Masami Hiramatsu…

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.

Quoted 14 lines · On Mon, 24 Nov 2014 10:18:31 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 8 lines · On Mon, 24 Nov 2014 10:07:27 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 3 lines · On Mon, 24 Nov 2014 23:11:12 +0900 Masami Hiramatsu…

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!

Quoted 6 lines

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 :-)

(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?

Quoted 12 lines · Masami Hiramatsu

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

Quoted 4 lines

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.

Quoted 23 lines · Shuah Khan

Quoted 12 lines · Masami Hiramatsu

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

Quoted 4 lines

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

Quoted 2 lines

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 :)

Quoted 24 lines · Masami Hiramatsu

Quoted 23 lines · Shuah Khan

Quoted 12 lines · Masami Hiramatsu

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

Quoted 4 lines

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

Quoted 7 lines

Quoted 2 lines

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

Quoted 33 lines · Shuah Khan

Quoted 31 lines · Masami Hiramatsu

Quoted 18 lines · Shuah Khan

Quoted 12 lines · Masami Hiramatsu

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

Quoted 4 lines

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?

Quoted 7 lines

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 :)

Quoted 6 lines

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.

Quoted 6 lines · Shuah Khan

Quoted 6 lines

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.

Quoted 1 line

Quoted 1 line

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/

Quoted 10 lines · Masami Hiramatsu

Quoted 7 lines · Shuah Khan

Quoted 7 lines

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

Quoted 4 lines

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.

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

Quoted 2 lines · On Mon, 24 Nov 2014 23:11:12 +0900 Masami Hiramatsu…

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?