[PATCH 00/10] ftrace: Add register_ftrace_direct()

[PATCH 00/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

Alexei mentioned that he would like a way to access the ftrace fentry
code to jump directly to a custom eBPF trampoline instead of using
ftrace regs caller, as he said it would be faster.

I proposed a new register_ftrace_direct() function that would allow
this to happen and still work with the ftrace infrastructure. I posted
a proof of concept patch here:

 https://lore.kernel.org/r/20191023122307.756b4978@gandalf.local.home

This patch series is a more complete version, and the start of the
actual implementation. I haven't run it through my full test suite but
it passes my smoke tests and some other custom tests I built.

I also realized that I need to make the sample modules depend on X86_64
as it has inlined assembly in it that requires that dependency.

This is based on 5.4-rc6 plus the permanent patches that prevent
a ftrace_ops from being disabled by /proc/sys/kernel/ftrace_enabled

Below is the tree that contains this code.

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
ftrace/direct

Head SHA1: 9492654d091cb90a487ca669c58f802fa99bcd6f

Enjoy,

-- Steve


Steven Rostedt (VMware) (10):
      ftrace: Separate out the copying of a ftrace_hash from __ftrace_hash_move()
      ftrace: Separate out functionality from ftrace_location_range()
      ftrace: Add register_ftrace_direct()
      ftrace: Add ftrace_find_direct_func()
      ftrace: Add sample module that uses register_ftrace_direct()
      ftrace/selftest: Add tests to test register_ftrace_direct()
      ftrace: Add another example of register_ftrace_direct() use case
      ftrace/selftests: Update the direct call selftests to test two direct calls
      ftrace/x86: Add register_ftrace_direct() for custom trampolines
      ftrace/x86: Add a counter to test function_graph with direct

----
 arch/x86/Kconfig                                   |   1 +
 arch/x86/include/asm/ftrace.h                      |  13 +
 arch/x86/kernel/ftrace.c                           |  14 +
 arch/x86/kernel/ftrace_64.S                        |  33 +-
 include/linux/ftrace.h                             |  50 ++-
 kernel/trace/Kconfig                               |   8 +
 kernel/trace/ftrace.c                              | 420 +++++++++++++++++++--
 samples/Kconfig                                    |   7 +
 samples/Makefile                                   |   1 +
 samples/ftrace/Makefile                            |   4 +
 samples/ftrace/ftrace-direct-too.c                 |  51 +++
 samples/ftrace/ftrace-direct.c                     |  45 +++
 .../ftrace/test.d/direct/ftrace-direct.tc          |  69 ++++
 .../ftrace/test.d/direct/kprobe-direct.tc          |  84 +++++
 14 files changed, 759 insertions(+), 41 deletions(-)
 create mode 100644 samples/ftrace/Makefile
 create mode 100644 samples/ftrace/ftrace-direct-too.c
 create mode 100644 samples/ftrace/ftrace-direct.c
 create mode 100644 tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
 create mode 100644 tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc

[PATCH 01/10] ftrace: Separate out the copying of a ftrace_hash from __ftrace_hash_move()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Most of the functionality of __ftrace_hash_move() can be reused, but not all
of it. That is, __ftrace_hash_move() is used to simply make a new hash from
an existing one, using the same size as the original. Creating a dup_hash(),
where we can specify a new size will be useful when we want to create a hash
with a default size, or simply copy the old one.

Signed-off-by: Steven Rostedt (VMWare) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 89e9128652ef..76e5de8c7822 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1372,23 +1372,15 @@ 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 struct ftrace_hash *
-__ftrace_hash_move(struct ftrace_hash *src)
+static struct ftrace_hash *dup_hash(struct ftrace_hash *src, int size)
 {
 	struct ftrace_func_entry *entry;
-	struct hlist_node *tn;
-	struct hlist_head *hhd;
 	struct ftrace_hash *new_hash;
-	int size = src->count;
+	struct hlist_head *hhd;
+	struct hlist_node *tn;
 	int bits = 0;
 	int i;
 
-	/*
-	 * If the new source is empty, just return the empty_hash.
-	 */
-	if (ftrace_hash_empty(src))
-		return EMPTY_HASH;
-
 	/*
 	 * Make the hash size about 1/2 the # found
 	 */
@@ -1413,10 +1405,23 @@ __ftrace_hash_move(struct ftrace_hash *src)
 			__add_hash_entry(new_hash, entry);
 		}
 	}
-
 	return new_hash;
 }
 
+static struct ftrace_hash *
+__ftrace_hash_move(struct ftrace_hash *src)
+{
+	int size = src->count;
+
+	/*
+	 * If the new source is empty, just return the empty_hash.
+	 */
+	if (ftrace_hash_empty(src))
+		return EMPTY_HASH;
+
+	return dup_hash(src, size);
+}
+
 static int
 ftrace_hash_move(struct ftrace_ops *ops, int enable,
 		 struct ftrace_hash **dst, struct ftrace_hash *src)
-- 
2.23.0

[PATCH 02/10] ftrace: Separate out functionality from ftrace_location_range()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Create a new function called lookup_rec() from the functionality of
ftrace_location_range(). The difference between lookup_rec() is that it
returns the record that it finds, where as ftrace_location_range() returns
only if it found a match or not.

The lookup_rec() is static, and can be used for new functionality where
ftrace needs to find a record of a specific address.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 76e5de8c7822..b0e7f03919de 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1541,6 +1541,26 @@ static int ftrace_cmp_recs(const void *a, const void *b)
 	return 0;
 }
 
+static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
+{
+	struct ftrace_page *pg;
+	struct dyn_ftrace *rec = NULL;
+	struct dyn_ftrace key;
+
+	key.ip = start;
+	key.flags = end;	/* overload flags, as it is unsigned long */
+
+	for (pg = ftrace_pages_start; pg; pg = pg->next) {
+		if (end < pg->records[0].ip ||
+		    start >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
+			continue;
+		rec = bsearch(&key, pg->records, pg->index,
+			      sizeof(struct dyn_ftrace),
+			      ftrace_cmp_recs);
+	}
+	return rec;
+}
+
 /**
  * ftrace_location_range - return the first address of a traced location
  *	if it touches the given ip range
@@ -1555,23 +1575,11 @@ static int ftrace_cmp_recs(const void *a, const void *b)
  */
 unsigned long ftrace_location_range(unsigned long start, unsigned long end)
 {
-	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
-	struct dyn_ftrace key;
 
-	key.ip = start;
-	key.flags = end;	/* overload flags, as it is unsigned long */
-
-	for (pg = ftrace_pages_start; pg; pg = pg->next) {
-		if (end < pg->records[0].ip ||
-		    start >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
-			continue;
-		rec = bsearch(&key, pg->records, pg->index,
-			      sizeof(struct dyn_ftrace),
-			      ftrace_cmp_recs);
-		if (rec)
-			return rec->ip;
-	}
+	rec = lookup_rec(start, end);
+	if (rec)
+		return rec->ip;
 
 	return 0;
 }
-- 
2.23.0

[PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Add the start of the functionality to allow other trampolines to use the
ftrace mcount/fentry/nop location. This adds two new functions:

 register_ftrace_direct() and unregister_ftrace_direct()

Both take two parameters: the first is the instruction address of where the
mcount/fentry/nop exists, and the second is the trampoline to have that
location called.

This will handle cases where ftrace is already used on that same location,
and will make it still work, where the registered direct called trampoline
will get called after all the registered ftrace callers are handled.

Currently, it will not allow for IP_MODIFY functions to be called at the
same locations, which include some kprobes and live kernel patching.

At this point, no architecture supports this. This is only the start of
implementing the framework.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  36 +++++-
 kernel/trace/Kconfig   |   8 ++
 kernel/trace/ftrace.c  | 272 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 309 insertions(+), 7 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8385cafe4f9f..efe3e521aff4 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -144,6 +144,8 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
  * TRACE_ARRAY - The ops->private points to a trace_array descriptor.
  * PERMANENT - Set when the ops is permanent and should not be affected by
  *             ftrace_enabled.
+ * DIRECT - Used by the direct ftrace_ops helper for direct functions
+ *            (internal ftrace only, should not be used by others)
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= 1 << 0,
@@ -163,6 +165,7 @@ enum {
 	FTRACE_OPS_FL_RCU			= 1 << 14,
 	FTRACE_OPS_FL_TRACE_ARRAY		= 1 << 15,
 	FTRACE_OPS_FL_PERMANENT                 = 1 << 16,
+	FTRACE_OPS_FL_DIRECT			= 1 << 17,
 };
 
 #ifdef CONFIG_DYNAMIC_FTRACE
@@ -242,6 +245,32 @@ static inline void ftrace_free_init_mem(void) { }
 static inline void ftrace_free_mem(struct module *mod, void *start, void *end) { }
 #endif /* CONFIG_FUNCTION_TRACER */
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+int register_ftrace_direct(unsigned long ip, unsigned long addr);
+int unregister_ftrace_direct(unsigned long ip, unsigned long addr);
+#else
+static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
+{
+	return -ENODEV;
+}
+static inline int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+/*
+ * This must be implemented by the architecture.
+ * It is the way the ftrace direct_ops helper, when called
+ * via ftrace (because there's other callbacks besides the
+ * direct call), can inform the architecture's trampoline that this
+ * routine has a direct caller, and what the caller is.
+ */
+static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
+						 unsigned long addr) { }
+#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
 #ifdef CONFIG_STACK_TRACER
 
 extern int stack_tracer_enabled;
@@ -333,6 +362,7 @@ bool is_ftrace_trampoline(unsigned long addr);
  *  REGS_EN - the function is set up to save regs.
  *  IPMODIFY - the record allows for the IP address to be changed.
  *  DISABLED - the record is not ready to be touched yet
+ *  DIRECT   - there is a direct function to call
  *
  * 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
@@ -348,10 +378,12 @@ enum {
 	FTRACE_FL_TRAMP_EN	= (1UL << 27),
 	FTRACE_FL_IPMODIFY	= (1UL << 26),
 	FTRACE_FL_DISABLED	= (1UL << 25),
+	FTRACE_FL_DIRECT	= (1UL << 24),
+	FTRACE_FL_DIRECT_EN	= (1UL << 23),
 };
 
-#define FTRACE_REF_MAX_SHIFT	25
-#define FTRACE_FL_BITS		7
+#define FTRACE_REF_MAX_SHIFT	23
+#define FTRACE_FL_BITS		9
 #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/Kconfig b/kernel/trace/Kconfig
index e08527f50d2a..624a05e99b0b 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -33,6 +33,9 @@ config HAVE_DYNAMIC_FTRACE
 config HAVE_DYNAMIC_FTRACE_WITH_REGS
 	bool
 
+config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	bool
+
 config HAVE_FTRACE_MCOUNT_RECORD
 	bool
 	help
@@ -557,6 +560,11 @@ config DYNAMIC_FTRACE_WITH_REGS
 	depends on DYNAMIC_FTRACE
 	depends on HAVE_DYNAMIC_FTRACE_WITH_REGS
 
+config DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+	def_bool y
+	depends on DYNAMIC_FTRACE
+	depends on HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+
 config FUNCTION_PROFILER
 	bool "Kernel function profiler"
 	depends on FUNCTION_TRACER
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b0e7f03919de..a06671f3a281 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1023,6 +1023,7 @@ static bool update_all_ops;
 struct ftrace_func_entry {
 	struct hlist_node hlist;
 	unsigned long ip;
+	unsigned long direct; /* for direct lookup only */
 };
 
 struct ftrace_func_probe {
@@ -1730,6 +1731,9 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 			if (FTRACE_WARN_ON(ftrace_rec_count(rec) == FTRACE_REF_MAX))
 				return false;
 
+			if (ops->flags & FTRACE_OPS_FL_DIRECT)
+				rec->flags |= FTRACE_FL_DIRECT;
+
 			/*
 			 * If there's only a single callback registered to a
 			 * function, and the ops has a trampoline registered
@@ -1757,6 +1761,18 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 				return false;
 			rec->flags--;
 
+			if (ops->flags & FTRACE_OPS_FL_DIRECT)
+				rec->flags &= ~FTRACE_FL_DIRECT;
+
+			/*
+			 * Only the internal direct_ops should have the
+			 * DIRECT flag set. Thus, if it is removing a
+			 * function, then that function should no longer
+			 * be direct.
+			 */
+			if (ops->flags & FTRACE_OPS_FL_DIRECT)
+				rec->flags &= ~FTRACE_FL_DIRECT;
+
 			/*
 			 * If the rec had REGS enabled and the ops that is
 			 * being removed had REGS set, then see if there is
@@ -2092,15 +2108,34 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update)
 	 * If enabling and the REGS flag does not match the REGS_EN, or
 	 * the TRAMP flag doesn't match the TRAMP_EN, then do not ignore
 	 * this record. Set flags to fail the compare against ENABLED.
+	 * Same for direct calls.
 	 */
 	if (flag) {
-		if (!(rec->flags & FTRACE_FL_REGS) != 
+		if (!(rec->flags & FTRACE_FL_REGS) !=
 		    !(rec->flags & FTRACE_FL_REGS_EN))
 			flag |= FTRACE_FL_REGS;
 
-		if (!(rec->flags & FTRACE_FL_TRAMP) != 
+		if (!(rec->flags & FTRACE_FL_TRAMP) !=
 		    !(rec->flags & FTRACE_FL_TRAMP_EN))
 			flag |= FTRACE_FL_TRAMP;
+
+		/*
+		 * Direct calls are special, as count matters.
+		 * We must test the record for direct, if the
+		 * DIRECT and DIRECT_EN do not match, but only
+		 * if the count is 1. That's because, if the
+		 * count is something other than one, we do not
+		 * want the direct enabled (it will be done via the
+		 * direct helper). But if DIRECT_EN is set, and
+		 * the count is not one, we need to clear it.
+		 */
+		if (ftrace_rec_count(rec) == 1) {
+			if (!(rec->flags & FTRACE_FL_DIRECT) !=
+			    !(rec->flags & FTRACE_FL_DIRECT_EN))
+				flag |= FTRACE_FL_DIRECT;
+		} else if (rec->flags & FTRACE_FL_DIRECT_EN) {
+			flag |= FTRACE_FL_DIRECT;
+		}
 	}
 
 	/* If the state of this record hasn't changed, then do nothing */
@@ -2125,6 +2160,25 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update)
 				else
 					rec->flags &= ~FTRACE_FL_TRAMP_EN;
 			}
+			if (flag & FTRACE_FL_DIRECT) {
+				/*
+				 * If there's only one user (direct_ops helper)
+				 * then we can call the direct function
+				 * directly (no ftrace trampoline).
+				 */
+				if (ftrace_rec_count(rec) == 1) {
+					if (rec->flags & FTRACE_FL_DIRECT)
+						rec->flags |= FTRACE_FL_DIRECT_EN;
+					else
+						rec->flags &= ~FTRACE_FL_DIRECT_EN;
+				} else {
+					/*
+					 * Can only call directly if there's
+					 * only one callback to the function.
+					 */
+					rec->flags &= ~FTRACE_FL_DIRECT_EN;
+				}
+			}
 		}
 
 		/*
@@ -2154,7 +2208,7 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update)
 			 * and REGS states. The _EN flags must be disabled though.
 			 */
 			rec->flags &= ~(FTRACE_FL_ENABLED | FTRACE_FL_TRAMP_EN |
-					FTRACE_FL_REGS_EN);
+					FTRACE_FL_REGS_EN | FTRACE_FL_DIRECT_EN);
 	}
 
 	ftrace_bug_type = FTRACE_BUG_NOP;
