[RFC][PATCH 0/2] ftrace: Add modify_ftrace_direct()

[RFC][PATCH 0/2] ftrace: Add modify_ftrace_direct()

[Steven Rostedt]

As discussed with Alexei, here's a simple implementation of
modify_ftrace_direct() without the need for any architecture changes.

Steven Rostedt (VMware) (2):
      ftrace: Add modify_ftrace_direct()
      ftrace/samples: Add a sample module that implements modify_ftrace_direct()

----
 include/linux/ftrace.h                |  6 +++
 kernel/trace/ftrace.c                 | 78 +++++++++++++++++++++++++++++++
 samples/ftrace/Makefile               |  1 +
 samples/ftrace/ftrace-direct-modify.c | 88 +++++++++++++++++++++++++++++++++++
 4 files changed, 173 insertions(+)
 create mode 100644 samples/ftrace/ftrace-direct-modify.c

[RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Steven Rostedt]

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

Add a new function modify_ftrace_direct() that will allow a user to update
an existing direct caller to a new trampoline, without missing hits due to
unregistering one and then adding another.

Link: https://lore.kernel.org/r/20191109022907.6zzo6orhxpt5n2sv@ast-mbp.dhcp.thefacebook.com

Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  6 ++++
 kernel/trace/ftrace.c  | 78 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 55647e185141..73eb2e93593f 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -250,6 +250,7 @@ static inline void ftrace_free_mem(struct module *mod, void *start, void *end) {
 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);
+int modify_ftrace_direct(unsigned long ip, unsigned long old_addr, unsigned long new_addr);
 struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr);
 #else
 # define ftrace_direct_func_count 0
@@ -261,6 +262,11 @@ static inline int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 {
 	return -ENODEV;
 }
