Re: [PATCH 0/4] x86: Fix ftrace recovery when code modification failed

[Steven Rostedt]

On Mon, 17 Feb 2014 16:22:49 +0100
Petr Mladek <pmladek@xxxxxxx> wrote:

> Ftrace modifies function calls using Int3 breakpoints on x86. It patches
> all functions in parallel to reduce the number of sync() calls. There is
> a code that removes pending Int3 breakpoints when something goes wrong.
> 
> The recovery does not work on x86_64. I simulated an error in
> ftrace_replace_code() and the system got rebooted.

Thanks for the report, I just did the same and it caused a reboot too.

> 
> This patch set fixes the recovery code, improves debugging of this
> type of problem, and does some clean up.
> 
> BTW: It is an echo from the patch set that tried to use text_poke_bp()
> instead of the ftrace-specific Int3 based patching code. Ftrace has
> some specific restrictions. I did not find a way how to make the 
> universal text_poke_bp effective, safe, and clean to be usable
> there.
> 
> The patch set is against linux/tip. Last commit is a5b3cca53c43c3ba7
> (Merge tag 'v3.14-rc3')
> 
> Petr Mladek (4):
>   x86: Clean up remove_breakpoint() in ftrace code
>   x86: Fix ftrace patching recovery code to work on x86_64
>   x86: BUG when ftrace patching recovery fails
>   x86: Fix order of warning messages when ftrace modifies code
> 
>  arch/x86/kernel/ftrace.c | 67 ++++++++++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 33 deletions(-)
> 

That's quite a bit of changes. I did a quick debug analysis of it, and
found two bugs.

One, I shouldn't be using direct access to the ip with
probe_kernel_write(), I need to do the within() magic (also have a
ftrace_write() wrapper now).

The second bug is that I need to do another run_sync() after the fix
up. Can you see if this patch fixes the issue?

-- Steve

 ftrace.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index e625319..6b566c8 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -455,7 +455,7 @@ static int remove_breakpoint(struct dyn_ftrace *rec)
 	}
 
  update:
-	return probe_kernel_write((void *)ip, &nop[0], 1);
+	return ftrace_write(ip, nop, 1);
 }
 
 static int add_update_code(unsigned long ip, unsigned const char *new)
@@ -634,6 +634,7 @@ void ftrace_replace_code(int enable)
 		rec = ftrace_rec_iter_record(iter);
 		remove_breakpoint(rec);
 	}
+	run_sync();
 }
 
 static int
@@ -664,7 +665,7 @@ ftrace_modify_code(unsigned long ip, unsigned const
char *old_code, return ret;
 
  fail_update:
-	probe_kernel_write((void *)ip, &old_code[0], 1);
+	ftrace_write(ip, old_code, 1);
 	goto out;
 }
 

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