All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] ftrace/x86: Recovery code fixes and improvements
@ 2014-02-24 16:12 Petr Mladek
  2014-02-24 16:12 ` [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure Petr Mladek
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Petr Mladek @ 2014-02-24 16:12 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Frederic Weisbecker, Masami Hiramatsu, Paul E. McKenney,
	Jiri Kosina, linux-kernel, x86, Petr Mladek

This is folowup from the discussion (Re: [PATCH 0/4] x86: Fix ftrace recovery
when code modification failed), see https://lkml.org/lkml/2014/2/21/748

I split the fixes into three patches:

1st patch adds the missing sync call. I guess that Steven would want to
squash this into the commit f06492158f4b0cdb (ftrace/x86: Run a sync
after fixup on failure) in the ftrace/urgent branch.

2nd patch adds the missing warnigns about ftrace modification failure
into the generic code as suggested by Steven[*]. I used "err" instead
of the suggested "ret" variable. The value is handled but it is not
returned. Also I left ftrace_replace_code() calls as is. It handles
the error, prints more details, and does not return the error value.

3rd patch adds the BUG calls. Steven did not comment on it, so I rather
put this into a separate patch, so it does not block the other things.

[*] I feel shame that I was not brave enough to do modify the generic code
when I sent the patch on Friday.

Petr Mladek (3):
  ftrace/x86: One more missing sync after fixup of function modification
    failure
  ftrace: Warn on error when modifying ftrace function
  ftrace/x86: BUG when ftrace recovery fails

 arch/x86/kernel/ftrace.c | 24 ++++++++++++++++--------
 kernel/trace/ftrace.c    | 17 ++++++++++++-----
 2 files changed, 28 insertions(+), 13 deletions(-)

-- 
1.8.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure
  2014-02-24 16:12 [PATCH v2 0/3] ftrace/x86: Recovery code fixes and improvements Petr Mladek
@ 2014-02-24 16:12 ` Petr Mladek
  2014-03-03 22:13   ` H. Peter Anvin
  2014-02-24 16:12 ` [PATCH 2/3] ftrace: Warn on error when modifying ftrace function Petr Mladek
  2014-02-24 16:12 ` [PATCH 3/3] ftrace/x86: BUG when ftrace recovery fails Petr Mladek
  2 siblings, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2014-02-24 16:12 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Frederic Weisbecker, Masami Hiramatsu, Paul E. McKenney,
	Jiri Kosina, linux-kernel, x86, Petr Mladek

If a failure occurs while modifying ftrace function, it bails out and will
remove the tracepoints to be back to what the code originally was.

There is missing the final sync run across the CPUs after the fix up is done
and before the ftrace int3 handler flag is reset.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 arch/x86/kernel/ftrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 6b566c82d82c..69885e2f2095 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -660,8 +660,8 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 		ret = -EPERM;
 		goto out;
 	}
-	run_sync();
  out:
+	run_sync();
 	return ret;
 
  fail_update:
-- 
1.8.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] ftrace: Warn on error when modifying ftrace function
  2014-02-24 16:12 [PATCH v2 0/3] ftrace/x86: Recovery code fixes and improvements Petr Mladek
  2014-02-24 16:12 ` [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure Petr Mladek
@ 2014-02-24 16:12 ` Petr Mladek
  2014-02-24 16:12 ` [PATCH 3/3] ftrace/x86: BUG when ftrace recovery fails Petr Mladek
  2 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2014-02-24 16:12 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Frederic Weisbecker, Masami Hiramatsu, Paul E. McKenney,
	Jiri Kosina, linux-kernel, x86, Petr Mladek

We should print some warning and kill ftrace functionality when the ftrace
function is not set correctly. Otherwise, ftrace might do crazy things without
an explanation. The error value has been ignored so far.

Note that an error that happens during updating all the traced calls is handled
in ftrace_replace_code(). We print more details about the particular
failing address via ftrace_bug() there.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 kernel/trace/ftrace.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index cd7f76d1eb86..2f139023e789 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1994,6 +1994,7 @@ int __weak ftrace_arch_code_modify_post_process(void)
 void ftrace_modify_all_code(int command)
 {
 	int update = command & FTRACE_UPDATE_TRACE_FUNC;
+	int err = 0;
 
 	/*
 	 * If the ftrace_caller calls a ftrace_ops func directly,
@@ -2005,8 +2006,11 @@ void ftrace_modify_all_code(int command)
 	 * to make sure the ops are having the right functions
 	 * traced.
 	 */
-	if (update)
-		ftrace_update_ftrace_func(ftrace_ops_list_func);
+	if (update) {
+		err = ftrace_update_ftrace_func(ftrace_ops_list_func);
+		if (FTRACE_WARN_ON(err))
+			return;
+	}
 
 	if (command & FTRACE_UPDATE_CALLS)
 		ftrace_replace_code(1);
@@ -2019,13 +2023,16 @@ void ftrace_modify_all_code(int command)
 		/* If irqs are disabled, we are in stop machine */
 		if (!irqs_disabled())
 			smp_call_function(ftrace_sync_ipi, NULL, 1);
-		ftrace_update_ftrace_func(ftrace_trace_function);
+		err = ftrace_update_ftrace_func(ftrace_trace_function);
+		if (FTRACE_WARN_ON(err))
+			return;
 	}
 
 	if (command & FTRACE_START_FUNC_RET)
-		ftrace_enable_ftrace_graph_caller();
+		err = ftrace_enable_ftrace_graph_caller();
 	else if (command & FTRACE_STOP_FUNC_RET)
-		ftrace_disable_ftrace_graph_caller();
+		err = ftrace_disable_ftrace_graph_caller();
+	FTRACE_WARN_ON(err);
 }
 
 static int __ftrace_modify_code(void *data)