@@ -2309,6 +2363,51 @@ ftrace_find_tramp_ops_new(struct dyn_ftrace *rec)
 	return NULL;
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+/* Protected by rcu_tasks for reading, and direct_mutex for writing */
+static struct ftrace_hash *direct_functions = EMPTY_HASH;
+static DEFINE_MUTEX(direct_mutex);
+
+/*
+ * Search the direct_functions hash to see if the given instruction pointer
+ * has a direct caller attached to it.
+ */
+static unsigned long find_rec_direct(unsigned long ip)
+{
+	struct ftrace_func_entry *entry;
+
+	entry = __ftrace_lookup_ip(direct_functions, ip);
+	if (!entry)
+		return 0;
+
+	return entry->direct;
+}
+
+static void call_direct_funcs(unsigned long ip, unsigned long pip,
+			      struct ftrace_ops *ops, struct pt_regs *regs)
+{
+	unsigned long addr;
+
+	addr = find_rec_direct(ip);
+	if (!addr)
+		return;
+
+	arch_ftrace_set_direct_caller(regs, addr);
+}
+
+struct ftrace_ops direct_ops = {
+	.func		= call_direct_funcs,
+	.flags		= FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_RECURSION_SAFE
+			  | FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS
+			  | FTRACE_OPS_FL_PERMANENT,
+};
+#else
+static inline unsigned long find_rec_direct(unsigned long ip)
+{
+	return 0;
+}
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
 /**
  * ftrace_get_addr_new - Get the call address to set to
  * @rec:  The ftrace record descriptor
@@ -2322,6 +2421,15 @@ ftrace_find_tramp_ops_new(struct dyn_ftrace *rec)
 unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec)
 {
 	struct ftrace_ops *ops;
+	unsigned long addr;
+
+	if ((rec->flags & FTRACE_FL_DIRECT) &&
+	    (ftrace_rec_count(rec) == 1)) {
+		addr = find_rec_direct(rec->ip);
+		if (addr)
+			return addr;
+		WARN_ON_ONCE(1);
+	}
 
 	/* Trampolines take precedence over regs */
 	if (rec->flags & FTRACE_FL_TRAMP) {
@@ -2354,6 +2462,15 @@ unsigned long ftrace_get_addr_new(struct dyn_ftrace *rec)
 unsigned long ftrace_get_addr_curr(struct dyn_ftrace *rec)
 {
 	struct ftrace_ops *ops;
+	unsigned long addr;
+
+	/* Direct calls take precedence over trampolines */
+	if (rec->flags & FTRACE_FL_DIRECT_EN) {
+		addr = find_rec_direct(rec->ip);
+		if (addr)
+			return addr;
+		WARN_ON_ONCE(1);
+	}
 
 	/* Trampolines take precedence over regs */
 	if (rec->flags & FTRACE_FL_TRAMP_EN) {
@@ -3465,10 +3582,11 @@ static int t_show(struct seq_file *m, void *v)
 	if (iter->flags & FTRACE_ITER_ENABLED) {
 		struct ftrace_ops *ops;
 
-		seq_printf(m, " (%ld)%s%s",
+		seq_printf(m, " (%ld)%s%s%s",
 			   ftrace_rec_count(rec),
 			   rec->flags & FTRACE_FL_REGS ? " R" : "  ",
-			   rec->flags & FTRACE_FL_IPMODIFY ? " I" : "  ");
+			   rec->flags & FTRACE_FL_IPMODIFY ? " I" : "  ",
+			   rec->flags & FTRACE_FL_DIRECT ? " D" : "  ");
 		if (rec->flags & FTRACE_FL_TRAMP_EN) {
 			ops = ftrace_find_tramp_ops_any(rec);
 			if (ops) {
@@ -3484,6 +3602,13 @@ static int t_show(struct seq_file *m, void *v)
 		} else {
 			add_trampoline_func(m, NULL, rec);
 		}
+		if (rec->flags & FTRACE_FL_DIRECT) {
+			unsigned long direct;
+
+			direct = find_rec_direct(rec->ip);
+			if (direct)
+				seq_printf(m, "\n\tdirect-->%pS", (void *)direct);
+		}
 	}	
 
 	seq_putc(m, '\n');
@@ -4815,6 +4940,143 @@ ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove,
 	return ftrace_set_hash(ops, NULL, 0, ip, remove, reset, enable);
 }
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+/**
+ * register_ftrace_direct - Call a custom trampoline directly
+ * @ip: The address of the nop at the beginning of a function
+ * @addr: The address of the trampoline to call at @ip
+ *
+ * This is used to connect a direct call from the nop location (@ip)
+ * at the start of ftrace traced functions. The location that it calls
+ * (@addr) must be able to handle a direct call, and save the parameters
+ * of the function being traced, and restore them (or inject new ones
+ * if needed), before returning.
+ *
+ * Returns:
+ *  0 on success
+ *  -EBUSY - Another direct function is already attached (there can be only one)
+ *  -ENODEV - @ip does not point to a ftrace nop location (or not supported)
+ *  -ENOMEM - There was an allocation failure.
+ */
+int register_ftrace_direct(unsigned long ip, unsigned long addr)
+{
+	struct ftrace_func_entry *entry;
+	struct ftrace_hash *free_hash = NULL;
+	struct dyn_ftrace *rec;
+	int ret = -EBUSY;
+
+	mutex_lock(&direct_mutex);
+
+	/* See if there's a direct function at @ip already */
+	if (find_rec_direct(ip))
+		goto out_unlock;
+
+	ret = -ENODEV;
+	rec = lookup_rec(ip, ip);
+	if (!rec)
+		goto out_unlock;
+
+	/*
+	 * Check if the rec says it has a direct call but we didn't
+	 * find one earlier?
+	 */
+	if (WARN_ON(rec->flags & FTRACE_FL_DIRECT))
+		goto out_unlock;
+
+	/* Make sure the ip points to the exact record */
+	ip = rec->ip;
+
+	ret = -ENOMEM;
+	if (ftrace_hash_empty(direct_functions) ||
+	    direct_functions->count > 2 * (1 << direct_functions->size_bits)) {
+		struct ftrace_hash *new_hash;
+		int size = ftrace_hash_empty(direct_functions) ? 0 :
+			direct_functions->count + 1;
+
+		if (size < 32)
+			size = 32;
+
+		new_hash = dup_hash(direct_functions, size);
+		if (!new_hash)
+			goto out_unlock;
+
+		free_hash = direct_functions;
+		direct_functions = new_hash;
+	}
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		goto out_unlock;
+
+	entry->ip = ip;
+	entry->direct = addr;
+	__add_hash_entry(direct_functions, entry);
+
+	ret = ftrace_set_filter_ip(&direct_ops, ip, 0, 0);
+	if (ret)
+		remove_hash_entry(direct_functions, entry);
+
+	if (!ret && !(direct_ops.flags & FTRACE_OPS_FL_ENABLED)) {
+		ret = register_ftrace_function(&direct_ops);
+		if (ret)
+			ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
+	}
+
+	if (ret)
+		kfree(entry);
+ out_unlock:
+	mutex_unlock(&direct_mutex);
+
+	if (free_hash) {
+		synchronize_rcu_tasks();
+		free_ftrace_hash(free_hash);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(register_ftrace_direct);
+
+int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
+{
+	struct ftrace_func_entry *entry;
+	struct dyn_ftrace *rec;
+	int ret = -ENODEV;
+
+	mutex_lock(&direct_mutex);
+
+	entry = __ftrace_lookup_ip(direct_functions, ip);
+	if (!entry) {
+		/* OK if it is off by a little */
+		rec = lookup_rec(ip, ip);
+		if (!rec || rec->ip == ip)
+			goto out_unlock;
+
+		entry = __ftrace_lookup_ip(direct_functions, rec->ip);
+		if (!entry) {
+			WARN_ON(rec->flags & FTRACE_FL_DIRECT);
+			goto out_unlock;
+		}
+
+		WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
+	}
+
+	if (direct_functions->count == 1)
+		unregister_ftrace_function(&direct_ops);
+
+	ret = ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
+
+	WARN_ON(ret);
+
+	remove_hash_entry(direct_functions, entry);
+
+ out_unlock:
+	mutex_unlock(&direct_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(unregister_ftrace_direct);
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
+
 /**
  * ftrace_set_filter_ip - set a function to filter on in ftrace by address
  * @ops - the ops to set the filter with
-- 
2.23.0

[PATCH 04/10] ftrace: Add ftrace_find_direct_func()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

As function_graph tracer modifies the return address to insert a trampoline
to trace the return of a function, it must be aware of a direct caller, as
when it gets called, the function's return address may not be at on the
stack where it expects. It may have to see if that return address points to
the a direct caller and adjust if it is.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  6 ++++
 kernel/trace/ftrace.c  | 79 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index efe3e521aff4..8b37b8105398 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -51,6 +51,7 @@ static inline void early_trace_init(void) { }
 
 struct module;
 struct ftrace_hash;
+struct ftrace_direct_func;
 
 #if defined(CONFIG_FUNCTION_TRACER) && defined(CONFIG_MODULES) && \
 	defined(CONFIG_DYNAMIC_FTRACE)
@@ -248,6 +249,7 @@ static inline void ftrace_free_mem(struct module *mod, void *start, void *end) {
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 int register_ftrace_direct(unsigned long ip, unsigned long addr);
 int unregister_ftrace_direct(unsigned long ip, unsigned long addr);
+struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr);
 #else
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
 {
@@ -257,6 +259,10 @@ static inline int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 {
 	return -ENODEV;
 }
+static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr)
+{
+	return NULL;
+}
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 #ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a06671f3a281..f57ab704dc2d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4941,6 +4941,46 @@ ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove,
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+
+struct ftrace_direct_func {
+	struct list_head	next;
+	unsigned long		addr;
+	int			count;
+};
+
+static LIST_HEAD(ftrace_direct_funcs);
+
+/**
+ * ftrace_find_direct_func - test an address if it is a registered direct caller
+ * @addr: The address of a registered direct caller
+ *
+ * This searches to see if a ftrace direct caller has been registered
+ * at a specific address, and if so, it returns a descriptor for it.
+ *
+ * This can be used by architecture code to see if an address is
+ * a direct caller (trampoline) attached to a fentry/mcount location.
+ * This is useful for the function_graph tracer, as it may need to
+ * do adjustments if it traced a location that also has a direct
+ * trampoline attached to it.
+ */
+struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr)
+{
+	struct ftrace_direct_func *entry;
+	bool found = false;
+
+	/* May be called by fgraph trampoline (protected by rcu tasks) */
+	list_for_each_entry_rcu(entry, &ftrace_direct_funcs, next) {
+		if (entry->addr == addr) {
+			found = true;
+			break;
+		}
+	}
+	if (found)
+		return entry;
+
+	return NULL;
+}
+
 /**
  * register_ftrace_direct - Call a custom trampoline directly
  * @ip: The address of the nop at the beginning of a function
@@ -4960,6 +5000,7 @@ ftrace_set_addr(struct ftrace_ops *ops, unsigned long ip, int remove,
  */
 int register_ftrace_direct(unsigned long ip, unsigned long addr)
 {
+	struct ftrace_direct_func *direct;
 	struct ftrace_func_entry *entry;
 	struct ftrace_hash *free_hash = NULL;
 	struct dyn_ftrace *rec;
@@ -5008,6 +5049,18 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 	if (!entry)
 		goto out_unlock;
 
+	direct = ftrace_find_direct_func(addr);
+	if (!direct) {
+		direct = kmalloc(sizeof(*direct), GFP_KERNEL);
+		if (!direct) {
+			kfree(entry);
+			goto out_unlock;
+		}
+		direct->addr = addr;
+		direct->count = 0;
+		list_add_rcu(&direct->next, &ftrace_direct_funcs);
+	}
+
 	entry->ip = ip;
 	entry->direct = addr;
 	__add_hash_entry(direct_functions, entry);
@@ -5022,8 +5075,20 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 			ftrace_set_filter_ip(&direct_ops, ip, 1, 0);
 	}
 
-	if (ret)
+	if (ret) {
 		kfree(entry);
+		if (!direct->count) {
+			list_del_rcu(&direct->next);
+			synchronize_rcu_tasks();
+			kfree(direct);
+			if (free_hash)
+				free_ftrace_hash(free_hash);
+			free_hash = NULL;
+		}
+	} else {
+		if (!direct->count)
+			direct->count++;
+	}
  out_unlock:
 	mutex_unlock(&direct_mutex);
 
@@ -5039,6 +5104,7 @@ EXPORT_SYMBOL_GPL(register_ftrace_direct);
 int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 {
 	struct ftrace_func_entry *entry;
+	struct ftrace_direct_func *direct;
 	struct dyn_ftrace *rec;
 	int ret = -ENODEV;
 
@@ -5069,6 +5135,17 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 
 	remove_hash_entry(direct_functions, entry);
 
+	direct = ftrace_find_direct_func(addr);
+	if (!WARN_ON(!direct)) {
+		/* This is the good path (see the ! before WARN) */
+		direct->count--;
+		WARN_ON(direct->count < 0);
+		if (!direct->count) {
+			list_del_rcu(&direct->next);
+			synchronize_rcu_tasks();
+			kfree(direct);
+		}
+	}
  out_unlock:
 	mutex_unlock(&direct_mutex);
 
-- 
2.23.0

[PATCH 05/10] ftrace: Add sample module that uses register_ftrace_direct()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Add a sample module that shows a simple use case for
regsiter_ftrace_direct(), and how to use it.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 samples/Kconfig                |  7 ++++++
 samples/Makefile               |  1 +
 samples/ftrace/Makefile        |  3 +++
 samples/ftrace/ftrace-direct.c | 45 ++++++++++++++++++++++++++++++++++
 4 files changed, 56 insertions(+)
 create mode 100644 samples/ftrace/Makefile
 create mode 100644 samples/ftrace/ftrace-direct.c

diff --git a/samples/Kconfig b/samples/Kconfig
index c8dacb4dda80..6481b1f7705d 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -19,6 +19,13 @@ config SAMPLE_TRACE_PRINTK
 	 This builds a module that calls trace_printk() and can be used to
 	 test various trace_printk() calls from a module.
 
+config SAMPLE_FTRACE_DIRECT
+	tristate "Build register_ftrace_direct() example"
+	depends on DYNAMIC_FTRACE_WITH_DIRECT_CALLS && m
+	help
+	  This builds an ftrace direct function example
+	  that hooks to wake_up_process and prints the parameters.
+
 config SAMPLE_KOBJECT
 	tristate "Build kobject examples"
 	help
diff --git a/samples/Makefile b/samples/Makefile
index 7d6e4ca28d69..cd496d633370 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_SAMPLE_RPMSG_CLIENT)	+= rpmsg/
 subdir-$(CONFIG_SAMPLE_SECCOMP)		+= seccomp
 obj-$(CONFIG_SAMPLE_TRACE_EVENTS)	+= trace_events/
 obj-$(CONFIG_SAMPLE_TRACE_PRINTK)	+= trace_printk/
+obj-$(CONFIG_SAMPLE_FTRACE_DIRECT)	+= ftrace/
 obj-$(CONFIG_VIDEO_PCI_SKELETON)	+= v4l/
 obj-y					+= vfio-mdev/
 subdir-$(CONFIG_SAMPLE_VFS)		+= vfs
diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
new file mode 100644
index 000000000000..3718ab39eba3
--- /dev/null
+++ b/samples/ftrace/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct.o
diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
new file mode 100644
index 000000000000..1483f067b000
--- /dev/null
+++ b/samples/ftrace/ftrace-direct.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+
+#include <linux/sched.h> /* for wake_up_process() */
+#include <linux/ftrace.h>
+
+void my_direct_func(struct task_struct *p)
+{
+	trace_printk("wakeing up %s-%d\n", p->comm, p->pid);
+}
+
+extern void my_tramp(void *);
+
+asm (
+"	.pushsection    .text, \"ax\", @progbits\n"
+"   my_tramp:"
+"	pushq %rbp\n"
+"	movq %rsp, %rbp\n"
+"	pushq %rdi\n"
+"	call my_direct_func\n"
+"	popq %rdi\n"
+"	leave\n"
+"	ret\n"
+"	.popsection\n"
+);
+
+
+static int __init ftrace_direct_init(void)
+{
+	return register_ftrace_direct((unsigned long)wake_up_process,
+				     (unsigned long)my_tramp);
+}
+
+static void __exit ftrace_direct_exit(void)
+{
+	unregister_ftrace_direct((unsigned long)wake_up_process,
+				 (unsigned long)my_tramp);
+}
+
+module_init(ftrace_direct_init);
+module_exit(ftrace_direct_exit);
+
+MODULE_AUTHOR("Steven Rostedt");
+MODULE_DESCRIPTION("Example use case of using register_ftrace_direct()");
+MODULE_LICENSE("GPL");
-- 
2.23.0

[PATCH 06/10] ftrace/selftest: Add tests to test register_ftrace_direct()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Add two test cases that test the new ftrace direct functionality if the
ftrace-direct sample module is available. One test case tests against each
available tracer (function, function_graph, mmiotrace, etc), and the other
test tests against a kprobe at the same location as the direct caller. Both
tests follow the same pattern of testing combinations:

  enable test (either the tracer or the kprobe)
  load direct function module
  unload direct function module
  disable test

  enable test
  load direct function module
  disable test
  unload direct function module

  load direct function module
  enable test
  disable test
  unload direct function module

  load direct function module
  enable test
  unload direct function module
  disable test

As most the bugs in development happened with various ways of enabling or
disabling the direct calls with function tracer in one of these
combinations.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 .../ftrace/test.d/direct/ftrace-direct.tc     | 53 ++++++++++++++
 .../ftrace/test.d/direct/kprobe-direct.tc     | 71 +++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
 create mode 100644 tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc

diff --git a/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc b/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
new file mode 100644
index 000000000000..8b8ed3cad51b
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
@@ -0,0 +1,53 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Test ftrace direct functions against tracers
+
+rmmod ftrace-direct ||:
+if ! modprobe ftrace-direct ; then
+  echo "No ftrace-direct sample module - please make CONFIG_SAMPLE_FTRACE_DIRECT=m"
+  exit_unresolved;
+fi
+
+echo "Let the module run a little"
+sleep 1
+
+grep -q "my_direct_func: wakeing up" trace
+
+rmmod ftrace-direct
+
+test_tracer() {
+	tracer=$1
+
+	# tracer -> direct -> no direct > no tracer
+	echo $tracer > current_tracer
+	modprobe ftrace-direct
+	rmmod ftrace-direct
+	echo nop > current_tracer
+
+	# tracer -> direct -> no tracer > no direct
+	echo $tracer > current_tracer
+	modprobe ftrace-direct
+	echo nop > current_tracer
+	rmmod ftrace-direct
+
+	# direct -> tracer -> no tracer > no direct
+	modprobe ftrace-direct
+	echo $tracer > current_tracer
+	echo nop > current_tracer
+	rmmod ftrace-direct
+
+	# direct -> tracer -> no direct > no notracer
+	modprobe ftrace-direct
+	echo $tracer > current_tracer
+	rmmod ftrace-direct
+	echo nop > current_tracer
+}
+
+for t in `cat available_tracers`; do
+	if [ "$t" != "nop" ]; then
+		test_tracer $t
+	fi
+done
+
+echo nop > current_tracer
+rmmod ftrace-direct ||:
diff --git a/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc b/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc
new file mode 100644
index 000000000000..1561106765e4
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc
@@ -0,0 +1,71 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Test ftrace direct functions against kprobes
+
+rmmod ftrace-direct ||:
+if ! modprobe ftrace-direct ; then
+  echo "No ftrace-direct sample module - please build with CONFIG_SAMPLE_FTRACE_DIRECT=m"
+  exit_unresolved;
+fi
+
+if [ ! -f kprobe_events ]; then
+	echo "No kprobe_events file -please build CONFIG_KPROBE_EVENTS"
+	exit_unresolved;
+fi
+
+echo "Let the module run a little"
+sleep 1
+
+grep -q "my_direct_func: wakeing up" trace
+
+rmmod ftrace-direct
+
+echo 'p:kwake wake_up_process task=$arg1' > kprobe_events
+
+start_direct() {
+	echo > trace
+	modprobe ftrace-direct
+	sleep 0.1
+	grep -q "my_direct_func: wakeing up" trace
+}
+
+stop_direct() {
+	rmmod ftrace-direct
+}
+
+enable_probe() {
+	echo > trace
+	echo 1 > events/kprobes/kwake/enable
+	sleep 0.1
+	grep -q "kwake:" trace
+}
+
+disable_probe() {
+	echo 0 > events/kprobes/kwake/enable
+}
+
+# probe -> direct -> no direct > no probe
+enable_probe
+start_direct
+stop_direct
+disable_probe
+
+# probe -> direct -> no probe > no direct
+enable_probe
+start_direct
+disable_probe
+stop_direct
+
+# direct -> probe -> no probe > no direct
+start_direct
+enable_probe
+disable_probe
+stop_direct
+
+# direct -> probe -> no direct > no noprobe
+start_direct
+enable_probe
+stop_direct
+disable_probe
+
+echo > kprobe_events
-- 
2.23.0

[PATCH 07/10] ftrace: Add another example of register_ftrace_direct() use case

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Add another module sample that registers a direct trampoline to a function
via register_ftrace_direct(). Having another module that does this allows to
test the use case of multiple direct callers registered, as more than one
direct caller goes into another path, and is needed to perform proper
testing of the register_ftrace_direct() call.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 samples/ftrace/Makefile            |  1 +
 samples/ftrace/ftrace-direct-too.c | 51 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 samples/ftrace/ftrace-direct-too.c

diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
index 3718ab39eba3..d8217c4e072e 100644
--- a/samples/ftrace/Makefile
+++ b/samples/ftrace/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct.o
+obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-too.o
diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
new file mode 100644
index 000000000000..27efa5f6ff52
--- /dev/null
+++ b/samples/ftrace/ftrace-direct-too.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+
+#include <linux/mm.h> /* for handle_mm_fault() */
+#include <linux/ftrace.h>
+
+void my_direct_func(struct vm_area_struct *vma,
+			unsigned long address, unsigned int flags)
+{
+	trace_printk("handle mm fault vma=%p address=%lx flags=%x\n",
+		     vma, address, flags);
+}
+
+extern void my_tramp(void *);
+
+asm (
+"	.pushsection    .text, \"ax\", @progbits\n"
+"   my_tramp:"
+"	pushq %rbp\n"
+"	movq %rsp, %rbp\n"
+"	pushq %rdi\n"
+"	pushq %rsi\n"
+"	pushq %rdx\n"
+"	call my_direct_func\n"
+"	popq %rdx\n"
+"	popq %rsi\n"
+"	popq %rdi\n"
+"	leave\n"
+"	ret\n"
+"	.popsection\n"
+);
+
+
+static int __init ftrace_direct_init(void)
+{
+	return register_ftrace_direct((unsigned long)handle_mm_fault,
+				     (unsigned long)my_tramp);
+}
+
+static void __exit ftrace_direct_exit(void)
+{
+	unregister_ftrace_direct((unsigned long)handle_mm_fault,
+				 (unsigned long)my_tramp);
+}
+
+module_init(ftrace_direct_init);
+module_exit(ftrace_direct_exit);
+
+MODULE_AUTHOR("Steven Rostedt");
+MODULE_DESCRIPTION("Another example use case of using register_ftrace_direct()");
+MODULE_LICENSE("GPL");
-- 
2.23.0

[PATCH 08/10] ftrace/selftests: Update the direct call selftests to test two direct calls

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The register_ftrace_direct() takes a different path if there's already a
direct call registered, but this was not tested in the self tests. Now that
there's a second direct caller test module, we can use this to test not only
one direct caller, but two.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 .../ftrace/test.d/direct/ftrace-direct.tc     | 16 +++++
 .../ftrace/test.d/direct/kprobe-direct.tc     | 59 +++++++++++--------
 2 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc b/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
index 8b8ed3cad51b..cbc7a30c2e0f 100644
--- a/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
+++ b/tools/testing/selftests/ftrace/test.d/direct/ftrace-direct.tc
@@ -51,3 +51,19 @@ done
 
 echo nop > current_tracer
 rmmod ftrace-direct ||:
+
+# Now do the same thing with another direct function registered
+echo "Running with another ftrace direct function"
+
+rmmod ftrace-direct-too ||:
+modprobe ftrace-direct-too
+
+for t in `cat available_tracers`; do
+	if [ "$t" != "nop" ]; then
+		test_tracer $t
+	fi
+done
+
+echo nop > current_tracer
+rmmod ftrace-direct ||:
+rmmod ftrace-direct-too ||:
diff --git a/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc b/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc
index 1561106765e4..d901416cb014 100644
--- a/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc
+++ b/tools/testing/selftests/ftrace/test.d/direct/kprobe-direct.tc
@@ -44,28 +44,41 @@ disable_probe() {
 	echo 0 > events/kprobes/kwake/enable
 }
 
-# probe -> direct -> no direct > no probe
-enable_probe
-start_direct
-stop_direct
-disable_probe
-
-# probe -> direct -> no probe > no direct
-enable_probe
-start_direct
-disable_probe
-stop_direct
-
-# direct -> probe -> no probe > no direct
-start_direct
-enable_probe
-disable_probe
-stop_direct
-
-# direct -> probe -> no direct > no noprobe
-start_direct
-enable_probe
-stop_direct
-disable_probe
+test_kprobes() {
+	# probe -> direct -> no direct > no probe
+	enable_probe
+	start_direct
+	stop_direct
+	disable_probe
+
+	# probe -> direct -> no probe > no direct
+	enable_probe
+	start_direct
+	disable_probe
+	stop_direct
+
+	# direct -> probe -> no probe > no direct
+	start_direct
+	enable_probe
+	disable_probe
+	stop_direct
+
+	# direct -> probe -> no direct > no noprobe
+	start_direct
+	enable_probe
+	stop_direct
+	disable_probe
+}
+
+test_kprobes
+
+# Now do this with a second registered direct function
+echo "Running with another ftrace direct function"
+
+modprobe ftrace-direct-too
+
+test_kprobes
+
+rmmod ftrace-direct-too
 
 echo > kprobe_events
-- 
2.23.0

[PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
may be called directly from an ftrace mcount/fentry location.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/Kconfig              |  1 +
 arch/x86/include/asm/ftrace.h | 13 +++++++++++++
 arch/x86/kernel/ftrace.c      | 12 ++++++++++++
 arch/x86/kernel/ftrace_64.S   | 33 ++++++++++++++++++++++++++-------
 include/linux/ftrace.h        |  6 ++++++
 5 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d6e1faa28c58..329d9c729ba3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -158,6 +158,7 @@ config X86
 	select HAVE_DMA_CONTIGUOUS
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_EISA
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index c38a66661576..c2a7458f912c 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -28,6 +28,19 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 	return addr;
 }
 
+/*
+ * When a ftrace registered caller is tracing a function that is
+ * also set by a register_ftrace_direct() call, it needs to be
+ * differentiated in the ftrace_caller trampoline. To do this, we
+ * place the direct caller in the ORIG_AX part of pt_regs. This
+ * tells the ftrace_caller that there's a direct caller.
+ */
+static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs, unsigned long addr)
+{
+	/* Emulate a call */
+	regs->orig_ax = addr;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 struct dyn_arch_ftrace {
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 024c3053dbba..fef283f6341d 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -1042,6 +1042,18 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
+	/*
+	 * If the return location is actually pointing directly to
+	 * the start of a direct trampoline (if we trace the trampoline
+	 * it will still be offset by MCOUNT_INSN_SIZE), then the
+	 * return address is actually off by one word, and we
+	 * need to adjust for that.
+	 */
+	if (ftrace_find_direct_func(self_addr + MCOUNT_INSN_SIZE)) {
+		self_addr = *parent;
+		parent++;
+	}
+
 	/*
 	 * Protect against fault, even if it shouldn't
 	 * happen. This tool is too much intrusive to
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 809d54397dba..5d946ab40b52 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
 	movq %rdi, RDI(%rsp)
 	movq %r8, R8(%rsp)
 	movq %r9, R9(%rsp)
+	movq $0, ORIG_RAX(%rsp)
 	/*
 	 * Save the original RBP. Even though the mcount ABI does not
 	 * require this, it helps out callers.
@@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
 	subq $MCOUNT_INSN_SIZE, %rdi
 	.endm
 
-.macro restore_mcount_regs
+.macro restore_mcount_regs save=0
+
 	movq R9(%rsp), %r9
 	movq R8(%rsp), %r8
 	movq RDI(%rsp), %rdi
@@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
 	movq RCX(%rsp), %rcx
 	movq RAX(%rsp), %rax
 
-	/* ftrace_regs_caller can modify %rbp */
-	movq RBP(%rsp), %rbp
-
-	addq $MCOUNT_REG_SIZE, %rsp
+	addq $MCOUNT_REG_SIZE-\save, %rsp
 
 	.endm
 
@@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
 	movq R10(%rsp), %r10
 	movq RBX(%rsp), %rbx
 
-	restore_mcount_regs
+	movq RBP(%rsp), %rbp
+
+	movq ORIG_RAX(%rsp), %rax
+	movq %rax, MCOUNT_REG_SIZE-8(%rsp)
+
+	/* If ORIG_RAX is anything but zero, make this a call to that */
+	movq ORIG_RAX(%rsp), %rax
+	cmpq	$0, %rax
+	je	1f
+
+	/* Swap the flags with orig_rax */
+	movq MCOUNT_REG_SIZE(%rsp), %rdi
+	movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
+	movq %rax, MCOUNT_REG_SIZE(%rsp)
+
+	restore_mcount_regs 8
+
+	jmp	2f
+
+1:	restore_mcount_regs
+
 
 	/* Restore flags */
-	popfq
+2:	popfq
 
 	/*
 	 * As this jmp to ftrace_epilogue can be a short jump
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8b37b8105398..2bc7bd6b8387 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -272,6 +272,12 @@ static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long a
  * via ftrace (because there's other callbacks besides the
  * direct call), can inform the architecture's trampoline that this
  * routine has a direct caller, and what the caller is.
+ *
+ * For example, in x86, it returns the direct caller
+ * callback function via the regs->orig_ax parameter.
+ * Then in the ftrace trampoline, if this is set, it makes
+ * the return from the trampoline jump to the direct caller
+ * instead of going back to the function it just traced.
  */
 static inline void arch_ftrace_set_direct_caller(struct pt_regs *regs,
 						 unsigned long addr) { }
-- 
2.23.0

[PATCH 10/10] ftrace/x86: Add a counter to test function_graph with direct

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

As testing for direct calls from the function graph tracer adds a little
overhead (which is a lot when tracing every function), add a counter that
can be used to test if function_graph tracer needs to test for a direct
caller or not.

It would have been nicer if we could use a static branch, but the static
branch logic fails when used within the function graph tracer trampoline.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c | 8 +++++---
 include/linux/ftrace.h   | 2 ++
 kernel/trace/ftrace.c    | 4 ++++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index fef283f6341d..060a361d9d11 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -1049,9 +1049,11 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	 * return address is actually off by one word, and we
 	 * need to adjust for that.
 	 */
-	if (ftrace_find_direct_func(self_addr + MCOUNT_INSN_SIZE)) {
-		self_addr = *parent;
-		parent++;
+	if (ftrace_direct_func_count) {
+		if (ftrace_find_direct_func(self_addr + MCOUNT_INSN_SIZE)) {
+			self_addr = *parent;
+			parent++;
+		}
 	}
 
 	/*
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 2bc7bd6b8387..55647e185141 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -247,10 +247,12 @@ static inline void ftrace_free_mem(struct module *mod, void *start, void *end) {
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+extern int ftrace_direct_func_count;
 int register_ftrace_direct(unsigned long ip, unsigned long addr);
 int unregister_ftrace_direct(unsigned long ip, unsigned long addr);
 struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr);
 #else
+# define ftrace_direct_func_count 0
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
 {
 	return -ENODEV;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f57ab704dc2d..0e18ec707a88 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2367,6 +2367,7 @@ ftrace_find_tramp_ops_new(struct dyn_ftrace *rec)
 /* Protected by rcu_tasks for reading, and direct_mutex for writing */
 static struct ftrace_hash *direct_functions = EMPTY_HASH;
 static DEFINE_MUTEX(direct_mutex);
+int ftrace_direct_func_count;
 
 /*
  * Search the direct_functions hash to see if the given instruction pointer
@@ -5059,6 +5060,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 		direct->addr = addr;
 		direct->count = 0;
 		list_add_rcu(&direct->next, &ftrace_direct_funcs);
+		ftrace_direct_func_count++;
 	}
 
 	entry->ip = ip;
@@ -5084,6 +5086,7 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 			if (free_hash)
 				free_ftrace_hash(free_hash);
 			free_hash = NULL;
+			ftrace_direct_func_count--;
 		}
 	} else {
 		if (!direct->count)
@@ -5144,6 +5147,7 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 			list_del_rcu(&direct->next);
 			synchronize_rcu_tasks();
 			kfree(direct);
+			ftrace_direct_func_count--;
 		}
 	}
  out_unlock:
-- 
2.23.0

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Josh Poimboeuf]

On Fri, Nov 08, 2019 at 04:28:34PM -0500, Steven Rostedt wrote:
> 
> Alexei mentioned that he would like a way to access the ftrace fentry
> code to jump directly to a custom eBPF trampoline instead of using
> ftrace regs caller, as he said it would be faster.
> 
> I proposed a new register_ftrace_direct() function that would allow
> this to happen and still work with the ftrace infrastructure. I posted
> a proof of concept patch here:
> 
>  https://lore.kernel.org/r/20191023122307.756b4978@gandalf.local.home
> 
> This patch series is a more complete version, and the start of the
> actual implementation. I haven't run it through my full test suite but
> it passes my smoke tests and some other custom tests I built.
> 
> I also realized that I need to make the sample modules depend on X86_64
> as it has inlined assembly in it that requires that dependency.
> 
> This is based on 5.4-rc6 plus the permanent patches that prevent
> a ftrace_ops from being disabled by /proc/sys/kernel/ftrace_enabled
> 
> Below is the tree that contains this code.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> ftrace/direct

Here's the fix for the objtool warning:

From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] ftrace/x86: Tell objtool to ignore nondeterministic ftrace stack layout

Objtool complains about the new ftrace direct trampoline code:

  arch/x86/kernel/ftrace_64.o: warning: objtool: ftrace_regs_caller()+0x190: stack state mismatch: cfa1=7+16 cfa2=7+24

Typically, code has a deterministic stack layout, such that at a given
instruction address, the stack frame size is always the same.

That's not the case for the new ftrace_regs_caller() code after it
adjusts the stack for the direct case.  Just plead ignorance and assume
it's always the non-direct path.  Note this creates a tiny window for
ORC to get confused.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/include/asm/unwind_hints.h |  8 ++++++++
 arch/x86/kernel/ftrace_64.S         | 12 +++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index 0bcdb1279361..f5e2eb12cb71 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,6 +86,14 @@
 	UNWIND_HINT sp_offset=\sp_offset
 .endm
 
+.macro UNWIND_HINT_SAVE
+	UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
+.endm
+
+.macro UNWIND_HINT_RESTORE
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.endm
+
 #else /* !__ASSEMBLY__ */
 
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)		\
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 5d946ab40b52..1c79624a36b2 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -175,6 +175,8 @@ ENTRY(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq
 
+	UNWIND_HINT_SAVE
+
 	/* added 8 bytes to save flags */
 	save_mcount_regs 8
 	/* save_mcount_regs fills in first two parameters */
@@ -249,8 +251,16 @@ GLOBAL(ftrace_regs_call)
 1:	restore_mcount_regs
 
 
+2:
+	/*
+	 * The stack layout is nondetermistic here, depending on which path was
+	 * taken.  This confuses objtool and ORC, rightfully so.  For now,
+	 * pretend the stack always looks like the non-direct case.
+	 */
+	UNWIND_HINT_RESTORE
+
 	/* Restore flags */
-2:	popfq
+	popfq
 
 	/*
 	 * As this jmp to ftrace_epilogue can be a short jump
-- 
2.20.1

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Fri, 8 Nov 2019 16:51:00 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Here's the fix for the objtool warning:

Thanks, I applied it (will push it soon).

-- Steve

> 
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] ftrace/x86: Tell objtool to ignore nondeterministic ftrace stack layout
> 
> Objtool complains about the new ftrace direct trampoline code:
> 
>   arch/x86/kernel/ftrace_64.o: warning: objtool: ftrace_regs_caller()+0x190: stack state mismatch: cfa1=7+16 cfa2=7+24
> 
> Typically, code has a deterministic stack layout, such that at a given
> instruction address, the stack frame size is always the same.
> 
> That's not the case for the new ftrace_regs_caller() code after it
> adjusts the stack for the direct case.  Just plead ignorance and assume
> it's always the non-direct path.  Note this creates a tiny window for
> ORC to get confused.
> 
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/include/asm/unwind_hints.h |  8 ++++++++
>  arch/x86/kernel/ftrace_64.S         | 12 +++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
> index 0bcdb1279361..f5e2eb12cb71 100644
> --- a/arch/x86/include/asm/unwind_hints.h
> +++ b/arch/x86/include/asm/unwind_hints.h
> @@ -86,6 +86,14 @@
>  	UNWIND_HINT sp_offset=\sp_offset
>  .endm
>  
> +.macro UNWIND_HINT_SAVE
> +	UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
> +.endm
> +
> +.macro UNWIND_HINT_RESTORE
> +	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
> +.endm
> +
>  #else /* !__ASSEMBLY__ */
>  
>  #define UNWIND_HINT(sp_reg, sp_offset, type, end)		\
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index 5d946ab40b52..1c79624a36b2 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -175,6 +175,8 @@ ENTRY(ftrace_regs_caller)
>  	/* Save the current flags before any operations that can change them */
>  	pushfq
>  
> +	UNWIND_HINT_SAVE
> +
>  	/* added 8 bytes to save flags */
>  	save_mcount_regs 8
>  	/* save_mcount_regs fills in first two parameters */
> @@ -249,8 +251,16 @@ GLOBAL(ftrace_regs_call)
>  1:	restore_mcount_regs
>  
>  
> +2:
> +	/*
> +	 * The stack layout is nondetermistic here, depending on which path was
> +	 * taken.  This confuses objtool and ORC, rightfully so.  For now,
> +	 * pretend the stack always looks like the non-direct case.
> +	 */
> +	UNWIND_HINT_RESTORE
> +
>  	/* Restore flags */
> -2:	popfq
> +	popfq
>  
>  	/*
>  	 * As this jmp to ftrace_epilogue can be a short jump

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Alexei Starovoitov]

On Fri, Nov 08, 2019 at 04:28:37PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Add the start of the functionality to allow other trampolines to use the
> ftrace mcount/fentry/nop location. This adds two new functions:
> 
>  register_ftrace_direct() and unregister_ftrace_direct()
> 
> Both take two parameters: the first is the instruction address of where the
> mcount/fentry/nop exists, and the second is the trampoline to have that
> location called.
> 
> This will handle cases where ftrace is already used on that same location,
> and will make it still work, where the registered direct called trampoline
> will get called after all the registered ftrace callers are handled.
> 
> Currently, it will not allow for IP_MODIFY functions to be called at the
> same locations, which include some kprobes and live kernel patching.
> 
> At this point, no architecture supports this. This is only the start of
> implementing the framework.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
...
> +struct ftrace_ops direct_ops = {
> +	.func		= call_direct_funcs,
> +	.flags		= FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_RECURSION_SAFE
> +			  | FTRACE_OPS_FL_DIRECT | FTRACE_OPS_FL_SAVE_REGS
> +			  | FTRACE_OPS_FL_PERMANENT,
> +};

The whole set looks great. Thank you for adding FL_PERMANENT to it.
Is there a way to do a replacement of direct call?
If I use unregister(old)+register(new) some events will be missed.
If I use register(new)+unregister(old) for short period of time both new and
old will be triggering on all cpus which will likely confuse bpf tracing.
Something like modify_ftrace_direct() should solve it. It's still racy. In a
sense that some cpus will be executing old while other cpus will be executing
new, but per-cpu there will be no double accounting. How difficult would be
to add such feature?

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Fri, 8 Nov 2019 18:29:09 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> Is there a way to do a replacement of direct call?

I'm curious to what the use case would be. The direct call added would
still need to be a trampoline, as the parameters need to be saved
before calling any other code, and then restored before going back to
the traced function. Couldn't you do a text poke on that trampoline to
change what gets called?

> If I use unregister(old)+register(new) some events will be missed.

> If I use register(new)+unregister(old) for short period of time both new and

Actually, as we only allow a single direct call to be added at any time,
the register(new) would fail if there was already an old at the
location.

> old will be triggering on all cpus which will likely confuse bpf tracing.
> Something like modify_ftrace_direct() should solve it. It's still racy. In a
> sense that some cpus will be executing old while other cpus will be executing
> new, but per-cpu there will be no double accounting. How difficult would be
> to add such feature?

All this said, it would actually be pretty trivial to implement this,
as when another ftrace_ops is attached to the direct call location, it
falls back to the direct helper. To implement a modify_ftrace_direct(),
all that would be needed to do is to:

1) Attached a ftrace_ops stub to the same function that has the direct
caller, that will cause ftrace to got to the loop routine, and the
direct helper would then define what gets called by what what
registered in the direct_functions array.

2) Change what gets called in the direct_functions array, and at that
moment, the helper function would be using that. May require syncing
CPUs to get all CPUs seeing the same thing.

3) Remove the ftrace_ops stub, which would put back the direct caller
in the fentry location, but this time with the new function.

-- Steve

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Peter Zijlstra]

On Fri, Nov 08, 2019 at 04:51:00PM -0600, Josh Poimboeuf wrote:

> From: Josh Poimboeuf <jpoimboe@redhat.com>
> Subject: [PATCH] ftrace/x86: Tell objtool to ignore nondeterministic ftrace stack layout
> 
> Objtool complains about the new ftrace direct trampoline code:
> 
>   arch/x86/kernel/ftrace_64.o: warning: objtool: ftrace_regs_caller()+0x190: stack state mismatch: cfa1=7+16 cfa2=7+24
> 
> Typically, code has a deterministic stack layout, such that at a given
> instruction address, the stack frame size is always the same.
> 
> That's not the case for the new ftrace_regs_caller() code after it
> adjusts the stack for the direct case.  Just plead ignorance and assume
> it's always the non-direct path.  Note this creates a tiny window for
> ORC to get confused.

How is that not a problem for livepatch?

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Josh Poimboeuf]

On Mon, Nov 11, 2019 at 09:47:28AM +0100, Peter Zijlstra wrote:
> On Fri, Nov 08, 2019 at 04:51:00PM -0600, Josh Poimboeuf wrote:
> 
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > Subject: [PATCH] ftrace/x86: Tell objtool to ignore nondeterministic ftrace stack layout
> > 
> > Objtool complains about the new ftrace direct trampoline code:
> > 
> >   arch/x86/kernel/ftrace_64.o: warning: objtool: ftrace_regs_caller()+0x190: stack state mismatch: cfa1=7+16 cfa2=7+24
> > 
> > Typically, code has a deterministic stack layout, such that at a given
> > instruction address, the stack frame size is always the same.
> > 
> > That's not the case for the new ftrace_regs_caller() code after it
> > adjusts the stack for the direct case.  Just plead ignorance and assume
> > it's always the non-direct path.  Note this creates a tiny window for
> > ORC to get confused.
> 
> How is that not a problem for livepatch?

If this code were preempted at the point where the ORC data is wrong,
and then livepatch tried to unwind it, the reliable unwinder would error
out because it doesn't get all the way to the user-space pt_regs.  Then
it will just try again later.

I view this as a temporary fix; the code should be restructured to
follow normal rules.

-- 
Josh

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Miroslav Benes]

> @@ -1757,6 +1761,18 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  				return false;
>  			rec->flags--;
>  
> +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> +				rec->flags &= ~FTRACE_FL_DIRECT;
> +
> +			/*
> +			 * Only the internal direct_ops should have the
> +			 * DIRECT flag set. Thus, if it is removing a
> +			 * function, then that function should no longer
> +			 * be direct.
> +			 */
> +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> +				rec->flags &= ~FTRACE_FL_DIRECT;
> +

The flag is dropped twice here.

Miroslav

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Wed, 13 Nov 2019 15:13:44 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> > @@ -1757,6 +1761,18 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> >  				return false;
> >  			rec->flags--;
> >  
> > +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> > +				rec->flags &= ~FTRACE_FL_DIRECT;
> > +
> > +			/*
> > +			 * Only the internal direct_ops should have the
> > +			 * DIRECT flag set. Thus, if it is removing a
> > +			 * function, then that function should no longer
> > +			 * be direct.
> > +			 */
> > +			if (ops->flags & FTRACE_OPS_FL_DIRECT)
> > +				rec->flags &= ~FTRACE_FL_DIRECT;
> > +  
> 
> The flag is dropped twice here.

Ah, thanks for pointing this out. It appears that a rebase I did (where
I modified and rebased on a previous version) add this as a new change
(with the comment).

-- Steve

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Miroslav Benes]

On Fri, 8 Nov 2019, Steven Rostedt wrote:

> 
> Alexei mentioned that he would like a way to access the ftrace fentry
> code to jump directly to a custom eBPF trampoline instead of using
> ftrace regs caller, as he said it would be faster.
> 
> I proposed a new register_ftrace_direct() function that would allow
> this to happen and still work with the ftrace infrastructure. I posted
> a proof of concept patch here:
> 
>  https://lore.kernel.org/r/20191023122307.756b4978@gandalf.local.home
> 
> This patch series is a more complete version, and the start of the
> actual implementation. I haven't run it through my full test suite but
> it passes my smoke tests and some other custom tests I built.
> 
> I also realized that I need to make the sample modules depend on X86_64
> as it has inlined assembly in it that requires that dependency.
> 
> This is based on 5.4-rc6 plus the permanent patches that prevent
> a ftrace_ops from being disabled by /proc/sys/kernel/ftrace_enabled
> 
> Below is the tree that contains this code.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> ftrace/direct
> 
> Head SHA1: 9492654d091cb90a487ca669c58f802fa99bcd6f
> 
> Enjoy,
> 
> -- Steve

So I tried to run the selftests and ran into the same timeout issue we had 
with live patching :/

See http://lkml.kernel.org/r/20191025115041.23186-1-mbenes@suse.cz for a possible solution.

Miroslav

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Wed, 13 Nov 2019 16:10:36 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> So I tried to run the selftests and ran into the same timeout issue we had 
> with live patching :/
> 
> See http://lkml.kernel.org/r/20191025115041.23186-1-mbenes@suse.cz for a possible solution.

Is this when you run the all the selftests?

I guess, I could add a timeout=0 here, as none of theses tests should
hang the machine, unless there's a crash.

-- Steve

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Miroslav Benes]

On Wed, 13 Nov 2019, Steven Rostedt wrote:

> On Wed, 13 Nov 2019 16:10:36 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > So I tried to run the selftests and ran into the same timeout issue we had 
> > with live patching :/
> > 
> > See http://lkml.kernel.org/r/20191025115041.23186-1-mbenes@suse.cz for a possible solution.
> 
> Is this when you run the all the selftests?

Yes, when I run all ftrace selftests (make run_tests in 
tools/testing/selftests/ftrace/).

Miroslav

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Thu, 14 Nov 2019 10:05:42 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> On Wed, 13 Nov 2019, Steven Rostedt wrote:
> 
> > On Wed, 13 Nov 2019 16:10:36 +0100 (CET)
> > Miroslav Benes <mbenes@suse.cz> wrote:
> >   
> > > So I tried to run the selftests and ran into the same timeout issue we had 
> > > with live patching :/
> > > 
> > > See http://lkml.kernel.org/r/20191025115041.23186-1-mbenes@suse.cz for a possible solution.  
> > 
> > Is this when you run the all the selftests?  
> 
> Yes, when I run all ftrace selftests (make run_tests in 
> tools/testing/selftests/ftrace/).

Thanks, I added this:

From 6105e34804bc3a9a35e8c10fcd9e10b8b5a22f8e Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Wed, 13 Nov 2019 11:48:39 -0500
Subject: [PATCH] tracing/selftests: Turn off timeout setting

As the ftrace selftests can run for a long period of time, disable the
timeout that the general selftests have. If a selftest hangs, then it
probably means the machine will hang too.

Link: https://lore.kernel.org/r/alpine.LSU.2.21.1911131604170.18679@pobox.suse.cz

Suggested-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/settings | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 tools/testing/selftests/ftrace/settings

diff --git a/tools/testing/selftests/ftrace/settings b/tools/testing/selftests/ftrace/settings
new file mode 100644
index 000000000000..e7b9417537fb
--- /dev/null
+++ b/tools/testing/selftests/ftrace/settings
@@ -0,0 +1 @@
+timeout=0
-- 
2.20.1


-- Steve

Re: [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines

[Miroslav Benes]

On Fri, 8 Nov 2019, Steven Rostedt wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
> may be called directly from an ftrace mcount/fentry location.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

[...]

> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
>  	movq %rdi, RDI(%rsp)
>  	movq %r8, R8(%rsp)
>  	movq %r9, R9(%rsp)
> +	movq $0, ORIG_RAX(%rsp)
>  	/*
>  	 * Save the original RBP. Even though the mcount ABI does not
>  	 * require this, it helps out callers.
> @@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
>  	subq $MCOUNT_INSN_SIZE, %rdi
>  	.endm
>  
> -.macro restore_mcount_regs
> +.macro restore_mcount_regs save=0
> +
>  	movq R9(%rsp), %r9
>  	movq R8(%rsp), %r8
>  	movq RDI(%rsp), %rdi
> @@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
>  	movq RCX(%rsp), %rcx
>  	movq RAX(%rsp), %rax
>  
> -	/* ftrace_regs_caller can modify %rbp */
> -	movq RBP(%rsp), %rbp
> -
> -	addq $MCOUNT_REG_SIZE, %rsp
> +	addq $MCOUNT_REG_SIZE-\save, %rsp
>  
>  	.endm
>  
> @@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
>  	movq R10(%rsp), %r10
>  	movq RBX(%rsp), %rbx
>  
> -	restore_mcount_regs
> +	movq RBP(%rsp), %rbp
> +
> +	movq ORIG_RAX(%rsp), %rax
> +	movq %rax, MCOUNT_REG_SIZE-8(%rsp)
> +
> +	/* If ORIG_RAX is anything but zero, make this a call to that */
> +	movq ORIG_RAX(%rsp), %rax
> +	cmpq	$0, %rax
> +	je	1f
> +
> +	/* Swap the flags with orig_rax */
> +	movq MCOUNT_REG_SIZE(%rsp), %rdi
> +	movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
> +	movq %rax, MCOUNT_REG_SIZE(%rsp)
> +
> +	restore_mcount_regs 8
> +
> +	jmp	2f
> +
> +1:	restore_mcount_regs
> +
>  
>  	/* Restore flags */
> -	popfq
> +2:	popfq

If I am reading the code correctly (and I was confused couple of times, so 
maybe I am not), this is what makes the direct fops incompatible with 
ipmodify and livepatching for now. Is that correct?

What are your plans regarding this?

Moreover, we could replace ftrace_regs_caller with direct fops for live 
patching when this is merged with all arch support we need. After all, all 
we need is to change the rip, which we could do easily in the direct 
trampoline. On the other hand, it would exclude coexistence of a live 
patch and a BPF filter (both direct now) on one function.

I may have missed something though.

Thanks
Miroslav

Re: [PATCH 00/10] ftrace: Add register_ftrace_direct()

[Miroslav Benes]

On Thu, 14 Nov 2019, Steven Rostedt wrote:

> On Thu, 14 Nov 2019 10:05:42 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > On Wed, 13 Nov 2019, Steven Rostedt wrote:
> > 
> > > On Wed, 13 Nov 2019 16:10:36 +0100 (CET)
> > > Miroslav Benes <mbenes@suse.cz> wrote:
> > >   
> > > > So I tried to run the selftests and ran into the same timeout issue we had 
> > > > with live patching :/
> > > > 
> > > > See http://lkml.kernel.org/r/20191025115041.23186-1-mbenes@suse.cz for a possible solution.  
> > > 
> > > Is this when you run the all the selftests?  
> > 
> > Yes, when I run all ftrace selftests (make run_tests in 
> > tools/testing/selftests/ftrace/).
> 
> Thanks, I added this:
> 
> From 6105e34804bc3a9a35e8c10fcd9e10b8b5a22f8e Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Date: Wed, 13 Nov 2019 11:48:39 -0500
> Subject: [PATCH] tracing/selftests: Turn off timeout setting
> 
> As the ftrace selftests can run for a long period of time, disable the
> timeout that the general selftests have. If a selftest hangs, then it
> probably means the machine will hang too.
> 
> Link: https://lore.kernel.org/r/alpine.LSU.2.21.1911131604170.18679@pobox.suse.cz
> 
> Suggested-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Tested-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>

...whichever you prefer

M

> ---
>  tools/testing/selftests/ftrace/settings | 1 +
>  1 file changed, 1 insertion(+)
>  create mode 100644 tools/testing/selftests/ftrace/settings
> 
> diff --git a/tools/testing/selftests/ftrace/settings b/tools/testing/selftests/ftrace/settings
> new file mode 100644
> index 000000000000..e7b9417537fb
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/settings
> @@ -0,0 +1 @@
> +timeout=0
> -- 
> 2.20.1
> 
> 
> -- Steve
> 

Re: [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines

[Steven Rostedt]

On Thu, 14 Nov 2019 16:34:58 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> On Fri, 8 Nov 2019, Steven Rostedt wrote:
> 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
> > may be called directly from an ftrace mcount/fentry location.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> [...]
> 
> > +++ b/arch/x86/kernel/ftrace_64.S
> > @@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
> >  	movq %rdi, RDI(%rsp)
> >  	movq %r8, R8(%rsp)
> >  	movq %r9, R9(%rsp)
> > +	movq $0, ORIG_RAX(%rsp)
> >  	/*
> >  	 * Save the original RBP. Even though the mcount ABI does not
> >  	 * require this, it helps out callers.
> > @@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
> >  	subq $MCOUNT_INSN_SIZE, %rdi
> >  	.endm
> >  
> > -.macro restore_mcount_regs
> > +.macro restore_mcount_regs save=0
> > +
> >  	movq R9(%rsp), %r9
> >  	movq R8(%rsp), %r8
> >  	movq RDI(%rsp), %rdi
> > @@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
> >  	movq RCX(%rsp), %rcx
> >  	movq RAX(%rsp), %rax
> >  
> > -	/* ftrace_regs_caller can modify %rbp */
> > -	movq RBP(%rsp), %rbp
> > -
> > -	addq $MCOUNT_REG_SIZE, %rsp
> > +	addq $MCOUNT_REG_SIZE-\save, %rsp
> >  
> >  	.endm
> >  
> > @@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
> >  	movq R10(%rsp), %r10
> >  	movq RBX(%rsp), %rbx
> >  
> > -	restore_mcount_regs
> > +	movq RBP(%rsp), %rbp
> > +
> > +	movq ORIG_RAX(%rsp), %rax
> > +	movq %rax, MCOUNT_REG_SIZE-8(%rsp)
> > +
> > +	/* If ORIG_RAX is anything but zero, make this a call to that */
> > +	movq ORIG_RAX(%rsp), %rax
> > +	cmpq	$0, %rax
> > +	je	1f
> > +
> > +	/* Swap the flags with orig_rax */
> > +	movq MCOUNT_REG_SIZE(%rsp), %rdi
> > +	movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
> > +	movq %rax, MCOUNT_REG_SIZE(%rsp)
> > +
> > +	restore_mcount_regs 8
> > +
> > +	jmp	2f
> > +
> > +1:	restore_mcount_regs
> > +
> >  
> >  	/* Restore flags */
> > -	popfq
> > +2:	popfq  
> 
> If I am reading the code correctly (and I was confused couple of times, so 
> maybe I am not), this is what makes the direct fops incompatible with 
> ipmodify and livepatching for now. Is that correct?

Actually, it's the fact that the return goes to some unknown trampoline
that may do something else as well.

> 
> What are your plans regarding this?

I wanted to see what the eBPF folks were doing, and then perhaps allow
the ip modify occur too. I could let it happen as well now, and then we
can see what the fallout is later ;-)

> 
> Moreover, we could replace ftrace_regs_caller with direct fops for live 
> patching when this is merged with all arch support we need. After all, all 

Note, direct call is currently only available for x86_64.

> we need is to change the rip, which we could do easily in the direct 
> trampoline. On the other hand, it would exclude coexistence of a live 
> patch and a BPF filter (both direct now) on one function.

It may also end up being more complex, and not much of a performance
benefit. I believe the BPF is injecting programs into the start of
functions, but your trampoline for live patching may be not much
different than what ftrace gives you today.

-- Steve

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Sat, 9 Nov 2019 07:33:10 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 8 Nov 2019 18:29:09 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> > Is there a way to do a replacement of direct call?  
> 
> I'm curious to what the use case would be. The direct call added would
> still need to be a trampoline, as the parameters need to be saved
> before calling any other code, and then restored before going back to
> the traced function. Couldn't you do a text poke on that trampoline to
> change what gets called?
> 
> > If I use unregister(old)+register(new) some events will be missed.  
> 
> > If I use register(new)+unregister(old) for short period of time both new and  
> 
> Actually, as we only allow a single direct call to be added at any time,
> the register(new) would fail if there was already an old at the
> location.
> 
> > old will be triggering on all cpus which will likely confuse bpf tracing.
> > Something like modify_ftrace_direct() should solve it. It's still racy. In a
> > sense that some cpus will be executing old while other cpus will be executing
> > new, but per-cpu there will be no double accounting. How difficult would be
> > to add such feature?  
> 
> All this said, it would actually be pretty trivial to implement this,
> as when another ftrace_ops is attached to the direct call location, it
> falls back to the direct helper. To implement a modify_ftrace_direct(),
> all that would be needed to do is to:
> 
> 1) Attached a ftrace_ops stub to the same function that has the direct
> caller, that will cause ftrace to got to the loop routine, and the
> direct helper would then define what gets called by what what
> registered in the direct_functions array.
> 
> 2) Change what gets called in the direct_functions array, and at that
> moment, the helper function would be using that. May require syncing
> CPUs to get all CPUs seeing the same thing.
> 
> 3) Remove the ftrace_ops stub, which would put back the direct caller
> in the fentry location, but this time with the new function.
>

Alexei,

Do you still need such a feature?

Note, I just pushed my tree to my for-next tree, and also have a
ftrace/direct branch that ends with this patch set that is the basis of
the rest of the work of my code. Feel free to build against that
branch if you need to have something built on the net tree.

-- Steve

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Alexei Starovoitov]

On Thu, Nov 14, 2019 at 10:29 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Sat, 9 Nov 2019 07:33:10 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Fri, 8 Nov 2019 18:29:09 -0800
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > > Is there a way to do a replacement of direct call?
> >
> > I'm curious to what the use case would be. The direct call added would
> > still need to be a trampoline, as the parameters need to be saved
> > before calling any other code, and then restored before going back to
> > the traced function. Couldn't you do a text poke on that trampoline to
> > change what gets called?
> >
> > > If I use unregister(old)+register(new) some events will be missed.
> >
> > > If I use register(new)+unregister(old) for short period of time both new and
> >
> > Actually, as we only allow a single direct call to be added at any time,
> > the register(new) would fail if there was already an old at the
> > location.
> >
> > > old will be triggering on all cpus which will likely confuse bpf tracing.
> > > Something like modify_ftrace_direct() should solve it. It's still racy. In a
> > > sense that some cpus will be executing old while other cpus will be executing
> > > new, but per-cpu there will be no double accounting. How difficult would be
> > > to add such feature?
> >
> > All this said, it would actually be pretty trivial to implement this,
> > as when another ftrace_ops is attached to the direct call location, it
> > falls back to the direct helper. To implement a modify_ftrace_direct(),
> > all that would be needed to do is to:
> >
> > 1) Attached a ftrace_ops stub to the same function that has the direct
> > caller, that will cause ftrace to got to the loop routine, and the
> > direct helper would then define what gets called by what what
> > registered in the direct_functions array.
> >
> > 2) Change what gets called in the direct_functions array, and at that
> > moment, the helper function would be using that. May require syncing
> > CPUs to get all CPUs seeing the same thing.
> >
> > 3) Remove the ftrace_ops stub, which would put back the direct caller
> > in the fentry location, but this time with the new function.
> >
>
> Alexei,
>
> Do you still need such a feature?
>
> Note, I just pushed my tree to my for-next tree, and also have a
> ftrace/direct branch that ends with this patch set that is the basis of
> the rest of the work of my code. Feel free to build against that
> branch if you need to have something built on the net tree.

