On Fri, Jun 28, 2019 at 01:09:08AM +0200, Thomas Gleixner wrote:

On Thu, 27 Jun 2019, Steven Rostedt wrote:

quoted

On Thu, 27 Jun 2019 17:47:29 -0500

quoted

Releasing the lock in a separate function seems a bit surprising and
fragile, would it be possible to do something like this instead?

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index b38c388d1087..89ea1af6fd13 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c

@@ -37,15 +37,21 @@
 int ftrace_arch_code_modify_prepare(void)
 {
 	mutex_lock(&text_mutex);
+
 	set_kernel_text_rw();
 	set_all_modules_text_rw();
+
+	mutex_unlock(&text_mutex);
 	return 0;
 }
 
 int ftrace_arch_code_modify_post_process(void)
 {
+	mutex_lock(&text_mutex);
+
 	set_all_modules_text_ro();
 	set_kernel_text_ro();
+
 	mutex_unlock(&text_mutex);
 	return 0;
 }

I agree with Josh on this. As the original bug was the race between
ftrace and live patching / modules changing the text from ro to rw and
vice versa. Just protecting the update to the text permissions is more
robust, and should be more self documenting when we need to handle
other architectures for this.

How is that supposed to work?

    ftrace  	     	
	prepare()
	 setrw()
			setro()
	patch <- FAIL

/me dodges frozen shark

You are right of course.  My brain has apparently already shut off for
the day.

Maybe a comment or two would help though.

-- 
Josh