-- 
1.8.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/3] ftrace/x86: BUG when ftrace recovery fails
  2014-02-24 16:12 [PATCH v2 0/3] ftrace/x86: Recovery code fixes and improvements Petr Mladek
  2014-02-24 16:12 ` [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure Petr Mladek
  2014-02-24 16:12 ` [PATCH 2/3] ftrace: Warn on error when modifying ftrace function Petr Mladek
@ 2014-02-24 16:12 ` Petr Mladek
  2 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2014-02-24 16:12 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Frederic Weisbecker, Masami Hiramatsu, Paul E. McKenney,
	Jiri Kosina, linux-kernel, x86, Petr Mladek

Ftrace modifies function calls using Int3 breakpoints on x86.
The breakpoints are handled only when the patching is in progress.
If something goes wrong, there is a recovery code that removes
the breakpoints. If this fails, the system might get silently
rebooted when a remaining break is not handled or an invalid
instruction is proceed.

We should BUG() when the breakpoint could not be removed. Otherwise,
the system silently crashes when the function finishes the Int3
handler is disabled.

Note that we need to modify remove_breakpoint() to return non-zero
value only when there is an error. The return value was ignored before,
so it does not cause any troubles.

Signed-off-by: Petr Mladek <pmladek@suse.cz>
---
 arch/x86/kernel/ftrace.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 69885e2f2095..51dffba12e24 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -425,7 +425,7 @@ static int remove_breakpoint(struct dyn_ftrace *rec)
 
 	/* If this does not have a breakpoint, we are done */
 	if (ins[0] != brk)
-		return -1;
+		return 0;
 
 	nop = ftrace_nop_replace();
 
@@ -632,7 +632,12 @@ void ftrace_replace_code(int enable)
 	printk(KERN_WARNING "Failed on %s (%d):\n", report, count);
 	for_ftrace_rec_iter(iter) {
 		rec = ftrace_rec_iter_record(iter);
-		remove_breakpoint(rec);
+		/*
+		 * Breakpoints are handled only when this function is in
+		 * progress. The system could not work with them.
+		 */
+		if (remove_breakpoint(rec))
+			BUG();
 	}
 	run_sync();
 }
@@ -656,16 +661,19 @@ ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
 	run_sync();
 
 	ret = ftrace_write(ip, new_code, 1);
-	if (ret) {
-		ret = -EPERM;
-		goto out;
-	}
+	/*
+	 * The breakpoint is handled only when this function is in progress.
+	 * The system could not work if we could not remove it.
+	 */
+	BUG_ON(ret);
  out:
 	run_sync();
 	return ret;
 
  fail_update:
-	ftrace_write(ip, old_code, 1);
+	/* Also here the system could not work with the breakpoint */
+	if (ftrace_write(ip, old_code, 1))
+		BUG();
 	goto out;
 }
 
-- 
1.8.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure
  2014-02-24 16:12 ` [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure Petr Mladek
@ 2014-03-03 22:13   ` H. Peter Anvin
  0 siblings, 0 replies; 5+ messages in thread
From: H. Peter Anvin @ 2014-03-03 22:13 UTC (permalink / raw)
  To: Petr Mladek, Steven Rostedt
  Cc: Frederic Weisbecker, Masami Hiramatsu, Paul E. McKenney,
	Jiri Kosina, linux-kernel, x86

On 02/24/2014 08:12 AM, Petr Mladek wrote:
> If a failure occurs while modifying ftrace function, it bails out and will
> remove the tracepoints to be back to what the code originally was.
> 
> There is missing the final sync run across the CPUs after the fix up is done
> and before the ftrace int3 handler flag is reset.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  arch/x86/kernel/ftrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: H. Peter Anvin <hpa@linux.intel.com>



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-03-03 22:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-24 16:12 [PATCH v2 0/3] ftrace/x86: Recovery code fixes and improvements Petr Mladek
2014-02-24 16:12 ` [PATCH 1/3] ftrace/x86: One more missing sync after fixup of function modification failure Petr Mladek
2014-03-03 22:13   ` H. Peter Anvin
2014-02-24 16:12 ` [PATCH 2/3] ftrace: Warn on error when modifying ftrace function Petr Mladek
2014-02-24 16:12 ` [PATCH 3/3] ftrace/x86: BUG when ftrace recovery fails Petr Mladek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.