+static inline int modify_ftrace_direct(unsigned long ip,
+				       unsigned long old_addr, unsigned long new_addr)
+{
+	return -ENODEV;
+}
 static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr)
 {
 	return NULL;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 82ef8d60a42b..834f3556ea1e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5160,6 +5160,84 @@ 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,
+};
+
+/**
+ * modify_ftrace_direct - Modify an existing direct call to call something else
+ * @ip: The instruction pointer to modify
+ * @old_addr: The address that the current @ip calls directly
+ * @new_addr: The address that the @ip should call
+ *
+ * This modifies a ftrace direct caller at an instruction pointer without
+ * having to disable it first. The direct call will switch over to the
+ * @new_addr without missing anything.
+ *
+ * Returns: zero on success. Non zero on error, which includes:
+ *  -ENODEV : the @ip given has no direct caller attached
+ *  -EINVAL : the @old_addr does not match the current direct caller
+ */
+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;
+
+		ip = rec->ip;
+		WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
+	}
+
+	ret = -EINVAL;
+	if (entry->direct != old_addr)
+		goto out_unlock;
+
+	/*
+	 * By setting a stub function at the same address, we force
+	 * the code to call the iterator and the direct_ops helper.
+	 * This means that @ip does not call the direct call, and
+	 * we can simply modify it.
+	 */
+	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;
+
+	/*
+	 * By removing the stub, we put back the direct call, calling
+	 * the @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 */
 
 /**
-- 
2.23.0

[RFC][PATCH 2/2] ftrace/samples: Add a sample module that implements modify_ftrace_direct()

[Steven Rostedt]

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

Add a sample module that tests modify_ftrace_direct(), and this can be used
by the selftests as well.

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

diff --git a/samples/ftrace/Makefile b/samples/ftrace/Makefile
index d8217c4e072e..fb0c3ae18295 100644
--- a/samples/ftrace/Makefile
+++ b/samples/ftrace/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct.o
 obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-too.o
+obj-$(CONFIG_SAMPLE_FTRACE_DIRECT) += ftrace-direct-modify.o
diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
new file mode 100644
index 000000000000..e04229d21475
--- /dev/null
+++ b/samples/ftrace/ftrace-direct-modify.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/ftrace.h>
+
+void my_direct_func1(void)
+{
+	trace_printk("my direct func1\n");
+}
+
+void my_direct_func2(void)
+{
+	trace_printk("my direct func2\n");
+}
+
+extern void my_tramp1(void *);
+extern void my_tramp2(void *);
+
+static unsigned long my_ip = (unsigned long)schedule;
+
+asm (
+"	.pushsection    .text, \"ax\", @progbits\n"
+"   my_tramp1:"
+"	pushq %rbp\n"
+"	movq %rsp, %rbp\n"
+"	call my_direct_func1\n"
+"	leave\n"
+"	ret\n"
+"   my_tramp2:"
+"	pushq %rbp\n"
+"	movq %rsp, %rbp\n"
+"	call my_direct_func2\n"
+"	leave\n"
+"	ret\n"
+"	.popsection\n"
+);
+
+static unsigned long my_tramp = (unsigned long)my_tramp1;
+static unsigned long tramps[2] = {
+	(unsigned long)my_tramp1,
+	(unsigned long)my_tramp2,
+};
+
+static int simple_thread(void *arg)
+{
+	static int t;
+	int ret = 0;
+
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(2 * HZ);
+
+		if (ret)
+			continue;
+		t ^= 1;
+		ret = modify_ftrace_direct(my_ip, my_tramp, tramps[t]);
+		if (!ret)
+			my_tramp = tramps[t];
+		WARN_ON_ONCE(ret);
+	}
+
+	return 0;
+}
+
+static struct task_struct *simple_tsk;
+
+static int __init ftrace_direct_init(void)
+{
+	int ret;
+
+	ret = register_ftrace_direct(my_ip, my_tramp);
+	if (!ret)
+		simple_tsk = kthread_run(simple_thread, NULL, "event-sample-fn");
+	return ret;
+}
+
+static void __exit ftrace_direct_exit(void)
+{
+	kthread_stop(simple_tsk);
+	unregister_ftrace_direct(my_ip, my_tramp);
+}
+
+module_init(ftrace_direct_init);
+module_exit(ftrace_direct_exit);
+
+MODULE_AUTHOR("Steven Rostedt");
+MODULE_DESCRIPTION("Example use case of using modify_ftrace_direct()");
+MODULE_LICENSE("GPL");
-- 
2.23.0

Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Alexei Starovoitov]

On Thu, Nov 14, 2019 at 02:46:37PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> Add a new function modify_ftrace_direct() that will allow a user to update
> an existing direct caller to a new trampoline, without missing hits due to
> unregistering one and then adding another.
> 
> Link: https://lore.kernel.org/r/20191109022907.6zzo6orhxpt5n2sv@ast-mbp.dhcp.thefacebook.com
> 
> Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  include/linux/ftrace.h |  6 ++++
>  kernel/trace/ftrace.c  | 78 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 55647e185141..73eb2e93593f 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -250,6 +250,7 @@ static inline void ftrace_free_mem(struct module *mod, void *start, void *end) {
>  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);
> +int modify_ftrace_direct(unsigned long ip, unsigned long old_addr, unsigned long new_addr);
>  struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr);
>  #else
>  # define ftrace_direct_func_count 0
> @@ -261,6 +262,11 @@ static inline int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
>  {
>  	return -ENODEV;
>  }
> +static inline int modify_ftrace_direct(unsigned long ip,
> +				       unsigned long old_addr, unsigned long new_addr)
> +{
> +	return -ENODEV;
> +}
>  static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr)
>  {
>  	return NULL;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 82ef8d60a42b..834f3556ea1e 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5160,6 +5160,84 @@ 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,
> +};
> +
> +/**
> + * modify_ftrace_direct - Modify an existing direct call to call something else
> + * @ip: The instruction pointer to modify
> + * @old_addr: The address that the current @ip calls directly
> + * @new_addr: The address that the @ip should call
> + *
> + * This modifies a ftrace direct caller at an instruction pointer without
> + * having to disable it first. The direct call will switch over to the
> + * @new_addr without missing anything.
> + *
> + * Returns: zero on success. Non zero on error, which includes:
> + *  -ENODEV : the @ip given has no direct caller attached
> + *  -EINVAL : the @old_addr does not match the current direct caller
> + */
> +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;
> +
> +		ip = rec->ip;
> +		WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
> +	}
> +
> +	ret = -EINVAL;
> +	if (entry->direct != old_addr)
> +		goto out_unlock;
> +
> +	/*
> +	 * By setting a stub function at the same address, we force
> +	 * the code to call the iterator and the direct_ops helper.
> +	 * This means that @ip does not call the direct call, and
> +	 * we can simply modify it.
> +	 */
> +	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;
> +
> +	/*
> +	 * By removing the stub, we put back the direct call, calling
> +	 * the @new_addr.
> +	 */
> +	unregister_ftrace_function(&stub_ops);
> +	ftrace_set_filter_ip(&stub_ops, ip, 1, 0);

