linux-kernel - [PATCH RFC tip/perf/core] ftrace/x86: Let dynamic trampolines call ops->func even for dynamic fops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 19 Feb 2015 15:56:14 +0100
From:	Miroslav Benes <mbenes@...e.cz>
To:	rostedt@...dmis.org, mingo@...hat.com
Cc:	tglx@...utronix.de, hpa@...or.com, linux-kernel@...r.kernel.org,
	x86@...nel.org, jkosina@...e.cz, Miroslav Benes <mbenes@...e.cz>
Subject: [PATCH RFC tip/perf/core] ftrace/x86: Let dynamic trampolines call ops->func even for dynamic fops

Dynamically allocated trampolines call ftrace_ops_get_func to get the
function which they should call. For dynamic fops (FTRACE_OPS_FL_DYNAMIC
flag is set) ftrace_ops_list_func is always returned. This is reasonable
for static trampolines but goes against the main advantage of dynamic
ones, that is avoidance of going through the list of all registered
callbacks for functions that are only being traced by a single callback.

We can fix it by returning ops->func (or recursion safe version) from
ftrace_ops_get_func whenever it is possible for dynamic trampolines.

Note that dynamic trampolines are not allowed for dynamic fops if
CONFIG_PREEMPT=y.

Signed-off-by: Miroslav Benes <mbenes@...e.cz>
---

The patch is the result of my discussion with Steven few weeks ago [1].
I feel content with the outcome but not with the way.
ftrace_ops_get_func is called at two different places now. One is
create_trampoline where dynamic trampoline is created (if allowed) and
the other is in update_ftrace_function for other cases. I haven't found
the way how to distinguish between these call places in the function
using present means. Thus I introduced new parameter. I do not consider
this optimum and that is the reason why this patch is RFC. I would
welcome any idea which would make it suitable for merge.

Steven, if you plan to fix this issue differently and in some larger
set, feel free to scratch this patch.

[1]: https://lkml.org/lkml/2015/1/29/300

 arch/x86/kernel/ftrace.c |  2 +-
 include/linux/ftrace.h   |  2 +-
 kernel/trace/ftrace.c    | 10 ++++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8b7b0a5..bfa9267 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -842,7 +842,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
 	ip = ops->trampoline + offset;
 
-	func = ftrace_ops_get_func(ops);
+	func = ftrace_ops_get_func(ops, true);
 
 	/* Do a safe modify in case the trampoline is executing */
 	new = ftrace_call_replace(ip, (unsigned long)func);
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..37444b5 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -62,7 +62,7 @@ struct ftrace_ops;
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct pt_regs *regs);
 
-ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
+ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops, bool dyntramp);
 
 /*
  * FTRACE_OPS_FL_* bits denote the state of ftrace_ops struct and are
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 224e768..5d964b3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -270,7 +270,7 @@ static void update_ftrace_function(void)
 	 * then have the mcount trampoline call the function directly.
 	 */
 	} else if (ftrace_ops_list->next == &ftrace_list_end) {
-		func = ftrace_ops_get_func(ftrace_ops_list);
+		func = ftrace_ops_get_func(ftrace_ops_list, false);
 
 	} else {
 		/* Just use the default ftrace_ops */
@@ -5176,6 +5176,7 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
 /**
  * ftrace_ops_get_func - get the function a trampoline should call
  * @ops: the ops to get the function for
+ * @dyntramp: whether the function is for dynamic trampoline or not
  *
  * Normally the mcount trampoline will call the ops->func, but there
  * are times that it should not. For example, if the ops does not
@@ -5184,13 +5185,14 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
  *
  * Returns the function that the trampoline should call for @ops.
  */
-ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops)
+ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops, bool dyntramp)
 {
 	/*
-	 * If this is a dynamic ops or we force list func,
+	 * If this is a dynamic ops and static trampoline or we force list func,
 	 * then it needs to call the list anyway.
 	 */
-	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
+	if ((!dyntramp && (ops->flags & FTRACE_OPS_FL_DYNAMIC)) ||
+	    FTRACE_FORCE_LIST_FUNC)
 		return ftrace_ops_list_func;
 
 	/*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/