I'm still trying to figure out what you meant
by your suggestion to implement a modify_ftrace_direct().
I was thinking something much simpler like
modify_ftrace_direct(ip, old_call, new_ca);
will just text poke that call addr from old to new if old matches
and will adjust ftrace inner bookkeeping.
I don't understand why you want to malloc/free ftrace_ops for that.

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Thu, 14 Nov 2019 10:34:09 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > Alexei,
> >
> > Do you still need such a feature?
> >
> > Note, I just pushed my tree to my for-next tree, and also have a
> > ftrace/direct branch that ends with this patch set that is the basis of
> > the rest of the work of my code. Feel free to build against that
> > branch if you need to have something built on the net tree.  
> 
> I'm still trying to figure out what you meant
> by your suggestion to implement a modify_ftrace_direct().
> I was thinking something much simpler like
> modify_ftrace_direct(ip, old_call, new_ca);
> will just text poke that call addr from old to new if old matches

The main reason, is that then we need to add another arch specific
change, where as, the solution I suggested doesn't need anything new.
The less arch specific code we need the better.

> and will adjust ftrace inner bookkeeping.
> I don't understand why you want to malloc/free ftrace_ops for that.

We wouldn't need to allocate it, the stub could be something as simple
as:

struct ftrace_ops dummy_ops = {
	.func = ftrace_stub;
}