Thanks a lot for implementing it.
Switching to iterator just to modify the call.. hmm.
So "call direct_bpf_A" gets replaced to "call ftrace_stub" to do the iterator
only to patch "call direct_bpf_B" later. I'm struggling to see why do that when
arch can provide call to call rewrite easily. x86 and others have such ability
already. I don't understand why you want to sacrifice simplicity here.
Anyway with all 3 apis (register, modify, unreg) it looks complete.
I'll start playing with it on Monday.

Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Steven Rostedt]

On Fri, 15 Nov 2019 13:51:26 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> Thanks a lot for implementing it.
> Switching to iterator just to modify the call.. hmm.
> So "call direct_bpf_A" gets replaced to "call ftrace_stub" to do the iterator
> only to patch "call direct_bpf_B" later. I'm struggling to see why do that when
> arch can provide call to call rewrite easily. x86 and others have such ability
> already. I don't understand why you want to sacrifice simplicity here.
> Anyway with all 3 apis (register, modify, unreg) it looks complete.
> I'll start playing with it on Monday.

Now if you take my latest for-next branch, and add the patch below, you
can then implement a ftrace_modify_direct_caller() in x86 (or whatever
architecture you'd like), that does what you want. I encapsulated the
code such that the stub trick is for those that don't implement their
own modification.

-- Steve

From 6220daa748fcf9e7f6a12cad94ea50026571bda1 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Sun, 17 Nov 2019 17:04:15 -0500
Subject: [PATCH] ftrace: Add a helper function to modify_ftrace_direct() to
 allow arch optimization

If a direct ftrace callback is at a location that does not have any other
ftrace helpers attached to it, it is possible to simply just change the
text to call the new caller (if the architecture supports it). But this
requires special architecture code. Currently, modify_ftrace_direct() uses a
trick to add a stub ftrace callback to the location forcing it to call the
ftrace iterator. Then it can change the direct helper to call the new
function in C, and then remove the stub. Removing the stub will have the
location now call the new location that the direct helper is using.

The new helper function does the registering the stub trick, but is a weak
function, allowing an architecture to override it to do something a bit more
direct.

Link: https://lore.kernel.org/r/20191115215125.mbqv7taqnx376yed@ast-mbp.dhcp.thefacebook.com

Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  18 +++++++
 kernel/trace/ftrace.c  | 120 ++++++++++++++++++++++++++++++-----------
 2 files changed, 106 insertions(+), 32 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 73eb2e93593f..ff89fd07a745 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -246,12 +246,23 @@ 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 */
 
+struct ftrace_func_entry {
+	struct hlist_node hlist;
+	unsigned long ip;
+	unsigned long direct; /* for direct lookup only */
+};
+
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
+struct dyn_ftrace;
 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);
 int modify_ftrace_direct(unsigned long ip, unsigned long old_addr, unsigned long new_addr);
 struct ftrace_direct_func *ftrace_find_direct_func(unsigned long addr);
+int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
+				struct dyn_ftrace *rec,
+				unsigned long old_addr,
+				unsigned long new_addr);
 #else
 # define ftrace_direct_func_count 0
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
@@ -271,6 +282,13 @@ static inline struct ftrace_direct_func *ftrace_find_direct_func(unsigned long a
 {
 	return NULL;
 }
+static inline int ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
+					      struct dyn_ftrace *rec,
+					      unsigned long old_addr,
+					      unsigned long new_addr)
+{
+	return -ENODEV;
+}
 #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 ef79c8393f53..caae523f4ef3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1020,12 +1020,6 @@ static bool update_all_ops;
 # error Dynamic ftrace depends on MCOUNT_RECORD
 #endif
 
-struct ftrace_func_entry {
-	struct hlist_node hlist;
-	unsigned long ip;
-	unsigned long direct; /* for direct lookup only */
-};
-
 struct ftrace_func_probe {
 	struct ftrace_probe_ops	*probe_ops;
 	struct ftrace_ops	ops;
@@ -5112,7 +5106,8 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)
 }
 EXPORT_SYMBOL_GPL(register_ftrace_direct);
 
-static struct ftrace_func_entry *find_direct_entry(unsigned long *ip)
+static struct ftrace_func_entry *find_direct_entry(unsigned long *ip,
+						   struct dyn_ftrace **recp)
 {
 	struct ftrace_func_entry *entry;
 	struct dyn_ftrace *rec;
@@ -5132,6 +5127,9 @@ static struct ftrace_func_entry *find_direct_entry(unsigned long *ip)
 	/* Passed in ip just needs to be on the call site */
 	*ip = rec->ip;
 
+	if (recp)
+		*recp = rec;
+
 	return entry;
 }
 
