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

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140220232308.3709bd83@gandalf.local.home>
Date:	Thu, 20 Feb 2014 23:23:08 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH 0/4] x86: Fix ftrace recovery when code modification
 failed

On Mon, 17 Feb 2014 16:22:49 +0100
Petr Mladek <pmladek@...e.cz> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/