And just register that, where we set the hash to it, which would
require an malloc and free, but is that really a problem? A modify
shouldn't be a hot path, as text poke itself is going to be slow.

-- Steve

Re: [PATCH 03/10] ftrace: Add register_ftrace_direct()

[Steven Rostedt]

On Thu, 14 Nov 2019 13:48:27 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> The main reason, is that then we need to add another arch specific
> change, where as, the solution I suggested doesn't need anything new.
> The less arch specific code we need the better.

Here's the change, to see how easy it is (compiled tested only):

/* still needs comments */

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 82ef8d60a42b..4231571db30f 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5160,6 +5160,59 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(unregister_ftrace_direct);
+
+static struct ftrace_ops stub_ops = {
+	.func		= ftrace_stub,
+};
+
+int modify_ftrace_direct(unsigned long ip,
+			 unsigned long old_addr, unsigned long new_addr)
+{
+	struct ftrace_func_entry *entry;
+	struct dyn_ftrace *rec;
+	int ret = -ENODEV;
+
+	mutex_lock(&direct_mutex);
+	entry = __ftrace_lookup_ip(direct_functions, ip);
+	if (!entry) {
+		/* OK if it is off by a little */
+		rec = lookup_rec(ip, ip);
+		if (!rec || rec->ip == ip)
+			goto out_unlock;
+
+		entry = __ftrace_lookup_ip(direct_functions, rec->ip);
+		if (!entry)
+			goto out_unlock;
+
+		WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
+	}
+
+	ret = -EINVAL;
+	if (entry->direct != old_addr)
+		goto out_unlock;
+
+	ret = ftrace_set_filter_ip(&stub_ops, ip, 0, 0);
+	if (ret)
+		goto out_unlock;
+
+	ret = register_ftrace_function(&stub_ops);
+	if (ret) {
+		ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
+		goto out_unlock;
+	}
+
+	entry->direct = new_addr;
+
+	unregister_ftrace_function(&stub_ops);
+	ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
+
+	ret = 0;
+
+ out_unlock:
+	mutex_unlock(&direct_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(modify_ftrace_direct);
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
 
 /**


-- Steve

Re: [PATCH 09/10] ftrace/x86: Add register_ftrace_direct() for custom trampolines

[Miroslav Benes]

On Thu, 14 Nov 2019, Steven Rostedt wrote:

> On Thu, 14 Nov 2019 16:34:58 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > On Fri, 8 Nov 2019, Steven Rostedt wrote:
> > 
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > 
> > > Enable x86 to allow for register_ftrace_direct(), where a custom trampoline
> > > may be called directly from an ftrace mcount/fentry location.
> > > 
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> > 
> > [...]
> > 
> > > +++ b/arch/x86/kernel/ftrace_64.S
> > > @@ -88,6 +88,7 @@ EXPORT_SYMBOL(__fentry__)
> > >  	movq %rdi, RDI(%rsp)
> > >  	movq %r8, R8(%rsp)
> > >  	movq %r9, R9(%rsp)
> > > +	movq $0, ORIG_RAX(%rsp)
> > >  	/*
> > >  	 * Save the original RBP. Even though the mcount ABI does not
> > >  	 * require this, it helps out callers.
> > > @@ -114,7 +115,8 @@ EXPORT_SYMBOL(__fentry__)
> > >  	subq $MCOUNT_INSN_SIZE, %rdi
> > >  	.endm
> > >  
> > > -.macro restore_mcount_regs
> > > +.macro restore_mcount_regs save=0
> > > +
> > >  	movq R9(%rsp), %r9
> > >  	movq R8(%rsp), %r8
> > >  	movq RDI(%rsp), %rdi
> > > @@ -123,10 +125,7 @@ EXPORT_SYMBOL(__fentry__)
> > >  	movq RCX(%rsp), %rcx
> > >  	movq RAX(%rsp), %rax
> > >  
> > > -	/* ftrace_regs_caller can modify %rbp */
> > > -	movq RBP(%rsp), %rbp
> > > -
> > > -	addq $MCOUNT_REG_SIZE, %rsp
> > > +	addq $MCOUNT_REG_SIZE-\save, %rsp
> > >  
> > >  	.endm
> > >  
> > > @@ -228,10 +227,30 @@ GLOBAL(ftrace_regs_call)
> > >  	movq R10(%rsp), %r10
> > >  	movq RBX(%rsp), %rbx
> > >  
> > > -	restore_mcount_regs
> > > +	movq RBP(%rsp), %rbp
> > > +
> > > +	movq ORIG_RAX(%rsp), %rax
> > > +	movq %rax, MCOUNT_REG_SIZE-8(%rsp)
> > > +
> > > +	/* If ORIG_RAX is anything but zero, make this a call to that */
> > > +	movq ORIG_RAX(%rsp), %rax
> > > +	cmpq	$0, %rax
> > > +	je	1f
> > > +
> > > +	/* Swap the flags with orig_rax */
> > > +	movq MCOUNT_REG_SIZE(%rsp), %rdi
> > > +	movq %rdi, MCOUNT_REG_SIZE-8(%rsp)
> > > +	movq %rax, MCOUNT_REG_SIZE(%rsp)
> > > +
> > > +	restore_mcount_regs 8
> > > +
> > > +	jmp	2f
> > > +
> > > +1:	restore_mcount_regs
> > > +
> > >  
> > >  	/* Restore flags */
> > > -	popfq
> > > +2:	popfq  
> > 
> > If I am reading the code correctly (and I was confused couple of times, so 
> > maybe I am not), this is what makes the direct fops incompatible with 
> > ipmodify and livepatching for now. Is that correct?
> 
> Actually, it's the fact that the return goes to some unknown trampoline
> that may do something else as well.

Right.
 
> > 
> > What are your plans regarding this?
> 
> I wanted to see what the eBPF folks were doing, and then perhaps allow
> the ip modify occur too. I could let it happen as well now, and then we
> can see what the fallout is later ;-)

Waiting for eBPF using this first seems to be a good plan. Your call.
 
> > Moreover, we could replace ftrace_regs_caller with direct fops for live 
> > patching when this is merged with all arch support we need. After all, all 
> 
> Note, direct call is currently only available for x86_64.

Yes, I was speculating over a possibility in the future.

> > we need is to change the rip, which we could do easily in the direct 
> > trampoline. On the other hand, it would exclude coexistence of a live 
> > patch and a BPF filter (both direct now) on one function.
> 
> It may also end up being more complex, and not much of a performance
> benefit. I believe the BPF is injecting programs into the start of
> functions, but your trampoline for live patching may be not much
> different than what ftrace gives you today.

Ok, I made a note in my TODO list and let's see how it will evolve. It is 
definitely not something urgent.

Thanks
Miroslav