@@ -5143,7 +5141,7 @@ int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 
 	mutex_lock(&direct_mutex);
 
-	entry = find_direct_entry(&ip);
+	entry = find_direct_entry(&ip, NULL);
 	if (!entry)
 		goto out_unlock;
 
@@ -5179,6 +5177,75 @@ static struct ftrace_ops stub_ops = {
 	.func		= ftrace_stub,
 };
 
+/**
+ * ftrace_modify_direct_caller - modify ftrace nop directly
+ * @entry: The ftrace hash entry of the direct helper for @rec
+ * @rec: The record representing the function site to patch
+ * @old_addr: The location that the site at @rec->ip currently calls
+ * @new_addr: The location that the site at @rec->ip should call
+ *
+ * An architecture may overwrite this function to optimize the
+ * changing of the direct callback on an ftrace nop location.
+ * This is called with the ftrace_lock mutex held, and no other
+ * ftrace callbacks are on the associated record (@rec). Thus,
+ * it is safe to modify the ftrace record, where it should be
+ * currently calling @old_addr directly, to call @new_addr.
+ *
+ * Safety checks should be made to make sure that the code at
+ * @rec->ip is currently calling @old_addr. And this must
+ * also update entry->direct to @new_addr.
+ */
+int __weak ftrace_modify_direct_caller(struct ftrace_func_entry *entry,
+				       struct dyn_ftrace *rec,
+				       unsigned long old_addr,
+				       unsigned long new_addr)
+{
+	unsigned long ip = rec->ip;
+	int ret;
+
+	/*
+	 * The ftrace_lock was used to determine if the record
+	 * had more than one registered user to it. If it did,
+	 * we needed to prevent that from changing to do the quick
+	 * switch. But if it did not (only a direct caller was attached)
+	 * then this function is called. But this function can deal
+	 * with attached callers to the rec that we care about, and
+	 * since this function uses standard ftrace calls that take
+	 * the ftrace_lock mutex, we need to release it.
+	 */
+	mutex_unlock(&ftrace_lock);
+
+	/*
+	 * By setting a stub function at the same address, we force
+	 * the code to call the iterator and the direct_ops helper.
+	 * This means that @ip does not call the direct call, and
+	 * we can simply modify it.
+	 */
+	ret = ftrace_set_filter_ip(&stub_ops, ip, 0, 0);
+	if (ret)
+		goto out_lock;
+
+	ret = register_ftrace_function(&stub_ops);
+	if (ret) {
+		ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
+		goto out_lock;
+	}
+
+	entry->direct = new_addr;
+
+	/*
+	 * By removing the stub, we put back the direct call, calling
+	 * the @new_addr.
+	 */
+	unregister_ftrace_function(&stub_ops);
+	ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
+
+ out_lock:
+	mutex_lock(&ftrace_lock);
+
+	return ret;
+}
+
 /**
  * modify_ftrace_direct - Modify an existing direct call to call something else
  * @ip: The instruction pointer to modify
@@ -5197,11 +5264,13 @@ 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 = find_direct_entry(&ip);
+	mutex_lock(&ftrace_lock);
+	entry = find_direct_entry(&ip, &rec);
 	if (!entry)
 		goto out_unlock;
 
@@ -5210,33 +5279,20 @@ int modify_ftrace_direct(unsigned long ip,
 		goto out_unlock;
 
 	/*
-	 * By setting a stub function at the same address, we force
-	 * the code to call the iterator and the direct_ops helper.
-	 * This means that @ip does not call the direct call, and
-	 * we can simply modify it.
+	 * If there's no other ftrace callback on the rec->ip location,
+	 * then it can be changed directly by the architecture.
+	 * If there is another caller, then we just need to change the
+	 * direct caller helper to point to @new_addr.
 	 */
-	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;
+	if (ftrace_rec_count(rec) == 1) {
+		ret = ftrace_modify_direct_caller(entry, rec, old_addr, new_addr);
+	} else {
+		entry->direct = new_addr;
+		ret = 0;
 	}
 
-	entry->direct = new_addr;
-
-	/*
-	 * By removing the stub, we put back the direct call, calling
-	 * the @new_addr.
-	 */
-	unregister_ftrace_function(&stub_ops);
-	ftrace_set_filter_ip(&stub_ops, ip, 1, 0);
-
-	ret = 0;
-
  out_unlock:
