Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 10, 2019 at 11:36:50AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 10, 2019 at 11:33:29AM +0200, Peter Zijlstra wrote:

> > I don't see any reason what so ever..
> > 
> > load_module()
> >   ...
> >   complete_formation()
> >     mutex_lock(&module_mutex);
> >     ...
> >     module_enable_ro();
> >     module_enable_nx();
> >     module_enable_x();
> > 
> >     mod->state = MODULE_STATE_COMING;
> >     mutex_unlock(&module_mutex);
> > 
> >   prepare_coming_module()
> >     ftrace_module_enable();
> >     ...
> > 
> > IOW, we're doing ftrace_module_enable() immediately after we flip it
> > RO+X. There is nothing in between that we can possibly rely on.
> > 
> > I was going to put:
> > 
> >   blocking_notifier_call_chain(&module_notify_list,
> > 			       MODULE_STATE_UNFORMED, mod);
> > 
> > right before module_enable_ro(), in complete_formation(), for jump_label
> > and static_call. It looks like ftrace (and possibly klp) want that too.
> 
> Also, you already have ftrace_module_init() right before that. The only
> thing inbetween ftrace_module_init() and ftrace_module_enable() is
> verify_exported_symbols() and module_bug_finalize().
> 
> Do you really need that for patching stuff?

If you rework the locking slightly, such that you have to call
ftrace_process_locs() with ftrace_lock already held, then it looks like
you can squash ftrace_module_enable() right in there.

There is absolutely no fundamental reason you cannot patch it from
ftrace_module_init().

Yes, your code is anal about checking the NOPs, so you first have to
write NOPs before you can write CALLs, if it is enabled. But afaict you
really can do all that from ftrace_module_init(), as long as you do it
all under the same ftrace_lock section.

If you have two sections, like now, then there is indeed that race that
ftrace can get enabled in between, and all the confusion that that
brings.

That is, what's fundamentally buggered about something like this?

---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 62a50bf399d6..5f7113f100ce 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5626,6 +5626,48 @@ static int ftrace_process_locs(struct module *mod,
 	ftrace_update_code(mod, start_pg);
 	if (!mod)
 		local_irq_restore(flags);
+
+	if (ftrace_disabled || !mod)
+		goto out_loop;
+
+	do_for_each_ftrace_rec(pg, rec) {
+		int cnt;
+		/*
+		 * do_for_each_ftrace_rec() is a double loop.
+		 * module text shares the pg. If a record is
+		 * not part of this module, then skip this pg,
+		 * which the "break" will do.
+		 */
+		if (!within_module_core(rec->ip, mod) &&
+		    !within_module_init(rec->ip, mod))
+			break;
+
+		cnt = 0;
+
+		/*
+		 * When adding a module, we need to check if tracers are
+		 * currently enabled and if they are, and can trace this record,
+		 * we need to enable the module functions as well as update the
+		 * reference counts for those function records.
+		 */
+		if (ftrace_start_up)
+			cnt += referenced_filters(rec);
+
+		/* This clears FTRACE_FL_DISABLED */
+		rec->flags = cnt;
+
+		if (ftrace_start_up && cnt) {
+			int failed = __ftrace_replace_code(rec, 1);
+			if (failed) {
+				ftrace_bug(failed, rec);
+				goto out_loop;
+			}
+		}
+
+	} while_for_each_ftrace_rec();
+
+ out_loop:
+
 	ret = 0;
  out:
 	mutex_unlock(&ftrace_lock);
@@ -5793,73 +5835,6 @@ void ftrace_release_mod(struct module *mod)
 
 void ftrace_module_enable(struct module *mod)
 {
-	struct dyn_ftrace *rec;
-	struct ftrace_page *pg;
-
-	mutex_lock(&ftrace_lock);
-
-	if (ftrace_disabled)
-		goto out_unlock;
-
-	/*
-	 * If the tracing is enabled, go ahead and enable the record.
-	 *
-	 * The reason not to enable the record immediately is the
-	 * inherent check of ftrace_make_nop/ftrace_make_call for
-	 * correct previous instructions.  Making first the NOP
-	 * conversion puts the module to the correct state, thus
-	 * passing the ftrace_make_call check.
-	 *
-	 * We also delay this to after the module code already set the
-	 * text to read-only, as we now need to set it back to read-write
-	 * so that we can modify the text.
-	 */
-	if (ftrace_start_up)
-		ftrace_arch_code_modify_prepare();
-
-	do_for_each_ftrace_rec(pg, rec) {
-		int cnt;
-		/*
-		 * do_for_each_ftrace_rec() is a double loop.
-		 * module text shares the pg. If a record is
-		 * not part of this module, then skip this pg,
-		 * which the "break" will do.
-		 */
-		if (!within_module_core(rec->ip, mod) &&
-		    !within_module_init(rec->ip, mod))
-			break;
-
-		cnt = 0;
-
-		/*
-		 * When adding a module, we need to check if tracers are
-		 * currently enabled and if they are, and can trace this record,
-		 * we need to enable the module functions as well as update the
-		 * reference counts for those function records.
-		 */
-		if (ftrace_start_up)
-			cnt += referenced_filters(rec);
-
-		/* This clears FTRACE_FL_DISABLED */
-		rec->flags = cnt;
-
-		if (ftrace_start_up && cnt) {
-			int failed = __ftrace_replace_code(rec, 1);
-			if (failed) {
-				ftrace_bug(failed, rec);
-				goto out_loop;
-			}
-		}
-
-	} while_for_each_ftrace_rec();
-
- out_loop:
-	if (ftrace_start_up)
-		ftrace_arch_code_modify_post_process();
-
- out_unlock:
-	mutex_unlock(&ftrace_lock);
-
 	process_cached_mods(mod->name);
 }