+	mutex_unlock(&ftrace_lock);
 	mutex_unlock(&direct_mutex);
 	return ret;
 }
-- 
2.20.1

Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Alexei Starovoitov]

On Sun, Nov 17, 2019 at 2:18 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 15 Nov 2019 13:51:26 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > Thanks a lot for implementing it.
> > Switching to iterator just to modify the call.. hmm.
> > So "call direct_bpf_A" gets replaced to "call ftrace_stub" to do the iterator
> > only to patch "call direct_bpf_B" later. I'm struggling to see why do that when
> > arch can provide call to call rewrite easily. x86 and others have such ability
> > already. I don't understand why you want to sacrifice simplicity here.
> > Anyway with all 3 apis (register, modify, unreg) it looks complete.
> > I'll start playing with it on Monday.
>
> Now if you take my latest for-next branch, and add the patch below,

I took your for-next without the extra patch and used it from bpf trampoline.
It's looking good so far. Passed basic testing. Will add more stress tests.

Do you mind doing:
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 73eb2e93593f..6ddb203ca550 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -256,16 +256,16 @@ struct ftrace_direct_func
*ftrace_find_direct_func(unsigned long addr);
 # define ftrace_direct_func_count 0
 static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
 {
-       return -ENODEV;
+       return -ENOTSUPP;
 }
 static inline int unregister_ftrace_direct(unsigned long ip, unsigned
long addr)
 {
-       return -ENODEV;
+       return -ENOTSUPP;
 }
 static inline int modify_ftrace_direct(unsigned long ip,
                                       unsigned long old_addr,
unsigned long new_addr)
 {
-       return -ENODEV;
+       return -ENOTSUPP;
 }

otherwise ENODEV is a valid error when ip is incorrect which is
indistinguishable from ftrace not compiled in.

Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Steven Rostedt]

On Mon, 18 Nov 2019 22:04:28 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> I took your for-next without the extra patch and used it from bpf trampoline.
> It's looking good so far. Passed basic testing. Will add more stress tests.
> 
> Do you mind doing:
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 73eb2e93593f..6ddb203ca550 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -256,16 +256,16 @@ struct ftrace_direct_func
> *ftrace_find_direct_func(unsigned long addr);
>  # define ftrace_direct_func_count 0
>  static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
>  {
> -       return -ENODEV;
> +       return -ENOTSUPP;
>  }
>  static inline int unregister_ftrace_direct(unsigned long ip, unsigned
> long addr)
>  {
> -       return -ENODEV;
> +       return -ENOTSUPP;
>  }
>  static inline int modify_ftrace_direct(unsigned long ip,
>                                        unsigned long old_addr,
> unsigned long new_addr)
>  {
> -       return -ENODEV;
> +       return -ENOTSUPP;
>  }
> 
> otherwise ENODEV is a valid error when ip is incorrect which is
> indistinguishable from ftrace not compiled in.

Sure I can add this. Want to add a Signed-off-by to it, and I'll just
pull it in directly? I can write up the change log.

-- Steve

Re: [RFC][PATCH 1/2] ftrace: Add modify_ftrace_direct()

[Alexei Starovoitov]

On Tue, Nov 19, 2019 at 6:13 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 18 Nov 2019 22:04:28 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > I took your for-next without the extra patch and used it from bpf trampoline.
> > It's looking good so far. Passed basic testing. Will add more stress tests.
> >
> > Do you mind doing:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 73eb2e93593f..6ddb203ca550 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -256,16 +256,16 @@ struct ftrace_direct_func
> > *ftrace_find_direct_func(unsigned long addr);
> >  # define ftrace_direct_func_count 0
> >  static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
> >  {
> > -       return -ENODEV;
> > +       return -ENOTSUPP;
> >  }
> >  static inline int unregister_ftrace_direct(unsigned long ip, unsigned
> > long addr)
> >  {
> > -       return -ENODEV;
> > +       return -ENOTSUPP;
> >  }
> >  static inline int modify_ftrace_direct(unsigned long ip,
> >                                        unsigned long old_addr,
> > unsigned long new_addr)
> >  {
> > -       return -ENODEV;
> > +       return -ENOTSUPP;
> >  }
> >
> > otherwise ENODEV is a valid error when ip is incorrect which is
> > indistinguishable from ftrace not compiled in.
>
> Sure I can add this. Want to add a Signed-off-by to it, and I'll just
> pull it in directly? I can write up the change log.

Whichever way is easier for you.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Thanks!

> -- Steve